security: validate override/response URLs, config path, JWT; warn on proxy

Rohannagariya1 · claude · Rohannagariya1 · commit e578fadb257e · 2026-07-02T18:02:26.000+05:30
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 <noreply@anthropic.com>
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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 <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}`);
     }
   });
 };
diff --git 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
@@ -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;
+    });
+  });
+});
