From e578fadb257e45781ac16ef1f6b2682a285e77cc Mon Sep 17 00:00:00 2001 From: Rohan <142411639+Rohannagariya1@users.noreply.github.com> Date: Thu, 2 Jul 2026 18:02:26 +0530 Subject: [PATCH 1/6] security: validate override/response URLs, config path, JWT; warn on proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ships the low-blast-radius subset of the CLI critical findings. APS-19010 — env-var API redirect: only honour BSTACK_CYPRESS_NODE_ENV url overrides (RAILS_HOST/UPLOAD_URL/DASHBOARD_URL/USAGE_REPORTING_URL) when they point at *.browserstack.com / *.bsstag.com / localhost; otherwise warn and fall back to the production defaults. APS-19011 — validate the API-supplied upload_url host before using it for the tests.zip upload; warn (do NOT cert-pin) when an HTTP(S) proxy routes all API traffic incl. credentials; structural-only JWT check on the TestHub token (defence-in-depth — the CLI has no key to verify the signature). APS-19008 (browserstack.json half) — read browserstack.json via JSON.parse(fs.readFileSync) instead of require() so a .js config cannot execute arbitrary code; require a .json extension and project-root path containment. New bin/helpers/securityValidation.js (stdlib-only): isAllowedBrowserstackUrl, isPathInsideBase, isWellFormedJwt, covered by test/unit/.../securityValidation.js (13 tests, both paths). Deliberately NOT changed (accepted-risk / opt-in — needs product decision): - cypress.config.js is legitimately JS that imports plugins; NOT sandboxed by default (a vm sandbox breaks real configs). APS-19008 cypress-config half. - npm_dependencies install still runs lifecycle scripts (APS-19009): NOT fixed here. Note: PR #1128's repo .npmrc (ignore-scripts=true) does NOT protect end users — packageInstaller copies the *user's* .npmrc into the temp install dir, not the CLI's. APS-19009's real fix (--ignore-scripts + package-name/version validation + shell:false) remains OPEN. Co-Authored-By: Claude Opus 4.8 --- bin/helpers/config.js | 27 +++++- bin/helpers/getInitialDetails.js | 12 ++- bin/helpers/helper.js | 5 ++ bin/helpers/securityValidation.js | 92 +++++++++++++++++++ bin/helpers/utils.js | 30 +++++-- bin/testhub/utils.js | 11 ++- test/unit/bin/helpers/securityValidation.js | 97 +++++++++++++++++++++ 7 files changed, 260 insertions(+), 14 deletions(-) create mode 100644 bin/helpers/securityValidation.js create mode 100644 test/unit/bin/helpers/securityValidation.js diff --git a/bin/helpers/config.js b/bin/helpers/config.js index e689a61c..f2e77987 100644 --- a/bin/helpers/config.js +++ b/bin/helpers/config.js @@ -1,15 +1,34 @@ var config = require('./config.json'); +const { isAllowedBrowserstackUrl } = require('./securityValidation'); config.env = process.env.BSTACK_CYPRESS_NODE_ENV || "production"; +// Only honour an env-var URL override if it points at a BrowserStack +// (prod/staging) host or localhost. Without this allowlist an attacker who can +// set CI env vars (BSTACK_CYPRESS_NODE_ENV + RAILS_HOST/UPLOAD_URL/...) could +// redirect all API calls — including Basic Auth credentials and the tests.zip +// upload — to their own server (APS-19010). Invalid overrides fall back to the +// production defaults from config.json. +const applyUrlOverride = (envValue, currentValue, label) => { + if (envValue === undefined || envValue === null || envValue === "") { + return currentValue; + } + if (isAllowedBrowserstackUrl(envValue)) { + return envValue; + } + // eslint-disable-next-line no-console + console.warn(`Ignoring ${label} override "${envValue}": only *.browserstack.com, *.bsstag.com or localhost URLs are allowed.`); + return currentValue; +}; + if(config.env !== "production") { // load config based on env require('custom-env').env(config.env); - config.uploadUrl = process.env.UPLOAD_URL; - config.rails_host = process.env.RAILS_HOST; - config.dashboardUrl = process.env.DASHBOARD_URL; - config.usageReportingUrl = process.env.USAGE_REPORTING_URL; + config.uploadUrl = applyUrlOverride(process.env.UPLOAD_URL, config.uploadUrl, "UPLOAD_URL"); + config.rails_host = applyUrlOverride(process.env.RAILS_HOST, config.rails_host, "RAILS_HOST"); + config.dashboardUrl = applyUrlOverride(process.env.DASHBOARD_URL, config.dashboardUrl, "DASHBOARD_URL"); + config.usageReportingUrl = applyUrlOverride(process.env.USAGE_REPORTING_URL, config.usageReportingUrl, "USAGE_REPORTING_URL"); } config.cypress_v1 = `${config.rails_host}/automate/cypress/v1`; diff --git a/bin/helpers/getInitialDetails.js b/bin/helpers/getInitialDetails.js index 61b1539d..ba3a8a86 100644 --- a/bin/helpers/getInitialDetails.js +++ b/bin/helpers/getInitialDetails.js @@ -6,6 +6,7 @@ const logger = require('./logger').winstonLogger, Constants = require('./constants'); const { setAxiosProxy } = require('./helper'); +const { isAllowedBrowserstackUrl } = require('./securityValidation'); exports.getInitialDetails = (bsConfig, args, rawArgs) => { return new Promise(async (resolve, reject) => { @@ -40,7 +41,16 @@ exports.getInitialDetails = (bsConfig, args, rawArgs) => { resolve({}); } else { if (!utils.isUndefined(responseData.grr) && responseData.grr.enabled && !utils.isUndefined(responseData.grr.urls)) { - config.uploadUrl = responseData.grr.urls.upload_url; + // Validate the API-supplied upload_url before trusting it: a MITM / + // proxy could rewrite it to redirect the tests.zip upload to an + // attacker host (APS-19011). Only accept BrowserStack hosts; otherwise + // keep the default uploadUrl. + const grrUploadUrl = responseData.grr.urls.upload_url; + if (isAllowedBrowserstackUrl(grrUploadUrl)) { + config.uploadUrl = grrUploadUrl; + } else { + logger.warn(`Ignoring upload_url from API response (not a BrowserStack host): ${grrUploadUrl}`); + } } resolve(responseData); } diff --git a/bin/helpers/helper.js b/bin/helpers/helper.js index cf6461da..eece6452 100644 --- a/bin/helpers/helper.js +++ b/bin/helpers/helper.js @@ -457,6 +457,11 @@ exports.truncateString = (field, truncateSizeInBytes) => { exports.setAxiosProxy = (axiosConfig) => { if (process.env.HTTP_PROXY || process.env.HTTPS_PROXY) { const httpProxy = process.env.HTTP_PROXY || process.env.HTTPS_PROXY + // Warn that all API traffic (including Basic Auth credentials) is being + // routed through this proxy, which can read/rewrite it if it terminates TLS + // (APS-19011). We honour the proxy (corporate CIs need it) but no longer do + // so silently. + logger.warn(`An HTTP(S) proxy is configured (${httpProxy}); all BrowserStack API traffic, including credentials, will be routed through it.`); axiosConfig.proxy = false; axiosConfig.httpsAgent = new HttpsProxyAgent(httpProxy); }; diff --git a/bin/helpers/securityValidation.js b/bin/helpers/securityValidation.js new file mode 100644 index 00000000..20cb8688 --- /dev/null +++ b/bin/helpers/securityValidation.js @@ -0,0 +1,92 @@ +'use strict'; + +const path = require('path'); + +/** + * Security validation helpers shared across the CLI. + * + * These guard the "untrusted edges" of the CLI: + * - override/response URLs that could redirect API traffic or uploads + * (APS-19010, APS-19011) + * - config-file paths that could escape the project directory (APS-19008) + * + * Kept dependency-free (stdlib only) so the logic can be unit tested without + * pulling in the CLI's network/config stack. + */ + +// Hosts the CLI is allowed to talk to for API / upload endpoints. Covers +// production, staging (bsstag.com) and local development. Anything else is +// treated as attacker-controlled and rejected. +const ALLOWED_HOST_SUFFIXES = ['.browserstack.com', '.bsstag.com']; +const ALLOWED_EXACT_HOSTS = ['browserstack.com', 'bsstag.com', 'localhost', '127.0.0.1', '::1']; + +/** + * Returns true if the given URL points at a BrowserStack (prod/staging) host or + * localhost. Only http/https are accepted. Any parse failure returns false + * (fail-closed). + * @param {string} urlString + * @returns {boolean} + */ +function isAllowedBrowserstackUrl(urlString) { + if (typeof urlString !== 'string' || urlString.trim() === '') { + return false; + } + let parsed; + try { + parsed = new URL(urlString); + } catch (e) { + return false; + } + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') { + return false; + } + const host = parsed.hostname.toLowerCase(); + if (ALLOWED_EXACT_HOSTS.includes(host)) { + return true; + } + return ALLOWED_HOST_SUFFIXES.some((suffix) => host.endsWith(suffix)); +} + +/** + * Resolves a candidate path and asserts it stays inside baseDir. Used to stop + * config-file path traversal (e.g. --config-file ../../outside/browserstack.json). + * @param {string} candidatePath + * @param {string} baseDir defaults to process.cwd() + * @returns {boolean} + */ +function isPathInsideBase(candidatePath, baseDir) { + if (typeof candidatePath !== 'string' || candidatePath === '') { + return false; + } + const base = path.resolve(baseDir || process.cwd()); + const resolved = path.resolve(base, candidatePath); + // Must be the base itself or a descendant (base + separator prefix). + return resolved === base || resolved.startsWith(base + path.sep); +} + +/** + * Structural (NOT cryptographic) validation of a JWT: three non-empty + * base64url segments. The CLI is not the token issuer and has no key to verify + * the signature, so this only rejects obviously-malformed / MITM-swapped + * garbage tokens. Defence-in-depth, not an integrity guarantee. + * @param {string} token + * @returns {boolean} + */ +function isWellFormedJwt(token) { + if (typeof token !== 'string') { + return false; + } + const parts = token.split('.'); + if (parts.length !== 3) { + return false; + } + return parts.every((p) => /^[A-Za-z0-9_-]+$/.test(p)); +} + +module.exports = { + isAllowedBrowserstackUrl, + isPathInsideBase, + isWellFormedJwt, + ALLOWED_HOST_SUFFIXES, + ALLOWED_EXACT_HOSTS, +}; diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index b15414fb..e3bd48e9 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -13,6 +13,7 @@ const readdir = promisify(fs.readdir); const stat = promisify(fs.stat); const TIMEZONE = require("../helpers/timezone.json"); const { setAxiosProxy } = require('./helper'); +const { isPathInsideBase } = require('./securityValidation'); const usageReporting = require("./usageReporting"), logger = require("./logger").winstonLogger, @@ -33,17 +34,30 @@ exports.validateBstackJson = (bsConfigPath) => { return new Promise(function (resolve, reject) { try { logger.info(`Reading config from ${bsConfigPath}`); - let bsConfig = require(bsConfigPath); + // browserstack.json is a pure-JSON config, so parse it as data rather than + // require()-ing it (require executes any JS the file contains — a + // PR-supplied .js config would run arbitrary code, APS-19008). Also require + // a .json extension and that the file resolves inside the project root so a + // crafted --config-file cannot point outside the project or at a script. + const resolvedPath = path.resolve(bsConfigPath); + if (path.extname(resolvedPath).toLowerCase() !== ".json") { + return reject(`Invalid browserstack.json file. Error : config file must be a .json file.`); + } + if (!isPathInsideBase(resolvedPath, process.cwd())) { + return reject(`Invalid browserstack.json file. Error : config file must be inside the project directory.`); + } + if (!fs.existsSync(resolvedPath)) { + return reject( + "Couldn't find the browserstack.json file at \"" + + bsConfigPath + + '". Please use --config-file .' + ); + } + let bsConfig = JSON.parse(fs.readFileSync(resolvedPath, "utf8")); bsConfig = exports.normalizeTestReportingConfig(bsConfig); resolve(bsConfig); } catch (e) { - reject( - e.code === "MODULE_NOT_FOUND" - ? "Couldn't find the browserstack.json file at \"" + - bsConfigPath + - '". Please use --config-file .' - : `Invalid browserstack.json file. Error : ${e.message}` - ); + reject(`Invalid browserstack.json file. Error : ${e.message}`); } }); }; diff --git a/bin/testhub/utils.js b/bin/testhub/utils.js index 718bb595..547b10d8 100644 --- a/bin/testhub/utils.js +++ b/bin/testhub/utils.js @@ -4,6 +4,7 @@ const logger = require("../../bin/helpers/logger").winstonLogger; const TESTHUB_CONSTANTS = require("./constants"); const testObservabilityHelper = require("../../bin/testObservability/helper/helper"); const helper = require("../helpers/helper"); +const { isWellFormedJwt } = require("../helpers/securityValidation"); const accessibilityHelper = require("../accessibility-automation/helper"); const detectPort = require('detect-port'); @@ -232,7 +233,15 @@ exports.findAvailablePort = async (preferredPort, maxAttempts = 10) => { } exports.setTestHubCommonMetaInfo = (user_config, responseData) => { - process.env.BROWSERSTACK_TESTHUB_JWT = responseData.jwt; + // Structural (not cryptographic) sanity check on the JWT from the API + // response. The CLI has no key to verify the signature, so this only rejects + // obviously-malformed / MITM-swapped garbage tokens — defence-in-depth, not + // an integrity guarantee (APS-19011). + if (responseData && responseData.jwt !== undefined && !isWellFormedJwt(responseData.jwt)) { + logger.warn('Received a malformed TestHub JWT from the API response; ignoring it.'); + } else { + process.env.BROWSERSTACK_TESTHUB_JWT = responseData.jwt; + } process.env.BROWSERSTACK_TESTHUB_UUID = responseData.build_hashed_id; user_config.run_settings.system_env_vars.push(`BROWSERSTACK_TESTHUB_JWT`); user_config.run_settings.system_env_vars.push(`BROWSERSTACK_TESTHUB_UUID`); diff --git a/test/unit/bin/helpers/securityValidation.js b/test/unit/bin/helpers/securityValidation.js new file mode 100644 index 00000000..b88a1733 --- /dev/null +++ b/test/unit/bin/helpers/securityValidation.js @@ -0,0 +1,97 @@ +'use strict'; +const path = require('path'); +const { expect } = require('chai'); + +const { + isAllowedBrowserstackUrl, + isPathInsideBase, + isWellFormedJwt, +} = require('../../../../bin/helpers/securityValidation'); + +describe('securityValidation', () => { + describe('isAllowedBrowserstackUrl', () => { + it('accepts BrowserStack production and staging hosts', () => { + expect(isAllowedBrowserstackUrl('https://api.browserstack.com')).to.be.true; + expect(isAllowedBrowserstackUrl('https://api-cloud.browserstack.com/automate-frameworks/cypress/upload')).to.be.true; + expect(isAllowedBrowserstackUrl('https://staging.bsstag.com')).to.be.true; + expect(isAllowedBrowserstackUrl('https://browserstack.com')).to.be.true; + }); + + it('accepts localhost for local development', () => { + expect(isAllowedBrowserstackUrl('http://localhost:3000')).to.be.true; + expect(isAllowedBrowserstackUrl('http://127.0.0.1:8080')).to.be.true; + }); + + it('rejects arbitrary attacker hosts', () => { + expect(isAllowedBrowserstackUrl('https://attacker.example')).to.be.false; + expect(isAllowedBrowserstackUrl('https://evil.com')).to.be.false; + }); + + it('rejects look-alike / suffix-spoofing hosts', () => { + // Not a real subdomain of browserstack.com — endsWith check uses a + // leading dot so this must be rejected. + expect(isAllowedBrowserstackUrl('https://browserstack.com.attacker.net')).to.be.false; + expect(isAllowedBrowserstackUrl('https://notbrowserstack.com')).to.be.false; + expect(isAllowedBrowserstackUrl('https://evilbrowserstack.com')).to.be.false; + }); + + it('rejects non-http(s) schemes and malformed input', () => { + expect(isAllowedBrowserstackUrl('file:///etc/passwd')).to.be.false; + expect(isAllowedBrowserstackUrl('ftp://api.browserstack.com')).to.be.false; + expect(isAllowedBrowserstackUrl('not a url')).to.be.false; + expect(isAllowedBrowserstackUrl('')).to.be.false; + expect(isAllowedBrowserstackUrl(undefined)).to.be.false; + expect(isAllowedBrowserstackUrl(null)).to.be.false; + }); + }); + + describe('isPathInsideBase', () => { + const base = path.resolve('/tmp/project'); + + it('accepts paths inside the base directory', () => { + expect(isPathInsideBase('browserstack.json', base)).to.be.true; + expect(isPathInsideBase('sub/dir/browserstack.json', base)).to.be.true; + expect(isPathInsideBase(path.join(base, 'browserstack.json'), base)).to.be.true; + }); + + it('accepts the base directory itself', () => { + expect(isPathInsideBase(base, base)).to.be.true; + }); + + it('rejects path traversal outside the base directory', () => { + expect(isPathInsideBase('../../etc/passwd', base)).to.be.false; + expect(isPathInsideBase('../outside/browserstack.json', base)).to.be.false; + expect(isPathInsideBase('/etc/passwd', base)).to.be.false; + }); + + it('rejects a sibling directory that shares a name prefix', () => { + // /tmp/project-evil must not be treated as inside /tmp/project. + expect(isPathInsideBase('/tmp/project-evil/x.json', base)).to.be.false; + }); + + it('rejects empty / non-string input', () => { + expect(isPathInsideBase('', base)).to.be.false; + expect(isPathInsideBase(undefined, base)).to.be.false; + }); + }); + + describe('isWellFormedJwt', () => { + it('accepts a structurally valid three-part token', () => { + expect(isWellFormedJwt('aaa.bbb.ccc')).to.be.true; + expect(isWellFormedJwt('eyJhbGci.eyJzdWIi.SflKxwRJ-abc_123')).to.be.true; + }); + + it('rejects tokens without exactly three parts', () => { + expect(isWellFormedJwt('aaa.bbb')).to.be.false; + expect(isWellFormedJwt('aaa.bbb.ccc.ddd')).to.be.false; + expect(isWellFormedJwt('notajwt')).to.be.false; + }); + + it('rejects tokens with empty or invalid segments', () => { + expect(isWellFormedJwt('aaa..ccc')).to.be.false; + expect(isWellFormedJwt('aaa.b b.ccc')).to.be.false; + expect(isWellFormedJwt('')).to.be.false; + expect(isWellFormedJwt(undefined)).to.be.false; + }); + }); +}); From ad117a0fccbaea5073c6d79afbaa0a4df4fd2321 Mon Sep 17 00:00:00 2001 From: Rohan <142411639+Rohannagariya1@users.noreply.github.com> Date: Wed, 5 Aug 2026 17:29:38 +0530 Subject: [PATCH 2/6] [APS-19009] security(cli): --ignore-scripts + validate npm_dependencies names/versions browserstack.json's npm_dependencies were merged into a temp package.json and installed with `npm install` and no `--ignore-scripts`, so a PR-supplied malicious package's lifecycle script (postinstall) executed on CI (RCE, credential theft). - Add `--ignore-scripts` to both npm install invocations (the RCE fix). npm_dependencies is documented pure-JS only. - Validate each dependency name (standard npm package-name regex) and version (semver/dist-tag charset only) before writing package.json, rejecting git-url / file: / path / alternate-registry specs (dependency confusion / code-exec via spec). - shell:true is retained deliberately: the command line is fully static (names live in package.json data, never on the command line -> no injection surface) and it is required for the output redirection and for invoking npm.cmd on Windows. Tested: validation rejects shell-metachar/git-url/file/$() specs, accepts normal semver; --ignore-scripts present in both installs; syntax clean. Co-Authored-By: Claude Opus 4.8 --- bin/helpers/packageInstaller.js | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/bin/helpers/packageInstaller.js b/bin/helpers/packageInstaller.js index 20823626..1ba90214 100644 --- a/bin/helpers/packageInstaller.js +++ b/bin/helpers/packageInstaller.js @@ -33,6 +33,18 @@ const setupPackageFolder = (runSettings, directoryPath) => { // Combine win and mac specific dependencies if present const combinedDependencies = combineMacWinNpmDependencies(runSettings); + // APS-19009: only allow standard npm package names + semver/dist-tag versions + // before writing them to package.json, so a browserstack.json cannot smuggle a + // git-url / file: / path / alternate-registry spec (dependency confusion or code + // execution) into `npm install`. + const NPM_NAME_RE = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; + const NPM_VERSION_RE = /^[A-Za-z0-9.\-+~^><=|*\s]+$/; + for (const depName of Object.keys(combinedDependencies || {})) { + const depVersion = combinedDependencies[depName]; + if (!NPM_NAME_RE.test(depName) || typeof depVersion !== 'string' || !NPM_VERSION_RE.test(depVersion)) { + return reject(`Invalid npm_dependencies entry "${depName}": only standard package names and semver/dist-tag versions are allowed.`); + } + } if (combinedDependencies && Object.keys(combinedDependencies).length > 0) { Object.assign(packageJSON, { devDependencies: combinedDependencies, @@ -97,12 +109,18 @@ const packageInstall = (packageDir, bsConfig) => { // add --legacy-peer-deps flag while installing dependencies for npm v7+ // For more info please read "Peer Dependencies" section here -> https://github.blog/2021-02-02-npm-7-is-now-generally-available/ + // APS-19009: --ignore-scripts prevents a user-supplied npm_dependencies package from + // executing lifecycle scripts (postinstall etc.) during this install, which was an RCE + // on CI. npm_dependencies is documented as pure-JS only. shell:true is retained on + // purpose: the command line is fully static (package names live in package.json, never + // on the command line, so there is no injection surface) and it is required for the + // output redirection and for invoking npm.cmd on Windows. if (parseInt(npm_major_version) >= 7) { - logger.debug(`Running NPM install command: npm install --legacy-peer-deps --loglevel verbose > ../npm_install_debug.log`); - nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--legacy-peer-deps', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true}); + logger.debug(`Running NPM install command: npm install --legacy-peer-deps --ignore-scripts --loglevel verbose > ../npm_install_debug.log`); + 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}); } else { - logger.debug(`Running NPM install command: 'npm install --loglevel verbose > ../npm_install_debug.log'`); - nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true}); + logger.debug(`Running NPM install command: 'npm install --ignore-scripts --loglevel verbose > ../npm_install_debug.log'`); + nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--ignore-scripts', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true}); } nodeProcess.on('close', nodeProcessCloseCallback); nodeProcess.on('error', nodeProcessErrorCallback); From 70fbc8e65e9e174220528472668729d20f68d4e1 Mon Sep 17 00:00:00 2001 From: Rohan <142411639+Rohannagariya1@users.noreply.github.com> Date: Thu, 6 Aug 2026 16:52:59 +0530 Subject: [PATCH 3/6] [APS-19009] test: assert --ignore-scripts + reject malicious npm_dependencies Locks the two security invariants the fix introduces: - packageInstall passes --ignore-scripts to the npm spawn (lifecycle-script RCE guard) - setupPackageFolder rejects a non-semver/git-url npm_dependencies version and never writes package.json (dependency-confusion / spec smuggling guard) Co-Authored-By: Claude Opus 4.8 --- test/unit/bin/helpers/packageInstaller.js | 64 +++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/test/unit/bin/helpers/packageInstaller.js b/test/unit/bin/helpers/packageInstaller.js index e45a8f46..ab6d5e35 100644 --- a/test/unit/bin/helpers/packageInstaller.js +++ b/test/unit/bin/helpers/packageInstaller.js @@ -210,6 +210,41 @@ describe("packageInstaller", () => { chai.assert.fail("Promise error"); }); }); + + it("should reject a malicious npm_dependencies entry and not write package.json (APS-19009)", () => { + packageInstaller.__set__({ + fileHelpers: {deletePackageArchieve: fileHelpersStub}, + fs: { + mkdir: fsmkdirStub, + writeFileSync: fswriteFileSyncStub, + existsSync: fsexistsSyncStub, + copyFileSync: fscopyFileSyncStub + }, + path: { + dirname: pathdirnameStub, + join: pathjoinStub + } + }); + let setupPackageFolderrewire = packageInstaller.__get__('setupPackageFolder'); + let runSettings = { + package_config_options: { + "name": "test" + }, + npm_dependencies: { + // git-url version smuggles a non-registry spec into npm install -> must be rejected + "evil-pkg": "git+ssh://git@github.com/attacker/evil.git" + } + }; + let directoryPath = "/random/path"; + return setupPackageFolderrewire(runSettings, directoryPath) + .then((_data) => { + chai.assert.fail("expected rejection for malicious npm_dependencies entry"); + }) + .catch((error) => { + chai.assert.match(error, /Invalid npm_dependencies entry "evil-pkg"/); + sinon.assert.notCalled(fswriteFileSyncStub); + }); + }); }); context("packageInstall", () => { @@ -271,6 +306,35 @@ describe("packageInstaller", () => { }); }); + it("should pass --ignore-scripts to the npm install spawn (APS-19009 lifecycle-script RCE guard)", () => { + let spawnStub = sandbox.stub(cp, 'spawn').returns({ + on: (_close, nodeProcessCloseCallback) => { + nodeProcessCloseCallback(0); + } + }); + let getMajorVersionStub = sandbox.stub(utils, 'getMajorVersion').returns('7'); + packageInstaller.__set__({ + nodeProcess: {}, + spawn: spawnStub, + utils: { + getMajorVersion: getMajorVersionStub + } + }); + let packageInstallrewire = packageInstaller.__get__('packageInstall'); + let directoryPath = "/random/path"; + return packageInstallrewire(directoryPath) + .then((_data) => { + sinon.assert.calledOnce(spawnStub); + let spawnArgs = spawnStub.firstCall.args[1]; + chai.assert.include(spawnArgs, '--ignore-scripts'); + spawnStub.restore(); + getMajorVersionStub.restore(); + }) + .catch((_error) => { + chai.assert.fail(`Promise error ${_error}`); + }); + }); + it("should call npm install on directory and reject if spawn is not closed successfully", () => { let spawnStub = sandbox.stub(cp, 'spawn').returns({ on: (_close, nodeProcessCloseCallback) => { From fba71e00b6d7906dbad22f4865a117aef5dcf818 Mon Sep 17 00:00:00 2001 From: Rohan <142411639+Rohannagariya1@users.noreply.github.com> Date: Thu, 6 Aug 2026 18:43:14 +0530 Subject: [PATCH 4/6] [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 --- bin/helpers/packageInstaller.js | 8 +++++- bin/helpers/securityValidation.js | 2 ++ bin/helpers/utils.js | 1 + test/unit/bin/helpers/packageInstaller.js | 32 +++++++++++++++++++++++ 4 files changed, 42 insertions(+), 1 deletion(-) diff --git a/bin/helpers/packageInstaller.js b/bin/helpers/packageInstaller.js index 1ba90214..7dc81e0a 100644 --- a/bin/helpers/packageInstaller.js +++ b/bin/helpers/packageInstaller.js @@ -37,7 +37,11 @@ const setupPackageFolder = (runSettings, directoryPath) => { // before writing them to package.json, so a browserstack.json cannot smuggle a // git-url / file: / path / alternate-registry spec (dependency confusion or code // execution) into `npm install`. - const NPM_NAME_RE = /^(@[a-z0-9-~][a-z0-9-._~]*\/)?[a-z0-9-~][a-z0-9-._~]*$/; + // Allow upper-case too: legacy registry packages (e.g. JSONStream) have + // capitals and must not be rejected. This still blocks git-url / file: / + // path / alternate-registry specs (those contain :, /, .. which are not in + // the class), which is the actual dependency-confusion / RCE guard. + const NPM_NAME_RE = /^(@[a-zA-Z0-9-~][a-zA-Z0-9-._~]*\/)?[a-zA-Z0-9-~][a-zA-Z0-9-._~]*$/; const NPM_VERSION_RE = /^[A-Za-z0-9.\-+~^><=|*\s]+$/; for (const depName of Object.keys(combinedDependencies || {})) { const depVersion = combinedDependencies[depName]; @@ -117,9 +121,11 @@ const packageInstall = (packageDir, bsConfig) => { // output redirection and for invoking npm.cmd on Windows. if (parseInt(npm_major_version) >= 7) { logger.debug(`Running NPM install command: npm install --legacy-peer-deps --ignore-scripts --loglevel verbose > ../npm_install_debug.log`); + // 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. 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}); } else { logger.debug(`Running NPM install command: 'npm install --ignore-scripts --loglevel verbose > ../npm_install_debug.log'`); + // 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. nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--ignore-scripts', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true}); } nodeProcess.on('close', nodeProcessCloseCallback); diff --git a/bin/helpers/securityValidation.js b/bin/helpers/securityValidation.js index 20cb8688..c9c75517 100644 --- a/bin/helpers/securityValidation.js +++ b/bin/helpers/securityValidation.js @@ -58,7 +58,9 @@ function isPathInsideBase(candidatePath, baseDir) { if (typeof candidatePath !== 'string' || candidatePath === '') { return false; } + // 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`. const base = path.resolve(baseDir || process.cwd()); + // 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. const resolved = path.resolve(base, candidatePath); // Must be the base itself or a descendant (base + separator prefix). return resolved === base || resolved.startsWith(base + path.sep); diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index e3bd48e9..2d930661 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -39,6 +39,7 @@ exports.validateBstackJson = (bsConfigPath) => { // PR-supplied .js config would run arbitrary code, APS-19008). Also require // a .json extension and that the file resolves inside the project root so a // crafted --config-file cannot point outside the project or at a script. + // 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. const resolvedPath = path.resolve(bsConfigPath); if (path.extname(resolvedPath).toLowerCase() !== ".json") { return reject(`Invalid browserstack.json file. Error : config file must be a .json file.`); diff --git a/test/unit/bin/helpers/packageInstaller.js b/test/unit/bin/helpers/packageInstaller.js index ab6d5e35..263b0a7b 100644 --- a/test/unit/bin/helpers/packageInstaller.js +++ b/test/unit/bin/helpers/packageInstaller.js @@ -245,6 +245,38 @@ describe("packageInstaller", () => { sinon.assert.notCalled(fswriteFileSyncStub); }); }); + + it("should accept a legacy upper-case npm package name (e.g. JSONStream) (APS-19009)", () => { + packageInstaller.__set__({ + fileHelpers: {deletePackageArchieve: fileHelpersStub}, + fs: { + mkdir: fsmkdirStub, + writeFileSync: fswriteFileSyncStub, + existsSync: fsexistsSyncStub, + copyFileSync: fscopyFileSyncStub + }, + path: { + dirname: pathdirnameStub, + join: pathjoinStub + } + }); + let setupPackageFolderrewire = packageInstaller.__get__('setupPackageFolder'); + let runSettings = { + npm_dependencies: { + "JSONStream": "1.3.5" + } + }; + let directoryPath = "/random/path"; + return setupPackageFolderrewire(runSettings, directoryPath) + .then((data) => { + sinon.assert.calledOnce(fswriteFileSyncStub); + chai.assert.equal(data, "Package file created"); + }) + .catch((_error) => { + console.log(_error); + chai.assert.fail("legacy upper-case package name should be accepted"); + }); + }); }); context("packageInstall", () => { From 37ef6adcd70df71d7885679e7ecde2d1e0d0ee66 Mon Sep 17 00:00:00 2001 From: Rohan <142411639+Rohannagariya1@users.noreply.github.com> Date: Thu, 6 Aug 2026 18:44:34 +0530 Subject: [PATCH 5/6] [APS-19009] sync securityValidation.js with #1141 (IPv6 [::1] allowlist fix) Co-Authored-By: Claude Opus 4.8 --- bin/helpers/securityValidation.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/bin/helpers/securityValidation.js b/bin/helpers/securityValidation.js index c9c75517..ec2114fe 100644 --- a/bin/helpers/securityValidation.js +++ b/bin/helpers/securityValidation.js @@ -18,7 +18,8 @@ const path = require('path'); // production, staging (bsstag.com) and local development. Anything else is // treated as attacker-controlled and rejected. const ALLOWED_HOST_SUFFIXES = ['.browserstack.com', '.bsstag.com']; -const ALLOWED_EXACT_HOSTS = ['browserstack.com', 'bsstag.com', 'localhost', '127.0.0.1', '::1']; +// Note: URL parsing yields '[::1]' (bracketed) as the hostname for IPv6 loopback. +const ALLOWED_EXACT_HOSTS = ['browserstack.com', 'bsstag.com', 'localhost', '127.0.0.1', '[::1]']; /** * Returns true if the given URL points at a BrowserStack (prod/staging) host or From 4e0aa6f6ddb04dc2f03d302bd3f418ec9155d4b4 Mon Sep 17 00:00:00 2001 From: Rohan <142411639+Rohannagariya1@users.noreply.github.com> Date: Thu, 6 Aug 2026 18:50:44 +0530 Subject: [PATCH 6/6] [APS-19009] move spawn-shell-true nosemgrep to same line (code-scanning honors same-line) Co-Authored-By: Claude Opus 4.8 --- bin/helpers/packageInstaller.js | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/bin/helpers/packageInstaller.js b/bin/helpers/packageInstaller.js index 7dc81e0a..e3fd49c2 100644 --- a/bin/helpers/packageInstaller.js +++ b/bin/helpers/packageInstaller.js @@ -121,12 +121,10 @@ const packageInstall = (packageDir, bsConfig) => { // output redirection and for invoking npm.cmd on Windows. if (parseInt(npm_major_version) >= 7) { logger.debug(`Running NPM install command: npm install --legacy-peer-deps --ignore-scripts --loglevel verbose > ../npm_install_debug.log`); - // 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. - 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}); + 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}); // nosemgrep: javascript.lang.security.audit.spawn-shell-true.spawn-shell-true } else { logger.debug(`Running NPM install command: 'npm install --ignore-scripts --loglevel verbose > ../npm_install_debug.log'`); - // 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. - nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--ignore-scripts', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true}); + nodeProcess = spawn(/^win/.test(process.platform) ? 'npm.cmd' : 'npm', ['install', '--ignore-scripts', '--loglevel', 'verbose', '>', '../npm_install_debug.log', '2>&1'], {cwd: packageDir, shell: true}); // nosemgrep: javascript.lang.security.audit.spawn-shell-true.spawn-shell-true } nodeProcess.on('close', nodeProcessCloseCallback); nodeProcess.on('error', nodeProcessErrorCallback);