[harshit-browserstack]

Summary

Jira: SDK-8000. Same fix as SDK-7999 in browserstack-binary#1960 and browserstack-node-agent#2742.

The private keys of 8 eMudhra intermediate CAs are recoverable (incident), and Node.js has not shipped the distrust yet (nodejs/node#66871 is unreleased). The CLI runs on the customer's Node and sends the username and access key to api, api-cloud, automate and eds.browserstack.com, so anyone on the network path can present a certificate the CLI accepts and read the key and the uploaded test suite.

Change

• New bin/helpers/distrustedCaCheck.js wraps tls.checkServerIdentity: Node's hostname check runs first, then the connection is refused (ERR_TLS_DISTRUSTED_INTERMEDIATE) if any certificate in the chain has one of the 8 published SPKI SHA-256 hashes. Matching the key catches both the emSign-issued and the Sectigo cross-signed copies.
• Installed at the top of bin/helpers/utils.js instead of bin/runner.js: utils.js is loaded by every CLI command and by the code that runs inside the customer's Cypress process (Test Observability reporter and crash reporter, Accessibility plugin), which a hook in runner.js would not reach.
• Local: the CLI starts Local with browserstack-local's async start() (1.5.13), which does the endpoint lookup and the binary download in-process, so this hook covers them too.
• The SPKI is re-exported with crypto.createPublicKey from the certificate: cert.pubkey is the raw point for EC keys (hashing it never matches), and X509Certificate needs Node 15.6.

Verification

• build-info with RAILS_HOST / USAGE_REPORTING_URL pointed at dv-g3b-emudhra and dv-g3b-aaa-emudhra.we-have-a-problem.com (fake credentials): master completes TLS and receives the host's response on Node 14 and 18 (both chains); this branch gets ERR_TLS_DISTRUSTED_INTERMEDIATE and receives nothing, on Node 14, 18 and 22. api.browserstack.com is still reached.
• Local start() with bs-host pointed at the incident host (fake key): master completes TLS and parses the host's reply; this branch refuses the connection.
• Unit tests: 723 passing, 2 pending, 16 failing; the same 16 fail on master.
• Not run yet: a full browserstack-cypress run on BrowserStack.

Summary by CodeRabbit

• Security
  • TLS validation now rejects certificate chains containing any of eight distrusted eMudhra intermediate certificate authorities.
  • Connections with affected chains fail with an error identifying the hostname and certificate authority.
  • Existing hostname validation behavior and error reporting remain unchanged.
  • The check is applied automatically when the command-line utility loads; if it cannot be installed, the utility continues loading.
  • Repeated installation attempts do not add duplicate validation checks.

[coderabbitai]

Note

Some coding-guideline sources were skipped; the reason is listed next to each one. Other guidelines were applied as usual. On a public repository, sources from your configuration are listed without their names.

Skipped guideline sources: 1

a configured source — couldn't be fetched

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Source mode: Auto
• Repository: browserstack/browserstack-cypress-cli
• Branch: fix/SDK-8000-emudhra-key-block
• File: .coderabbit.yaml
• Commit: 96b3a51
• Layers: Repository YAML (base), Central YAML (inherited), Organization UI (inherited), Workspace UI (inherited); Enforced overrides: Organization
• Review profile: ASSERTIVE
• Plan: Enterprise
• Run ID: b34539f7-6333-406c-bd5e-392a7c19077b

📥 Commits

Reviewing files that changed from the base of the PR and between 2603ad7 and 96b3a51.

📒 Files selected for processing: 2

• bin/helpers/utils.js
• test/unit/bin/helpers/distrustedCaCheck.js

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.

📜 Recent review details

🔇 Additional comments: 2

bin/helpers/utils.js: 2

3-4: 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier

Security Misconfiguration

Exploitability: Difficult
CWE: CWE-295 — Improper Certificate Validation

Reachability path

● Entry
  test/unit/bin/helpers/distrustedCaCheck.js:49
│
▼
● Hop
  bin/helpers/distrustedCaCheck.js:85
  installDistrustedCaCheck
│
▼
● Sink
  bin/helpers/utils.js

Load bin/helpers/utils.js before the first TLS connection. An entry point that connects first cannot use the distrust check. The import order for index.js, bin/runner.js, and the Local startup path is not established here.

4-4: 🔒 Security & Privacy | 🛡️ Detected with Advanced Tier

Security Misconfiguration

Reachability: External
Exploitability: Difficult
CWE: CWE-295 — Improper Certificate Validation

Reachability path

● Entry
  test/unit/bin/helpers/distrustedCaCheck.js:49
│
▼
● Hop
  bin/helpers/distrustedCaCheck.js:85
  installDistrustedCaCheck
│
▼
● Sink
  bin/helpers/utils.js

The supported clients do not override checkServerIdentity.

The repository-owned HTTPS agents do not set a custom callback. browserstack-local v1.5.12 calls https.get(options, ...) with an optional proxy agent, and its complete LocalBinary.js source contains no checkServerIdentity. The wrapper therefore remains the default identity check on these paths, so the proposed callback bypass is not present.

Likely an incorrect or invalid review comment.

📝 Walkthrough

The change adds an idempotent TLS identity-validation wrapper that rejects certificate chains matching eight distrusted eMudhra intermediates. The utility module installs the check when it loads. Tests cover certificate matching, hostname-error precedence, and repeat installation.

Changes: Distrusted CA validation

Layer / File(s)	Summary
TLS check, initialization, and tests bin/helpers/distrustedCaCheck.js, bin/helpers/utils.js, test/unit/bin/helpers/distrustedCaCheck.js	The module matches issuer chains using SPKI SHA-256 hashes or normalized certificate names and rejects matching chains with ERR_TLS_DISTRUSTED_INTERMEDIATE. The utility module attempts installation at load and logs installation errors at debug level. Tests cover matching chains, unrelated chains, hostname-error precedence, and repeated installation.

Priority: ⬆️ High

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Bug fix

Sequence Diagram(s)

sequenceDiagram
  participant UtilsModule
  participant installDistrustedCaCheck
  participant TLSIdentityValidation
  UtilsModule->>installDistrustedCaCheck: Install check at module load
  installDistrustedCaCheck->>TLSIdentityValidation: Add identity-validation wrapper
  TLSIdentityValidation->>TLSIdentityValidation: Match certificate chain
  TLSIdentityValidation-->>TLSIdentityValidation: Return identity error or distrusted-intermediate error

Loading

Suggested reviewers: karanshah-browserstack

Merge Risk: ⚪ Minimal · up to 96b3a

The change adds a TLS check that rejects certificate chains containing the compromised eMudhra intermediates, with tests for matching, hostname-error precedence, and repeat installation. No concrete merge-blocking problem was found.

Security architecture risk: 🔵 Low · up to 96b3a

The change adds a certificate-distrust defense while preserving existing hostname validation. Inspected startup paths install it before their network operations. Remaining uncertainty concerns runtime compatibility, installation failures, and external networking components; no introduced exploitable weakness was established.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The trust-policy change is not restricted to BrowserStack hostnames. It can affect any TLS connection in the same Node process that uses the replaced default identity callback, including unrelated connections in a customer's Cypress process. Separate processes and callers using independent callbacks are not automatically covered.

Security Findings and Attack Paths

• inferred — The inspected repository startup ordering reduces the deferred concern about requests occurring before installation. Complete protection is still not established across external networking implementations and supported runtimes. This remains a coverage limitation, not a verified introduced attack path.

Trust Boundaries and Controls

• observed — The wrapper preserves the preceding hostname-validation result and adds a rejection decision based on peer-certificate chain data. Repository-owned HTTPS agent configurations inspected by the boundary probe do not supply a competing identity callback, although complete external adapter behavior remains unverified.

Resilience and Maintainability Implications

• observed — If installation is skipped or throws, this code leaves the prior identity validator in place rather than disabling it. The new distrust guarantee can therefore be absent without weakening the pre-existing validator; this distinction limits the demonstrated PR-introduced risk.

Hardening Proposals

• proposed — Make installation outcomes distinguishable and retry-safe, and validate enforcement across supported Node versions, proxy agents, and Local startup. These measures would strengthen assurance of the added defense rather than remedy a demonstrated regression in existing TLS validation.

Pre-merge checks: 4

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: rejecting TLS certificate chains that contain compromised eMudhra intermediate keys.
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{A rabbit checks each chain with care,
Eight names and hashes wait in there.
A trusted path may pass along,
A match returns an error strong.
The hostname check keeps its place,
Then hops away with TLS grace.}

_{Comment @coderabbitai help to get the list of available commands.}

[harshit-browserstack]

✅ Good to go

Re-review at 96b3a51 ("guard the distrusted CA check install and add unit tests"). Both findings from the previous run on 2603ad7 are resolved, and the review is clean with no blocking findings.

1. Missing unit test: resolved. test/unit/bin/helpers/distrustedCaCheck.js adds 6 cases, all passing when run at this commit:

• a chain carrying a distrusted intermediate's name is refused (ERR_TLS_DISTRUSTED_INTERMEDIATE)
• names are compared case-insensitively with whitespace collapsed
• a distrusted key under another name is refused, using the real EM DV TLS CA - G3B certificate
• a clean chain returns undefined
• the hostname error is returned before the chain is checked
• a second install returns false and does not wrap tls.checkServerIdentity again

I also broke the module one change at a time and re-ran the file. Each break was caught: a wrong G3B hash fails the key case (1 failure); the chain check disabled fails 3 cases; name matching removed fails both name cases (2); the hostname early-return removed fails the hostname-order case (1); the existing-install guard removed fails the double-wrap case (1).

2. Unguarded install in utils.js: resolved. The call is now inside try/catch that logs at debug level. With tls frozen (Object.freeze), utils.js loads without throwing and the check is simply not installed. The logger required in the catch adds no new failure mode, because utils.js already requires the same logger at line 25, and logger.js requires only winston, fs and path.

3. Suggestion (non-blocking, does not gate): when the install fails, the protection turns off silently and the only trace is a debug-level log. Not breaking the customer's run is the right call, but this is a security control failing open. Consider logging that single line at warn instead of debug so the unprotected state is visible.

4. Caveat on test coverage: the tests exercise only 1 of the 8 SPKI hashes (G3B DV) and 1 of the 8 names. The PR author reports checking all 16 published certificates against the advisory JSON; that was not re-verified here. A mismatch in the other 7 hashes or names would not be caught by these tests.

Per-File Confidence

File	Status
bin/helpers/distrustedCaCheck.js	✅ All Clear
bin/helpers/utils.js	✅ All Clear
test/unit/bin/helpers/distrustedCaCheck.js	✅ All Clear

Change map (generated deterministically from the diff)

graph LR
  subgraph nnode_agent["node-agent"]
    nbin_helpers_distrustedCaCheck_js["distrustedCaCheck.js<br/>~119 lines"]
    ntest_unit_bin_helpers_distrustedCaCheck_js["distrustedCaCheck.js<br/>~95 lines"]
    nbin_helpers_utils_js["utils.js<br/>~7 lines"]
  end

Loading

↻ This verdict comment is the review anchor — it's updated in place on each run (the gate posts its status separately).

— SDK PR Review Agent

[harshit-browserstack]

Review follow-up — two gaps in the chain check, fixed in the latest commit (same change in browserstack-binary#1960, browserstack-node-agent#2742, browserstack-cypress-cli#1203, browserstack-local-nodejs#188):

1. One unparseable certificate ended the check for the whole chain (fail-open). The catch is now per certificate: that certificate is skipped and its issuers are still checked.
2. Decoy bypass. Node links issuerCertificate by name, so a server sending [leaf, expired decoy with the intermediate's name and key id, real intermediate] passes OpenSSL verification (it skips the expired decoy) while Node's links lead the walk to the decoy and never reach the distrusted key. Reproduced locally on Node 18 and 22. The check now also matches the 8 intermediates' subject names against each certificate's subject and issuer; they are all pathlen:0, so the leaf's issuer is always one of those names and a decoy cannot divert that.

Verified locally: the decoy chain is now refused in both orders on Node 16/18/22 (20/20), the earlier cases still hold (50/50, including a forced parse failure on the leaf), the 8 listed names match the parsed subjects of all 16 published certificates, and 20 hosts the SDK, binary and Local use still connect.

[harshit-browserstack]

Security-review follow-up (latest commit): names are now compared the way OpenSSL chains them (case-insensitive, whitespace collapsed). Reproduced the gap first: a leaf naming its issuer test intermediate validates through the real Test Intermediate, and with a TEST INTERMEDIATE decoy diverting Node's issuer links, exact matching let the chain through on the previous commit. Now refused on Node 16/18/22; full matrix 80/80 decoy runs + 50/50 standard cases, 16/16 published certificates still matched, 20/20 legitimate hosts still connect.

[harshit-browserstack]

Re-review follow-up (latest commit): the install state now lives on the tls module instead of global. Reproduced first: Jest gives each test file its own global but shares core modules, so three Jest-style contexts wrapped tls.checkServerIdentity three times; now once (Node 16/18/22). Worker threads still install their own check and refuse the incident host. Regression unchanged: 80/80 decoy runs, 50/50 standard cases, legitimate hosts still connect.

[shivam5643]

SDK PR Review: changes requested (2 findings)

Reviewed HEAD 2603ad75. Both changed files were fully reviewed. The hand trace of the chain-walk logic found no logic defects: the hostname check runs first, the walk stops at self-signed roots, an unparseable key falls back to the name check, and install is idempotent via tls[STATE_KEY]. Two items remain.

1. 🟠 No unit test for the distrusted-CA check (bin/helpers/distrustedCaCheck.js:85, installDistrustedCaCheck)

The PR adds the check but no test. A later change could stop distrusted chains from being refused, and nothing would catch it. That change could be a wrong hash, a change to name normalisation, or a refactor of the issuerCertificate walk. The PR body also notes that a full browserstack-cypress run hasn't been done yet.

Suggested fix: add a unit test using fake peer-cert objects ({subject, issuer, raw, issuerCertificate}) that asserts:

• a chain with a distrusted name returns ERR_TLS_DISTRUSTED_INTERMEDIATE
• a chain with a distrusted SPKI hash returns ERR_TLS_DISTRUSTED_INTERMEDIATE
• a clean chain returns undefined
• a second installDistrustedCaCheck() returns false (no double-wrap)

2. 🟡 Unguarded install at module load (bin/helpers/utils.js:3), needs a quick human check

require('./distrustedCaCheck').installDistrustedCaCheck(); runs when utils.js loads, with no try/catch. As the PR's own comment says, utils.js is also loaded inside the customer's Cypress process (reporter/plugins). If the install ever threw, utils.js would fail to load and the customer's run would break.

This is low likelihood. createPublicKey runs inside the wrapped callback, not at install, so a throw would need something unusual like a frozen tls object. There is no circular-require risk, since distrustedCaCheck.js only requires tls and crypto. Still, a try { … } catch (e) { logger.debug(…) } around the call would guarantee graceful degradation. Alternatively, confirm the install can't throw and dismiss this.

Not covered by this review (worth a look)

• The 8 SPKI hashes and the name strings were not checked against the eMudhra advisory.
• On Node versions without crypto.createPublicKey, installDistrustedCaCheck() returns false, so the protection is silently off. Consider a debug log there.
• Only connections that use the default tls.checkServerIdentity are protected. Callers passing a custom checkServerIdentity or ca, or rejectUnauthorized: false, are not.
• Name matching uses CN/O/C only.
• The diff can't show whether every entry point loads utils.js before its first TLS connection.

— SDK PR Review Agent

[harshit-browserstack]

Review follow-up (96b3a51)

1. Unit tests. I added test/unit/bin/helpers/distrustedCaCheck.js with 6 cases:

   • a distrusted name is refused
   • names are compared case-insensitively, with whitespace collapsed
   • a distrusted key under a different name is refused. This case uses the real EM DV TLS CA - G3B certificate, so it also checks the hard-coded hash.
   • a clean chain returns undefined
   • the hostname error is returned before the chain is checked
   • a second install returns false and doesn't wrap again

   I also broke the module on purpose to confirm the tests catch it. With a wrong G3B hash, the key case fails. Without the name normalisation, both name cases fail. The utils.js suite gives 399 passing and 5 failing; the same 5 fail on the base commit.

2. Unguarded install. The install is now wrapped in try/catch with a debug log. I reproduced the problem first: with a frozen tls module, utils.js failed to load on 2603ad7. It now loads, with the check off.

On the "not covered" notes:

• Hashes and names: I checked them against the advisory's impacted-intermediates.json. All 16 certificates (8 keys) match, by SPKI and by name.
• No createPublicKey: that only happens on Node < 11.6, which can't run any supported Cypress version.
• Custom ca: still covered, because Node calls tls.checkServerIdentity whatever ca is set to. rejectUnauthorized: false already accepts any certificate, so there's nothing to protect there. No code in bin/ passes its own checkServerIdentity.
• CN/O/C: these three fields are enough to identify the 8 intermediates uniquely.
• Entry points: loading commands/runs.js, the observability reporter or the accessibility plugin installs the check. The observability plugin doesn't load it, but it makes no network calls; it only forwards events over node-ipc to the reporter, which does the HTTPS uploads.

[harshit-browserstack]

RUN_TESTS