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