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 a6903720..a27189ab 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_ENV, REMOTE_API_URL_ENV, apiUrl, remoteReporterApiUrl } = require('../testObservability/helper/constants'); const { default: axios } = require("axios"); const { shouldProcessEventForTesthub } = require("../testhub/utils"); @@ -572,6 +572,22 @@ 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 = () => { + // 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(); + // 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. 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 ${new URL(remoteReporterApiUrl()).host}. Both must be the same environment.`); + } +}; + exports.setSystemEnvs = (bsConfig) => { let envKeys = {}; @@ -610,6 +626,10 @@ 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[API_URL_ENV] = remoteReporterApiUrl(); + try { OBSERVABILITY_ENV_VARS.forEach(key => { envKeys[key] = process.env[key]; 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 d3013f40..19e7517c 100644 --- a/bin/testObservability/helper/constants.js +++ b/bin/testObservability/helper/constants.js @@ -3,7 +3,42 @@ 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'; +const DEFAULT_API_URL = 'https://collector-observability.browserstack.com'; + +// 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; + try { + parsed = new URL(value); + } catch (err) { + 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`); + } + 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(/\/+$/, ''); +}; + +const API_URL_ENV = 'BROWSERSTACK_TEST_OBSERVABILITY_API_URL'; +// Separate override for when the remote machine cannot reach the cli's own collector. +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/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/helpers/utils.js b/test/unit/bin/helpers/utils.js index 480cb5f4..d20d42db 100644 --- a/test/unit/bin/helpers/utils.js +++ b/test/unit/bin/helpers/utils.js @@ -961,6 +961,156 @@ describe('utils', () => { delete process.env.CYPRESS_TEST_1; 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 } }; + + 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', () => { + process.env.BROWSERSTACK_TEST_OBSERVABILITY_API_URL = 'https://collector-example.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-example.bsstag.com'); + }); + + 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 } }; + + 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('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('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'; + const warn = sinon.stub(logger, 'warn'); + + try { + utils.validateCollectorUrls(); + sinon.assert.notCalled(warn); + } finally { + warn.restore(); + } + }); + + 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'; + + 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 } }; + + 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('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 } }; + + 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', () => { diff --git a/test/unit/bin/testObservability/collectorUrl.js b/test/unit/bin/testObservability/collectorUrl.js new file mode 100644 index 00000000..f5df6898 --- /dev/null +++ b/test/unit/bin/testObservability/collectorUrl.js @@ -0,0 +1,72 @@ +'use strict'; +const chai = require('chai'); +const expect = chai.expect; +const sinon = require('sinon'); +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 +// pin the request destination itself, not just the forwarding done by setSystemEnvs. +describe('observability collector url', () => { + let savedApiUrl; + + beforeEach(() => { + savedApiUrl = process.env[API_URL_ENV]; + delete process.env[API_URL_ENV]; + }); + + afterEach(() => { + if (savedApiUrl === undefined) { + delete process.env[API_URL_ENV]; + } else { + process.env[API_URL_ENV] = savedApiUrl; + } + }); + + const loadHelperWithAxios = () => { + const axiosStub = sinon.stub().resolves({ status: 200, data: '{}' }); + axiosStub['@noCallThru'] = true; + // 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 }; + }; + + it('sends requests to the overridden collector', async () => { + const { helper, axiosStub } = loadHelperWithAxios(); + process.env[API_URL_ENV] = 'https://collector-example.bsstag.com'; + + 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`); + }); + + // 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'; + + 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; }); + expect(String(thrown)).to.match(/must be an https:\/\/ URL/); + }); +});