Harden maven-workflow-run.yml (pin actions, validate commit_sha, scope token+secrets) — APS security-ai-review - #22
Closed
Rohannagariya1 wants to merge 1 commit into
Conversation
Pin actions to commit SHAs, validate commit_sha (40-hex gate job), scope GITHUB_TOKEN (contents:read, checks:write), move BrowserStack secrets to step-level env, persist-credentials:false on checkout. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Author
|
Closing — out of scope. |
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
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.
What & why
Hardens
.github/workflows/maven-workflow-run.ymlagainst the GitHub-Actions findings raised by BrowserStack's security-ai-review scan (umbrella APPSEC-409). This is CI/workflow-only — no change to any runtime code, the SDK, or the sample tests, so it cannot affect BrowserStack session behaviour.Jira: APS-19434 (chain C-001) and the component/chain tickets it closes — see below.
Changes
@v3tags):actions/checkout→a37ce91…(v3.7.0)actions/setup-java→e9fbacd…(v3.14.2)actions/github-scriptwas already SHA-pinned.)GITHUB_TOKEN: added a top-levelpermissions:block (contents: read,checks: write). Previously no block was declared, so the token defaulted to write-all.checks: writeis retained because the twogithub-scriptsteps callchecks.create.commit_sha: newvalidate-inputgate job (permissions: {}) that rejects anything not matching^[0-9a-f]{40}$before any checkout/build runs. The value is read from the environment and matched with a bash regex — never interpolated into a shell command — so the check itself is injection-safe.comment-rungates on it vianeeds:. (Single pre-job instead of an in-job step so the 3×3 matrix doesn't spin up 9 runners for a bad input.)BROWSERSTACK_USERNAME/BROWSERSTACK_ACCESS_KEYmoved from the job-levelenv:(visible to every step) to step-levelenv:on the threemvnsteps only.persist-credentials: falseon checkout so the token isn't left in.git/configfor later steps.Honest scoping (what this does and does not fix)
${{ … }}→run:expression-injection sink in this workflow (the input only reachescheckout'sref:andgithub-script'senv:/process.env, which is the safe pattern). The real exposure is checkout of an unvalidated arbitrary ref; the regex gate constrains that to a real 40-hex SHA. It does not remove the inherent risk that aworkflow_dispatchactor (who already needs repo write) can point it at a valid SHA of a malicious commit whose Maven plugins then execute on the runner — that is intrinsic to a "build an arbitrary commit" workflow. Reviewers who want to close that residual should gate this workflow behind a GitHub Environment with required reviewers (a repo setting, intentionally not done here to avoid changing the sample's UX).checks: writeis required by the workflow, so a writer can still create a check run for an arbitrary SHA. Droppingcontentstoreadis the correct in-scope mitigation; the complete control is a repo setting — this workflow's check must not be configured as a required status check in branch protection.LATEST(F-002 / C-002): handled separately as By-Design —LATESTis BrowserStack's documented, recommended way to declarebrowserstack-java-sdk, and this sample exists to demonstrate that pattern. Those two tickets are being closed Won't-Do with that rationale rather than pinned here.Testing
CI-config change;
workflow_dispatchcan only be exercised after merge. Verified by: (a)yaml.safe_loadparses clean, both jobs present, top-level permissions applied; (b) both pinned SHAs independently resolved to their v3.7.0 / v3.14.2 tags via the GitHub API; (c) the regex gate reviewed for injection-safety (env-read + bash[[ =~ ]], rejects multiline). No behavioural change to the Maven test steps themselves.Tickets closed by this PR
APS-19434 (C-001), APS-19433 (C-003), APS-19441 (C-004), APS-19439 (F-003), APS-19446 (F-004), APS-19440 (F-001), APS-19438 (F-005). APS-19444 (F-002) & APS-19442 (C-002) closed separately as By-Design.