[APS-19013] fix(security): shell injection (INJ-004) and module hijack (INJ-012) in Test Observability

[Jimesh-browserstack]

Security Fix: APS-19013

Two High-severity vulnerabilities reported on master of this repo. Both remediations follow the exact guidance in the ticket (INJ-004, INJ-012). Total change: 2 insertions, 4 deletions across 2 files. Commit history kept logically split (one commit per CWE) for reviewability.

Issues

#	File	CWE	Type
1	bin/testObservability/helper/helper.js	CWE-78	Shell injection via spawn with shell: true
2	bin/testObservability/crashReporter/index.js	CWE-427	Module hijack via env-controlled module path

CVSS 3.1: 8.4 (AV:L/AC:L/PR:N/UI:N/S:U/C:H/I:H/A:H) | OWASP A03:2021

Root Cause

1. runCypressTestsLocally invokes spawn('npx', [argv...], { ..., shell: true }). With shell: true, the entire argv is concatenated and re-parsed by the shell — any metacharacter (;, &&, backticks, $(), |) inside --spec or any other passed flag executes arbitrary commands. rawArgs originate from CLI input (CI configs, PR diffs).
2. crashReporter.requireModule prepends process.env["browserStackCwd"] to module paths before require(). An attacker who can set this env var (CI config, supply-chain compromise of any other dep that sets env vars) can plant a trojanized mocha / cypress package and have it loaded.

Fix Applied

helper.js (1 line):

-      { stdio: 'inherit', cwd: process.cwd(), env: process.env, shell: true }
+      { stdio: 'inherit', cwd: process.cwd(), env: process.env }

spawn already passes argv as a separate array — no shell needed. Cypress runs the same way; only the shell-interpolation hazard goes away.

crashReporter/index.js (3 lines deleted, 1 inserted):

-  if(process.env["browserStackCwd"]){
-   local_path = path.join(process.env["browserStackCwd"], 'node_modules', module);
-  } else if(internal) {
+  if(internal) {
     local_path = path.join(process.cwd(), 'node_modules', 'browserstack-cypress-cli', 'node_modules', module);
   } else {
     local_path = path.join(process.cwd(), 'node_modules', module);
   }

Module resolution now uses process.cwd() (or the internal nested node_modules path when internal=true) only — no env input.

Testing

Branch	Suite	passing	failing	pending	Notes
master (baseline)	npm test	663	13	2	All 13 failures pre-existing
fix/APS-19013-...	npm test	663	13	2	Identical — no regression

Failure list is character-for-character identical between branches (verified by diffing the mocha failure summaries). All pre-existing failures live in runs, fileHelpers, syncSpecsLogs, utils, init, helpers/utils — none touch the changed code paths.

Direct fix verification:

• grep -n "shell:" bin/testObservability/helper/helper.js → no matches (INJ-004 ✓)
• grep -n "browserStackCwd" bin/testObservability/crashReporter/index.js → no matches (INJ-012 ✓)
• Reproduction from ticket (browserStackCwd=/tmp/hijack node ...) — /tmp/hijacked is not written; module path resolution no longer honors the env var.

BrowserStack Session Sanity: Not applicable to this PR. The two changed code paths are:

• runCypressTestsLocally — local-only fallback runner (developer workflow when not running on BrowserStack)
• crashReporter.requireModule — internal helper for crash-report telemetry

Neither touches BrowserStack hub interaction, capability handling, session creation, or the session lifecycle. The CLI's BrowserStack-side execution path (the runs command via runs.js) is unaffected by either edit. Existing pre-merge CI on this repo will exercise the standard test surface.

Jira Ticket

APS-19013

Checklist

• Security issue addressed (INJ-004 + INJ-012)
• Unit tests passing — comparative regression vs master shows 0 new failures
• No BrowserStack session dependency in the changed code paths
• Surgical change (2 ins / 4 del across 2 files; no scope creep)