[APS-19017] fix: pin Semgrep CI image to sha256 digest - #1191
Open
Raghav11-11 wants to merge 3 commits into
Open
Raghav11-11 wants to merge 3 commits into
Raghav11-11 wants to merge 3 commits into
Conversation
…mage, add files allowlist [APS-19017] INF-005: remove mocha from dependencies (kept in devDependencies); CLI loads mocha from user project via requireModule(), prod copy unnecessary. npm ls mocha --omit=dev now empty. INF-007: pin Semgrep CI image returntocorp/semgrep -> @sha256:f4791a54c891eabe1188248135574e6e03dfc31dfd3f3b747c7bec7079bfed1b (latest as of 2026-06-15). INF-008: add package.json files allowlist [bin/, README.md, LICENSE.md] so npm pack no longer ships .github/, CODEOWNERS, .nycrc.yml, test/. Verified via npm pack --dry-run. NOT applied: CSL-003 md5->sha256 (constants.js) — behavioral change to upload-dedup hash; needs human sign-off. Resolves: APS-19017 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…ride; mocha bundling load-bearing for reporter/index.js:12 require
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited) Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
jasbir-browserstack
approved these changes
Sep 22, 2026
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.
Security Fix: APS-19017 (Low, supply-chain hygiene)
Single-item PR — pins the Semgrep CI image from a mutable tag to its immutable digest. The other four items in APS-19017 were reviewed and are not included in this PR — reasoning at the bottom.
Actual diff
.github/workflows/Semgrep.yml:Why (INF-007)
A version tag (
:1.166.0) can be re-published by whoever owns the Docker Hub account at any time. Attack chain:returntocorp/semgrep:1.166.0with malicious layers added.browserstack-cypress-cli.A sha256 digest is cryptographically immutable — Docker refuses to pull anything whose bytes don't hash to
c180f0c9.... Even if Return-to-corp's account is compromised and the tag is re-published, Docker rejects the pull unless the digest matches the specific bytes we pinned to today.Verification
Other APS-19017 items — NOT in this PR
INF-001 (
axios≥ 1.15.0) — already on masterConfirmed via
npm ls axios. No change needed.INF-005 (
mocha→devDependencies) — REJECTEDTicket proposed this to shed the transitive
serialize-javascriptCVE (GHSA-5c6j-r48x-rmvq). Two problems:CVE is already mitigated on master.
package.jsonhas"overrides": { "serialize-javascript": ">=7.0.5" }— npm respects this override during resolution, so mocha's transitiveserialize-javascriptresolves to the patched version regardless of mocha's dependency section. The scanner that generated the ticket didn't account for the existing override.The move would break TestObservability for every default-config Cypress ≥ 10 customer.
bin/testObservability/reporter/index.js:12has:This is a regular
require, not therequireModule()helper. Regularrequirewalks up from the file's own location — from a globally-installed CLI's/usr/local/lib/node_modules/browserstack-cypress-cli/bin/testObservability/reporter/, it cannot reach the customer's projectnode_modules. Combined with:→ moving mocha to devDeps →
npm install -g browserstack-cypress-cliuses--production→ mocha not installed → next customer session →Runnablerequire throwsMODULE_NOT_FOUND→ TestObservability crashes on startup for every default-config Cypress ≥ 10 customer.INF-008 (
filesallowlist inpackage.json) — REJECTEDThe files this would hide from the npm tarball (
.github/,CODEOWNERS,.eslintrc*,.editorconfig) are already public in this GitHub repo. Excluding them from the tarball doesn't protect any information that isn't already at github.com/browserstack/browserstack-cypress-cli. Framed as security in the ticket; is actually hygiene. Deferred as an optional hygiene PR if we want it later.CSL-003 (
constants.js:401md5 → sha256) — DEFERREDChanges the upload-dedup hash algorithm. A client-only bump would leave the CLI sending SHA-256 hashes that BS's server-side dedup (currently keyed on MD5 length/format) doesn't recognize → every next upload for every customer becomes a full re-upload until the SHA-256 cache warms. Needs client+server coordination and one-time bandwidth-spike acceptance. Separate ticket.
Note on branch history
This PR supersedes #1129 (
fix/APS-19017-supply-chain-hygiene), which was closed to standardize branch naming toAPS-<ticket>-<info>format. Identical commits, same review outcome pending.Jira Ticket
https://browserstack.atlassian.net/browse/APS-19017