fix(browser): surface bad-token handshake rejections instead of flooding#55
fix(browser): surface bad-token handshake rejections instead of flooding#55stephane-segning wants to merge 1 commit into
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. |
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>
7f4547c to
8484a64
Compare
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