security: URL/path/JWT validation + proxy warning (APS-19008 partial / 19010 / 19011)#1141
Open
Rohannagariya1 wants to merge 1 commit into
Open
security: URL/path/JWT validation + proxy warning (APS-19008 partial / 19010 / 19011)#1141Rohannagariya1 wants to merge 1 commit into
Rohannagariya1 wants to merge 1 commit 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>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if (typeof candidatePath !== 'string' || candidatePath === '') { | ||
| return false; | ||
| } | ||
| const base = path.resolve(baseDir || process.cwd()); |
| 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); |
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.
Summary
Ships the low-blast-radius subset of the CLI critical findings (per the agreed proposal); high-blast-radius items are left as documented accepted-risk / opt-in.
BSTACK_CYPRESS_NODE_ENVURL 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. Preserves all internal dev/QA usage; blocks arbitrary external redirects.upload_urlhost before using it for the tests.zip upload; warn (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).browserstack.jsonviaJSON.parse(fs.readFileSync)instead ofrequire(), so a.jsconfig can't execute code; require.jsonextension + project-root path containment.New
bin/helpers/securityValidation.js(stdlib-only):isAllowedBrowserstackUrl,isPathInsideBase,isWellFormedJwt.Tested
test/unit/bin/helpers/securityValidation.js— 13 tests, both paths (accepts prod/staging/localhost; rejects attacker hosts, suffix-spoofing likebrowserstack.com.attacker.net, non-http schemes; path-containment accepts inside / rejects../+ sibling-prefix; JWT structural accept/reject). All pass.validateBstackJsontests still pass with theJSON.parserewrite.config.jsboth paths: attackerRAILS_HOSTwarned+ignored (keeps prod default); legit*.bsstag.comoverride applied.Deliberately NOT changed (accepted-risk / opt-in — needs product decision)
cypress.config.jsis legitimately JS that imports plugins; avmsandbox breaks real configs, so it is NOT sandboxed by default..npmrc(ignore-scripts=true) does not protect end users —packageInstallercopies the user's.npmrcinto the temp install dir, not the CLI's own. APS-19009's real fix (--ignore-scripts+ package name/version validation +shell:false) remains open.Fixes APS-19010; partially fixes APS-19008 and APS-19011.