[APS-19009] security(cli): --ignore-scripts + validate npm_dependencies (lifecycle-script RCE) - #1172
Open
Rohannagariya1 wants to merge 7 commits into
Open
[APS-19009] security(cli): --ignore-scripts + validate npm_dependencies (lifecycle-script RCE)#1172Rohannagariya1 wants to merge 7 commits into
Rohannagariya1 wants to merge 7 commits into
Conversation
…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>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| 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
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
2 participants
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.
Issue (APS-19009, Critical)
browserstack.jsonnpm_dependenciesare merged into a temppackage.jsonand installed vianpm installwithout--ignore-scripts. A PR-supplied malicious package'spostinstalltherefore executes on the CI runner → RCE + theft ofBROWSERSTACK_ACCESS_KEY/GITHUB_TOKEN.Fix (minimal)
--ignore-scriptsadded to bothnpm installinvocations — the core RCE fix (blocks lifecycle scripts).npm_dependenciesis documented pure-JS only.package.json: standard npm package-name regex + a semver/dist-tag-only version charset, rejectinggit+ssh:///file:/ path / alternate-registry specs (dependency confusion / code-exec via spec).shell:trueretained deliberately (documented in code): the command line is fully static — dependency names live inpackage.jsondata, never on the command line, so there is no injection surface — andshell:trueis required for the>output redirection and to invokenpm.cmdon Windows. Flipping toshell:falsewould risk breaking Windows for zero security gain.Testing
evil; curl|sh,git+ssh://...,file:../../etc,$(env); accepts^4.17.21,~2.0.0,13.6.0.--ignore-scriptspresent in both install arg arrays;node --checkclean.Refs: APS-19009 (INJ-007 / INF-006)