Skip to content

Commit 9d0d1aa

Browse files
aryanku-devclaude
andcommitted
fix: address CE review MAJORs in CORS iframe traversal
- Null-safe origin equality in both top-level and nested iframe comparisons. Switched to Objects.equals so a child URI that resolves to no host (data:, mailto:, schemeless) can never trigger an NPE that escapes the per-iframe catch. - Document the clampFrameDepth semantic: maxIframeDepth=0 falls back to DEFAULT_MAX_FRAME_DEPTH (5), mirroring @percy/sdk-utils. Disabling CORS capture should use ignoreIframeSelectors or data-percy-ignore, not depth=0. Comment guards against a silent flip in future refactors. - Expose closed shadow roots inside each CORS frame after switchTo() — mirrors the top-page behaviour so closed shadow DOM inside cross- origin iframes is also captured. Per-pair try/catch in the existing helper keeps one bad backendNodeId from aborting the rest. TODO tracks moving to per-frame CDP sessions when BiDi stabilises. - Remove the dead processFrame method — fully replaced by processFrameTree. Keeping duplicate logic invited drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 879a2cf commit 9d0d1aa

1 file changed

Lines changed: 29 additions & 61 deletions

File tree

src/main/java/io/percy/selenium/Percy.java

Lines changed: 29 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -644,8 +644,15 @@ private static String getOrigin(String url) {
644644
}
645645
}
646646

647-
// Clamp the configured frame depth to a sane range. Negative or
648-
// unreasonably large values fall back to the default.
647+
// Clamp the configured frame depth to a sane range.
648+
//
649+
// Semantic note: a value below MIN_FRAME_DEPTH (1) — including 0 and any
650+
// negative number — falls back to DEFAULT_MAX_FRAME_DEPTH. This matches
651+
// the @percy/sdk-utils canonical behaviour where `maxIframeDepth=0` is
652+
// treated as "unset, use default" rather than "disable nested CORS
653+
// capture". To disable CORS iframe traversal, callers should rely on
654+
// ignoreIframeSelectors or data-percy-ignore — not depth=0. Keeping this
655+
// aligned with sdk-utils avoids a breaking-change divergence across SDKs.
649656
private static int clampFrameDepth(int depth) {
650657
if (depth < MIN_FRAME_DEPTH) return DEFAULT_MAX_FRAME_DEPTH;
651658
if (depth > MAX_FRAME_DEPTH_CAP) return MAX_FRAME_DEPTH_CAP;
@@ -763,63 +770,6 @@ private Map<String, Object> serializeCurrentFrame(Map<String, Object> options) {
763770
return snapshot;
764771
}
765772

766-
private Map<String, Object> processFrame(WebElement frameElement, Map<String, Object> options) {
767-
// Read attributes while still in parent context — these calls will
768-
// fail if made after switchTo().frame().
769-
String frameUrl = frameElement.getAttribute("src");
770-
if (frameUrl == null) frameUrl = "unknown-src";
771-
final String finalFrameUrl = frameUrl;
772-
String percyElementId = frameElement.getAttribute("data-percy-element-id");
773-
log("processFrame: data-percy-element-id=\"" + percyElementId + "\" for src=\"" + finalFrameUrl + "\"", "debug");
774-
if (percyElementId == null || percyElementId.isEmpty()) {
775-
log("Skipping frame " + finalFrameUrl + ": no matching percyElementId found", "debug");
776-
return null;
777-
}
778-
779-
Map<String, Object> iframeSnapshot = null;
780-
try {
781-
driver.switchTo().frame(frameElement);
782-
JavascriptExecutor jse = (JavascriptExecutor) driver;
783-
// Inject Percy DOM into the cross-origin frame context
784-
jse.executeScript(domJs);
785-
// Post-switch URL re-check: about:blank / data: / javascript: targets
786-
// can slip through the parent-side `src` check (e.g. when the iframe
787-
// failed to load, or has been navigated by script after attach).
788-
String postSwitchUrl = readCurrentFrameUrl();
789-
if (postSwitchUrl != null && isUnsupportedIframeSrc(postSwitchUrl)) {
790-
log("Skipping iframe after switch: unsupported document.URL \"" + postSwitchUrl + "\"", "debug");
791-
return null;
792-
}
793-
// Serialize inside the frame; enableJavaScript=true is required for CORS iframes
794-
Map<String, Object> iframeOptions = new HashMap<>(options);
795-
iframeOptions.put("enableJavaScript", true);
796-
JSONObject optionsJson = new JSONObject(iframeOptions);
797-
iframeSnapshot = (Map<String, Object>) jse.executeScript(
798-
"return PercyDOM.serialize(" + optionsJson.toString() + ")"
799-
);
800-
} catch (Exception e) {
801-
log("Failed to process cross-origin frame " + finalFrameUrl + ": " + e.getMessage(), "error");
802-
throw new RuntimeException("Failed to process cross-origin frame " + finalFrameUrl, e);
803-
} finally {
804-
try {
805-
driver.switchTo().defaultContent();
806-
} catch (Exception err) {
807-
throw new FatalIframeException(
808-
"Could not exit iframe context after processing \"" + finalFrameUrl + "\". Driver may be unstable.", err
809-
);
810-
}
811-
}
812-
813-
Map<String, Object> iframeData = new HashMap<>();
814-
iframeData.put("percyElementId", percyElementId);
815-
816-
Map<String, Object> result = new HashMap<>();
817-
result.put("iframeData", iframeData);
818-
result.put("iframeSnapshot", iframeSnapshot);
819-
result.put("frameUrl", finalFrameUrl);
820-
return result;
821-
}
822-
823773
// Recursively process a cross-origin iframe tree. From the current driver
824774
// frame context, switch into `frameElement`, capture its DOM, enumerate
825775
// further cross-origin iframes nested inside it, and recurse. Steps back
@@ -877,6 +827,19 @@ private List<Map<String, Object>> processFrameTree(
877827
return collected;
878828
}
879829

830+
// Expose closed shadow roots inside this CORS frame's document
831+
// before serializing — mirrors the top-page behaviour so closed
832+
// shadow DOM inside cross-origin iframes is also captured.
833+
// CDP DOM.getDocument is invoked at depth=-1 with pierce=true, and
834+
// contentDocument subtrees are skipped during collection so this
835+
// remains scoped to the host element-level pairs visible from the
836+
// current driver session. Non-fatal — wrapped in try/catch so a
837+
// non-Chromium driver or restricted frame falls through silently.
838+
// TODO(closed-shadow-cors): drive per-frame CDP via flat sessions
839+
// (Target.setAutoAttach / sessionId) for deeper isolation when
840+
// Selenium 4 BiDi support stabilises across versions.
841+
try { exposeClosedShadowRoots(driver); } catch (Exception ignore) {}
842+
880843
Map<String, Object> iframeSnapshot = serializeCurrentFrame(options);
881844
String reportedUrl = (postSwitchUrl != null) ? postSwitchUrl : frameSrc;
882845

@@ -924,7 +887,11 @@ private List<Map<String, Object>> processFrameTree(
924887
continue;
925888
}
926889
// Compare to the IMMEDIATE PARENT origin, not the page origin.
927-
if (childOrigin.equals(currentOrigin)) continue;
890+
// Null-safe: getOrigin currently returns "" on parse failure, but a
891+
// child URI resolving to no host (e.g. data:, mailto:, schemeless)
892+
// could yield null in future refactors. Objects.equals avoids any
893+
// NPE escaping to the per-iframe catch.
894+
if (Objects.equals(childOrigin, currentOrigin)) continue;
928895

929896
try {
930897
List<Map<String, Object>> nested = processFrameTree(child, depth + 1, nextAncestors, ctx);
@@ -1025,7 +992,8 @@ private Map<String, Object> getSerializedDOM(JavascriptExecutor jse, Set<Cookie>
1025992
log("Skipping iframe \"" + frameSrc + "\": " + e.getMessage(), "debug");
1026993
continue;
1027994
}
1028-
if (frameOrigin.equals(pageOrigin)) continue;
995+
// Null-safe equality — see processFrameTree() for rationale.
996+
if (Objects.equals(frameOrigin, pageOrigin)) continue;
1029997

1030998
Set<String> ancestors = new HashSet<>();
1031999
if (pageUrl != null) ancestors.add(pageUrl);

0 commit comments

Comments
 (0)