Skip to content

Commit fba71e0

Browse files
[APS-19009] resolve semgrep CI + allow legacy upper-case npm names
- Add justified nosemgrep for spawn-shell-true on both npm-install spawns (static argv, shell needed only for '>' redirect + npm.cmd on Windows). - Sync securityValidation.js/utils.js with the path-join nosemgrep suppressions from the #1141 branch so the (false-positive) path-traversal findings clear. - NPM_NAME_RE: allow A-Z so legacy registry names (e.g. JSONStream) are not rejected; still blocks git-url/file:/path/alternate-registry specs. Test added. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 70fbc8e commit fba71e0

4 files changed

Lines changed: 42 additions & 1 deletion

File tree

bin/helpers/packageInstaller.js

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,11 @@ const setupPackageFolder = (runSettings, directoryPath) => {
3737
// before writing them to package.json, so a browserstack.json cannot smuggle a
3838
// git-url / file: / path / alternate-registry spec (dependency confusion or code
3939
// execution) into `npm install`.
40-
const NPM_NAME_RE = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/;
40+
// Allow upper-case too: legacy registry packages (e.g. JSONStream) have
41+
// capitals and must not be rejected. This still blocks git-url / file: /
42+
// path / alternate-registry specs (those contain :, /, .. which are not in
43+
// the class), which is the actual dependency-confusion / RCE guard.
44+
const NPM_NAME_RE = /^(@[a-zA-Z0-9-~][a-zA-Z0-9-._~]*\/)?[a-zA-Z0-9-~][a-zA-Z0-9-._~]*$/;
4145
const NPM_VERSION_RE = /^[A-Za-z0-9.\-+~^><=|*\s]+$/;
4246
for (const depName of Object.keys(combinedDependencies || {})) {
4347
const depVersion = combinedDependencies[depName];
@@ -117,9 +121,11 @@ const packageInstall = (packageDir, bsConfig) => {
117121
// output redirection and for invoking npm.cmd on Windows.
118122
if (parseInt(npm_major_version) >= 7) {
119123
logger.debug(`Running NPM install command: npm install --legacy-peer-deps --ignore-scripts --loglevel verbose > ../npm_install_debug.log`);
124+
// nosemgrep: javascript.lang.security.audit.spawn-shell-true.spawn-shell-true -- static argv (see comment above); shell:true needed for '>' redirection + npm.cmd on Windows, no user input on the command line.
120125
nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--legacy-peer-deps', '--ignore-scripts', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true});
121126
} else {
122127
logger.debug(`Running NPM install command: 'npm install --ignore-scripts --loglevel verbose > ../npm_install_debug.log'`);
128+
// nosemgrep: javascript.lang.security.audit.spawn-shell-true.spawn-shell-true -- static argv (see comment above); shell:true needed for '>' redirection + npm.cmd on Windows, no user input on the command line.
123129
nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--ignore-scripts', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true});
124130
}
125131
nodeProcess.on('close', nodeProcessCloseCallback);

bin/helpers/securityValidation.js

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,9 @@ function isPathInsideBase(candidatePath, baseDir) {
5858
if (typeof candidatePath !== 'string' || candidatePath === '') {
5959
return false;
6060
}
61+
// nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal -- these resolves ARE the traversal guard: the value is normalized here only so the containment check below can reject anything outside `base`.
6162
const base = path.resolve(baseDir || process.cwd());
63+
// nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal -- see above; resolved path is validated by the startsWith(base) check, not used to read the FS unchecked.
6264
const resolved = path.resolve(base, candidatePath);
6365
// Must be the base itself or a descendant (base + separator prefix).
6466
return resolved === base || resolved.startsWith(base + path.sep);

bin/helpers/utils.js

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ exports.validateBstackJson = (bsConfigPath) => {
3939
// PR-supplied .js config would run arbitrary code, APS-19008). Also require
4040
// a .json extension and that the file resolves inside the project root so a
4141
// crafted --config-file cannot point outside the project or at a script.
42+
// nosemgrep: javascript.lang.security.audit.path-traversal.path-join-resolve-traversal.path-join-resolve-traversal -- this resolve IS the traversal guard: the path is normalized here so the .json-extension + isPathInsideBase() containment checks below can reject anything outside the project root.
4243
const resolvedPath = path.resolve(bsConfigPath);
4344
if (path.extname(resolvedPath).toLowerCase() !== ".json") {
4445
return reject(`Invalid browserstack.json file. Error : config file must be a .json file.`);

test/unit/bin/helpers/packageInstaller.js

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -245,6 +245,38 @@ describe("packageInstaller", () => {
245245
sinon.assert.notCalled(fswriteFileSyncStub);
246246
});
247247
});
248+
249+
it("should accept a legacy upper-case npm package name (e.g. JSONStream) (APS-19009)", () => {
250+
packageInstaller.__set__({
251+
fileHelpers: {deletePackageArchieve: fileHelpersStub},
252+
fs: {
253+
mkdir: fsmkdirStub,
254+
writeFileSync: fswriteFileSyncStub,
255+
existsSync: fsexistsSyncStub,
256+
copyFileSync: fscopyFileSyncStub
257+
},
258+
path: {
259+
dirname: pathdirnameStub,
260+
join: pathjoinStub
261+
}
262+
});
263+
let setupPackageFolderrewire = packageInstaller.__get__('setupPackageFolder');
264+
let runSettings = {
265+
npm_dependencies: {
266+
"JSONStream": "1.3.5"
267+
}
268+
};
269+
let directoryPath = "/random/path";
270+
return setupPackageFolderrewire(runSettings, directoryPath)
271+
.then((data) => {
272+
sinon.assert.calledOnce(fswriteFileSyncStub);
273+
chai.assert.equal(data, "Package file created");
274+
})
275+
.catch((_error) => {
276+
console.log(_error);
277+
chai.assert.fail("legacy upper-case package name should be accepted");
278+
});
279+
});
248280
});
249281

250282
context("packageInstall", () => {

0 commit comments

Comments
 (0)