fix(cli): verify downloaded CLI binary integrity before exec (DEVA11Y-473/474)

[Crash0v3rrid3]

What & why

DEVA11Y-473 (F-001, Swift plugin) and DEVA11Y-474 (F-002, launcher scripts) flagged that the BrowserStack CLI binary was downloaded and executed with no integrity check. The plaintext-HTTP half of both findings was already fixed (HTTPS is now enforced); this PR addresses the remaining "without integrity check" half.

Both download paths now verify the archive against a server-published SHA-256 sidecar (<versioned-asset>.sha256) before it is extracted, chmod 0755'd, and run.

Changes

• Shell (scripts/{bash,zsh,fish}/cli.sh) — download_binary captures the resolved, versioned asset URL (curl -w '%{url_effective}'); a new verify_binary_integrity fetches <asset>.sha256 and compares (reusing the existing _self_update_sha256 helper). The invocation now aborts (download_binary || exit $?) on failure instead of falling through to exec.
• Swift plugin (BrowserStackAccessibilityLint.swift) — both platforms now download to a temp file, verify, then extract. This removes the streaming curl | bsdtar path (extractRemoteArchive) that left no opportunity to check the payload; download(from:to:) is promoted to cross-platform. SHA-256 is computed via shasum/sha256sum to avoid pulling in CryptoKit/swift-crypto (Apple-only; this path also serves Linux).
• Regenerated cli.sh.sha256 for bash/zsh/fish so the self-update checksum gate (verify-selfupdate-checksums.yml) stays green.

Behaviour (mirrors the existing self-update verification)

• Fail CLOSED on a checksum mismatch → refuse to use the binary (shell exit 2; Swift throws).
• Fail OPEN (warn + proceed) when no sidecar is published yet, so this is non-breaking today.
• Download-integrity check, not an authenticity signature (the same caveat the self-update code documents).

⚠️ Requires a server-side dependency to fully close these tickets

There is currently no .sha256 published for the CLI binary — api.browserstack.com/sdk/v1/download_cli 302-redirects to a versioned immutable asset (sdk-assets.browserstack.com/binary-<os>-<arch>-<ver>.zip), and every checksum/signature path probed returns 403/404. Until the SDK-assets/API team publishes <asset>.sha256 next to each binary, this verification is inert (fail-open).

Publishing the checksum from the API/control plane (distinct infra from the CDN/S3 data plane that serves the binary) is what gives the check real anti-tamper value. This PR is the client half; the tickets can't be marked Done until the server half lands — hence draft.

Testing

• bash -n clean on all three scripts.
• verify_binary_integrity exercised for all four cases: empty URL → skip; missing sidecar (404) → fail-open, keep zip; wrong checksum → fail-closed, zip removed, exit 2; correct checksum → pass.
• swiftc -parse clean; brace/paren balance verified. Full swift build/swift test (the Tests/spm consumer smoke test) not run locally — needs the plugin build env + network.
• All 6 scripts/**/*.sha256 sidecars pass sha256sum -c.

🤖 Generated with Claude Code

---

Review round 2 (commit 3e0ff69)

Addresses @nmnishant-browserstack's review:

• Windows now verifies. verifyArchiveChecksum / sha256Hex moved out of the #if !os(Windows) block and the call site is unconditional; sha256Hex gained a Windows branch (powershell -NoProfile -Command "(Get-FileHash -Algorithm SHA256 -LiteralPath '…').Hash"). Only extractLocalArchive stays Unix-only (Windows already uses unzip).
• Atomic extraction. All three cli.sh now extract to ${BINARY_PATH}.tmp, chmod, then mv -f into place, so a corrupt payload can't truncate a working cached binary.
• -z intent is satisfied. The -z (If-Modified-Since) flag stays, but verify_binary_integrity runs against $BINARY_ZIP_PATH on every invocation, including the 304 cache-hit path — so a poisoned cache is re-checked once sidecars exist. (No behavioural change needed; noting it so an auditor can see it against the diff.)
• spm.sh is not a gap. spm.sh does not download the binary — it delegates to swift package plugin … scan, which routes through the Swift plugin this PR fixes. The full download surface is 3× cli.sh + the plugin; the scanner over-counted "six scripts".
• Malformed-hash fail-open + CI. Non-64-hex sidecar bodies (e.g. an S3 AccessDenied XML answered 200) now fail open in both shell and Swift (empty bodies still fail closed). Added a functional verify_binary_integrity matrix job and an advisory sidecar-availability probe to spm-smoke-test.yml.
• README documents the new binary-integrity gate, phrased so it reads as "verified when a checksum is published", not a guarantee that holds today.

[nmnishant-browserstack]

Review — binary integrity verification (DEVA11Y-473/474)

Read the diff against both tickets, the surrounding launcher/plugin logic, and the two CI gates. I also ran the new code rather than just reading it: lifted verify_binary_integrity verbatim off this branch and exercised it against a local fixture server, and probed the real download_cli → sdk-assets path.

The shape of the change is right. Verification lands before extract/chmod 0755/exec on both paths, semantics mirror the existing self-update gate, and removing the streaming curl | bsdtar path is the correct call — that pipe structurally could not be verified. All eight cases the description claims behave exactly as claimed:

case	rc	zip kept
empty resolved URL	0 (skip)	yes
good sidecar	0	yes
good + ?token= query	0	yes
missing sidecar (404)	0 (fail open)	yes
uppercase published hash	0	yes
wrong checksum	2	removed
empty sidecar (200)	2	removed

The RETURN trap leaves zero bs-a11y-clisum.* temp files behind, all six scripts/**/*.sha256 sidecars pass sha256sum -c, swiftc -parse is clean, and verify_binary_integrity/download_binary are byte-identical across bash/zsh/fish (I hashed the function bodies).

Two things worth stating for whoever closes the tickets:

• Coverage is complete despite the ticket text. DEVA11Y-474 says "six shell installer scripts (bash/zsh/fish variants of cli.sh and spm.sh)". spm.sh doesn't download the binary — it delegates to swift package plugin … scan, which routes through the Swift plugin this PR also fixes. 3× cli.sh + the plugin is the right surface; the scanner over-counted.
• An unintended bonus fix. On main, a failed download still ran bsdtar … -O > "$BINARY_PATH", truncating a previously-good cached binary to 0 bytes. download_binary || exit $? now aborts first. Verified: main → cached binary 0 bytes, this branch → intact.

---

1. Windows gets no verification at all — and the description says it does · must fix

verifyArchiveChecksum (346) and sha256Hex (390) sit inside the #if !os(Windows) block that opens at line 337 and closes at 452, and the call site at 256–260 is correspondingly guarded:

try await download(from: info.resolvedURL, to: archiveURL)
#if !os(Windows)
try await verifyArchiveChecksum(archiveURL: archiveURL, resolvedURL: info.resolvedURL)
#endif

So on Windows the archive is downloaded, extracted, made executable and run unverified — precisely the F-001 condition. The description says "both platforms now download to a temp file, verify, then extract"; for Windows the only actual change is the comment (it already downloaded to a temp file).

Windows already shells out to PowerShell for unzip, so the fix is cheap — move both functions out of the !os(Windows) block and add a Windows branch to sha256Hex:

powershell -NoProfile -Command "(Get-FileHash -Algorithm SHA256 -LiteralPath '<path>').Hash"

If Windows isn't a supported target for this plugin, that's a fine answer too — but then say so and drop the branch. Either way the description shouldn't claim coverage that isn't there.

2. Extraction still truncates a working cached binary · cheap fix

scripts/*/cli.sh:236 is unchanged:

bsdtar -xvf "$BINARY_ZIP_PATH" -O > "$BINARY_PATH" && chmod 0755 "$BINARY_PATH" && strip_quarantine

> truncates $BINARY_PATH before bsdtar is known to have succeeded. You fixed the network-failure route into this; the corrupt-payload route is still open, and it's the live one today — no sidecars are published, so verification fails open and hands a corrupt zip straight to bsdtar. Reproduced: download succeeds, payload corrupt, no sidecar → previously-good cached binary ends at 0 bytes.

bsdtar -xvf "$BINARY_ZIP_PATH" -O > "${BINARY_PATH}.tmp" \
  && chmod 0755 "${BINARY_PATH}.tmp" \
  && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH" \
  && strip_quarantine

3. Nothing will tell you when this stops being inert — or when it regresses

I probed the real path: api.browserstack.com/sdk/v1/download_cli?os=macos&os_arch=arm64 → sdk-assets.browserstack.com/binary-macos-arm64-1.52.0.zip, and that bucket returns 403 + S3 AccessDenied XML for both missing and non-public objects — a missing key and a published-but-wrong-ACL key are indistinguishable from the client.

That's the failure mode I'd worry about most: SDK-assets publishes the .sha256 objects, forgets public-read, every client keeps failing open forever, and the only evidence is a >&2 line buried in Xcode build-phase output or a Diagnostics.remark. The tickets get closed, and nothing is actually verified.

Worth adding a positive assertion in spm-smoke-test.yml: resolve the current asset URL, assert the sidecar returns 200 with a 64-hex body. ::warning today, hard failure once the server half lands. That's what turns "we shipped the client half" into "verification demonstrably runs".

4. No test for a security control that currently cannot fail

scripts-lint is bash -n only. Because verification is fail-open today, a change that breaks verify_binary_integrity outright produces zero red signal — the regression surfaces the day the sidecars go live, which is the worst possible day. The manual matrix in the description is the right set of cases; it took ~30 lines of bash and a python3 -m http.server fixture to automate the whole thing. Worth committing as a CI job.

5. No shape validation on the published hash · minor, latent

Any 2xx body's first whitespace-delimited token becomes the expected digest. Latent today given the 403 above — but if that bucket ever sits behind a CDN or S3 website endpoint that answers 200 with an XML/HTML error page, the first token of that page becomes the "expected hash" and every client hard-fails (exit 2, cached zip deleted) on every invocation. I reproduced exactly that with a 200 + AccessDenied XML body. One line restores the intended fail-open:

if ! [[ "$expected" =~ ^[0-9a-f]{64}$ ]]; then
  echo "CLI download: malformed checksum at ${sum_url}; proceeding WITHOUT verification." >&2
  return 0
fi

plus the equivalent guard in Swift before caseInsensitiveCompare.

6. -z reads as an unaddressed remediation item · description only

DEVA11Y-474's remediation says: "Remove the -z (If-Modified-Since) conditional flag — it silently skips verification when the cached file is current, creating a window where a previously poisoned cache entry is never re-checked." -z is still there.

As written the PR satisfies that intent — verify_binary_integrity runs against $BINARY_ZIP_PATH on every invocation, including the 304 cache-hit path, so a poisoned cache does get re-checked once sidecars exist. But nobody auditing the ticket against this diff can see that. One line in the description saves the next retest; both tickets already carry ai-retest-status:not-fixed.

---

On the server half

Agree this can't close the tickets alone, and agree with your instinct in the description: a sidecar fetched from the same CDN the artifact came from detects corruption but buys little against an attacker who can write to that CDN — they can replace both. Having api.browserstack.com hand back the digest (a header on the redirect, or a small JSON endpoint) is the version with real anti-tamper value, since it's separate infrastructure. The ticket's own remediation text only asks for "same HTTPS origin", so what's here meets the bar as written — but the control-plane version is worth arguing for while the server work is still unscoped.

Is there a ticket for it? The description references the dependency but links nothing. That's the single highest-leverage follow-up here, and without it this code stays dormant indefinitely.

Also: DEVA11Y-473 and -474 both currently show resolution: Done while status is Dev in Progress — worth clearing, given the description explicitly says they can't be Done until the server half lands.

Docs

README.md:214 documents the self-update checksum gate but not this new one. Worth a sentence — phrased so it's clear the binary is verified when a checksum is published, so nobody reads it as a guarantee that holds today.

---

Verdict: approve once (1) is resolved — Windows is a genuine hole in the fix, not a nit — and (2), which is three lines. (3) and (4) are what decide whether this ends up delivering security value or just the appearance of it; I'd push to land them before the tickets close rather than after. Nice work on the ?token= stripping and the case-insensitive compare — both were real, and both were easy to miss.

_{🤖 Reviewed with Claude Code}

[Crash0v3rrid3]

Thanks for the thorough pass @nmnishant-browserstack — all actionable items addressed in 3e0ff69.

1. Windows verification hole (must fix) — done. verifyArchiveChecksum and sha256Hex moved out of the #if !os(Windows) block; the call site is now unconditional. sha256Hex has a Windows branch using powershell -NoProfile -Command "(Get-FileHash -Algorithm SHA256 -LiteralPath '…').Hash". Only extractLocalArchive stays Unix-only (Windows path already uses unzip). swiftc -parse clean, #if/#endif balanced.

2. Extraction truncates cached binary (cheap fix) — done. All three cli.sh now extract to ${BINARY_PATH}.tmp, chmod 0755, then mv -f into place before strip_quarantine. A corrupt payload can no longer zero out a previously-good cached binary.

3. Sidecar-availability signal — added. New advisory sidecar-availability job in spm-smoke-test.yml: resolves the real (unauthenticated) download_cli redirect, strips the query, and asserts <asset>.sha256 returns 200 with a 64-hex body. ::warning today, with a marked TODO to flip to exit 1 once the server half ships — so a published-but-wrong-ACL sidecar (your 403-indistinguishability concern) surfaces instead of silently failing open forever.

4. Test for the control — added. New verify-integrity-fn job runs the full matrix against a python3 -m http.server fixture: empty-URL skip (rc0), good sidecar (rc0), good + ?token= (rc0), missing/404 (fail-open rc0), uppercase hash (rc0), wrong checksum (rc2 + zip removed), empty sidecar 200 (rc2 + removed), and malformed-200 (fail-open). All 8 pass locally.

5. Shape validation on the published hash — done. Non-empty bodies that aren't ^[0-9a-f]{64}$ (e.g. an S3 AccessDenied XML answered 200) now fail open with a stderr note, in all three shells and in Swift (guard before caseInsensitiveCompare). Empty bodies still fail closed, per the matrix.

6. -z reads as unaddressed — clarified, no code change. verify_binary_integrity runs against $BINARY_ZIP_PATH on every invocation including the 304 cache-hit path, so a poisoned cache is re-checked once sidecars exist — the remediation intent is met. Noted in the PR description so it's visible against the diff.

Docs — done. README.md (by the self-update gate) now documents the binary-integrity check, phrased so it's clear the binary is verified when a checksum is published, not a guarantee that holds today.

spm.sh over-count — confirmed, agree. spm.sh delegates to the Swift plugin rather than downloading the binary, so 3× cli.sh + the plugin is the complete surface.

Jira resolution field — flagged for cleanup. Good catch that 473/474 show resolution: Done while status is Dev in Progress; that's a Jira-side inconsistency (not touched by this PR). Leaving the tickets open and flagging the resolution field to be cleared to Unresolved — they can't be Done until the server half lands.

Server half — agreed, and it's the blocker. Fully agree the control-plane (api.browserstack.com) version has the real anti-tamper value over a same-origin CDN sidecar. Tracking the dependency on SDK-assets publishing <asset>.sha256; this PR is the client half and stays open until that lands.

[nmnishant-browserstack]

Re-review — 3e0ff69 + 66249fe

Re-reviewed clean against main, then checked each claim in your reply by re-running the original repros rather than reading the diff. Everything you listed is genuinely done. Details worth recording:

1. Windows hole — closed. verifyArchiveChecksum (344) and sha256Hex (397) are now outside every conditional; the call site (259) is unconditional; extractLocalArchive correctly stays #if !os(Windows). #if/#endif balance 11/11, swiftc -parse clean.

2. Cached-binary truncation — fixed. Re-ran the exact repro (download succeeds, payload corrupt, no sidecar → fail open). Before: cached binary → 0 bytes. Now: 24 bytes before and after, download_binary returns 1, scan skipped. Confirmed.

5. Shape validation — correct in both directions. Empty body still fails closed (rc2 + zip removed); malformed 200 fails open (rc0 + zip kept). That's the right split.

4. The new test is a real test — I mutation-tested it. This is the part I most wanted to check, since a matrix that passes unconditionally is worse than no matrix. Lifting the function off the branch with awk rather than re-implementing it is the right call. Results:

A) real branch code   → PASS=8 FAIL=0, exit 0
B) control gutted     → PASS=6 FAIL=2, exit 1
   FAIL | wrong checksum     | got rc=0 zip=kept want rc=2 zip=gone
   FAIL | empty sidecar (200)| got rc=0 zip=kept want rc=2 zip=gone

It goes red when the control is removed. And your && rc=0 || rc=$? note in 66249fe is right — under GHA's injected bash -e, ; rc=$? would have aborted the step at the first fail-closed case and the [ "$fail" -eq 0 ] gate would never have run. Good catch on your own patch.

Also re-verified: all three sidecars regenerated (sha256sum -c OK), the three shell copies still byte-identical, bash -n clean on all three.

---

Everything below is minor. None of it blocks.

A. sidecar-availability reports HTTP 403000 · low

Confirmed live against the real endpoint:

code=$(curl -fsSL -o body.txt -w '%{http_code}' "$sum_url" || echo 000)
→ code='403000'

curl writes 403 via -w even under --fail, then || echo 000 appends. The success path is unaffected (code is exactly 200, || never fires), so the flip-to-exit 1 logic is sound — but the warning string is the thing a human reads to decide whether the server half has shipped, and it'll say 403000. Simplest fix is to drop -f, since you check code explicitly anyway: curl then exits 0, code=403, and body.txt actually gets created (right now it doesn't, which is why the awk || true guard is needed at all).

B. The matrix only covers scripts/bash/cli.sh · low

SRC=scripts/bash/cli.sh. The zsh and fish copies are byte-identical today — I hashed the function bodies — but nothing enforces that. verify-selfupdate-checksums checks each file against its own sidecar, not against each other, so a hand-copy drift in the zsh verify_binary_integrity ships with a green board. Either loop the matrix over all three SRCs, or assert the three function bodies hash equal.

C. Windows verification is implemented but never compiled · low

No workflow runs on Windows (macos-14 + ubuntu-latest only), and Package.swift declares no platforms. That branch is now load-bearing: once sidecars publish, a sha256Hex failure throws → verifyArchiveChecksum throws → hard build failure. Two things I'd want exercised on a real Windows runner before the server half lands:

• URL(fileURLWithPath: "powershell.exe") doesn't do PATH resolution — it builds a file URL relative to cwd. The existing unzip uses the same pattern ("powershell"), so this is consistent — but consistent with code that has also never run isn't much reassurance.
• Get-FileHash(...).Hash returns uppercase; the doc comment on sha256Hex says "lowercase hex string". Harmless because of caseInsensitiveCompare, but the comment is now wrong on one of the three platforms it describes.

Worth an explicit call either way: add a Windows build job, or say Windows is best-effort.

D. ${BINARY_PATH}.tmp — fixed name, not cleaned up · nit

Verified a 0-byte .tmp is left behind when bsdtar fails. It's also a fixed path, and cli.sh has no locking at all (spm.sh has one; cli.sh has none), so two concurrent scans race on the same temp file. Strictly better than the old > "$BINARY_PATH" — the mv makes the final publish atomic — so this is residual, not a regression. mktemp "${BINARY_PATH}.XXXXXX" plus a trap closes both.

E. Still no ticket for the server half · process

The flip-to-red is guarded by TODO(DEVA11Y-473/474 server half) in a YAML comment. Your reply says "tracking the dependency" but there's no ID here or in the description. A TODO in a workflow file isn't a tracked dependency, and this one is the only thing standing between "we shipped a security control" and "we shipped a security control that runs" — the sidecar-availability job will sit warning-yellow indefinitely and nobody will be paged about it. Please put the ticket ID in that TODO and in the description before merge; it's a 30-second action and it's the highest-leverage item left on this PR.

---

Verdict

Approve. Both must-fix items from the last round are genuinely fixed — verified by re-running the repros, not by reading the reply — and the CI gate is a real gate, which I confirmed by mutation-testing it. The control now does what it says on macOS, Linux and (modulo C) Windows, fails open in exactly the cases it should, fails closed in exactly the cases it should, and there's a test that goes red if that stops being true.

A–D are non-blocking; fold them into a follow-up push or a separate PR, your call. E I'd want before merge — not because the code is wrong, but because without that ticket this lands as dormant code with no owner for waking it up.

Nice work on the turnaround, and on catching the bash -e interaction yourself.

_{Note: this is a review comment, not a formal GitHub approval — say the word and I'll submit it as one to clear REVIEW_REQUIRED.}

_{🤖 Reviewed with Claude Code}