Skip to content

fix(browser): surface bad-token handshake rejections instead of flooding#55

Open
stephane-segning wants to merge 1 commit into
mainfrom
claude/browser-bad-token-hardening
Open

fix(browser): surface bad-token handshake rejections instead of flooding#55
stephane-segning wants to merge 1 commit into
mainfrom
claude/browser-bad-token-hardening

Conversation

@stephane-segning

@stephane-segning stephane-segning commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

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_rejected now logs non-secret token fingerprintsexpected vs got (plus role/client). A same-length, different-value pair points at a stale/rotated host, not a paste typo.
  • Sends a rejected frame (with reason) just before closing a refused handshake.

Protocol — new rejected frame + tokenFingerprint() helper, mirrored into the extension.

Extension (browser-extension)

  • Handles 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.
  • Not dormant. Real-world incident showed the rejection is often the host's fault (a long-lived IDE/desktop OpenCode squatting the port with a pre-rotation token), fixed host-side without touching the extension. So it keeps retrying slowly and auto-recovers the moment a good host returns — no manual reconnect. A successful ready clears 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

  • Broker: rejected frame sent before close; fingerprints logged and never contain the raw token.
  • Protocol: rejectedFrame round-trip + tokenFingerprint stability/non-leak.
  • Extension: rejection → neutral error + no flood; auto-recovers at the slow interval; ready restores fast backoff; manual reconnect() re-dials at once.
  • docs/browser.md troubleshooting row (stale-host case + lsof tip + explicit-token suggestion) and CHANGELOG.md.

Rebased onto main (post-#56); full pre-push gate green (449 tests, every coverage threshold met).

🤖 Generated with Claude Code

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +335 to +344
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}`;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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;
}

Comment on lines +285 to +292
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}`;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

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.

Suggested change
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;
}

@chatgpt-codex-connector

Copy link
Copy Markdown

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>
@stephane-segning stephane-segning force-pushed the claude/browser-bad-token-hardening branch from 7f4547c to 8484a64 Compare June 15, 2026 17:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant