fix(percy): replace runPercyScan token interpolation with placeholder (PMAA-103)#288
Open
manoj-k04 wants to merge 1 commit intobrowserstack:mainfrom
Open
fix(percy): replace runPercyScan token interpolation with placeholder (PMAA-103)#288manoj-k04 wants to merge 1 commit intobrowserstack:mainfrom
manoj-k04 wants to merge 1 commit intobrowserstack:mainfrom
Conversation
The Percy token fetched from api.browserstack.com was being interpolated verbatim into runPercyScan tool output (and the percy-web / percy-automate SDK handlers), exposing a server-side credential across a trust boundary into MCP client / AI-assistant context (HackerOne #3576387, CVSS 6.5). PMAA-100 (PR browserstack#285) mitigated this by disabling the runPercyScan tool registration. This change removes the underlying leak so the tool can be safely re-enabled. Output sanitization - run-percy-scan.ts: generatePercyTokenInstructions returns placeholder-only text and points users to the Percy dashboard. The token is still fetched (preserves the existing project-validation side effect of fetchPercyToken) but is intentionally not echoed. - percy-web/handler.ts and percy-automate/handler.ts: same treatment in runPercyWeb and runPercyAutomateOnly. Parameter signatures are kept for upstream compatibility; SECURITY comments document why the token must not be interpolated. Tool re-enabled - percy-sdk.ts: restore the runPercyScan import, schema import, and tool registration block that PMAA-100 commented out. - percySdk.test.ts: restore the matching import, mock, name assertion, and "runPercyScan - SUCCESS" test. Regression coverage - runPercyScan.test.ts: assertions inverted — output must NOT contain the fetched token; the placeholder must be present. - percyTokenLeak.test.ts (new): pin the no-leak contract on runPercyWeb and runPercyAutomateOnly directly so any future regression fails CI. Refs: HackerOne #3576387, PMAA-100, PMAA-103 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
manoj-k04
force-pushed
the
PMAA-103-fix-percy-token-leak
branch
from
330e92c to
72f0a9f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
runPercyScantool output (HackerOne #3576387, CVSS 6.5) by replacing token interpolation with placeholder-only instructions.runPercyScanregistration that PMAA-100 (fix(percy): disable runPercyScan #285) had to comment out as a temporary mitigation.Refs: HackerOne #3576387, PMAA-100 / #285, PMAA-103.
Background
The token fetched from
api.browserstack.com/api/app_percy/get_project_tokenwas being interpolated verbatim into MCP tool output asexport PERCY_TOKEN="<actual_token>". Because MCP responses are consumed by AI assistants (Claude Desktop, Cursor, etc.), any path that could trigger the tool — prompt injection, malicious MCP client, compromised assistant session — would exfiltrate a valid Percy credential across a trust boundary.PR #285 mitigated the issue by disabling the
runPercyScanregistration entirely. This PR delivers the proper fix so the tool can be turned back on.Changes
Output sanitization (the actual fix)
src/tools/run-percy-scan.ts—generatePercyTokenInstructionsreturns placeholder text only (<your Percy project token>) and points users at the Percy dashboard. The token is still fetched sofetchPercyToken's project-validation side effect is preserved, but is intentionally not echoed.src/tools/sdk-utils/percy-web/handler.ts— same treatment inrunPercyWeb. Parameter kept for upstream compatibility, with aSECURITY:comment.src/tools/sdk-utils/percy-automate/handler.ts— same treatment inrunPercyAutomateOnly; replaces theHere is percy token if required {${percyToken}}line.Re-enable the tool (revert PMAA-100 disable)
src/tools/percy-sdk.ts— restorerunPercyScanimport,RunPercyScanParamsShapeimport, and the tool registration block.tests/tools/percySdk.test.ts— restore matching import, mock, name assertion, andrunPercyScan - SUCCESStest.Regression coverage
tests/tools/runPercyScan.test.ts— assertions inverted: output must not contain the fetched token, and must contain the placeholder.tests/tools/percyTokenLeak.test.ts(new) — pins the no-leak contract onrunPercyWebandrunPercyAutomateOnlydirectly.Test plan
npx tsc— clean.npx vitest run— 129/129 pass (4 percySdk tests restored from PMAA-100 disable; 2 new tests inpercyTokenLeak.test.ts).main+ re-enable, no sanitization):outputContainsSecret: true— the fake token appeared 3× inPERCY_TOKENlines (export,setx,set).outputContainsSecret: false— allPERCY_TOKENlines show the<your Percy project token>placeholder.expandPercyVisualTesting) still produces usable instructions despite the token being absent — users now retrieve their token from the Percy dashboard, which matches the public Percy onboarding flow.🤖 Generated with Claude Code