Skip to content

Commit 9bb1f6f

Browse files
fix(security): allow Windows backslash paths and add file-not-found UX [APS-18613]
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: <path>" 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
1 parent 6dbf8f9 commit 9bb1f6f

2 files changed

Lines changed: 60 additions & 6 deletions

File tree

bin/helpers/readCypressConfigUtil.js

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,17 @@ const logger = require('./logger').winstonLogger;
1111
// Defense-in-depth: reject file paths containing shell metacharacters.
1212
// This guards against command injection even if execFileSync is ever
1313
// replaced with a shell-based exec in the future.
14-
const DANGEROUS_PATH_CHARS = /[;"`$|&(){}\\]/;
14+
//
15+
// Note: backslash (\) is intentionally NOT included here because it is a
16+
// legitimate path separator on Windows (e.g. C:\Users\me\cypress.config.js).
17+
// The actual security boundary is execFileSync (no shell), not this regex.
18+
const DANGEROUS_PATH_CHARS = /[;"`$|&(){}]/;
1519

1620
function validateFilePath(filepath) {
1721
if (DANGEROUS_PATH_CHARS.test(filepath)) {
1822
throw new Error(
1923
`Invalid cypress config file path: "${filepath}" contains disallowed characters. ` +
20-
'File paths must not include shell metacharacters such as ; " ` $ | & ( ) { } \\'
24+
'File paths must not include shell metacharacters such as ; " ` $ | & ( ) { }'
2125
);
2226
}
2327
}
@@ -205,6 +209,13 @@ exports.loadJsFile = (cypress_config_filepath, bstack_node_modules_path) => {
205209
// Security: validate file path to reject shell metacharacters (defense-in-depth)
206210
validateFilePath(cypress_config_filepath);
207211

212+
// UX: surface a clear error if the cypress config file is missing.
213+
// (This is purely a UX check — the security boundary is execFileSync above
214+
// plus the metacharacter regex; existsSync alone would NOT prevent injection.)
215+
if (!fs.existsSync(cypress_config_filepath)) {
216+
throw new Error(`Cypress config file not found at: ${cypress_config_filepath}`);
217+
}
218+
208219
const require_module_helper_path = path.join(__dirname, 'requireModule.js')
209220

210221
// Security fix: use execFileSync instead of execSync to avoid shell interpolation.

test/unit/bin/helpers/readCypressConfigUtil.js

Lines changed: 47 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,22 @@ describe("readCypressConfigUtil", () => {
4949
expect(() => readCypressConfigUtil.validateFilePath('path/to my project/cypress.config.js')).to.not.throw();
5050
});
5151

52+
it('should accept Windows absolute paths with backslashes', () => {
53+
expect(() => readCypressConfigUtil.validateFilePath('C:\\Users\\test\\cypress.config.js')).to.not.throw();
54+
});
55+
56+
it('should accept Windows absolute paths with spaces and backslashes (Program Files)', () => {
57+
expect(() => readCypressConfigUtil.validateFilePath('C:\\Program Files\\my app\\cypress.config.js')).to.not.throw();
58+
});
59+
60+
it('should accept Windows relative paths with backslashes', () => {
61+
expect(() => readCypressConfigUtil.validateFilePath('.\\subdir\\cypress.config.js')).to.not.throw();
62+
});
63+
64+
it('should accept UNC-style Windows paths', () => {
65+
expect(() => readCypressConfigUtil.validateFilePath('\\\\server\\share\\cypress.config.js')).to.not.throw();
66+
});
67+
5268
it('should reject paths with semicolons (command injection)', () => {
5369
expect(() => readCypressConfigUtil.validateFilePath('cypress.config";curl localhost:8000/shell.sh|sh;".js'))
5470
.to.throw(/disallowed characters/);
@@ -82,9 +98,9 @@ describe("readCypressConfigUtil", () => {
8298
const existsSyncStub = sandbox.stub(fs, 'existsSync').returns(true);
8399
const unlinkSyncSyncStub = sandbox.stub(fs, 'unlinkSync');
84100
const requireModulePath = path.join(__dirname, '../../../../', 'bin', 'helpers', 'requireModule.js');
85-
101+
86102
const result = readCypressConfigUtil.loadJsFile('path/to/cypress.config.ts', 'path/to/tmpBstackPackages');
87-
103+
88104
expect(result).to.eql({ e2e: {} });
89105
// Verify execFileSync is called with 'node' as first arg and array of args
90106
sinon.assert.calledOnce(execFileStub);
@@ -94,7 +110,8 @@ describe("readCypressConfigUtil", () => {
94110
expect(execFileStub.getCall(0).args[2].env.NODE_PATH).to.eql('path/to/tmpBstackPackages');
95111
sinon.assert.calledOnce(readFileSyncStub);
96112
sinon.assert.calledOnce(unlinkSyncSyncStub);
97-
sinon.assert.calledOnce(existsSyncStub);
113+
// existsSync is now called twice: once for the file-not-found UX check, once for the unlink cleanup
114+
sinon.assert.calledTwice(existsSyncStub);
98115
});
99116

100117
it('should load js file using execFileSync on Windows too (no platform-specific branching needed)', () => {
@@ -115,7 +132,33 @@ describe("readCypressConfigUtil", () => {
115132
expect(execFileStub.getCall(0).args[2].env.NODE_PATH).to.eql('path/to/tmpBstackPackages');
116133
sinon.assert.calledOnce(readFileSyncStub);
117134
sinon.assert.calledOnce(unlinkSyncSyncStub);
118-
sinon.assert.calledOnce(existsSyncStub);
135+
// existsSync called twice: file-not-found UX check + unlink cleanup
136+
sinon.assert.calledTwice(existsSyncStub);
137+
});
138+
139+
it('should accept Windows-style absolute paths in loadJsFile (no rejection)', () => {
140+
sandbox.stub(cp, "execFileSync").returns("random string");
141+
sandbox.stub(fs, 'readFileSync').returns('{"e2e": {}}');
142+
sandbox.stub(fs, 'existsSync').returns(true);
143+
sandbox.stub(fs, 'unlinkSync');
144+
145+
// None of these should throw
146+
expect(() => readCypressConfigUtil.loadJsFile('C:\\Users\\test\\cypress.config.js', 'path/to/tmpBstackPackages'))
147+
.to.not.throw();
148+
expect(() => readCypressConfigUtil.loadJsFile('C:\\Program Files\\my app\\cypress.config.js', 'path/to/tmpBstackPackages'))
149+
.to.not.throw();
150+
expect(() => readCypressConfigUtil.loadJsFile('.\\subdir\\cypress.config.js', 'path/to/tmpBstackPackages'))
151+
.to.not.throw();
152+
});
153+
154+
it('should throw a clear error when the cypress config file does not exist (UX)', () => {
155+
sandbox.stub(fs, 'existsSync').returns(false);
156+
const execFileStub = sandbox.stub(cp, "execFileSync");
157+
158+
expect(() => readCypressConfigUtil.loadJsFile('path/to/missing/cypress.config.js', 'path/to/tmpBstackPackages'))
159+
.to.throw(/Cypress config file not found at:/);
160+
// execFileSync must NOT be invoked when the file is missing
161+
sinon.assert.notCalled(execFileStub);
119162
});
120163

121164
it('should reject file paths containing command injection characters', () => {

0 commit comments

Comments
 (0)