fix(cc-widgets): remediate P0 security scan findings - #728
Conversation
- Scope CI secrets to only the e2e jobs that need them (pull-request.yml) - Replace execSync with execFileSync in publish.js to prevent shell injection - Validate branchName and workspace names before use as npm tags/args - Remove access token persistence in localStorage (samples apps) - Remove window['store'] debug exposure in samples-cc-react-app
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
akulakum
changed the title
akulakum
added
validated
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
widgets/widgets-samples/cc/samples-cc-react-app/src/App.tsx
Lines 124 to 125 in 003582d
Clear tokens left by previous sample versions
When a browser has run an earlier build, the existing localStorage.accessToken remains indefinitely because this load path stops reading and writing the key but never removes it. Consequently, an XSS on the same sample origin can still retrieve the credential, so the remediation only protects fresh users; clear the legacy key during startup in both this app and the Web Component load handler.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env: | ||
| PW_SANDBOX_PASSWORD: ${{ secrets.PW_SANDBOX_PASSWORD }} |
There was a problem hiding this comment.
This commit changes credential persistence, CI secret scoping, and the publish command's trust boundary without updating any module spec or standing document. That leaves the repository's security design and verification guidance out of sync with the implementation despite the mandatory same-change spec-currency rule; update the relevant standing security documentation alongside these controls.
AGENTS.md reference: AGENTS.md:L68-L68
Useful? React with 👍 / 👎.
akulakum
left a comment
akulakum
left a comment
There was a problem hiding this comment.
Approved for P0 sec-scan pilot. CI green; Amplify smoke-tested (manual token + Init Widgets, no token persist after refresh).
github-actions
Bot
removed
the
validated
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 678011ae0f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| env: | ||
| PW_SANDBOX_PASSWORD: ${{ secrets.PW_SANDBOX_PASSWORD }} |
There was a problem hiding this comment.
When a fork PR receives both the validated and run_e2e labels, this pull_request_target job checks out the PR head while these job-level secrets are available to every subsequent step. PR-controlled install scripts, Webpack configuration, build scripts, and Playwright code can therefore read and exfiltrate all PW_* credentials; moving them from workflow scope to job scope narrows exposure but does not satisfy AC-1's requirement to keep repository secrets out of PR-head execution. Run secret-bearing E2E work from trusted base code/artifacts rather than executing the checked-out head with repository secrets.
Useful? React with 👍 / 👎.
| @@ -29,10 +29,6 @@ import './App.scss'; | |||
| import {observer} from 'mobx-react-lite'; | |||
| import EngageWidget from './EngageWidget'; | |||
|
|
|||
There was a problem hiding this comment.
When playwright/suites/campaign-preview-tests.spec.ts is run against this React sample, removing the global store makes every helper in playwright/Utils/campaignPreviewUtils.ts fail at its explicit Store not found on window object guard. That suite injects campaign tasks and stubs SDK actions through window['store'], so the security change needs a replacement test-only control path or corresponding Playwright rewrite rather than only deleting the assignment.
Useful? React with 👍 / 👎.
| // Clear the hash from the URL to remove the token from browser history | ||
| window.history.replaceState({}, document.title, window.location.pathname + window.location.search); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
How are we verifying if this change doesn't break anything ?
| group: e2e-cc-widgets | ||
| cancel-in-progress: false | ||
| env: | ||
| PW_SANDBOX_PASSWORD: ${{ secrets.PW_SANDBOX_PASSWORD }} |
There was a problem hiding this comment.
Maybe we can have a common file from where these are exported and that can be used here in PR pipeline
| const validPackageNamePattern = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; | ||
|
|
||
| const publishWorkspace = (workspace) => { | ||
| if (!validPackageNamePattern.test(workspace)) { |
There was a problem hiding this comment.
Why are we validating this in 2 places ? Validating here should take care of it right
3 participants
COMPLETES SPARK-833336
This pull request addresses
Six P0 security findings reported in SPARK-833336: CI secrets exposed at workflow scope, command-injection risks in the publish tooling, access-token persistence in sample applications, and an internal MobX store exposed on
window.Root Cause:
pull-request.ymldeclared Playwright secrets as workflow-level environment variables, making them available to jobs that did not need them.publish.jsbuilt a shell command string from branch and workspace values without validation.localStorageand restored them on page load.window.Fix:
execSyncwithexecFileSyncand added validation for branch and workspace values.localStoragein both sample applications.window['store']assignment.by making the following changes
.github/workflows/pull-request.yml— scope Playwright secrets toe2e_test_meetingsande2e_test_cc_widgetstooling/src/publish.js— use an argument-vector process API and validate publish inputstooling/tests/publish.js— update process mocks and add injection-rejection testswidgets-samples/cc/samples-cc-react-app/src/App.tsx— remove access-token persistence and the global store assignmentwidgets-samples/cc/samples-cc-wc-app/app.js— remove access-token persistenceChange Type
The following scenarios were tested
yarn build:dev.yarn test:cc-widgets.The GAI Coding Policy And Copyright Annotation Best Practices
Acceptance Criteria
PW_*values moved from workflow scope to the two label-gated E2E jobslocalStorageApp.tsxlocalStorageapp.jsexecFileSyncargument vectors and input validation added; injection tests passwindowwindow['store'] = storeremoved fromApp.tsxyarn build:devpassed, andyarn test:cc-widgetspassedAcceptance criteria and recorded evidence were generated by the Jira-to-PR workflow. Human review is required to confirm semantic and production correctness.
Checklist before merging
Notes:
Jira: SPARK-833336