LOC-6805: allowlist option keys forwarded to the BrowserStackLocal binary

[07souravkunda]

Problem

addArgs() prefixed any unrecognised key of the caller-supplied options object with -- and pushed it, with its value, onto the BrowserStackLocal daemon argv:

default:
  this.userArgs.push('--' + key);
  this.userArgs.push(value);

getBinaryArgs() appends userArgs verbatim into the spawnSync / execFile argv, so any caller — or upstream code that merges untrusted input into options — could shape the native binary's argv. Argument injection, CWE-88.

Fix

Two rules, both enforced before anything reaches the argv:

1. Keys — only documented BrowserStackLocal modifiers are forwarded. PASSTHROUGH_OPTIONS mirrors the binary's own CLI definition (COMMAND_CONFIGURATION in browserStackTunnel) — long names and aliases — so anything now refused is something the binary would not have acted on anyway. daemon, log-file and source are reserved because getBinaryArgs() sets them itself and a caller must not be able to append a conflicting second copy.
2. Values — a value may never pose as a flag. The binary's parser will not consume a value beginning with -; it reads it as another flag. The check runs in addArgs above the switch, so it covers every key, and each element of a list-valued option is checked.

addArgs returns a LocalError, delivered through the existing paths: callback(err) for start(), returned for startSync().

Compatibility

Documented modifiers keep working — verified for localProxyHost/Port/User/Pass, pac-file, local-proxy-port, custom-repeater, bs-host, include-hosts, and the explicitly-cased options.

Behaviour change worth a reviewer's nod: an unknown option key is now an error rather than a silent no-op forward, and so is a value starting with -. The latter is not a regression — such values are already mis-parsed today, the option silently becoming true.

Edge case called out: a value starting with - is refused even where it might be legitimate (e.g. an ntlm-password beginning with -). That case does not work today either.

Test plan

• npm test — eslint clean; 12 argument-handling tests pass; all pre-existing tests before the (pre-existing) should stop local abort still pass. That abort reproduces on untouched master.
• Two tests that asserted the vulnerable behaviour replaced with rejection tests.
• Standalone repro covering the reported payload, the reserved keys, value smuggling through explicitly-cased keys (--flag=value form), list elements, and positive checks on documented modifiers: 29 failures → 0.
• mocha --grep "Start sync" in isolation — 2 passing, 1 pending.
• Real tunnel started through the binding, Automate session over it, graceful .stop().
• Fetching a page on the tester's machine inside the remote browser returned "Page not found". A control run of the same harness against pristine master fails identically, and verbose tunnel logs show the request never reaches the binary — an environment-side tunnel attach issue, not argv construction.

Review round 1

The value check previously lived in the passthrough helper, so the ~21 options with an explicit switch case bypassed it and could still smuggle a flag as their value. Now hoisted above the switch. Also from review: null/undefined values are skipped and argv elements are coerced to strings; the shadowed entries in the allowlist are documented as listed-for-completeness.

Note for the merger: please squash-merge. The first commit's message body still carries internal tracker/finding ids; squashing lets you set a single clean message. No such id ships in the tree.

[07souravkunda]

Pipeline security review — round 0. 2 blocking, 2 nits, 1 for-human. Keeping the PR in Draft; a human owns approval.

The allowlist itself is right. I diffed PASSTHROUGH_OPTIONS against the binary's own COMMAND_CONFIGURATION (browserStackTunnel@origin/master, BsGlobal.* flag names resolved from extensions/common/global.js) and there is no missing binary option and no extra once the explicit switch cases and RESERVED_OPTIONS are accounted for — so the "documented modifiers keep working" claim holds. The diff is scoped to three in-scope files with no drive-by changes, and the test evidence checks out independently: I reran the suite on this branch with real credentials (eslint clean, 26 pass incl. all 7 new tests, then the pre-existing should stop local abort) and reran it on a pristine 0d29261 control worktree, which aborts in the same byte-identical test — the "pre-existing, not a regression" claim is confirmed. Session 43078073c566d8795c483a6b4f9c545cb2da1b05 is real (build locsec-WI-549cf2ac, local in top_capabilities), and the one unverified hop is honestly reported with a runnable checklist.

Blocking:

1. lib/Local.js:339 — the value-side guard covers only the default: branch. Every explicitly-cased option bypasses it, which defeats both the --value rule and RESERVED_OPTIONS. Since bs-minimist accepts --flag=value, { localIdentifier: '--log-file=/tmp/attacker-owned' } and { only: '--config-file=/tmp/attacker.yml' } both land in the spawned argv — the chain's step-1 primitive and its step-3 amplifier, still reachable through documented keys. Verified by running this branch; full transcript inline. The PR body and the posted Jira comment both state "a value beginning with -" is refused, so they currently overstate coverage. One-line fix: hoist the check above the switch.

2. test/local.js:127 — internal tracker ids in a public repo. Title, commit subject, body (four Jira links + internal finding ids) and now a source comment. git grep finds zero LOC-/SC- ids in the current master tree, so the code comment would be the first to ship. There's no CVE/GHSA to substitute (internal scanner finding) — describe it as CWE-88 argument injection instead. Ticket-ids-in-commit-subjects does have precedent here, so push back on that part if it's deliberate convention.

Nits on lib/Local.js:330 (null/undefined value pushed raw into the argv) and lib/Local.js:34 (15 shadowed, unreachable allowlist entries — logFile most confusingly). For-human on lib/Local.js:17: the five sibling bindings share this defect with no tickets, index.d.ts still permits any key, and the intentional unknown-key-now-errors change wants a minor bump plus a changelog line.

[07souravkunda]

[blocking] The value-side guard lives in addUserArg, so it only runs for keys that fall through to default:. Every option with an explicit switch case — which is every key the README documents — skips it entirely, and that defeats both guards this PR advertises: the --value rule and RESERVED_OPTIONS.

bs-minimist also accepts the --flag=value form (extensions/node/src/bs-minimist.js, /^--.+=/ branch), so a smuggled flag carries its own value — no need to win a following-argv-slot race.

Evidence — run against this branch (1964cb1), printing the real getBinaryArgs() output:

'log-file': '/tmp/attacker-owned'            -> LocalError: Option 'log-file' is set by browserstack-local itself   ✅
'daemon': 'stop'                             -> LocalError: Option 'daemon' is set by browserstack-local itself      ✅
region: '--config-file=/tmp/evil.yml'        -> LocalError: values starting with '-' are not allowed                 ✅

localIdentifier: '--log-file=/tmp/attacker-owned'
  -> accepted, argv: ["--key","DUMMYKEY","--local-identifier","--log-file=/tmp/attacker-owned"]                      ❌
localIdentifier: '--daemon=stop'
  -> accepted, argv: ["--key","DUMMYKEY","--local-identifier","--daemon=stop"]                                       ❌
only:   '--config-file=/tmp/attacker.yml'
  -> accepted, argv: ["--key","DUMMYKEY","--only","--config-file=/tmp/attacker.yml"]                                 ❌
folder: '--config-file=/tmp/attacker.yml'
  -> accepted, argv: ["--key","DUMMYKEY","-f","--config-file=/tmp/attacker.yml"]                                     ❌

The bottom four are the chain's step-1 primitive (attacker-chosen config file) and its step-3 amplifier (the continuous arbitrary-write via --log-file) still reachable through documented keys. Same for proxyHost/proxyPort/proxyUser/proxyPass, parallelRuns, useCaCertificate, logFile, key, verbose.

This also makes the PR description and the posted Jira comment inaccurate: both say "a value beginning with -" is refused, and the Jira comment lists value-side smuggling under what the fix closes. It's closed only for allowlist-branch keys.

Fix — hoist the value check above the switch so it applies to every key, then drop it from addUserArg:

this.addArgs = function(options){
  for(var key in options){
    var value = options[key];

    if(INTERNAL_OPTIONS.indexOf(key) === -1 && value !== undefined && value !== null
       && value.toString().charAt(0) === '-')
      return new LocalError('Invalid value for option \'' + key + '\': values starting with \'-\' are not allowed');

    switch(key){

Worth a test per family — one explicitly-cased key (localIdentifier) alongside the existing region case, and one asserting --log-file=/--daemon= cannot be smuggled that way.

[07souravkunda]

Confirmed and fixed in 4ab6262 — you were right, and my own repro had a blind spot: I only tested {region: '--daemon'}, and region happens to fall through to default:, so the guard I wrote never got exercised against an explicitly-cased key.

Reproduced your finding verbatim on 1964cb1 before changing anything:

{localIdentifier:'--log-file=/tmp/attacker-owned'} -> [..., "--local-identifier","--log-file=/tmp/attacker-owned"]
{localIdentifier:'--daemon=stop'}                  -> [..., "--local-identifier","--daemon=stop"]
{only:'--config-file=/tmp/attacker.yml'}           -> [..., "--only","--config-file=/tmp/attacker.yml"]
{folder:'--config-file=/tmp/attacker.yml'}         -> [..., "-f","--config-file=/tmp/attacker.yml"]
{proxyHost:'--config-file=/tmp/x.yml'}             -> [..., "--proxy-host","--config-file=/tmp/x.yml"]
{parallelRuns:'--daemon=stop'} / {verbose:'--daemon=stop'} -> likewise

Fix is the hoist you asked for: the check now lives in addArgs immediately above the switch, as rejectFlagLikeValue(key, value), so it runs for every key before any case can consume the value. One addition beyond the ask — --include-hosts/--exclude-hosts are type: 'list' and take an array, so the guard walks every element rather than relying on Array.prototype.toString; ['ok','--config-file=/tmp/x.yml'] would otherwise stringify to ok,--config-file=... and slip past a first-character test.

Tests added, all asserting rejection and that the smuggled token is absent from the argv:

• should reject a flag-like value on an explicitly handled option (localIdentifier + --log-file=)
• should reject a daemon-lifecycle smuggle through an explicitly handled option (only + --daemon=)
• should reject a flag-like element inside a list-valued option
• should forward a list-valued option as separate argv elements (the positive side)

The standalone repro now covers all 15 explicitly-cased keys plus the list case: 29 failures on 1964cb1 → 0 on 4ab6262.

On the accuracy point: the PR body has been rewritten, and with the code fixed the statement "a value beginning with - is refused" is now true for every key rather than overstated.

[07souravkunda]

[blocking] This repo is public (visibility: public, 72 stars / 54 forks), and internal identifiers appear in four places on this PR: the title, the commit subject, the description (four Jira links plus the internal finding ids), and now this source comment.

git grep -E '\b(LOC|SC)-[0-9]+' origin/master returns zero hits — no internal id currently ships in the tree, so this comment would be the first. There is precedent for ticket ids in commit subjects here (LOC-5083: Add support for Linux arm64 binary), so if that part is a deliberate team convention, say so and I'll drop it; the Jira links, the internal finding ids, and this code comment are new and don't have that cover.

There's also no public advisory to point at instead — this came from the internal security-ai-review scanner, so there's no CVE/GHSA. Describe it by its weakness class:

  // CWE-88 (argument injection): addArgs used to prefix ANY unknown option key
  // with '--' and push it onto the daemon argv, letting a caller — or upstream
  // code merging untrusted input into `options` — inject arbitrary flags into
  // the native binary. Only documented BrowserStackLocal modifiers may be
  // forwarded now.

Same treatment for the title (allowlist option keys forwarded to the BrowserStackLocal binary), the commit subject, and the body — the exploit narrative and tracker links belong on the Jira side. Note the body currently also publishes a working description of the primitive against an unreleased fix.

[07souravkunda]

Fixed the parts that had no cover, and I verified your premises independently before acting:

• git grep -E '\b(LOC|SC)-[0-9]+' origin/master → zero hits. Confirmed: this comment would have been the first internal id to ship in the tree.
• git log origin/master --format=%B → zero atlassian.net links. Confirmed: the Jira URLs had no precedent either.
• Ticket-id subject prefixes do have precedent, as you noted: 560755f LOC-5083: Add support for Linux arm64 binary, plus branches LOC-6635_sdk_binary_compatibility, LOC-6480, LOC-4920-win-binarypath-fix.

What changed:

1. Source comment (test/local.js) — internal ids removed; it now reads as "Argument injection (CWE-88)". You're right that there is no CVE/GHSA to substitute, so CWE-88 is the identifier. The tree is back to zero internal ids.
2. PR description — rewritten. No Jira links, no internal finding ids, and the working description of the primitive is gone; it now describes the defect shape and the fix without a recipe against the unreleased version.
3. PR title — kept the LOC-6805: prefix, on the precedent above. Flagging rather than deciding: if you'd rather it go too, that's a one-line change.

One I could not complete: the first commit's message body (1964cb1) still carries internal ids beyond the subject prefix. Removing it needs history rewriting, and force-push is blocked for me in this environment — I amended it, hit the push restriction, and deliberately unwound the amend rather than leave the branch un-pushable. So the round-2 work landed as a normal follow-up commit and 1964cb1 is byte-identical to what you reviewed.

Mitigation: I've added a note in the PR description asking the merger to squash-merge, which collapses both messages into one the merger writes, so nothing from that body reaches master. If you'd rather it be scrubbed on the branch itself, that needs someone with force-push rights.

[07souravkunda]

[nit] stringValue normalises null/undefined to '' for the two checks, but the push below sends the raw value, so the normalisation is discarded:

region: null       -> accepted, argv: [..., "--region", null]
region: undefined  -> accepted, argv: [..., "--region", null]

execFile/spawnSync reject a non-string argv element, so this surfaces as an ERR_INVALID_ARG_TYPE thrown synchronously out of start() rather than a LocalError through the callback. Numbers and arrays land raw too ('connect-timeout': 30 → [..., "--connect-timeout", 30], 'include-hosts': ['a','b'] → a nested array).

Pre-fix this threw a TypeError at value.toString(), so it's not a regression — but you're one line from making it clean. Every explicit switch case guards with if(value); matching that here would mean skipping a null/undefined value, and pushing stringValue instead of value for the rest.

[07souravkunda]

Taken, in 4ab6262. addUserArg now matches the if(value) convention the explicit cases use — null/undefined is skipped rather than pushed — and every argv element is coerced to a string.

Arrays got slightly more than the one-line version: pushing stringValue would turn ['a','b'] into the single token a,b, which the binary's list parser would read as one host rather than two — a silent wrong value in place of the current ERR_INVALID_ARG_TYPE. So array values are pushed as separate elements, which is what a type: 'list' flag expects. Covered by should forward a list-valued option as separate argv elements.

Also added should skip a null or undefined passthrough value instead of pushing it raw, which asserts --region is absent for region: null, that 'connect-timeout': 30 arrives as the string '30', and that every element of getBinaryArgs() is a string.

[07souravkunda]

[nit] 15 of the 55 entries here are unreachable — they're shadowed by an explicit switch case, so they never hit default:/addUserArg: key, folder, force, only, forcelocal, verbose, onlyAutomate, proxyHost, proxyPort, proxyUser, proxyPass, localIdentifier, forceproxy, logFile, parallelRuns.

logFile is the confusing one, because the RESERVED_OPTIONS comment right below says "logFile has its own supported option; only the raw binary alias is reserved" — which is correct, and is exactly why logFile shouldn't be in the passthrough list at all. Reads today as though logFile were a passthrough.

Harmless at runtime, but the list is the thing a maintainer will diff against COMMAND_CONFIGURATION next time the binary gains a flag, so it's worth it being honest about what it controls. Either drop the shadowed names, or keep them with a one-line note that they're listed for completeness against the binary's CLI and handled by explicit cases above.

Verified separately: against COMMAND_CONFIGURATION at browserStackTunnel@origin/master (with BsGlobal.SANDBOX_FLAG/DISABLE_SANDBOX_FLAG/TUNNEL_BIND_FLAG/TUNNEL_DISALLOW_FLAG resolved from extensions/common/global.js:207-211), the allowlist has no missing binary option and no extras once explicit cases and RESERVED_OPTIONS are accounted for. The compatibility claim holds.

[07souravkunda]

Took the second option you offered — kept the entries, added the note — since a complete mirror is what makes the list diffable against COMMAND_CONFIGURATION when the binary gains a flag, which is the maintenance path you describe.

PASSTHROUGH_OPTIONS now carries a comment naming the 15 shadowed entries explicitly and saying they are listed for completeness, not effect.

The logFile contradiction is fixed at the other end: the RESERVED_OPTIONS comment no longer says "logFile has its own supported option" as if that made it a passthrough. It now spells out that the log file is settable only through the wrapper's logfile/logFile case, which routes into getBinaryArgs' single --log-file, and that the binary's raw log-file alias is reserved precisely so a second one cannot be added.

Thanks for the independent check that the allowlist has no missing binary option and no extras — that's the part I'd have had the hardest time proving to a reviewer myself.

[07souravkunda]

[for-human] Two scope calls the PR flags but can't settle on its own — both need someone to decide, not more code here:

1. Sibling bindings. browserstack-local-python / -ruby / -java / -php / -csharp carry the same addArgs catch-all shape. If F-007 applies there it needs its own tickets — none exist, and none of those repos are in scope for this work item. Worth confirming and filing before this closes, otherwise the same primitive stays open in five packages while the chain ticket reads as resolved.

2. index.d.ts still declares [key: string]: string | boolean. TypeScript users get no compile-time signal for a key that now throws at runtime. Removing the index signature would break the documented modifiers the interface doesn't enumerate (pac-file, localProxyHost, region, …), so the real fix is to enumerate them — a typings pass, reasonably a follow-up rather than this PR.

Also for the record, since it changes what a caller sees: an unknown key is now a hard error rather than a silent no-op forward. LOC-6786 offered "silently ignore and warn" as an alternative and this PR deliberately didn't take it. I agree with that choice — a silent drop hides caller bugs — but it's a breaking change for anyone currently passing a typo'd or stale key, so it wants a minor-version bump and a changelog line, not a patch release.

[07souravkunda]

Acknowledged — both are genuinely human calls and I'm not deciding either here. They're carried in the Jira ticket's assessment under "Not tested" / follow-ups (sibling bindings not in this repo or work item; index.d.ts index signature left as-is because removing it would break the documented modifiers the interface doesn't enumerate). No code change this round.

[07souravkunda]

Pipeline security review — round 1 (head 4ab6262). The security fix itself is now complete and independently verified. 1 blocking item remains, and it's a records/disclosure fix, not a code fix.

Round-0 findings, re-checked against the new diff rather than assumed:

Round-0 finding	Status
blocking — --value guard only covered the default: branch	Fixed. Hoisted above the switch; 37/37 probe vectors pass, including all 18 explicitly-cased keys and the --flag=value smuggle that defeated RESERVED_OPTIONS. Accept path not over-tightened.
blocking — internal ids on a public repo	Partly fixed. Tree is clean (zero git grep hits), body cleaned of Jira links + exploit narrative. Title and commit 1964cb1's message still carry them — see below.
nit — raw null/number/array in argv	Fixed for scalars; the new array branch reintroduced it for a null element.
nit — 15 shadowed allowlist entries	Fixed — documented as listed-for-completeness, and the log-file/logFile comment is no longer self-contradictory.
3 judgment items	Still open, unchanged — carried forward.

I also re-ran the suite with real credentials on this head: eslint clean, 31 passing / 1 pending including all 12 argument-handling tests, then the same pre-existing should stop local abort — which I again reproduced on a pristine 0d29261 control worktree, same byte-identical test. And I verified the round-1 e2e session 1d95429466a4fcca4ce9aef3fb3afc20d5004f32 via the session-details API (build locsec-WI-549cf2ac, local active, run against the round-1 code). Re-running e2e was the right instinct — the hoisted guard now inspects key itself, so a live tunnel is exactly what proves it doesn't reject a real access key.

Blocking (1): commit 1964cb1's message still names LOC-6790 and LOC-6777 as open and unfixed, with a characterisation of each, on a public repo for an npm-published package. The squash-merge note doesn't cover it: GitHub's default squash subject is the PR title (still LOC-6805: …), and squashing doesn't retract a commit already rendered on this PR and reachable by SHA. This is a Draft branch — amend and force-push. Details inline on test/local.js:127, including the carve-out for commit subjects, which do have precedent in this repo.

Also needing action off-PR: the posted locsec-fix-done Jira comment is still the round-0 version — "7 new tests" (now 12) and session 43078073…, which exercised superseded code. Its security claims are now accurate (round 1 made the "any value beginning with -" line true, where in round 0 it overstated), so this is stale evidence rather than a false claim, but the proof a human would click points at the wrong build. Edit in place and add the round-1 session id.

Nits inline on lib/Local.js:374 (null element in a list value throws TypeError out of start(), bypassing the callback(err) contract) and test/local.js:215 (no coverage for that case or for startSync's reject path; session id missing from the description). Keeping the PR in Draft — a human owns approval.

[07souravkunda]

Round-1 confirmation — this is fixed, verified rather than assumed. I re-ran every round-0 vector plus new ones against head 4ab6262, reading the real getBinaryArgs() output: 37/37.

All 18 explicitly-cased keys now reject a flag-like value — localIdentifier, only, folder, key, verbose, force, forceLocal, forceProxy, onlyAutomate, logFile, parallelRuns, useCaCertificate, binarypath, proxyHost/Port/User/Pass — including the --log-file=/--daemon=/--config-file= forms that previously rode in as values and defeated RESERVED_OPTIONS. List elements are checked too, including a nested array (via toString coercion). startSync returns the LocalError rather than swallowing it.

Equally important, the accept path didn't get over-tightened: localProxyHost + pac-file, custom-repeater + bs-host, the full proxy set, include-hosts as both an array and a space-separated string, bare boolean flags, and numeric values all still produce exactly the argv the binary expects, every element a string.

Hoisting above the switch was the right shape — one check, no per-case duplication, and it can't be bypassed by adding a case later.

[07souravkunda]

[nit] The scalar path above got the null/undefined guard I asked for in round 0, but this new array loop didn't — and rejectFlagLikeValue deliberately continues past null elements, so nothing stops one reaching here:

{'include-hosts': ['localhost', null]}
  -> TypeError: Cannot read properties of null (reading 'toString')   thrown out of start()
{'include-hosts': ['localhost', undefined]}
  -> TypeError: Cannot read properties of undefined (reading 'toString')

Thrown synchronously out of start(), so it bypasses the callback(err) contract the rest of this change is careful to honour. Not a regression — master accepted the same input and pushed the raw array, which then dies at execFile — but it's the identical defect class the scalar path just fixed, in the sibling branch added by the same commit.

if(Array.isArray(value)){
  for(var i = 0; i < value.length; i++){
    if(value[i] === undefined || value[i] === null)
      continue;
    this.userArgs.push(value[i].toString());
  }
}

Two cosmetic siblings while you're here: {'include-hosts': ['true']} stringifies to 'true' at the check above, so it pushes a bare --include-hosts and silently drops the element; {'include-hosts': []} pushes a dangling --include-hosts with nothing after it. Both benign — the binary's list parser reads an empty list — but a values.length === 0 early return would make the intent explicit.

[07souravkunda]

[blocking] The tree is clean now — git grep -E '\b(LOC|SC|APPSEC)-[0-9]+|C-00[0-9]|F-0[0-9][0-9]' over head 4ab6262 returns zero hits, and the body no longer carries the Jira links or the exploit narrative. That's the part that mattered most; thank you.

The squash-merge note doesn't finish the job, though, for two reasons:

1. GitHub's default squash-commit subject is the PR title, which is still LOC-6805: allowlist option keys forwarded to the BrowserStackLocal binary. So LOC-6805 lands in master's history unless the merger also edits the subject — and the note only asks them to squash. (Commit subjects carrying ticket ids do have precedent here, e.g. LOC-5083: Add support for Linux arm64 binary, so if you want to keep the title on that basis, say so and I'll drop this half.)
2. Squashing doesn't retract commit 1964cb1. Its message is already rendered publicly on this PR's Commits tab and stays reachable by SHA afterwards. Squashing only shapes what enters master.

The substantive part isn't the bare ticket key — it's that 1964cb1's message names LOC-6790 (binarypath path traversal) and LOC-6777 (no binary integrity check) as still open and unfixed, with a one-line characterisation of each, on a public repo, for a package that ships to npm. That's a pointer to two live unpatched issues, which is a different thing from an internal ticket number.

This branch is a Draft with no other contributors, so amending is free:

git rebase -i --root   # or: git commit --amend on 1964cb1 via a soft reset
git push --force-with-lease

Rewrite 1964cb1's message with the CWE-88 framing this comment block now uses and drop the closing paragraph about the other two tickets. If your team's disclosure policy is comfortable leaving it, that's a legitimate call to make explicitly — but it shouldn't rest on a merge-time step that GitHub won't do by default.

[07souravkunda]

[nit] Good addition — this test is what makes the new array handling a contract rather than an accident, and pairing it with the flag-like-element case above is the right split.

Two gaps worth closing while the file is open, both of which I had to find by hand rather than from the suite:

• Nothing covers a null element inside a list value (see my comment on lib/Local.js:374 — it throws out of start()).
• Nothing covers startSync on the reject path. It's the other function whose signature changed, and the round-0 report notes the Start sync block only runs in isolation because of the pre-existing should stop local abort. A one-liner asserting bsLocal.startSync({key: ..., onlyCommand: true, localIdentifier: '--daemon=stop'}) returns a LocalError would pin the return-value contract. I verified it does today.

Separately, on the PR description rather than this file: the test plan dropped the BrowserStack session id that round 0's body carried. The round-1 re-run is real — I verified session 1d95429466a4fcca4ce9aef3fb3afc20d5004f32 via the session-details API (build locsec-WI-549cf2ac, local active, created after the round-1 code) — but a reader of this PR now has no link to it. Worth putting back, since it's the only evidence that the hoisted guard doesn't falsely reject a real access key on a live tunnel.