Repository navigation
PTAAA-881: remove extension-based self-healing and the ai-sdk-node dependency (v8) - #322
Conversation
…pendency v8 port of #315, which landed the same removal on main (wdio v9). The security fix has to ship in every supported major, and v8 carries the identical vulnerable path pinned to @browserstack/ai-sdk-node 1.5.17. The client-side healing path downloaded the AI browser extension via BrowserstackHealing.init() and loaded it into the local browser, either through goog:chromeOptions.extensions or browser.installAddOn(). That extension ships a window.postMessage handler that forwards a page-controlled endpoint and bearer to a privileged background fetch, giving any visited page an SSRF primitive against the machine running the test. The path is deprecated and only ever ran off BrowserStack infra, so it is removed rather than hardened. Removes AiHandler and its two entry points, the healing branch of the funnel instrumentation, the capability helpers and constants left without callers, the self-heal performance events, and the @browserstack/ai-sdk-node dependency. Self-healing on BrowserStack infra is unaffected: it is driven by the browserstack.selfHeal capability, resolved hub-side, and never went through this code. BrowserstackConfig.selfHeal is deliberately kept, and an off-infra run that sets it now gets a one-time warning instead of silently losing healing. Typecheck 12 -> 9 errors (3 removed, 0 added; the rest pre-existing). Tests 738 -> 728 passing with 0 failures either side; the 10-test drop is the deleted healing suites. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
|
🔴 Blocking findings — fix required 4 blocking issue(s) — all objectively grounded:
Full review: see the linked review.md. Change map (generated deterministically from the diff)graph LR
subgraph nwdio_service["wdio-service"]
npackages_browserstack_service_tests_ai_handler_test_ts["ai-handler.test.ts<br/>~750 lines"]
npackages_browserstack_service_src_ai_handler_ts["ai-handler.ts<br/>~234 lines"]
npackages_browserstack_service_src_instrumentation_funnelInstrumentation_ts["funnelInstrumentation.ts<br/>~181 lines"]
npackages_browserstack_service_tests_instrumentation_funnelInstrumentation_test_ts["funnelInstrumentation.test.ts<br/>~181 lines"]
npackages_browserstack_service_src_util_ts["util.ts<br/>~41 lines"]
npackages_browserstack_service_src_launcher_ts["launcher.ts<br/>~27 lines"]
npackages_browserstack_service_src_constants_ts["constants.ts<br/>~12 lines"]
npackages_browserstack_service_src_service_ts["service.ts<br/>~12 lines"]
npackages_browserstack_service_src_instrumentation_performance_constants_ts["constants.ts<br/>~10 lines"]
n_changeset_pr_322_md["pr-322.md<br/>~5 lines"]
npackages_browserstack_service_package_json["package.json<br/>~1 lines"]
end
↻ 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 |
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
sendEvent was only read by the healing instrumentation helpers removed with AiHandler, so it is dead once they go — the same no-unused-vars failure this file hit on main. The new selfHeal warning gated on !shouldAddServiceVersion(...), which also returns false for a chromedriver service with observability left on. A genuine BrowserStack session could therefore be told it was not running on BrowserStack infrastructure. Gate on isBrowserstackInfra() instead, which is the condition the message actually describes. shouldAddServiceVersion stays for its four other callers. Typecheck unchanged at 9 pre-existing errors; tests unchanged at 728 passing / 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
…e.ts Its only call site was the healing block removed in this PR; the call went and the import stayed. Same no-unused-vars shape as the sendEvent map. Swept the named imports of every file this PR touches — this was the only one left dangling. Typecheck unchanged at 9 pre-existing errors; tests unchanged at 728 passing / 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
|
✅ SDK PR Review — approve. No code changes required. Re-review of Verified clean
Port fidelity vs #315Verified file-by-file against #315's final merged state ( #322 is strictly better than #315 in one place. The warning gate here is One correction to the PR description (non-blocking)The verification section claims Considered and dismissedAn earlier pass flagged Security reasoningGating is sound — the removed path only ran off-infra, and infra healing is capability-driven ( |
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
1 similar comment
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
Picks up the root-level mocha hook fix (#309). One conflict in service.ts: its new _setCliSuiteFile(specs?.[0]) call sits directly above the healing block this branch removes, so the delete read as covering both. Kept the mocha call, dropped only the healing block. Tests 731 passing / 0 failures, including all 12 of the new mocha-hook tests; typecheck unchanged at 9 pre-existing errors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
1 similar comment
|
🔴 SDK PR Review gate is red. Pending:
It turns green once the SDK PR Review Agent has run on the current head commit (any verdict — the gate only requires that the review ran). A native reviewer approval is separately required by branch protection before merge. |
SDK PR Review Agent — verdict: ✅ successReviewed against #315 (the Scope reviewed
Warning gate — reviewed, correct as written
#325 ports this same correction back to Non-blocking notes
Verdict is advisory per |
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
1 similar comment
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
|
RUN_TESTS |
|
[SDK Wdio Test] TRA build state: failed | Stability 96% — verdict: success. Passed: 51, Failed: 2, Aggregate: 53. TRA: https://observability.browserstack.com/builds/y5rii2cnlwligbxhksiedcl2zfk8qeftfqk0matn |
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
1 similar comment
|
🟢 SDK PR Review gate is green — the SDK PR Review Agent has run on the current head commit (verdict: This gate confirms a review ran on the latest commit. The verdict itself is advisory — read the findings and use your judgement; it does not block merge. A native GitHub reviewer approval is still separately required by branch protection before this PR can merge. |
What is this about?
v8 port of #315, which landed the same removal on
main(wdio v9). This is a security fix, so it has to ship in every supported major — v8 carries the identical vulnerable path, pinned to@browserstack/ai-sdk-node@1.5.17.BrowserstackHealing.init()downloaded the AI browser extension and loaded it into the local browser — viagoog:chromeOptions.extensions(Chrome/Edge) orbrowser.installAddOn()(Firefox). That extension ships awindow.postMessagehandler that accepts a page-controlled endpoint and bearer token and forwards them to a privileged background fetch, giving any page a test visits an SSRF / CORS-bypass primitive against the machine running the test (localhost, private network). Reported via HackerOne; the path is deprecated, so it is removed rather than hardened.This path only ever ran for sessions not on BrowserStack infra (gated on
!shouldAddServiceVersion(...)).Self-healing on BrowserStack infra is unaffected. That is driven by the
browserstack.selfHealcapability, resolved hub-side, and never went through this code.BrowserstackConfig.selfHealis deliberately kept so existing capability configs keep type-checking.Removed:
AiHandler(src/ai-handler.ts) and its two entry points inlauncher.ts/service.tshandleHealingInstrumentation+ its file-local helpers)isValidCapsForHealing/hasBrowserName/getBrowserStackUserAndKeyinutil.ts— no callers left after the aboveTCG_URL,TCG_INFO,SUPPORTED_BROWSERS_FOR_AI,BSTACK_TCG_AUTH_RESULTinconstants.tsSDK_AI_SELF_HEAL_STEP/SDK_AI_SELF_HEAL_GET_RESULTand theAI_EVENTSmap@browserstack/ai-sdk-nodedependency (andaxios, which it was the only source of)~1,607 deletions. The only addition is a one-time warning when
selfHeal: trueis set on an off-infra session, so the behaviour change is visible rather than silent.This matches the final state of #315 after its review rounds — the orphan cleanups that took three follow-up commits there are folded in here.
Related Jira task/s
https://browserstack.atlassian.net/browse/PTAAA-881
Release (mandatory for every PR — required for the
ready-for-reviewlabel)Version bump: (required — tick exactly one)
Release notes type: (optional)
Release notes (customer-facing): (optional but encouraged)
browserstack.selfHealcapability.Release notes (internal): (required — engineer-facing; what actually changed / why)
AiHandlerand the extension-based healing path (PTAAA-881). The bundled extension exposed an unvalidated cross-originpostMessagehandler that forwarded a page-controlled endpoint and bearer to a privileged fetch — a client-side SSRF reachable from any page a test visits. The path ran only off BrowserStack infra and is deprecated.@browserstack/ai-sdk-nodedependency (and transitivelyaxios). The pin was1.5.17while the SDK is at 1.8.x;logData/healFailurehad already been removed upstream, so this code only resolved because of the stale pin.Checklist
Verification (measured on two clean worktrees — PR head
89892a3vs its merge-base722e27d, each with its ownnpm ci. The merge-base is the right baseline here: the branch sits behindv8by release commit #327, so currentv8would fold unrelated drift into the comparison.)Build / typecheck —
npm run build(buf generate && tsc -p tsconfig.prod.json) exits 0 on both.tsc --noEmitunder bothtsconfig.jsonandtsconfig.prod.json: 0 errors → 0 errors. No typecheck change either way. (Note: both tsconfigs scope tosrc/**/*; tests are excluded from typechecking.)Tests —
npm test(scripts/test-isolated.mjs, which runs each file in its ownvitest run): 1177 passing / 0 failing → 1137 passing / 0 failing, 47 → 46 files. Delta −40, reconciling exactly:tests/ai-handler.test.tsdeleted — −30tests/instrumentation/funnelInstrumentation.test.tstrimmed 19 → 9 — −10Verified per-file across all 47: those two are the only files whose test count changed. No file fails to load on either revision.
PR Validations
Run Tests: Comment RUN_TESTS to trigger sanity tests.
🤖 Generated with Claude Code