security: readiness config as script arg, cookie opt-out, per-thread Cucumber state, CI hardening (PER-8898, PER-8897, PER-8895, PER-8907)

.github/workflows/Semgrep.yml

@@ -34,6 +34,8 @@ jobs:
     steps:
       # Fetch project source with GitHub Actions Checkout.
       - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3
+        with:
+          persist-credentials: false
       # Run the "semgrep ci" command on the command line of the docker image.
       - run: semgrep ci --sarif --output=semgrep.sarif
         env:

.github/workflows/test.yml

@@ -23,7 +23,7 @@ jobs:
         if: ${{ github.event_name == 'workflow_dispatch' }}
         with:
           text: ${{ github.event.inputs.branch }}
-          regex: '^[a-zA-Z0-9_/\-]+$'
+          regex: '^[A-Za-z0-9][A-Za-z0-9._/-]*$'
       - name: Break on invalid branch name
         run: exit 1
         if: ${{ github.event_name == 'workflow_dispatch' && steps.regex-match.outputs && steps.regex-match.outputs.match == '' }}
@@ -35,7 +35,7 @@ jobs:
           java-version: ${{ matrix.java }}
       - name: Get yarn cache directory path
         id: yarn-cache-dir-path
-        run: echo "::set-output name=dir::$(yarn cache dir)"
+        run: echo "dir=$(yarn cache dir)" >> "$GITHUB_OUTPUT"
       - uses: actions/cache@6f8efc29b200d32929f49075959781ed54ec270c # v3
         with:
           path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
@@ -47,6 +47,10 @@ jobs:
         env:
           CLI_BRANCH: ${{ github.event.inputs.branch }}
         run: |
+          if ! [[ "$CLI_BRANCH" =~ ^[A-Za-z0-9][A-Za-z0-9._/-]*$ ]] || [[ "$CLI_BRANCH" == *..* ]]; then
+            echo "Invalid branch name" >&2
+            exit 1
+          fi
           cd /tmp
           git clone --branch "$CLI_BRANCH" --depth 1 https://github.com/percy/cli
           cd cli
@@ -55,7 +59,7 @@ jobs:
           yarn
           yarn build
           yarn global:link
-          cd ${{ github.workspace }} 
+          cd "$GITHUB_WORKSPACE"
           yarn remove @percy/cli && yarn link `echo $PERCY_PACKAGES`
           npx percy --version
       - run: MOZ_HEADLESS=1 npx percy exec -- mvn test

README.md

@@ -107,6 +107,13 @@ The snapshot method arguments:
   - `sync` - For getting syncronous results https://www.browserstack.com/docs/percy/advanced/sync-comparison-results
   - `responsiveSnapshotCapture` - For capturing snapshot of responsive websites
 
+### Cookies
+
+By default the SDK sends the browser's cookies to the Percy CLI so that pages and assets behind a
+logged-in session load during asset discovery. To stop sending them, set
+`PERCY_DO_NOT_USE_CAPTURED_COOKIES=true`. With this set, assets that need a session may fail to load
+and snapshots can render incorrectly; the SDK logs a warning once when the option is active.
+
 
 ## Upgrading
 

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

@@ -18,6 +18,7 @@
 import java.time.Duration;
 import java.util.*;
 import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 import javax.annotation.Nullable;
 
@@ -55,6 +56,12 @@ public class Percy {
 
     private static boolean PERCY_RESPONSIVE_CAPTURE_RELOAD_PAGE = Boolean.parseBoolean(System.getenv().getOrDefault("PERCY_RESPONSIVE_CAPTURE_RELOAD_PAGE", "false").toLowerCase());
 
+    // Opt-out of forwarding browser cookies to the CLI (same knob the CLI honours)
+    private static boolean PERCY_DO_NOT_USE_CAPTURED_COOKIES = "true".equalsIgnoreCase(
+        System.getenv().getOrDefault("PERCY_DO_NOT_USE_CAPTURED_COOKIES", "false").trim());
+
+    private static final AtomicBoolean cookieOptOutWarned = new AtomicBoolean(false);
+
     private static boolean PERCY_RESPONSIVE_CAPTURE_MIN_HEIGHT = Boolean.parseBoolean(System.getenv().getOrDefault("PERCY_RESPONSIVE_CAPTURE_MIN_HEIGHT", "false").toLowerCase());
     private static final int WIDTHS_CONFIG_TIMEOUT_MS = 30000;
     // for logging
@@ -461,12 +468,7 @@ public JSONObject snapshot(String name, Map<String, Object> options) {
         try {
             JavascriptExecutor jse = (JavascriptExecutor) driver;
             jse.executeScript(fetchPercyDOM());
-            Set<Cookie> cookies = new HashSet<>();
-            try {
-                cookies = driver.manage().getCookies();
-            } catch(Exception e) {
-                log("Cookie collection failed " + e.getMessage(), "debug");
-            }
+            Set<Cookie> cookies = collectCookies();
             if (isCaptureResponsiveDOM(mergedOptions)) {
                 domSnapshot = captureResponsiveDom(driver, cookies, mergedOptions);
             } else {
@@ -709,6 +711,35 @@ private String buildSnapshotJS(Map<String, Object> options) {
         return jsBuilder.toString();
     }
 
+    /**
+     * Collects the browser cookies the CLI replays during asset discovery.
+     * Returns an empty set when PERCY_DO_NOT_USE_CAPTURED_COOKIES=true, and
+     * never throws.
+     */
+    private Set<Cookie> collectCookies() {
+        if (isCookieForwardingDisabled()) {
+            return new HashSet<>();
+        }
+        try {
+            return driver.manage().getCookies();
+        } catch (Exception e) {
+            log("Cookie collection failed " + e.getMessage(), "debug");
+            return new HashSet<>();
+        }
+    }
+
+    private static boolean isCookieForwardingDisabled() {
+        if (!PERCY_DO_NOT_USE_CAPTURED_COOKIES) {
+            return false;
+        }
+        if (cookieOptOutWarned.compareAndSet(false, true)) {
+            log("Not sending browser cookies to Percy because PERCY_DO_NOT_USE_CAPTURED_COOKIES is set. "
+                + "Pages or assets that need a logged-in session may fail to load during asset discovery, "
+                + "so snapshots can render incorrectly.", "warn");
+        }
+        return true;
+    }
+
     /**
      * Readiness gate: runs PercyDOM.waitForReady BEFORE serialize.
      *
@@ -747,15 +778,17 @@ protected Object waitForReady(JavascriptExecutor jse, Map<String, Object> option
             }
         }
         try {
+            // The config is passed as a script argument rather than spliced
+            // into the script source, so its values can never become code.
             String script =
-                "var cfg = " + readinessConfig.toString() + ";"
+                "var cfg = arguments[0];"
                 + "var done = arguments[arguments.length - 1];"
                 + "try {"
                 + "  if (typeof PercyDOM !== 'undefined' && typeof PercyDOM.waitForReady === 'function') {"
                 + "    PercyDOM.waitForReady(cfg).then(function(r){ done(r); }).catch(function(){ done(); });"
                 + "  } else { done(); }"
                 + "} catch (e) { done(); }";
-            return jse.executeAsyncScript(script);
+            return jse.executeAsyncScript(script, readinessConfig.toMap());
         } catch (Exception e) {
             log("waitForReady failed, proceeding to serialize: " + e.getMessage(), "debug");
             return null;
@@ -847,6 +880,11 @@ private Map<String, Object> processFrame(WebElement frameElement, Map<String, Ob
             iframeSnapshot = (Map<String, Object>) jse.executeScript(
                 "return PercyDOM.serialize(" + optionsJson.toString() + ")"
             );
+            // PercyDOM.serialize captures the frame's document.cookie; drop it when opted out.
+            if (iframeSnapshot != null && isCookieForwardingDisabled()) {
+                iframeSnapshot = new HashMap<>(iframeSnapshot);
+                iframeSnapshot.remove("cookies");
+            }
         } catch (Exception e) {
             log("Failed to process cross-origin frame " + finalFrameUrl + ": " + e.getMessage(), "error");
             throw new RuntimeException("Failed to process cross-origin frame " + finalFrameUrl, e);
@@ -876,7 +914,7 @@ Map<String, Object> getSerializedDOM(JavascriptExecutor jse, Set<Cookie> cookies
 
         Map<String, Object> domSnapshot = (Map<String, Object>) jse.executeScript(buildSnapshotJS(options));
         Map<String, Object> mutableSnapshot = new HashMap<>(domSnapshot);
-        mutableSnapshot.put("cookies", cookies);
+        mutableSnapshot.put("cookies", isCookieForwardingDisabled() ? Collections.<Cookie>emptySet() : cookies);
 
         // Attach readiness diagnostics so the CLI can log timing and pass/fail
         if (readinessDiagnostics != null) {