From 024cd0ab8ac283e4eb5fa00c154852ff37693eec Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Fri, 30 Oct 2026 12:04:42 +0530 Subject: [PATCH 01/11] Forward the Test Observability collector URL to the remote reporter The reporter that sends TestRunStarted/TestRunFinished runs on the BrowserStack machine from its own install of this package, which has the prod collector hard-coded. Against any other collector (staging environments) those events never arrive, so TestHub keeps nameless test cases and Accessibility collapses every test in a spec into one. Make API_URL overridable via BROWSERSTACK_TEST_OBSERVABILITY_API_URL and forward the launching cli's effective collector in system_env_vars, the same channel BS_TESTOPS_JWT already uses. Prod behaviour is unchanged. Jira: A11Y-13909 Co-Authored-By: Claude Fable 5.1 --- bin/helpers/utils.js | 4 ++- bin/testObservability/helper/constants.js | 3 ++- test/unit/bin/helpers/utils.js | 30 +++++++++++++++++++++++ 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index a6903720..7acaa4a1 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -31,7 +31,7 @@ const usageReporting = require("./usageReporting"), pkg = require('../../package.json'), transports = require('./logger').transports, o11yHelpers = require('../testObservability/helper/helper'), - { OBSERVABILITY_ENV_VARS, TEST_OBSERVABILITY_REPORTER } = require('../testObservability/helper/constants'); + { OBSERVABILITY_ENV_VARS, TEST_OBSERVABILITY_REPORTER, API_URL: OBSERVABILITY_API_URL } = require('../testObservability/helper/constants'); const { default: axios } = require("axios"); const { shouldProcessEventForTesthub } = require("../testhub/utils"); @@ -614,6 +614,8 @@ exports.setSystemEnvs = (bsConfig) => { OBSERVABILITY_ENV_VARS.forEach(key => { envKeys[key] = process.env[key]; }); + // Remote reporter must post to the collector this build was created on, not its package default + envKeys['BROWSERSTACK_TEST_OBSERVABILITY_API_URL'] = OBSERVABILITY_API_URL; let gitConfigPath = o11yHelpers.findGitConfig(process.cwd()); if(!o11yHelpers.isBrowserstackInfra()) process.env.OBSERVABILITY_GIT_CONFIG_PATH_LOCAL = gitConfigPath; diff --git a/bin/testObservability/helper/constants.js b/bin/testObservability/helper/constants.js index d3013f40..40475f5f 100644 --- a/bin/testObservability/helper/constants.js +++ b/bin/testObservability/helper/constants.js @@ -3,7 +3,8 @@ const path = require('path'); exports.consoleHolder = Object.assign({},console); exports.BATCH_SIZE = 1000; exports.BATCH_INTERVAL = 2000; -exports.API_URL = 'https://collector-observability.browserstack.com'; +// Overridable so the remote reporter (own install of this package) can target the launching cli's collector +exports.API_URL = process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL || 'https://collector-observability.browserstack.com'; exports.IPC_EVENTS = { LOG: 'testObservability:cypressLog', diff --git a/test/unit/bin/helpers/utils.js b/test/unit/bin/helpers/utils.js index 480cb5f4..7429a43a 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -961,6 +961,36 @@ describe('utils', () => { delete process.env.CYPRESS_TEST_1; delete process.env.cypress_test_2; }); + + it('forwards the observability collector url for the remote reporter', () => { + let bsConfig = { run_settings: { env: null } }; + + utils.setSystemEnvs(bsConfig); + expect(bsConfig.run_settings.system_env_vars) + .to.be.an('array') + .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-observability.browserstack.com'); + }); + + it('forwards an overridden observability collector url', () => { + const constantsPath = require.resolve('../../../../bin/testObservability/helper/constants'); + const utilsPath = require.resolve('../../../../bin/helpers/utils'); + process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'https://collector-example.bsstag.com'; + delete require.cache[constantsPath]; + delete require.cache[utilsPath]; + try { + const freshUtils = require('../../../../bin/helpers/utils'); + let bsConfig = { run_settings: { env: null } }; + freshUtils.setSystemEnvs(bsConfig); + expect(require(constantsPath).API_URL).to.equal('https://collector-example.bsstag.com'); + expect(bsConfig.run_settings.system_env_vars) + .to.be.an('array') + .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-example.bsstag.com'); + } finally { + delete process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL; + delete require.cache[constantsPath]; + delete require.cache[utilsPath]; + } + }); }); describe('fixCommaSeparatedString', () => { From bf87243ae85deee0fafee88cf80e24c1098ec0ff Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Fri, 30 Oct 2026 12:51:54 +0530 Subject: [PATCH 02/11] Allow a separate collector for the remote reporter On staging the collector the cli talks to is a private ingress the BrowserStack machine cannot reach, while the machine-facing public ingress is a different host. BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL, when set, is what gets forwarded to the machine; otherwise the cli's own collector is forwarded as before. Read at call time so the tests need no module reloads. Jira: A11Y-13909 Co-Authored-By: Claude Fable 5.1 --- bin/helpers/utils.js | 6 ++--- bin/testObservability/helper/constants.js | 6 ++++- test/unit/bin/helpers/utils.js | 29 +++++++++++++++-------- 3 files changed, 27 insertions(+), 14 deletions(-) diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index 7acaa4a1..c1601846 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -31,7 +31,7 @@ const usageReporting = require("./usageReporting"), pkg = require('../../package.json'), transports = require('./logger').transports, o11yHelpers = require('../testObservability/helper/helper'), - { OBSERVABILITY_ENV_VARS, TEST_OBSERVABILITY_REPORTER, API_URL: OBSERVABILITY_API_URL } = require('../testObservability/helper/constants'); + { OBSERVABILITY_ENV_VARS, TEST_OBSERVABILITY_REPORTER, remoteReporterApiUrl } = require('../testObservability/helper/constants'); const { default: axios } = require("axios"); const { shouldProcessEventForTesthub } = require("../testhub/utils"); @@ -614,8 +614,8 @@ exports.setSystemEnvs = (bsConfig) => { OBSERVABILITY_ENV_VARS.forEach(key => { envKeys[key] = process.env[key]; }); - // Remote reporter must post to the collector this build was created on, not its package default - envKeys['BROWSERSTACK_TEST_OBSERVABILITY_API_URL'] = OBSERVABILITY_API_URL; + // Remote reporter must post to this build's collector, not its package default + envKeys['BROWSERSTACK_TEST_OBSERVABILITY_API_URL'] = remoteReporterApiUrl(); let gitConfigPath = o11yHelpers.findGitConfig(process.cwd()); if(!o11yHelpers.isBrowserstackInfra()) process.env.OBSERVABILITY_GIT_CONFIG_PATH_LOCAL = gitConfigPath; diff --git a/bin/testObservability/helper/constants.js b/bin/testObservability/helper/constants.js index 40475f5f..040503bb 100644 --- a/bin/testObservability/helper/constants.js +++ b/bin/testObservability/helper/constants.js @@ -3,8 +3,12 @@ const path = require('path'); exports.consoleHolder = Object.assign({},console); exports.BATCH_SIZE = 1000; exports.BATCH_INTERVAL = 2000; +const DEFAULT_API_URL = 'https://collector-observability.browserstack.com'; // Overridable so the remote reporter (own install of this package) can target the launching cli's collector -exports.API_URL = process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL || 'https://collector-observability.browserstack.com'; +exports.API_URL = process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL || DEFAULT_API_URL; +// Collector handed to the reporter on the remote machine; separate override for when it cannot reach API_URL +exports.remoteReporterApiUrl = () => process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL + || process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL || DEFAULT_API_URL; exports.IPC_EVENTS = { LOG: 'testObservability:cypressLog', diff --git a/test/unit/bin/helpers/utils.js b/test/unit/bin/helpers/utils.js index 7429a43a..41e0fe38 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -972,23 +972,32 @@ describe('utils', () => { }); it('forwards an overridden observability collector url', () => { - const constantsPath = require.resolve('../../../../bin/testObservability/helper/constants'); - const utilsPath = require.resolve('../../../../bin/helpers/utils'); process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'https://collector-example.bsstag.com'; - delete require.cache[constantsPath]; - delete require.cache[utilsPath]; + let bsConfig = { run_settings: { env: null } }; + try { - const freshUtils = require('../../../../bin/helpers/utils'); - let bsConfig = { run_settings: { env: null } }; - freshUtils.setSystemEnvs(bsConfig); - expect(require(constantsPath).API_URL).to.equal('https://collector-example.bsstag.com'); + utils.setSystemEnvs(bsConfig); expect(bsConfig.run_settings.system_env_vars) .to.be.an('array') .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-example.bsstag.com'); } finally { delete process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL; - delete require.cache[constantsPath]; - delete require.cache[utilsPath]; + } + }); + + it('prefers a separate remote collector url for the remote reporter', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'https://collector-private.bsstag.com'; + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com'; + let bsConfig = { run_settings: { env: null } }; + + try { + utils.setSystemEnvs(bsConfig); + expect(bsConfig.run_settings.system_env_vars) + .to.be.an('array') + .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-public.bsstag.com'); + } finally { + delete process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL; + delete process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL; } }); }); From 0166a03a9ceb26a8d48a4288cfff79a062b16e16 Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Fri, 30 Oct 2026 20:02:47 +0530 Subject: [PATCH 03/11] Require https for the collector overrides and isolate the env vars in tests Review feedback on #1210. Collector requests carry BS_TESTOPS_JWT, so an http:// override would send it in cleartext: reject such a value instead of falling back to the default collector. The assignment moves out of the try block in setSystemEnvs because that block swallows errors, which would have turned the rejection into a silent fallback. Tests now clear both URL variables before each case and restore the originals after. Jira: A11Y-13909 Co-Authored-By: Claude Opus 5 (1M context) --- bin/helpers/utils.js | 6 ++- bin/testObservability/helper/constants.js | 18 ++++++-- test/unit/bin/helpers/utils.js | 55 ++++++++++++++++------- 3 files changed, 57 insertions(+), 22 deletions(-) diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index c1601846..c15cc69d 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -610,12 +610,14 @@ exports.setSystemEnvs = (bsConfig) => { logger.error(`Error in adding accessibility configs ${error}`) } + // Remote reporter must post to this build's collector, not its package default. Kept out of the + // try below, which swallows errors: a rejected override must fail rather than fall back silently. + envKeys['BROWSERSTACK_TEST_OBSERVABILITY_API_URL'] = remoteReporterApiUrl(); + try { OBSERVABILITY_ENV_VARS.forEach(key => { envKeys[key] = process.env[key]; }); - // Remote reporter must post to this build's collector, not its package default - envKeys['BROWSERSTACK_TEST_OBSERVABILITY_API_URL'] = remoteReporterApiUrl(); let gitConfigPath = o11yHelpers.findGitConfig(process.cwd()); if(!o11yHelpers.isBrowserstackInfra()) process.env.OBSERVABILITY_GIT_CONFIG_PATH_LOCAL = gitConfigPath; diff --git a/bin/testObservability/helper/constants.js b/bin/testObservability/helper/constants.js index 040503bb..da6ebe18 100644 --- a/bin/testObservability/helper/constants.js +++ b/bin/testObservability/helper/constants.js @@ -4,11 +4,23 @@ exports.consoleHolder = Object.assign({},console); exports.BATCH_SIZE = 1000; exports.BATCH_INTERVAL = 2000; const DEFAULT_API_URL = 'https://collector-observability.browserstack.com'; + +// Requests to the collector carry BS_TESTOPS_JWT, so an http:// override would put the build's token +// on the wire in cleartext. Reject it instead of quietly using the default collector. +const httpsOverride = (name) => { + const value = process.env[name]; + if (!value) return undefined; + if (!value.startsWith('https://')) { + throw new Error(`${name} must be an https:// URL, got "${value}"`); + } + return value; +}; + // Overridable so the remote reporter (own install of this package) can target the launching cli's collector -exports.API_URL = process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL || DEFAULT_API_URL; +exports.API_URL = httpsOverride('BROWSERSTACK_TEST_OBSERVABILITY_API_URL') || DEFAULT_API_URL; // Collector handed to the reporter on the remote machine; separate override for when it cannot reach API_URL -exports.remoteReporterApiUrl = () => process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL - || process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL || DEFAULT_API_URL; +exports.remoteReporterApiUrl = () => httpsOverride('BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL') + || httpsOverride('BROWSERSTACK_TEST_OBSERVABILITY_API_URL') || DEFAULT_API_URL; exports.IPC_EVENTS = { LOG: 'testObservability:cypressLog', diff --git a/test/unit/bin/helpers/utils.js b/test/unit/bin/helpers/utils.js index 41e0fe38..d83ebc4f 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -962,6 +962,29 @@ describe('utils', () => { delete process.env.cypress_test_2; }); + const URL_ENV_VARS = [ + 'BROWSERSTACK_TEST_OBSERVABILITY_API_URL', + 'BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL', + ]; + let savedUrlEnvs = {}; + + beforeEach(() => { + URL_ENV_VARS.forEach((key) => { + savedUrlEnvs[key] = process.env[key]; + delete process.env[key]; + }); + }); + + afterEach(() => { + URL_ENV_VARS.forEach((key) => { + if (savedUrlEnvs[key] === undefined) { + delete process.env[key]; + } else { + process.env[key] = savedUrlEnvs[key]; + } + }); + }); + it('forwards the observability collector url for the remote reporter', () => { let bsConfig = { run_settings: { env: null } }; @@ -975,14 +998,10 @@ describe('utils', () => { process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'https://collector-example.bsstag.com'; let bsConfig = { run_settings: { env: null } }; - try { - utils.setSystemEnvs(bsConfig); - expect(bsConfig.run_settings.system_env_vars) - .to.be.an('array') - .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-example.bsstag.com'); - } finally { - delete process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL; - } + utils.setSystemEnvs(bsConfig); + expect(bsConfig.run_settings.system_env_vars) + .to.be.an('array') + .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-example.bsstag.com'); }); it('prefers a separate remote collector url for the remote reporter', () => { @@ -990,15 +1009,17 @@ describe('utils', () => { process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com'; let bsConfig = { run_settings: { env: null } }; - try { - utils.setSystemEnvs(bsConfig); - expect(bsConfig.run_settings.system_env_vars) - .to.be.an('array') - .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-public.bsstag.com'); - } finally { - delete process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL; - delete process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL; - } + utils.setSystemEnvs(bsConfig); + expect(bsConfig.run_settings.system_env_vars) + .to.be.an('array') + .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-public.bsstag.com'); + }); + + it('rejects a collector url that is not https', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'http://collector-public.bsstag.com'; + let bsConfig = { run_settings: { env: null } }; + + expect(() => utils.setSystemEnvs(bsConfig)).to.throw(/must be an https:\/\/ URL/); }); }); From e7d01ac3f63647e806ac9c70e58b0d60974c2ccc Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Fri, 30 Oct 2026 20:31:22 +0530 Subject: [PATCH 04/11] Parse the collector override as a URL and reject a query or fragment Request urls are built as `${API_URL}/${path}`, so an override carrying a query or fragment would swallow the endpoint path and the upload would hit / instead. Parse with URL, require an https protocol and a hostname, reject search and hash, and trim a trailing slash so paths are not doubled. Jira: A11Y-13909 Co-Authored-By: Claude Opus 5 (1M context) --- bin/testObservability/helper/constants.js | 16 +++++++++++++--- test/unit/bin/helpers/utils.js | 17 +++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/bin/testObservability/helper/constants.js b/bin/testObservability/helper/constants.js index da6ebe18..ee09c153 100644 --- a/bin/testObservability/helper/constants.js +++ b/bin/testObservability/helper/constants.js @@ -6,14 +6,24 @@ exports.BATCH_INTERVAL = 2000; const DEFAULT_API_URL = 'https://collector-observability.browserstack.com'; // Requests to the collector carry BS_TESTOPS_JWT, so an http:// override would put the build's token -// on the wire in cleartext. Reject it instead of quietly using the default collector. +// on the wire in cleartext. Request urls are also built as `${base}/${path}`, so a query or fragment +// would swallow the endpoint. Reject such values instead of quietly using the default collector. const httpsOverride = (name) => { const value = process.env[name]; if (!value) return undefined; - if (!value.startsWith('https://')) { + let parsed; + try { + parsed = new URL(value); + } catch (err) { + throw new Error(`${name} must be a valid https:// URL, got "${value}"`); + } + if (parsed.protocol !== 'https:' || !parsed.hostname) { throw new Error(`${name} must be an https:// URL, got "${value}"`); } - return value; + if (parsed.search || parsed.hash) { + throw new Error(`${name} must not contain a query or fragment, got "${value}"`); + } + return value.replace(/\/+$/, ''); }; // Overridable so the remote reporter (own install of this package) can target the launching cli's collector diff --git a/test/unit/bin/helpers/utils.js b/test/unit/bin/helpers/utils.js index d83ebc4f..7b11e92f 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -1021,6 +1021,23 @@ describe('utils', () => { expect(() => utils.setSystemEnvs(bsConfig)).to.throw(/must be an https:\/\/ URL/); }); + + it('rejects a collector url carrying a query or fragment', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com/#staging'; + let bsConfig = { run_settings: { env: null } }; + + expect(() => utils.setSystemEnvs(bsConfig)).to.throw(/must not contain a query or fragment/); + }); + + it('trims a trailing slash so endpoint paths are not doubled', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com/'; + let bsConfig = { run_settings: { env: null } }; + + utils.setSystemEnvs(bsConfig); + expect(bsConfig.run_settings.system_env_vars) + .to.be.an('array') + .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-public.bsstag.com'); + }); }); describe('fixCommaSeparatedString', () => { From fc37d26fbe351428b346c448fd8323dfed3815b1 Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Mon, 2 Nov 2026 09:17:05 +0530 Subject: [PATCH 05/11] Normalise the collector override and keep its value out of errors A bare trailing ? or # leaves URL.search and URL.hash empty, so the value passed validation and the appended endpoint became a query or fragment; return origin + pathname with no trailing separator instead. The override can also carry credentials and callers log the error, so the messages no longer echo the value. Jira: A11Y-13909 Co-Authored-By: Claude Opus 5 (1M context) --- bin/testObservability/helper/constants.js | 11 +++++++---- test/unit/bin/helpers/utils.js | 10 ++++++++++ 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/bin/testObservability/helper/constants.js b/bin/testObservability/helper/constants.js index ee09c153..c2cb0c8f 100644 --- a/bin/testObservability/helper/constants.js +++ b/bin/testObservability/helper/constants.js @@ -12,18 +12,21 @@ const httpsOverride = (name) => { const value = process.env[name]; if (!value) return undefined; let parsed; + // The value can carry credentials, and callers log the error, so never echo it back. try { parsed = new URL(value); } catch (err) { - throw new Error(`${name} must be a valid https:// URL, got "${value}"`); + throw new Error(`${name} must be a valid https:// URL`); } if (parsed.protocol !== 'https:' || !parsed.hostname) { - throw new Error(`${name} must be an https:// URL, got "${value}"`); + throw new Error(`${name} must be an https:// URL`); } if (parsed.search || parsed.hash) { - throw new Error(`${name} must not contain a query or fragment, got "${value}"`); + throw new Error(`${name} must not contain a query or fragment`); } - return value.replace(/\/+$/, ''); + // Normalise to origin + path with no trailing separator: a stray `/`, `?` or `#` would otherwise + // turn the appended endpoint into a query or fragment. + return `${parsed.origin}${parsed.pathname}`.replace(/\/+$/, ''); }; // Overridable so the remote reporter (own install of this package) can target the launching cli's collector diff --git a/test/unit/bin/helpers/utils.js b/test/unit/bin/helpers/utils.js index 7b11e92f..cac8faf5 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -1029,6 +1029,16 @@ describe('utils', () => { expect(() => utils.setSystemEnvs(bsConfig)).to.throw(/must not contain a query or fragment/); }); + it('normalises a bare query or fragment delimiter', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com?'; + let bsConfig = { run_settings: { env: null } }; + + utils.setSystemEnvs(bsConfig); + expect(bsConfig.run_settings.system_env_vars) + .to.be.an('array') + .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-public.bsstag.com'); + }); + it('trims a trailing slash so endpoint paths are not doubled', () => { process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com/'; let bsConfig = { run_settings: { env: null } }; From c40a12b7c7c2bce03902bceaa48e3c60caee7ddb Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Mon, 2 Nov 2026 09:55:00 +0530 Subject: [PATCH 06/11] Cover the request destination, not just the forwarded env var setSystemEnvs tests only prove the url is handed to the remote machine. These load the helper with the override set and a stubbed axios, and assert nodeRequest builds its request url from it, with the default covered when nothing is set. Jira: A11Y-13909 Co-Authored-By: Claude Opus 5 (1M context) --- .../bin/testObservability/collectorUrl.js | 60 +++++++++++++++++++ 1 file changed, 60 insertions(+) create mode 100644 test/unit/bin/testObservability/collectorUrl.js diff --git a/test/unit/bin/testObservability/collectorUrl.js b/test/unit/bin/testObservability/collectorUrl.js new file mode 100644 index 00000000..abd31a5d --- /dev/null +++ b/test/unit/bin/testObservability/collectorUrl.js @@ -0,0 +1,60 @@ +'use strict'; +const chai = require('chai'); +const expect = chai.expect; +const sinon = require('sinon'); +const proxyquire = require('proxyquire').noPreserveCache(); + +const CONSTANTS_PATH = require.resolve('../../../../bin/testObservability/helper/constants'); +const API_URL_ENV = 'BROWSERSTACK_TEST_OBSERVABILITY_API_URL'; +const DEFAULT_API_URL = 'https://collector-observability.browserstack.com'; + +// A11Y-13909: the reporter runs on the BrowserStack machine from its own install of this package, +// so the collector it posts to has to come from the environment the launching cli forwards. These +// pin the request destination itself, not just the forwarding done by setSystemEnvs. +describe('observability collector url', () => { + let savedApiUrl, cachedConstants; + + beforeEach(() => { + savedApiUrl = process.env[API_URL_ENV]; + delete process.env[API_URL_ENV]; + // constants.js reads the override at load, so it must be re-evaluated per case. + cachedConstants = require.cache[CONSTANTS_PATH]; + delete require.cache[CONSTANTS_PATH]; + }); + + afterEach(() => { + if (savedApiUrl === undefined) { + delete process.env[API_URL_ENV]; + } else { + process.env[API_URL_ENV] = savedApiUrl; + } + delete require.cache[CONSTANTS_PATH]; + if (cachedConstants) { + require.cache[CONSTANTS_PATH] = cachedConstants; + } + }); + + const loadHelperWithAxios = () => { + const axiosStub = sinon.stub().resolves({ status: 200, data: '{}' }); + axiosStub['@noCallThru'] = true; + const helper = proxyquire('../../../../bin/testObservability/helper/helper', { axios: axiosStub }); + return { helper, axiosStub }; + }; + + it('sends requests to the overridden collector', async () => { + process.env[API_URL_ENV] = 'https://collector-example.bsstag.com'; + const { helper, axiosStub } = loadHelperWithAxios(); + + await helper.nodeRequest('POST', 'api/v1/batch', {}, { headers: {} }); + + expect(axiosStub.firstCall.args[0].url).to.equal('https://collector-example.bsstag.com/api/v1/batch'); + }); + + it('sends requests to the default collector when no override is set', async () => { + const { helper, axiosStub } = loadHelperWithAxios(); + + await helper.nodeRequest('POST', 'api/v1/batch', {}, { headers: {} }); + + expect(axiosStub.firstCall.args[0].url).to.equal(`${DEFAULT_API_URL}/api/v1/batch`); + }); +}); From 3bf5017016045a425bd873e69386e2fd496926c7 Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Mon, 2 Nov 2026 13:06:45 +0530 Subject: [PATCH 07/11] Read the collector override lazily and validate it on the runs path constants.js is loaded through bin/helpers/utils.js, which every command loads, so validating at module load meant a bad BROWSERSTACK_TEST_OBSERVABILITY_API_URL crashed init, info, stop and the reporter on the machine at startup, outside any handler. API_URL becomes apiUrl(), read when a request is built; nodeRequest rejects rather than throws, since its executor is async and a throw there would never reach the returned promise. validateCollectorUrls() runs at runs.js:136, before TestHubHandler.launchBuild, so a bad value fails before a build exists. Comments in constants.js trimmed to the one that earns its place. Jira: A11Y-13909 Co-Authored-By: Claude Opus 5 (1M context) --- bin/commands/runs.js | 2 ++ bin/helpers/utils.js | 6 +++++ bin/testObservability/crashReporter/index.js | 4 ++-- bin/testObservability/helper/constants.js | 16 +++++-------- bin/testObservability/helper/helper.js | 12 ++++++++-- .../bin/testObservability/collectorUrl.js | 24 +++++++++++-------- 6 files changed, 40 insertions(+), 24 deletions(-) diff --git a/bin/commands/runs.js b/bin/commands/runs.js index daea68b7..cbaf6a26 100644 --- a/bin/commands/runs.js +++ b/bin/commands/runs.js @@ -133,6 +133,8 @@ module.exports = function run(args, rawArgs) { const port = await findAvailablePort(preferredPort); process.env.REPORTER_API_PORT_NO = port + utils.validateCollectorUrls(); + // Send build start to TEST REPORTING AND ANALYTICS if(shouldProcessEventForTesthub()) { await TestHubHandler.launchBuild(bsConfig, bsConfigPath); diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index c15cc69d..bf294cac 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -572,6 +572,12 @@ exports.setBuildTags = (bsConfig, args) => { logger.debug(`Setting the build tag = ${bsConfig.run_settings.build_tag}`); }; +// Called before anything is created server-side: setSystemEnvs runs after the TestHub build +// exists, so a malformed collector override would otherwise fail with a build already open. +exports.validateCollectorUrls = () => { + remoteReporterApiUrl(); +}; + exports.setSystemEnvs = (bsConfig) => { let envKeys = {}; diff --git a/bin/testObservability/crashReporter/index.js b/bin/testObservability/crashReporter/index.js index 95c01881..31ea6a06 100644 --- a/bin/testObservability/crashReporter/index.js +++ b/bin/testObservability/crashReporter/index.js @@ -7,7 +7,7 @@ const { HttpsProxyAgent = require('https-proxy-agent') } = require('https-proxy- const logger = require("../../helpers/logger").winstonLogger; const utils = require('../../helpers/utils'); -const { API_URL, consoleHolder, TEST_REPORTING_ANALYTICS } = require('../helper/constants'); +const { apiUrl, consoleHolder, TEST_REPORTING_ANALYTICS } = require('../helper/constants'); /* Below global methods are added here to remove cyclic dependency with helper.js, refactor later */ const httpsKeepAliveAgent = new https.Agent({ @@ -148,7 +148,7 @@ class CrashReporter { 'X-BSTACK-TESTOPS': 'true' }, method: 'POST', - url: `${API_URL}/api/v1/analytics`, + url: `${apiUrl()}/api/v1/analytics`, data: data, json: true, agent: httpsKeepAliveAgent diff --git a/bin/testObservability/helper/constants.js b/bin/testObservability/helper/constants.js index c2cb0c8f..1a0d0c1d 100644 --- a/bin/testObservability/helper/constants.js +++ b/bin/testObservability/helper/constants.js @@ -5,14 +5,12 @@ exports.BATCH_SIZE = 1000; exports.BATCH_INTERVAL = 2000; const DEFAULT_API_URL = 'https://collector-observability.browserstack.com'; -// Requests to the collector carry BS_TESTOPS_JWT, so an http:// override would put the build's token -// on the wire in cleartext. Request urls are also built as `${base}/${path}`, so a query or fragment -// would swallow the endpoint. Reject such values instead of quietly using the default collector. +// Overridable so the reporter on the remote machine posts to the collector the build was created +// on. Errors never echo the value: it can carry credentials and callers log them. const httpsOverride = (name) => { const value = process.env[name]; if (!value) return undefined; let parsed; - // The value can carry credentials, and callers log the error, so never echo it back. try { parsed = new URL(value); } catch (err) { @@ -24,16 +22,14 @@ const httpsOverride = (name) => { if (parsed.search || parsed.hash) { throw new Error(`${name} must not contain a query or fragment`); } - // Normalise to origin + path with no trailing separator: a stray `/`, `?` or `#` would otherwise - // turn the appended endpoint into a query or fragment. + // Endpoints are appended as `${base}/${path}`. return `${parsed.origin}${parsed.pathname}`.replace(/\/+$/, ''); }; -// Overridable so the remote reporter (own install of this package) can target the launching cli's collector -exports.API_URL = httpsOverride('BROWSERSTACK_TEST_OBSERVABILITY_API_URL') || DEFAULT_API_URL; -// Collector handed to the reporter on the remote machine; separate override for when it cannot reach API_URL +exports.apiUrl = () => httpsOverride('BROWSERSTACK_TEST_OBSERVABILITY_API_URL') || DEFAULT_API_URL; +// Separate override for when the remote machine cannot reach the cli's own collector. exports.remoteReporterApiUrl = () => httpsOverride('BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL') - || httpsOverride('BROWSERSTACK_TEST_OBSERVABILITY_API_URL') || DEFAULT_API_URL; + || exports.apiUrl(); exports.IPC_EVENTS = { LOG: 'testObservability:cypressLog', diff --git a/bin/testObservability/helper/helper.js b/bin/testObservability/helper/helper.js index 434d4a42..ddd2e8c7 100644 --- a/bin/testObservability/helper/helper.js +++ b/bin/testObservability/helper/helper.js @@ -27,7 +27,7 @@ const GLOBAL_MODULE_PATH = execSync('npm root -g').toString().trim(); const { name, version } = require('../../../package.json'); const { CYPRESS_V10_AND_ABOVE_CONFIG_FILE_EXTENSIONS } = require('../../helpers/constants'); -const { consoleHolder, API_URL, TEST_OBSERVABILITY_REPORTER, TEST_OBSERVABILITY_REPORTER_LOCAL, TEST_REPORTING_ANALYTICS } = require('./constants'); +const { consoleHolder, apiUrl, TEST_OBSERVABILITY_REPORTER, TEST_OBSERVABILITY_REPORTER_LOCAL, TEST_REPORTING_ANALYTICS } = require('./constants'); const ALLOWED_MODULES = [ 'cypress/package.json', @@ -128,10 +128,18 @@ exports.printBuildLink = async (shouldStopSession, exitCode = null) => { exports.nodeRequest = (type, url, data, config) => { const requestQueueHandler = require('./requestQueueHandler'); return new Promise(async (resolve, reject) => { + // Rejected rather than thrown: this executor is async, so a throw here would never reach + // the returned promise and the caller would hang. + let collectorUrl; + try { + collectorUrl = apiUrl(); + } catch (err) { + return reject(err); + } const options = { ...config, method: type, - url: `${API_URL}/${url}`, + url: `${collectorUrl}/${url}`, data: data, httpsAgent: this.httpsKeepAliveAgent, maxAttempts: 2, diff --git a/test/unit/bin/testObservability/collectorUrl.js b/test/unit/bin/testObservability/collectorUrl.js index abd31a5d..cad0b3b0 100644 --- a/test/unit/bin/testObservability/collectorUrl.js +++ b/test/unit/bin/testObservability/collectorUrl.js @@ -4,7 +4,6 @@ const expect = chai.expect; const sinon = require('sinon'); const proxyquire = require('proxyquire').noPreserveCache(); -const CONSTANTS_PATH = require.resolve('../../../../bin/testObservability/helper/constants'); const API_URL_ENV = 'BROWSERSTACK_TEST_OBSERVABILITY_API_URL'; const DEFAULT_API_URL = 'https://collector-observability.browserstack.com'; @@ -12,14 +11,11 @@ const DEFAULT_API_URL = 'https://collector-observability.browserstack.com'; // so the collector it posts to has to come from the environment the launching cli forwards. These // pin the request destination itself, not just the forwarding done by setSystemEnvs. describe('observability collector url', () => { - let savedApiUrl, cachedConstants; + let savedApiUrl; beforeEach(() => { savedApiUrl = process.env[API_URL_ENV]; delete process.env[API_URL_ENV]; - // constants.js reads the override at load, so it must be re-evaluated per case. - cachedConstants = require.cache[CONSTANTS_PATH]; - delete require.cache[CONSTANTS_PATH]; }); afterEach(() => { @@ -28,10 +24,6 @@ describe('observability collector url', () => { } else { process.env[API_URL_ENV] = savedApiUrl; } - delete require.cache[CONSTANTS_PATH]; - if (cachedConstants) { - require.cache[CONSTANTS_PATH] = cachedConstants; - } }); const loadHelperWithAxios = () => { @@ -42,8 +34,8 @@ describe('observability collector url', () => { }; it('sends requests to the overridden collector', async () => { - process.env[API_URL_ENV] = 'https://collector-example.bsstag.com'; const { helper, axiosStub } = loadHelperWithAxios(); + process.env[API_URL_ENV] = 'https://collector-example.bsstag.com'; await helper.nodeRequest('POST', 'api/v1/batch', {}, { headers: {} }); @@ -57,4 +49,16 @@ describe('observability collector url', () => { expect(axiosStub.firstCall.args[0].url).to.equal(`${DEFAULT_API_URL}/api/v1/batch`); }); + + // Loading this module must never throw on a bad value: every command loads it through + // bin/helpers/utils.js, so init, info and stop would die at startup, as would the reporter. + it('does not reject an invalid collector url until a request is made', async () => { + process.env[API_URL_ENV] = 'http://collector-example.bsstag.com'; + + const { helper } = loadHelperWithAxios(); + + let thrown; + await helper.nodeRequest('POST', 'api/v1/batch', {}, { headers: {} }).catch((err) => { thrown = err; }); + expect(String(thrown)).to.match(/must be an https:\/\/ URL/); + }); }); From 7169d3dc7ffc6426292ee2c8e8328a9c86bed9b1 Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Mon, 2 Nov 2026 14:00:16 +0530 Subject: [PATCH 08/11] Validate the general collector override too, not only the remote one remoteReporterApiUrl() short-circuits past apiUrl() when the remote override is set, so a malformed general override went unchecked until the first request. validateCollectorUrls() now calls both, with a test for the short-circuit case. Jira: A11Y-13909 Co-Authored-By: Claude Opus 5 (1M context) --- bin/helpers/utils.js | 5 ++++- test/unit/bin/helpers/utils.js | 7 +++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index bf294cac..9908efab 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -31,7 +31,7 @@ const usageReporting = require("./usageReporting"), pkg = require('../../package.json'), transports = require('./logger').transports, o11yHelpers = require('../testObservability/helper/helper'), - { OBSERVABILITY_ENV_VARS, TEST_OBSERVABILITY_REPORTER, remoteReporterApiUrl } = require('../testObservability/helper/constants'); + { OBSERVABILITY_ENV_VARS, TEST_OBSERVABILITY_REPORTER, apiUrl, remoteReporterApiUrl } = require('../testObservability/helper/constants'); const { default: axios } = require("axios"); const { shouldProcessEventForTesthub } = require("../testhub/utils"); @@ -575,6 +575,9 @@ exports.setBuildTags = (bsConfig, args) => { // Called before anything is created server-side: setSystemEnvs runs after the TestHub build // exists, so a malformed collector override would otherwise fail with a build already open. exports.validateCollectorUrls = () => { + // Both, because remoteReporterApiUrl() short-circuits past apiUrl() when the remote override + // is set, which would leave a malformed general override unchecked until the first request. + apiUrl(); remoteReporterApiUrl(); }; diff --git a/test/unit/bin/helpers/utils.js b/test/unit/bin/helpers/utils.js index cac8faf5..60502100 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -1015,6 +1015,13 @@ describe('utils', () => { .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-public.bsstag.com'); }); + it('validates both overrides, even when the remote one is valid', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'http://collector-private.bsstag.com'; + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com'; + + expect(() => utils.validateCollectorUrls()).to.throw(/BROWSERSTACK_TEST_OBSERVABILITY_API_URL must be an https/); + }); + it('rejects a collector url that is not https', () => { process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'http://collector-public.bsstag.com'; let bsConfig = { run_settings: { env: null } }; From 068bbe826285451a3be77bf1714a37ad9a005cb2 Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Mon, 2 Nov 2026 23:32:27 +0530 Subject: [PATCH 09/11] A11Y-13909: warn on a remote-only collector, and make the load-safety test real Two findings from the SDK PR review: Setting only BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL leaves this process on its built-in collector while the machine reports to another one, so the build and its test events can land in two places with nothing detecting it. That split is correct in the Jenkins flow, where the built-in default has been pointed at the same environment, and wrong anywhere else - which the code cannot tell apart, so this warns rather than rejects. The third case in collectorUrl.js promised that loading the module never throws on a bad value, but proxyquire's noPreserveCache clears only the proxied module and its stubs, and the url is resolved in constants.js. That stayed a cache hit, so hoisting the resolution to module scope would not have turned the test red - verified by doing exactly that, which now fails. The loader drops constants.js and the test asserts the load explicitly. Env var names move into constants.js so the validator and the resolvers share one spelling. Co-Authored-By: Claude Opus 5 (1M context) --- bin/helpers/utils.js | 8 +++++- bin/testObservability/helper/constants.js | 10 ++++--- test/unit/bin/helpers/utils.js | 27 +++++++++++++++++++ .../bin/testObservability/collectorUrl.js | 12 +++++++-- 4 files changed, 51 insertions(+), 6 deletions(-) diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index 9908efab..9774b8ab 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -31,7 +31,7 @@ const usageReporting = require("./usageReporting"), pkg = require('../../package.json'), transports = require('./logger').transports, o11yHelpers = require('../testObservability/helper/helper'), - { OBSERVABILITY_ENV_VARS, TEST_OBSERVABILITY_REPORTER, apiUrl, remoteReporterApiUrl } = require('../testObservability/helper/constants'); + { OBSERVABILITY_ENV_VARS, TEST_OBSERVABILITY_REPORTER, API_URL_ENV, REMOTE_API_URL_ENV, apiUrl, remoteReporterApiUrl } = require('../testObservability/helper/constants'); const { default: axios } = require("axios"); const { shouldProcessEventForTesthub } = require("../testhub/utils"); @@ -579,6 +579,12 @@ exports.validateCollectorUrls = () => { // is set, which would leave a malformed general override unchecked until the first request. apiUrl(); remoteReporterApiUrl(); + // Remote-only means this process keeps its built-in collector while the machine reports to + // another one. That is correct when the built-in default has been pointed at the same + // environment, and a silently split build when it has not. + if (process.env[REMOTE_API_URL_ENV] && !process.env[API_URL_ENV]) { + logger.warn(`${REMOTE_API_URL_ENV} is set without ${API_URL_ENV}: this run will create its build on the built-in collector and the remote reporter will post test results to ${process.env[REMOTE_API_URL_ENV]}. Both must be the same environment.`); + } }; exports.setSystemEnvs = (bsConfig) => { diff --git a/bin/testObservability/helper/constants.js b/bin/testObservability/helper/constants.js index 1a0d0c1d..dbcae250 100644 --- a/bin/testObservability/helper/constants.js +++ b/bin/testObservability/helper/constants.js @@ -26,10 +26,14 @@ const httpsOverride = (name) => { return `${parsed.origin}${parsed.pathname}`.replace(/\/+$/, ''); }; -exports.apiUrl = () => httpsOverride('BROWSERSTACK_TEST_OBSERVABILITY_API_URL') || DEFAULT_API_URL; +const API_URL_ENV = 'BROWSERSTACK_TEST_OBSERVABILITY_API_URL'; // Separate override for when the remote machine cannot reach the cli's own collector. -exports.remoteReporterApiUrl = () => httpsOverride('BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL') - || exports.apiUrl(); +const REMOTE_API_URL_ENV = 'BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL'; +exports.API_URL_ENV = API_URL_ENV; +exports.REMOTE_API_URL_ENV = REMOTE_API_URL_ENV; + +exports.apiUrl = () => httpsOverride(API_URL_ENV) || DEFAULT_API_URL; +exports.remoteReporterApiUrl = () => httpsOverride(REMOTE_API_URL_ENV) || exports.apiUrl(); exports.IPC_EVENTS = { LOG: 'testObservability:cypressLog', diff --git a/test/unit/bin/helpers/utils.js b/test/unit/bin/helpers/utils.js index 60502100..e64a8ad7 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -1015,6 +1015,33 @@ describe('utils', () => { .that.includes('BROWSERSTACK_TEST_OBSERVABILITY_API_URL=https://collector-public.bsstag.com'); }); + it('warns when only the remote collector is overridden', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com'; + const warn = sinon.stub(logger, 'warn'); + + try { + utils.validateCollectorUrls(); + sinon.assert.calledOnce(warn); + expect(warn.firstCall.args[0]).to.match(/collector-public\.bsstag\.com/); + expect(warn.firstCall.args[0]).to.match(/same environment/); + } finally { + warn.restore(); + } + }); + + it('does not warn when both collector urls are set', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'https://collector-private.bsstag.com'; + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com'; + const warn = sinon.stub(logger, 'warn'); + + try { + utils.validateCollectorUrls(); + sinon.assert.notCalled(warn); + } finally { + warn.restore(); + } + }); + it('validates both overrides, even when the remote one is valid', () => { process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'http://collector-private.bsstag.com'; process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com'; diff --git a/test/unit/bin/testObservability/collectorUrl.js b/test/unit/bin/testObservability/collectorUrl.js index cad0b3b0..f5df6898 100644 --- a/test/unit/bin/testObservability/collectorUrl.js +++ b/test/unit/bin/testObservability/collectorUrl.js @@ -6,6 +6,8 @@ const proxyquire = require('proxyquire').noPreserveCache(); const API_URL_ENV = 'BROWSERSTACK_TEST_OBSERVABILITY_API_URL'; const DEFAULT_API_URL = 'https://collector-observability.browserstack.com'; +const HELPER_PATH = '../../../../bin/testObservability/helper/helper'; +const CONSTANTS_PATH = '../../../../bin/testObservability/helper/constants'; // A11Y-13909: the reporter runs on the BrowserStack machine from its own install of this package, // so the collector it posts to has to come from the environment the launching cli forwards. These @@ -29,7 +31,11 @@ describe('observability collector url', () => { const loadHelperWithAxios = () => { const axiosStub = sinon.stub().resolves({ status: 200, data: '{}' }); axiosStub['@noCallThru'] = true; - const helper = proxyquire('../../../../bin/testObservability/helper/helper', { axios: axiosStub }); + // proxyquire clears only the proxied module and its stubs, and the url is resolved in + // constants.js, so without this the url code is a cache hit and a module-load-time + // regression there would never turn these red. + delete require.cache[require.resolve(CONSTANTS_PATH)]; + const helper = proxyquire(HELPER_PATH, { axios: axiosStub }); return { helper, axiosStub }; }; @@ -55,7 +61,9 @@ describe('observability collector url', () => { it('does not reject an invalid collector url until a request is made', async () => { process.env[API_URL_ENV] = 'http://collector-example.bsstag.com'; - const { helper } = loadHelperWithAxios(); + let loaded; + expect(() => { loaded = loadHelperWithAxios(); }).to.not.throw(); + const { helper } = loaded; let thrown; await helper.nodeRequest('POST', 'api/v1/batch', {}, { headers: {} }).catch((err) => { thrown = err; }); From 1c70e715b85151148a2fca4d89cdb0a2c3490a58 Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Mon, 2 Nov 2026 23:44:41 +0530 Subject: [PATCH 10/11] A11Y-13909: keep the collector value out of the log, and share the env key Second round of SDK PR review findings. The warning added in the last commit interpolated the raw env value, which contradicts the rule the validator states a few lines up: a collector url can carry credentials and callers log it. It now logs the normalized accessor, whose origin has already dropped any userinfo. The validator also rejects a url carrying a username or password outright, so the stripping is explicit rather than a silent side effect of normalization. setSystemEnvs still spelled the forwarded key as a literal while every other reference used the imported constant, so a rename could have split the producer from the consumer - the exact failure this PR exists to prevent. Tests: the no-override path (what every build takes) now has a case, and so does the userinfo rejection. Co-Authored-By: Claude Opus 5 (1M context) --- bin/helpers/utils.js | 4 ++-- bin/testObservability/helper/constants.js | 5 +++++ test/unit/bin/helpers/utils.js | 19 ++++++++++++++++++- 3 files changed, 25 insertions(+), 3 deletions(-) diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index 9774b8ab..67d78a6f 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -583,7 +583,7 @@ exports.validateCollectorUrls = () => { // another one. That is correct when the built-in default has been pointed at the same // environment, and a silently split build when it has not. if (process.env[REMOTE_API_URL_ENV] && !process.env[API_URL_ENV]) { - logger.warn(`${REMOTE_API_URL_ENV} is set without ${API_URL_ENV}: this run will create its build on the built-in collector and the remote reporter will post test results to ${process.env[REMOTE_API_URL_ENV]}. Both must be the same environment.`); + logger.warn(`${REMOTE_API_URL_ENV} is set without ${API_URL_ENV}: this run will create its build on the built-in collector and the remote reporter will post test results to ${remoteReporterApiUrl()}. Both must be the same environment.`); } }; @@ -627,7 +627,7 @@ exports.setSystemEnvs = (bsConfig) => { // Remote reporter must post to this build's collector, not its package default. Kept out of the // try below, which swallows errors: a rejected override must fail rather than fall back silently. - envKeys['BROWSERSTACK_TEST_OBSERVABILITY_API_URL'] = remoteReporterApiUrl(); + envKeys[API_URL_ENV] = remoteReporterApiUrl(); try { OBSERVABILITY_ENV_VARS.forEach(key => { diff --git a/bin/testObservability/helper/constants.js b/bin/testObservability/helper/constants.js index dbcae250..19e7517c 100644 --- a/bin/testObservability/helper/constants.js +++ b/bin/testObservability/helper/constants.js @@ -22,6 +22,11 @@ const httpsOverride = (name) => { if (parsed.search || parsed.hash) { throw new Error(`${name} must not contain a query or fragment`); } + // URL.origin drops userinfo, so accepting it would silently discard a credential the + // caller believes it is sending. + if (parsed.username || parsed.password) { + throw new Error(`${name} must not contain a username or password`); + } // Endpoints are appended as `${base}/${path}`. return `${parsed.origin}${parsed.pathname}`.replace(/\/+$/, ''); }; diff --git a/test/unit/bin/helpers/utils.js b/test/unit/bin/helpers/utils.js index e64a8ad7..b0a5841b 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -1022,7 +1022,7 @@ describe('utils', () => { try { utils.validateCollectorUrls(); sinon.assert.calledOnce(warn); - expect(warn.firstCall.args[0]).to.match(/collector-public\.bsstag\.com/); + expect(warn.firstCall.args[0]).to.match(/https:\/\/collector-public\.bsstag\.com/); expect(warn.firstCall.args[0]).to.match(/same environment/); } finally { warn.restore(); @@ -1042,6 +1042,23 @@ describe('utils', () => { } }); + it('neither warns nor throws when no collector url is overridden', () => { + const warn = sinon.stub(logger, 'warn'); + + try { + expect(() => utils.validateCollectorUrls()).to.not.throw(); + sinon.assert.notCalled(warn); + } finally { + warn.restore(); + } + }); + + it('rejects a collector url carrying a username or password', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://user:secret@collector-public.bsstag.com'; + + expect(() => utils.validateCollectorUrls()).to.throw(/must not contain a username or password/); + }); + it('validates both overrides, even when the remote one is valid', () => { process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'http://collector-private.bsstag.com'; process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com'; From 632b2c5c57558d9811e9bdcf357184425a69628a Mon Sep 17 00:00:00 2001 From: manish-bothra Date: Tue, 3 Nov 2026 00:27:23 +0530 Subject: [PATCH 11/11] A11Y-13909: name only the collector host in the split-collector warning httpsOverride normalizes to origin + pathname, so the path survives and can carry a token. The warning now names the host, which is the part that answers the question it asks - whether the two collectors are the same environment - and a test pins that a path in the value never reaches the log. Co-Authored-By: Claude Opus 5 (1M context) --- bin/helpers/utils.js | 5 +++-- test/unit/bin/helpers/utils.js | 14 +++++++++++++- 2 files changed, 16 insertions(+), 3 deletions(-) diff --git a/bin/helpers/utils.js b/bin/helpers/utils.js index 67d78a6f..a27189ab 100644 --- a/bin/helpers/utils.js +++ b/bin/helpers/utils.js @@ -581,9 +581,10 @@ exports.validateCollectorUrls = () => { remoteReporterApiUrl(); // Remote-only means this process keeps its built-in collector while the machine reports to // another one. That is correct when the built-in default has been pointed at the same - // environment, and a silently split build when it has not. + // environment, and a silently split build when it has not. Only the host is named: the + // path survives normalization and can carry a token. if (process.env[REMOTE_API_URL_ENV] && !process.env[API_URL_ENV]) { - logger.warn(`${REMOTE_API_URL_ENV} is set without ${API_URL_ENV}: this run will create its build on the built-in collector and the remote reporter will post test results to ${remoteReporterApiUrl()}. Both must be the same environment.`); + logger.warn(`${REMOTE_API_URL_ENV} is set without ${API_URL_ENV}: this run will create its build on the built-in collector and the remote reporter will post test results to ${new URL(remoteReporterApiUrl()).host}. Both must be the same environment.`); } }; diff --git a/test/unit/bin/helpers/utils.js b/test/unit/bin/helpers/utils.js index b0a5841b..d20d42db 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -1022,13 +1022,25 @@ describe('utils', () => { try { utils.validateCollectorUrls(); sinon.assert.calledOnce(warn); - expect(warn.firstCall.args[0]).to.match(/https:\/\/collector-public\.bsstag\.com/); + expect(warn.firstCall.args[0]).to.match(/collector-public\.bsstag\.com/); expect(warn.firstCall.args[0]).to.match(/same environment/); } finally { warn.restore(); } }); + it('names only the host in that warning, never the path', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com/t0ken'; + const warn = sinon.stub(logger, 'warn'); + + try { + utils.validateCollectorUrls(); + expect(warn.firstCall.args[0]).to.not.match(/t0ken/); + } finally { + warn.restore(); + } + }); + it('does not warn when both collector urls are set', () => { process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'https://collector-private.bsstag.com'; process.env.BROWSERSTACK_TEST_OBSERVABILITY_REMOTE_API_URL = 'https://collector-public.bsstag.com';