[APS-19009] security(cli): --ignore-scripts + validate npm_dependencies (lifecycle-script RCE)

bin/commands/runs.js

@@ -189,7 +189,10 @@ module.exports = function run(args, rawArgs) {
       }
     }
 
-    const { packagesInstalled } = !isBrowserstackInfra ? false : await packageInstaller.packageSetupAndInstaller(bsConfig, config.packageDirName, {markBlockStart, markBlockEnd});
+    let packagesInstalled;
+    if (isBrowserstackInfra) {
+      ({ packagesInstalled } = await packageInstaller.packageSetupAndInstaller(bsConfig, config.packageDirName, {markBlockStart, markBlockEnd}));
+    }
 
     if(isBrowserstackInfra) {
       // set node version

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`;

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);
       }

bin/helpers/helper.js

@@ -468,6 +468,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);
   };

bin/helpers/packageInstaller.js

@@ -33,9 +33,23 @@
 
         // Combine win and mac specific dependencies if present
         const combinedDependencies = combineMacWinNpmDependencies(runSettings);
-        if (combinedDependencies && Object.keys(combinedDependencies).length > 0) {
+        // APS-19009: drop any dependency whose name is not a valid npm package name (strips
+        // shell-metacharacter payloads like "left-pad; cat /flag"). Version specs are NOT validated
+        // (git / file: / tarball-url are legitimate), and a bad entry is skipped, not aborted, so a
+        // customer session is never blocked. RCE itself is closed by --ignore-scripts (see below).
+        const NPM_NAME_RE = /^(@[a-zA-Z0-9-~][a-zA-Z0-9-._~]*\/)?[a-zA-Z0-9-~][a-zA-Z0-9-._~]*$/;
+        const safeDependencies = {};
+        for (const depName of Object.keys(combinedDependencies || {})) {
+          const depVersion = combinedDependencies[depName];
+          if (!NPM_NAME_RE.test(depName) || typeof depVersion !== 'string') {
+            logger.warn(`Skipping npm_dependencies entry "${depName}": not a valid npm package name. This dependency will not be installed.`);
+            continue;
+          }
+          safeDependencies[depName] = depVersion;
+        }
+        if (Object.keys(safeDependencies).length > 0) {
           Object.assign(packageJSON, {
-            devDependencies: combinedDependencies,
+            devDependencies: safeDependencies,
           });
         }
 
@@ -97,12 +111,15 @@
 
     // 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 blocks lifecycle-script (postinstall) RCE. shell:true is kept
+    // on purpose — the command line is static (package names live in package.json, not on the CLI)
+    // and shell:true is needed for the output redirection and for 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}); // nosemgrep: javascript.lang.security.audit.spawn-shell-true.spawn-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}); // nosemgrep: javascript.lang.security.audit.spawn-shell-true.spawn-shell-true
     }
     nodeProcess.on('close', nodeProcessCloseCallback);
     nodeProcess.on('error', nodeProcessErrorCallback);

bin/helpers/securityValidation.js

@@ -0,0 +1,95 @@
+'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'];
+// 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
+ * 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;
+  }
+  // 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);
+}
+
+/**
+ * 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,
+};

bin/helpers/utils.js

@@ -13,6 +13,7 @@
 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,31 @@
   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.
+      // 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.`);
+      }
+      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 <path to browserstack.json>.'
+        );
+      }
+      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 <path to browserstack.json>.'
-          : `Invalid browserstack.json file. Error : ${e.message}`
-      );
+      reject(`Invalid browserstack.json file. Error : ${e.message}`);
     }
   });
 };

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`);