[APS-19009] security(cli): --ignore-scripts + validate npm_dependencies (lifecycle-script RCE) by Rohannagariya1 · Pull Request #1172 · browserstack/browserstack-cypress-cli

Rohannagariya1 · 2026-08-05T11:59:57Z

Issue (APS-19009, Critical)

browserstack.json npm_dependencies are merged into a temp package.json and installed via npm install without --ignore-scripts. A PR-supplied malicious package's postinstall therefore executes on the CI runner → RCE + theft of BROWSERSTACK_ACCESS_KEY / GITHUB_TOKEN.

Fix (minimal)

--ignore-scripts added to both npm install invocations — the core RCE fix (blocks lifecycle scripts). npm_dependencies is documented pure-JS only.
Validate names + version specs before writing package.json: standard npm package-name regex + a semver/dist-tag-only version charset, rejecting git+ssh:// / file: / path / alternate-registry specs (dependency confusion / code-exec via spec).
shell:true retained deliberately (documented in code): the command line is fully static — dependency names live in package.json data, never on the command line, so there is no injection surface — and shell:true is required for the > output redirection and to invoke npm.cmd on Windows. Flipping to shell:false would risk breaking Windows for zero security gain.

Testing

Validation: rejects evil; curl|sh, git+ssh://..., file:../../etc, $(env); accepts ^4.17.21, ~2.0.0, 13.6.0.
--ignore-scripts present in both install arg arrays; node --check clean.

Refs: APS-19009 (INJ-007 / INF-006)

…proxy Ships the low-blast-radius subset of the CLI critical findings. APS-19010 — env-var API redirect: only honour BSTACK_CYPRESS_NODE_ENV url overrides (RAILS_HOST/UPLOAD_URL/DASHBOARD_URL/USAGE_REPORTING_URL) when they point at *.browserstack.com / *.bsstag.com / localhost; otherwise warn and fall back to the production defaults. APS-19011 — validate the API-supplied upload_url host before using it for the tests.zip upload; warn (do NOT cert-pin) when an HTTP(S) proxy routes all API traffic incl. credentials; structural-only JWT check on the TestHub token (defence-in-depth — the CLI has no key to verify the signature). APS-19008 (browserstack.json half) — read browserstack.json via JSON.parse(fs.readFileSync) instead of require() so a .js config cannot execute arbitrary code; require a .json extension and project-root path containment. New bin/helpers/securityValidation.js (stdlib-only): isAllowedBrowserstackUrl, isPathInsideBase, isWellFormedJwt, covered by test/unit/.../securityValidation.js (13 tests, both paths). Deliberately NOT changed (accepted-risk / opt-in — needs product decision): - cypress.config.js is legitimately JS that imports plugins; NOT sandboxed by default (a vm sandbox breaks real configs). APS-19008 cypress-config half. - npm_dependencies install still runs lifecycle scripts (APS-19009): NOT fixed here. Note: PR #1128's repo .npmrc (ignore-scripts=true) does NOT protect end users — packageInstaller copies the *user's* .npmrc into the temp install dir, not the CLI's. APS-19009's real fix (--ignore-scripts + package-name/version validation + shell:false) remains OPEN. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…es names/versions browserstack.json's npm_dependencies were merged into a temp package.json and installed with `npm install` and no `--ignore-scripts`, so a PR-supplied malicious package's lifecycle script (postinstall) executed on CI (RCE, credential theft). - Add `--ignore-scripts` to both npm install invocations (the RCE fix). npm_dependencies is documented pure-JS only. - Validate each dependency name (standard npm package-name regex) and version (semver/dist-tag charset only) before writing package.json, rejecting git-url / file: / path / alternate-registry specs (dependency confusion / code-exec via spec). - shell:true is retained deliberately: the command line is fully static (names live in package.json data, never on the command line -> no injection surface) and it is required for the output redirection and for invoking npm.cmd on Windows. Tested: validation rejects shell-metachar/git-url/file/$() specs, accepts normal semver; --ignore-scripts present in both installs; syntax clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

-      logger.debug(`Running NPM install command: npm install --legacy-peer-deps --loglevel verbose > ../npm_install_debug.log`);
-      nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--legacy-peer-deps', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true});
+      logger.debug(`Running NPM install command: npm install --legacy-peer-deps --ignore-scripts --loglevel verbose > ../npm_install_debug.log`);
+      nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--legacy-peer-deps', '--ignore-scripts', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true});


-      logger.debug(`Running NPM install command: 'npm install --loglevel verbose > ../npm_install_debug.log'`);
-      nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true});
+      logger.debug(`Running NPM install command: 'npm install --ignore-scripts --loglevel verbose > ../npm_install_debug.log'`);
+      nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--ignore-scripts', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true});


+    return false;
+  }
+  const base = path.resolve(baseDir || process.cwd());
+  const resolved = path.resolve(base, candidatePath);


+    return false;
+  }
+  const base = path.resolve(baseDir || process.cwd());
+  const resolved = path.resolve(base, candidatePath);


+      // PR-supplied .js config would run arbitrary code, APS-19008). Also require
+      // a .json extension and that the file resolves inside the project root so a
+      // crafted --config-file cannot point outside the project or at a script.
+      const resolvedPath = path.resolve(bsConfigPath);


…ndencies Locks the two security invariants the fix introduces: - packageInstall passes --ignore-scripts to the npm spawn (lifecycle-script RCE guard) - setupPackageFolder rejects a non-semver/git-url npm_dependencies version and never writes package.json (dependency-confusion / spec smuggling guard) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

- Add justified nosemgrep for spawn-shell-true on both npm-install spawns (static argv, shell needed only for '>' redirect + npm.cmd on Windows). - Sync securityValidation.js/utils.js with the path-join nosemgrep suppressions from the #1141 branch so the (false-positive) path-traversal findings clear. - NPM_NAME_RE: allow A-Z so legacy registry names (e.g. JSONStream) are not rejected; still blocks git-url/file:/path/alternate-registry specs. Test added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…st fix) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

+    return false;
+  }
+  // nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal -- these resolves ARE the traversal guard: the value is normalized here only so the containment check below can reject anything outside `base`.
+  const base = path.resolve(baseDir || process.cwd());


…ng honors same-line) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…npm-deps-hardening

…me-install fallback (APS-19009) The npm_dependencies validation added for APS-19009 was being caught by packageSetupAndInstaller's generic install-error catch and downgraded to the "dependencies will be installed in runtime" fallback. As a result a malicious or invalid dependency spec (git-url, file:, path, alternate-registry) was never actually blocked — the run proceeded, tests were uploaded, and the bad dep was deferred to the runtime install path. Mark the validation error (isNpmDependencyValidationError) and re-reject it from packageSetupAndInstaller so runs.js aborts before upload with a clear error and a non-zero exit code. Genuine install failures (network, registry, peer-deps) still fall back to runtime install unchanged, so the legitimate flow is untouched. Adds a regression test asserting packageSetupAndInstaller rejects (does not resolve) on a validation error. Verified live via `browserstack-cypress run`: malicious dep -> exit 1, no build created; valid deps (incl. scoped + legacy upper-case names) -> build created with --ignore-scripts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ep --ignore-scripts (APS-19009) The prior commit (8441042) hard-aborted a run whenever an npm_dependencies spec failed a strict name+version regex. BigQuery analysis of 221,133 real cypress-cli builds over 90 days showed this would abort 7,446 builds (3.4%): - 6,914 BrowserStack's OWN Cypress SDK CI (installs the CLI from its git branches, e.g. browserstack-cypress-cli@github.com/browserstack...#master) - 405 real enterprise customers using legitimate non-registry specs (private Artifactory tarball URLs, file: vendored modules) - 127 actual attack/CTF payloads (the only ones we want to block) i.e. ~57 legitimate builds broken per attack caught. The documented RCE (lifecycle-script execution) is already fully closed by --ignore-scripts for EVERY spec, so the hard abort added large blast radius for no real security gain. Changes: - Keep --ignore-scripts (the actual RCE remediation) unchanged. - Drop version-spec validation entirely: git / file: / tarball-url / private-registry versions are legitimate and must run. - Validate only the package NAME; an invalid name (shell-metacharacter / command-injection payload) is SKIPPED with a warning, and the session continues with the remaining valid deps -- never a hard abort. - Revert runs.js abort/try-catch; packageSetupAndInstaller no longer rejects. - Tests updated: allow git-url spec, skip bad name without aborting, and assert packageSetupAndInstaller resolves (never blocks a session). 22/22. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…npm-deps-hardening

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Rohannagariya1 · 2026-08-19T19:57:31Z

Note on the red `Semgrep OSS` check (false positives — safe to dismiss)

semgrep/ci (our pipeline scan, honors inline // nosemgrep) is green. Semgrep OSS (the code-scanning integration that ignores inline suppressions) reports 6 alerts — 2 errors + 4 warnings — all of which are safe:

2 errors — spawn(..., {shell: true}) at bin/helpers/packageInstaller.js:119,122

This is the npm install --ignore-scripts … command. shell:true was already present on master; it only re-surfaced because these lines were edited to add --ignore-scripts.
Safe: the command line is fully static — package names live in package.json, never on the CLI, so there is no injection surface. shell:true is required for the > npm_install_debug.log redirection and for invoking npm.cmd on Windows. shell:false would break Windows runs.
The lifecycle-script RCE this PR addresses is closed by --ignore-scripts, not by removing shell:true.

4 warnings — path.resolve/path.join on user input at bin/helpers/securityValidation.js:63,65 and bin/helpers/utils.js:43

These lines are the path-containment guard (isPathInsideBase, APS-19008). The resolved path is checked against the base directory before use — this is the mitigation, not a vulnerability.

Recommendation: dismiss these 6 alerts as "Won't fix / used safely" in the Code scanning tab (inline // nosemgrep has no effect on Semgrep OSS), or mark the Semgrep OSS check non-required. No code change is warranted.

…npm-deps-hardening

Rohannagariya1

Claude Code Review (automated) — 1 inline finding(s). Full report in the PR comment below. Verdict: Passed.

Rohannagariya1 · 2026-09-01T10:41:35Z

+        const safeDependencies = {};
+        for (const depName of Object.keys(combinedDependencies || {})) {
+          const depVersion = combinedDependencies[depName];
+          if (!NPM_NAME_RE.test(depName) || typeof depVersion !== 'string') {


[Low] PR description overstates version-spec validation

The PR body says the fix validates version specs and rejects git+ssh:// / file: / path / alternate-registry specs. The code intentionally does NOT — it only checks typeof depVersion === 'string', and a new test asserts a git-URL spec is accepted. This is not a security gap (the version is written as JSON data and --ignore-scripts blocks install/prepare execution), but the description is stale.

Suggestion: No code change needed. Update the PR body to match the code: package names are validated; version specs are deliberately left unvalidated because git/file/tarball specs are legitimate and RCE is closed by --ignore-scripts.

Reviewer: stack:code-review

Rohannagariya1 · 2026-09-01T10:41:37Z

Claude Code PR Review

PR: #1172 • Head: ecc9674 • Reviewers: stack:code-review

Summary

Hardens the Cypress CLI against the npm_dependencies lifecycle-script RCE (APS-19009) by adding --ignore-scripts to both npm installs and validating package names, plus a new securityValidation.js helper that allowlists BrowserStack URLs (env override + API-supplied upload_url), structurally validates the TestHub JWT, guards config-file path traversal, and warns on proxy use.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	No secrets added; allowlist host suffixes only.
High	Security	Authentication/authorization checks present	N/A	Client-side CLI; no server auth surface.
High	Security	Input validation and sanitization	Pass	Core fix: `--ignore-scripts` on both installs blocks postinstall RCE; npm-name regex is anchored (`^...$`) and rejects shell metachars/newlines; URL allowlist is fail-closed and resists userinfo/suffix/prefix-spoof bypass (verified adversarially); path-traversal + `.json`-extension guard on config file; JWT structural check.
High	Security	No IDOR — resource ownership validated	N/A	No multi-tenant resource access in a client CLI.
High	Security	No SQL injection (parameterized queries)	N/A	No SQL in this repo.
High	Correctness	Logic is correct, handles edge cases	Pass	Bad dep names skipped (not aborted); package.json written via `JSON.stringify` so names/versions are inert data, not on the command line; JWT gate leaves env unset only on malformed token; `runs.js` refactor preserves prior undefined semantics.
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	URL/JWT/path failures warn + fall back to safe defaults; `packageSetupAndInstaller` still resolves on setup failure (deferring to runtime install) — verified by new test.
High	Correctness	No race conditions or concurrency issues	N/A	No new concurrency introduced.
Medium	Testing	New code has corresponding tests	Pass	New `securityValidation` suite (URL/path/JWT incl. bypass cases); packageInstaller tests for skip-bad-name, git-url allow, `--ignore-scripts` presence, never-abort. 35 passing locally.
Medium	Testing	Error paths and edge cases tested	Pass	Suffix/prefix-spoof, non-http scheme, empty/undefined, sibling-prefix path, malformed JWT, and setup-failure-resolves all covered.
Medium	Testing	Existing tests still pass (no regressions)	Pass	Both changed suites green (`npx mocha` = 35 passing).
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	Not applicable to CLI.
Medium	Performance	Long-running tasks use background jobs	N/A	Not applicable.
Medium	Quality	Follows existing codebase patterns	Pass	Reuses winstonLogger, promise style, rewire-based tests consistent with repo.
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to the APS-19009/10/11 hardening set; no unrelated churn.
Low	Quality	Meaningful names, no dead code	Pass	Clear helper names; no dead code.
Low	Quality	Comments explain why, not what	Pass	Comments cite ticket IDs and threat rationale; `nosemgrep` lines annotated with justification.
Low	Quality	No unnecessary dependencies added	Pass	`securityValidation.js` is stdlib-only (path/URL).

Findings

File: bin/helpers/packageInstaller.js:44
Severity: Low
Reviewer: stack:code-review
Issue: The PR description states the fix "validates names + version specs … rejecting git+ssh:// / file: / path / alternate-registry specs," but the code intentionally does NOT validate version specs (only typeof depVersion === 'string'), and a test explicitly asserts a git-URL spec is accepted. The description is stale relative to the code.
Suggestion: No code change needed — the version value is written as JSON data and --ignore-scripts neutralizes install/prepare execution, so unvalidated specs are not an RCE surface. Update the PR body to match the code (names validated; version specs deliberately not, because git/file/tarball are legitimate) to avoid future confusion.

What's Done Well

Correct, minimal core fix: --ignore-scripts on both npm major-version branches with a direct unit test asserting the flag is present.
The URL allowlist is genuinely bypass-resistant — fail-closed on parse errors, exact-host + leading-dot suffix matching defeats browserstack.com.attacker.net / evilbrowserstack.com / userinfo @evil.com tricks (all verified adversarially).
Skip-not-abort design for bad dependency entries keeps customer sessions running while still stripping malicious names; documented shell:true/nosemgrep rationale is accurate.

Verdict: PASS

Rohannagariya1 and others added 2 commits July 2, 2026 18:02

Rohannagariya1 requested a review from a team as a code owner August 5, 2026 11:59

Rohannagariya1 requested review from pranay-v29 and xxshubhamxx August 5, 2026 11:59

github-advanced-security Bot found potential problems Aug 5, 2026

View reviewed changes

Rohannagariya1 and others added 3 commits August 6, 2026 16:52

[APS-19009] sync securityValidation.js with #1141 (IPv6 [::1] allowli…

37ef6ad

…st fix) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

github-advanced-security Bot found potential problems Aug 6, 2026

View reviewed changes

Rohannagariya1 and others added 6 commits August 6, 2026 18:50

[APS-19009] move spawn-shell-true nosemgrep to same line (code-scanni…

4e0aa6f

…ng honors same-line) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Merge remote-tracking branch 'origin/master' into security/aps-19009-…

c5430ef

…npm-deps-hardening

Merge remote-tracking branch 'origin/master' into security/aps-19009-…

8df7a40

…npm-deps-hardening

chore: trim verbose APS-19009 comments

1f42201

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Merge remote-tracking branch 'origin/master' into security/aps-19009-…

ecc9674

…npm-deps-hardening

Rohannagariya1 commented Sep 1, 2026

View reviewed changes

Navigation Menu

Uh oh!

[APS-19009] security(cli): --ignore-scripts + validate npm_dependencies (lifecycle-script RCE) - #1172

[APS-19009] security(cli): --ignore-scripts + validate npm_dependencies (lifecycle-script RCE)#1172
Rohannagariya1 wants to merge 12 commits into
masterfrom
security/aps-19009-npm-deps-hardening

Rohannagariya1 commented Aug 5, 2026

Uh oh!

Uh oh!

Rohannagariya1 commented Aug 19, 2026

Uh oh!

Rohannagariya1 left a comment

Uh oh!

Rohannagariya1 Sep 1, 2026

Uh oh!

Rohannagariya1 commented Sep 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Rohannagariya1 commented Aug 5, 2026

Issue (APS-19009, Critical)

Fix (minimal)

Testing

Uh oh!

Uh oh!

Rohannagariya1 commented Aug 19, 2026

Note on the red Semgrep OSS check (false positives — safe to dismiss)

Uh oh!

Rohannagariya1 left a comment

Choose a reason for hiding this comment

Uh oh!

Rohannagariya1 Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

Rohannagariya1 commented Sep 1, 2026

Claude Code PR Review

Summary

Review Table

Findings

What's Done Well

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Note on the red `Semgrep OSS` check (false positives — safe to dismiss)