Repository navigation
security: readiness config as script arg, cookie opt-out, per-thread Cucumber state, CI hardening (PER-8898, PER-8897, PER-8895, PER-8907) - #326
Conversation
…read cucumber state, CI hardening - waitForReady passes the merged readiness config to executeAsyncScript as arguments[0] instead of splicing it into the script source. - Honour PERCY_DO_NOT_USE_CAPTURED_COOKIES=true: send no browser cookies (including cookies serialized inside cross-origin iframes) and warn once. Cookies are still forwarded by default for asset discovery. - PercySteps keeps driver/Percy/regions per thread so parallel Cucumber scenarios don't overwrite each other; threads that never call setDriver fall back to the last driver set, as before. - test.yml: stricter dispatch branch pattern, re-validated in the shell step, $GITHUB_OUTPUT instead of set-output; Semgrep checkout without persisted credentials. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Caution Review failedFailed to post review comments. GitHub was unavailable or timed out while CodeRabbit was posting the review. Please request a new review later if the pull request still needs one. This happened while posting 4 inline comments. Use
 |
| Layer / File(s) | Summary |
|---|---|
Cookie collection and serialization src/main/java/io/percy/selenium/Percy.java, README.md, src/test/java/io/percy/selenium/PercyDriverPathTest.java, src/test/java/io/percy/selenium/PercyLogicTest.java |
The SDK can omit cookies from serialized DOM and iframe snapshots when PERCY_DO_NOT_USE_CAPTURED_COOKIES=true. It skips cookie retrieval in this mode and logs the warning once. Tests cover default forwarding and opt-out behavior. |
Readiness script argument src/main/java/io/percy/selenium/Percy.java, src/test/java/io/percy/selenium/PercyDriverPathTest.java, src/test/java/io/percy/selenium/PercyLogicTest.java, src/test/java/io/percy/selenium/SdkTest.java |
waitForReady passes readiness configuration as an argument to executeAsyncScript instead of embedding it in the script. Readiness tests use the updated call signature. |
Cucumber thread-local state
| Layer / File(s) | Summary |
|---|---|
Session and region handling src/main/java/io/percy/selenium/cucumber/PercySteps.java, src/test/java/io/percy/selenium/cucumber/PercyStepsTest.java |
Cucumber steps use the current thread’s session and region list, with a shared session fallback. Tests check session selection and region isolation across threads. |
Workflow input and checkout changes
| Layer / File(s) | Summary |
|---|---|
Checkout and branch validation .github/workflows/Semgrep.yml, .github/workflows/test.yml |
The Semgrep checkout disables persisted credentials. The test workflow validates branch names and rejects .. before cloning. Yarn output uses GITHUB_OUTPUT, and the workflow returns to the workspace through the quoted GITHUB_WORKSPACE variable. |
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix · Severity of issue fixed: High
Possibly related PRs
- percy/percy-selenium-java#322: This PR continues the branch-input hardening for the workflow-dispatch CLI branch already introduced there.
Suggested reviewers: neha-sanse
Pre-merge checks: 
-
Check name Status Explanation Resolution Linked Issues check The PR implements the argument-based waitForReadycall for PER-8898, but it does not implement the issue's required strict schema validation for healthcheck readiness data. For [PER-8897], the def…For PER-8898, validate the healthcheck response with a strict readiness schema before storing or merging it. For [PER-8897], remove cookie values from the default payload, filter sensitive cookies, or protect the SDK-to-CLI channel with m… -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title contains the Jira ticket IDs PER-8898, PER-8897, PER-8895, and PER-8907, which match the required pattern. It also gives a plain-English description of the main changes: readiness config pas… Out of Scope Changes check The changed files map to the four directly linked issue objectives. The readiness argument change, cookie handling and documentation, Cucumber state changes, related tests, and workflow hardening supp… Full details: Title check
-
Explanation
The title contains the Jira ticket IDs PER-8898, PER-8897, PER-8895, and PER-8907, which match the required pattern. It also gives a plain-English description of the main changes: readiness config passed as a script argument, a cookie opt-out, per-thread Cucumber state, and CI hardening. The changeset matches each of these parts, so the title meets the requirement.
Full details: Out of Scope Changes check
-
Explanation
The changed files map to the four directly linked issue objectives. The readiness argument change, cookie handling and documentation, Cucumber state changes, related tests, and workflow hardening support those objectives. No unrelated change is demonstrated by the reviewed diff.
-
Full details: Linked Issues check
-
Explanation
The PR implements the argument-based
waitForReadycall for PER-8898, but it does not implement the issue's required strict schema validation for healthcheck readiness data. For [PER-8897], the default path still sends the full cookie jar, including cookie values. The opt-out does not filter cookies by default and does not add an authenticated channel. For [PER-8895],PercySteps.reset()still sets the process-widesharedSessionto null, so one thread can remove the fallback session used by other threads. For [PER-8907],.github/workflows/test.ymlstill clones and buildsgit clone --branch "$CLI_BRANCH"from workflow input. Regex validation and the..check do not restrict the branch to a vetted SHA or remove the arbitrary branch override.Resolution
For PER-8898, validate the healthcheck response with a strict readiness schema before storing or merging it. For [PER-8897], remove cookie values from the default payload, filter sensitive cookies, or protect the SDK-to-CLI channel with mutual authentication. For [PER-8895], make reset state thread-local, including the fallback behavior, without clearing another thread's session. For [PER-8907], remove the workflow-dispatch branch override or resolve it only to a vetted immutable SHA in a protected workflow.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Summary
waitForReadynow passes the merged readiness config toexecuteAsyncScriptasarguments[0]instead of building it into the script source, the same change made in the webdriverio and puppeteer SDKs.PERCY_DO_NOT_USE_CAPTURED_COOKIES=true(the same variable the CLI reads) now makes the SDK send no cookies. That covers the cookie jar and thedocument.cookiethatPercyDOM.serializecaptures inside cross-origin iframes. The SDK logs a warning once per process explaining the impact. The README documents the option.PercyStepskeeps the driver, thePercyinstance and the regions per thread, so parallel Cucumber scenarios no longer overwrite each other. A thread that never callssetDriverstill uses the last driver set, as it did before. The public API is unchanged.test.ymluses a stricter pattern for the dispatch branch and checks it again inside the shell step. It also writes step output through$GITHUB_OUTPUTand uses$GITHUB_WORKSPACEinstead of an inline expression. The Semgrep checkout no longer persists credentials. Earlier hardening (pinned actions, read-only token, input passed through env) landed in security: harden CI workflows, deps, cache and log body (PER-8891/8892/8896/8890/8889/8901/8900/8911) #322.All error paths stay fail-open.
Tests
npx percy exec --testing -- mvn testwith headless Firefox: 169/169 pass and the JaCoCo coverage check passes. A live snapshot loggedReadiness passed, which confirms the argument-based script works in a real browser.🤖 Generated with Claude Code
Summary by CodeRabbit