GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — security: URL/path/JWT validation + proxy warning (APS-19008 partial / 19010 / 19011) by Rohannagariya1 · Pull Request #1141 · browserstack/browserstack-cypress-cli

Conversation

Rohannagariya1 commented Jul 2, 2026

Copy link

Copy Markdown

Collaborator

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.

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. Preserves all internal dev/QA usage; blocks arbitrary external redirects.
APS-19011: validate the API-supplied upload_url host 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).
APS-19008 (browserstack.json half): read browserstack.json via JSON.parse(fs.readFileSync) instead of require(), so a .js config can't execute code; require .json extension + project-root path containment.

New bin/helpers/securityValidation.js (stdlib-only): isAllowedBrowserstackUrl, isPathInsideBase, isWellFormedJwt.

Tested

New test/unit/bin/helpers/securityValidation.js — 13 tests, both paths (accepts prod/staging/localhost; rejects attacker hosts, suffix-spoofing like browserstack.com.attacker.net, non-http schemes; path-containment accepts inside / rejects ../ + sibling-prefix; JWT structural accept/reject). All pass.
Existing validateBstackJson tests still pass with the JSON.parse rewrite.
Smoke-tested config.js both paths: attacker RAILS_HOST warned+ignored (keeps prod default); legit *.bsstag.com override applied.

Deliberately NOT changed (accepted-risk / opt-in — needs product decision)

cypress.config.js sandbox (APS-19008 other half): cypress.config.js is legitimately JS that imports plugins; a vm sandbox breaks real configs, so it is NOT sandboxed by default.
npm_dependencies lifecycle scripts (APS-19009): STILL OPEN. Not fixed here. ⚠️ Note: PR [APS-19734] fix: harden .npmrc with supply-chain security directives #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 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          security: validate override/response URLs, config path, JWT; warn on …

e578fad

…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 <[email protected]>

Rohannagariya1 requested a review from a team as a code owner

July 2, 2026 12:32

Rohannagariya1 requested review from 07souravkunda and hamza-browserstack

July 2, 2026 12:32

github-advanced-security Bot found potential problems

Jul 2, 2026

View reviewed changes

Comment thread

bin/helpers/securityValidation.js Fixed

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

bin/helpers/securityValidation.js

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

Comment thread

bin/helpers/securityValidation.js

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

Comment thread

bin/helpers/utils.js

		// 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);


          security(cli): suppress false-positive path-traversal Semgrep finding…

999a0b5

…s on the path-containment guard (APS-19008)

semgrep/ci blocked #1141 on 4 path-join-resolve-traversal findings, all on the
new path-CONTAINMENT code itself: isPathInsideBase() (securityValidation.js:61-62)
and validateBstackJson's resolve (utils.js:42). The path.resolve() calls ARE the
anti-traversal guard — each is immediately followed by a containment check
(startsWith(base+sep) / isPathInsideBase + .json-extension). Semgrep flags
resolve(userInput) without seeing the guard, so these are false positives.

Added // nosemgrep on the exact lines with justification (per Security Ops book #68).
No behaviour change.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

github-advanced-security Bot found potential problems

Aug 6, 2026

View reviewed changes

Comment thread

bin/helpers/securityValidation.js

		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());


          [APS-19008] fix IPv6-loopback allowlist entry (::1 -> [::1])

12f73d3

new URL('http://[::1]:x').hostname returns the bracketed form, so the bare '::1'
entry never matched (dead). Use '[::1]' so IPv6 localhost is actually allowed for
local dev, consistent with 'localhost'/'127.0.0.1'.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

Rohannagariya1 added a commit that referenced this pull request

Aug 6, 2026


          [APS-19009] resolve semgrep CI + allow legacy upper-case npm names

fba71e0

- 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 <[email protected]>

Rohannagariya1 added a commit that referenced this pull request

Aug 6, 2026


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

37ef6ad

…st fix)

Co-Authored-By: Claude Opus 4.8 <[email protected]>


          Merge remote-tracking branch 'origin/master' into security/cypress-cl…

c5b17c2

…i-safe-subset

Reviewers

07souravkunda

Awaiting requested review from 07souravkunda

07souravkunda is a code owner automatically assigned from browserstack/sdk-dev

hamza-browserstack

Awaiting requested review from hamza-browserstack

hamza-browserstack is a code owner automatically assigned from browserstack/sdk-dev

1 more reviewer

github-advanced-security[bot]

github-advanced-security[bot] left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

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.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.