fix: address PR review findings in CORS iframe + shadow DOM capture

aryanku-dev · claude · aryanku-dev · commit c7ffcb1c46b7 · 2026-06-26T19:27:28.000+05:30
- Skip CORS frame when PercyDOM.serialize returns null instead of
  emitting a poisoned snapshot entry (mirror getSerializedDOM guard) [High]
- Guard exposeClosedShadowRoots with isCdpSupported before CDP calls [Med]
- Honour single-string ignoreIframeSelectors CLI config value [Med]
- Pass ignore selectors as executeScript argument, not interpolated [Med]
- Add post-switch absolute-URL cycle check for relative-src cycles [Med]
- Remove unused CSS/XPath imports [Low]
- Add tests for null-snapshot skip, resolved-URL cycle guard, single-string CLI

Co-Authored-By: Claude Opus 4.8 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/src/main/java/io/percy/selenium/Percy.java b/src/main/java/io/percy/selenium/Percy.java
@@ -29,9 +29,6 @@
 
 import java.util.stream.Collectors;
 
-import javax.swing.text.html.CSS;
-import javax.xml.xpath.XPath;
-
 /**
  * Percy client for visual testing.
  */
@@ -803,6 +800,10 @@ private List<String> resolveIgnoreSelectors(Map<String, Object> options) {
                     for (int i = 0; i < arr.length(); i++) out.add(arr.opt(i));
                     return normalizeIgnoreSelectors(out);
                 }
+                // A single string value (e.g. "ignoreIframeSelectors": "iframe.ads")
+                // isn't a JSONArray; normalize it the same way the options-map path does.
+                String single = snap.optString("ignoreIframeSelectors", null);
+                if (single != null) return normalizeIgnoreSelectors(single);
             }
         }
         return Collections.emptyList();
@@ -815,12 +816,14 @@ private boolean iframeMatchesIgnoreSelector(WebElement iframe, List<String> sele
         if (selectors == null || selectors.isEmpty()) return false;
         try {
             JavascriptExecutor jse = (JavascriptExecutor) driver;
-            JSONArray sel = new JSONArray(selectors);
-            String script = "var el = arguments[0]; var selectors = " + sel.toString() + ";"
+            // Pass the selector list as a script argument rather than interpolating
+            // it into the source — a selector containing a quote or control char
+            // would otherwise break the script literal and throw.
+            String script = "var el = arguments[0]; var selectors = arguments[1];"
                 + "for (var i = 0; i < selectors.length; i++) {"
                 + "  try { if (el.matches(selectors[i])) return true; } catch (e) {}"
                 + "} return false;";
-            Object res = jse.executeScript(script, iframe);
+            Object res = jse.executeScript(script, iframe, selectors);
             return res instanceof Boolean && (Boolean) res;
         } catch (Exception e) {
             return false;
@@ -876,10 +879,16 @@ private Map<String, Object> serializeCurrentFrame(Map<String, Object> options) {
         Map<String, Object> iframeOptions = new HashMap<>(options == null ? Collections.emptyMap() : options);
         iframeOptions.put("enableJavaScript", true);
         JSONObject optionsJson = new JSONObject(iframeOptions);
-        @SuppressWarnings("unchecked")
-        Map<String, Object> snapshot = (Map<String, Object>) jse.executeScript(
+        Object raw = jse.executeScript(
             "return PercyDOM.serialize(" + optionsJson.toString() + ")"
         );
+        // PercyDOM.serialize yields null (or a non-object) when @percy/dom failed
+        // to load inside this frame — e.g. a sandboxed CORS document. Return null
+        // so the caller can skip the frame instead of letting a poisoned snapshot
+        // through, mirroring the main-page guard in getSerializedDOM.
+        if (!(raw instanceof Map)) return null;
+        @SuppressWarnings("unchecked")
+        Map<String, Object> snapshot = (Map<String, Object>) raw;
         return snapshot;
     }
 
@@ -940,6 +949,17 @@ private List<Map<String, Object>> processFrameTree(
                 return collected;
             }
 
+            // Absolute-URL cycle re-check. The pre-switch guard above compares the
+            // raw `src` attribute, which can be relative; a cycle expressed as a
+            // relative src at one level and an absolute URL at another would slip
+            // past it. document.URL here is the resolved absolute URL, and the
+            // ancestor set is seeded with each level's absolute reportedUrl, so
+            // this catches the cycle reliably regardless of how src was written.
+            if (ancestorUrls != null && postSwitchUrl != null && ancestorUrls.contains(postSwitchUrl)) {
+                log("Skipping cyclic iframe (resolved URL " + postSwitchUrl + " appears in ancestor chain)", "debug");
+                return collected;
+            }
+
             // Expose closed shadow roots inside this CORS frame's document
             // before serializing — mirrors the top-page behaviour so closed
             // shadow DOM inside cross-origin iframes is also captured.
@@ -956,6 +976,15 @@ private List<Map<String, Object>> processFrameTree(
             Map<String, Object> iframeSnapshot = serializeCurrentFrame(options);
             String reportedUrl = (postSwitchUrl != null) ? postSwitchUrl : frameSrc;
 
+            // serializeCurrentFrame returns null when PercyDOM.serialize did not
+            // produce a DOM object (script failed to load in this frame). Skip the
+            // frame rather than emitting an entry with a null snapshot, which the
+            // CLI would otherwise have to defend against.
+            if (iframeSnapshot == null) {
+                log("PercyDOM.serialize returned null inside CORS iframe " + reportedUrl + "; skipping", "warn");
+                return collected;
+            }
+
             Map<String, Object> iframeData = new HashMap<>();
             iframeData.put("percyElementId", percyElementId);
             Map<String, Object> entry = new HashMap<>();
@@ -1159,6 +1188,10 @@ private void exposeClosedShadowRoots(WebDriver driver) {
         if (!(driver instanceof ChromeDriver)) return;
         ChromeDriver chrome;
         try { chrome = (ChromeDriver) driver; } catch (ClassCastException e) { return; }
+        // Reuse the same CDP availability guard as changeWindowDimensionAndWait so
+        // ChromeDriver builds/subclasses without executeCdpCommand bail cleanly
+        // rather than relying on the catch below to absorb a NoSuchMethodError.
+        if (!isCdpSupported(chrome)) return;
         boolean domEnabled = false;
         try {
             chrome.executeCdpCommand("DOM.enable", new HashMap<>());
diff --git a/src/test/java/io/percy/selenium/IframeFeatureTest.java b/src/test/java/io/percy/selenium/IframeFeatureTest.java
@@ -108,6 +108,21 @@ public void resolveIgnoreSelectorsCoercesOptionAndCliConfig() throws Exception {
     assertEquals(Arrays.asList("iframe.cli-only"), fromCli);
   }
 
+  @Test
+  public void resolveIgnoreSelectorsAcceptsSingleStringCliConfig() throws Exception {
+    RemoteWebDriver mockedDriver = mock(RemoteWebDriver.class);
+    Percy percy = new Percy(mockedDriver);
+    // A scalar string (not an array) in CLI config must still be honoured.
+    setField(percy, "cliConfig",
+      new JSONObject().put("snapshot",
+        new JSONObject().put("ignoreIframeSelectors", "iframe.ads")));
+
+    @SuppressWarnings("unchecked")
+    List<String> fromCli = (List<String>) invokePrivate(percy, "resolveIgnoreSelectors", new Class[]{Map.class}, new HashMap<>());
+    assertEquals(Arrays.asList("iframe.ads"), fromCli,
+      "A single-string ignoreIframeSelectors CLI config must not be dropped");
+  }
+
   @Test
   public void skipsIframeMarkedWithDataPercyIgnore() throws Exception {
     RemoteWebDriver mockedDriver = mock(RemoteWebDriver.class);
@@ -215,6 +230,83 @@ public void processFrameTreeSkipsAfterSwitchWhenDocumentUrlIsUnsupported() throw
     assertTrue(result.isEmpty(), "Frame must be skipped when document.URL is unsupported after switch");
   }
 
+  @Test
+  public void processFrameTreeSkipsWhenSerializeReturnsNull() throws Exception {
+    RemoteWebDriver mockedDriver = mock(RemoteWebDriver.class);
+    Percy percy = spy(new Percy(mockedDriver));
+    setField(percy, "domJs", "window.PercyDOM = window.PercyDOM || {};");
+
+    WebElement iframe = mock(WebElement.class);
+    when(iframe.getAttribute("src")).thenReturn("https://cdn.other.com/frame");
+    when(iframe.getAttribute("data-percy-element-id")).thenReturn("frame-null");
+
+    TargetLocator targetLocator = mock(TargetLocator.class);
+    when(mockedDriver.switchTo()).thenReturn(targetLocator);
+    when(targetLocator.frame(iframe)).thenReturn(mockedDriver);
+    when(targetLocator.defaultContent()).thenReturn(mockedDriver);
+    when(targetLocator.parentFrame()).thenReturn(mockedDriver);
+
+    // dom.js inject -> null; document.URL -> supported; PercyDOM.serialize -> null
+    // (e.g. @percy/dom failed to load in the frame). The frame must be skipped, not
+    // emitted with a null snapshot.
+    when(((JavascriptExecutor) mockedDriver).executeScript(any(String.class))).thenAnswer(invocation -> {
+      String script = invocation.getArgument(0);
+      if (script.startsWith("return PercyDOM.serialize(")) return null;
+      if (script.equals("return document.URL")) return "https://cdn.other.com/frame";
+      return null;
+    });
+
+    Map<String, Object> ctx = new HashMap<>();
+    ctx.put("options", new HashMap<String, Object>());
+    ctx.put("maxFrameDepth", 5);
+    ctx.put("ignoreSelectors", java.util.Collections.<String>emptyList());
+
+    @SuppressWarnings("unchecked")
+    List<Map<String, Object>> result = (List<Map<String, Object>>) invokePrivate(
+      percy, "processFrameTree",
+      new Class[]{WebElement.class, int.class, Set.class, Map.class},
+      iframe, 1, new HashSet<String>(), ctx);
+    assertTrue(result.isEmpty(), "Frame must be skipped when PercyDOM.serialize returns null");
+  }
+
+  @Test
+  public void processFrameTreeSkipsCyclicFrameByResolvedUrl() throws Exception {
+    RemoteWebDriver mockedDriver = mock(RemoteWebDriver.class);
+    Percy percy = spy(new Percy(mockedDriver));
+    setField(percy, "domJs", "window.PercyDOM = window.PercyDOM || {};");
+
+    // Relative src slips past the pre-switch raw-src cycle guard; the post-switch
+    // absolute document.URL matches an ancestor, so the cycle must still be caught.
+    WebElement iframe = mock(WebElement.class);
+    when(iframe.getAttribute("src")).thenReturn("/frame");
+    when(iframe.getAttribute("data-percy-element-id")).thenReturn("frame-cyc");
+
+    TargetLocator targetLocator = mock(TargetLocator.class);
+    when(mockedDriver.switchTo()).thenReturn(targetLocator);
+    when(targetLocator.frame(iframe)).thenReturn(mockedDriver);
+    when(targetLocator.defaultContent()).thenReturn(mockedDriver);
+    when(targetLocator.parentFrame()).thenReturn(mockedDriver);
+
+    when(((JavascriptExecutor) mockedDriver).executeScript(any(String.class)))
+      .thenReturn(null)
+      .thenReturn("https://cdn.other.com/frame");
+
+    Map<String, Object> ctx = new HashMap<>();
+    ctx.put("options", new HashMap<String, Object>());
+    ctx.put("maxFrameDepth", 5);
+    ctx.put("ignoreSelectors", java.util.Collections.<String>emptyList());
+
+    Set<String> ancestors = new HashSet<>();
+    ancestors.add("https://cdn.other.com/frame");
+
+    @SuppressWarnings("unchecked")
+    List<Map<String, Object>> result = (List<Map<String, Object>>) invokePrivate(
+      percy, "processFrameTree",
+      new Class[]{WebElement.class, int.class, Set.class, Map.class},
+      iframe, 1, ancestors, ctx);
+    assertTrue(result.isEmpty(), "Cyclic frame must be skipped when its resolved URL is already an ancestor");
+  }
+
   @Test
   public void percyContextLostExceptionCarriesPartialCapture() throws Exception {
     RemoteWebDriver mockedDriver = mock(RemoteWebDriver.class);
