fix(browser): surface bad-token handshake rejections instead of flooding - #55
Conversation
There was a problem hiding this comment.
Code Review
This pull request improves the handling of invalid or stale browser extension tokens. Instead of continuously retrying and flooding the bridge, the broker now sends a rejected frame before closing the connection, which allows the extension to enter a dormant state and display an actionable error message. Additionally, non-secret token fingerprints are logged to aid in diagnostics without exposing raw secrets. The review feedback suggests adding runtime type checks to the tokenFingerprint function in both protocol implementations to prevent potential TypeError crashes if a non-string value is processed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| export function tokenFingerprint(token: string): string { | ||
| // djb2 — tiny, dependency-free, stable. Collisions are irrelevant: this is a | ||
| // visual match aid, not a security primitive. | ||
| let hash = 5381; | ||
| for (let i = 0; i < token.length; i++) { | ||
| hash = (hash * 33) ^ token.charCodeAt(i); | ||
| } | ||
| const digest = (hash >>> 0).toString(36); | ||
| return token.length === 0 ? "empty" : `len${token.length}.${digest}`; | ||
| } |
There was a problem hiding this comment.
To ensure robust defensive programming, we should guard against non-string or nullish values passed to tokenFingerprint. If this.opts.token or frame.token is somehow not a string at runtime, calling .length or .charCodeAt on it will throw a TypeError and potentially crash the connection handler or broker process. Adding a type check prevents this.
export function tokenFingerprint(token: string): string {
if (typeof token !== "string") {
return "invalid";
}
// djb2 — tiny, dependency-free, stable. Collisions are irrelevant: this is a
// visual match aid, not a security primitive.
let hash = 5381;
for (let i = 0; i < token.length; i++) {
hash = (hash * 33) ^ token.charCodeAt(i);
}
const digest = (hash >>> 0).toString(36);
return token.length === 0 ? "empty" : "len" + token.length + "." + digest;
}| export function tokenFingerprint(token: string): string { | ||
| let hash = 5381; | ||
| for (let i = 0; i < token.length; i++) { | ||
| hash = (hash * 33) ^ token.charCodeAt(i); | ||
| } | ||
| const digest = (hash >>> 0).toString(36); | ||
| return token.length === 0 ? "empty" : `len${token.length}.${digest}`; | ||
| } |
There was a problem hiding this comment.
To ensure robust defensive programming and keep the mirrored protocol files perfectly in sync, we should guard against non-string or nullish values passed to tokenFingerprint. If the token is somehow not a string at runtime, calling .length or .charCodeAt on it will throw a TypeError and crash the background script/service worker. Adding a type check prevents this.
| export function tokenFingerprint(token: string): string { | |
| let hash = 5381; | |
| for (let i = 0; i < token.length; i++) { | |
| hash = (hash * 33) ^ token.charCodeAt(i); | |
| } | |
| const digest = (hash >>> 0).toString(36); | |
| return token.length === 0 ? "empty" : `len${token.length}.${digest}`; | |
| } | |
| export function tokenFingerprint(token: string): string { | |
| if (typeof token !== "string") { | |
| return "invalid"; | |
| } | |
| let hash = 5381; | |
| for (let i = 0; i < token.length; i++) { | |
| hash = (hash * 33) ^ token.charCodeAt(i); | |
| } | |
| const digest = (hash >>> 0).toString(36); | |
| return token.length === 0 ? "empty" : "len" + token.length + "." + digest; | |
| } |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
7f4547c to
8484a64
Compare
8484a64 to
9cbbb74
Compare
|
Addressed the Gemini review: added a Also note this PR was revised since the bot reviewed it — the extension no longer goes dormant; it slow-retries ( |
A rejected bridge handshake (`browser_handshake_rejected reason=bad_token`) used to flood ~once a second forever: the broker closed the socket with no reason, so the extension couldn't tell a token rejection from a network drop — it showed "connecting" and kept hammering. Broker: - logs non-secret token fingerprints (`expected` vs `got`, plus role/client) on rejection, so a mismatch is diagnosable from the log alone without exposing the secret. A same-length, different-value pair points at a stale/rotated host, not a paste typo; - sends a `rejected` frame (reason) just before closing a refused handshake. Extension: on `rejected` it shows a clear, neutral error (the token may be stale/rotated, or another host is running) and switches to a slow retry (REJECTED_RETRY_MS) rather than the every-second flood. Crucially it does NOT go dormant — the cause is often host-side (a stale/rotated token, or an old host still squatting the port) and is fixed without touching the extension, so it keeps retrying slowly and **auto-recovers** the moment a good host returns. A successful `ready` clears the flag and restores fast backoff; a manual reconnect (dashboard save) re-dials immediately. Adds a `rejected` frame + `tokenFingerprint` to the wire protocol (mirrored into the extension), broker/protocol/bridge-client tests, and docs (browser.md troubleshooting row). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9cbbb74 to
4a10042
Compare
Bump all nine workspace packages to 0.8.1 (one version line) and date the CHANGELOG [Unreleased] section as [0.8.1] — 2026-06-17. No code changes; workspace:* deps mean no lockfile change. This release line covers: the experimental code-index plugin, the `trace` log tier (#56), and the oauth2/browser concurrency + token fixes (#54, #55, #57). Changelog intro count updated eight → nine for the new package. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (ADR-0005–0008) (#62) Capture the load-bearing, non-obvious decisions behind the #54–#57 work as Architecture Decision Records, matching ADR-0001's MADR-ish shape (Context / Decision / Consequences / Alternatives-with-why): - ADR-0005 — atomic on-disk state (per-writer pid+uuid temp + rename), unifying the oauth2/models-info caches (#54) and bridge.json (#57). - ADR-0006 — bridge.json as the token source of truth: host-only write, reload-on-mismatch, explicit-token authority (#57). - ADR-0007 — handshake rejection: explicit `rejected` frame, slow-retry (not dormant — records the reversal + the incident behind it), fingerprint logging (#55). - ADR-0008 — a `trace` log tier gated by OpenCode's DEBUG (#56). Renumbered to 0005–0008 (0002–0004 are the existing code-index ADRs), wired the inter-ADR cross-references and the index table, and cross-linked them from browser.md / architecture.md / troubleshooting.md and the CLAUDE.md/AGENTS.md ADR pointer. CHANGELOG Documentation entry added. Docs-only. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
What
A rejected bridge handshake (
browser_handshake_rejected reason=bad_token) used to flood the bridge ~once a second forever — the symptom seen in a real desktop-app log.Root cause
On a token mismatch the broker just
conn.close()d — no reason on the wire. The extension couldn't tell a rejection from an ordinary network drop, so it sat at "connecting" and kept reconnecting a token that can never succeed (or, as we later found, a token that is correct while a stale host holds an old one).Changes
Broker (
opencode-browser)browser_handshake_rejectednow logs non-secret token fingerprints —expectedvsgot(plusrole/client). A same-length, different-value pair points at a stale/rotated host, not a paste typo.rejectedframe (withreason) just before closing a refused handshake.Protocol — new
rejectedframe +tokenFingerprint()helper, mirrored into the extension.Extension (
browser-extension)rejected: shows a clear, neutral error (the token may be stale/rotated, or another host is running — re-paste or restart the host) and switches to a slow retry (REJECTED_RETRY_MS = 60s) instead of the every-second flood.readyclears the flag and restores fast backoff; a dashboard save (reconnect()) re-dials immediately.Why this design (updated)
The first cut went dormant on rejection. But debugging a live flood proved the fault is frequently host-side and self-resolves (e.g. after restarting the IDE that owned the port) — a dormant extension would not have reconnected without a manual click. Slow-retry kills the flood (60× fewer attempts) and preserves auto-recovery.
Tests / docs
rejectedframe sent before close; fingerprints logged and never contain the raw token.rejectedFrameround-trip +tokenFingerprintstability/non-leak.readyrestores fast backoff; manualreconnect()re-dials at once.docs/browser.mdtroubleshooting row (stale-host case +lsoftip + explicit-token suggestion) andCHANGELOG.md.Rebased onto
main(post-#56); full pre-push gate green (449 tests, every coverage threshold met).🤖 Generated with Claude Code