A11Y-13909: forward the Test Observability collector to the remote reporter

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

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

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

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',

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,

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', () => {

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