Greptile Summary

This PR successfully fixes a self-DoS bug where opening the web UI without credentials would exhaust the browser rate-limit and lock out all connections from that IP. The fix is well-scoped and applied in two complementary layers: the server stops counting absent credentials as rate-limit failures, and the browser client stops auto-reconnecting when it receives an auth error that cannot resolve without user action.

Key strengths:

• The recordFailure() removal for token_missing/password_missing is safe — the rate-limit check still runs first, so an already-exhausted IP is still rejected before credentials are even inspected.
• The AUTH_TOKEN_MISMATCH exclusion from the non-recoverable list is correctly intentional: the stale device-token → clear → retry fallback loop requires at least one reconnect, and the rate limiter bounds the total number of retries before AUTH_RATE_LIMITED stops the loop.
• pendingConnectError capture/reset ordering in the close handler is correct: the error is snapshotted into connectError before the field is cleared, so the isNonRecoverableAuthError gate always sees the intended value.
• Test coverage is thorough for the happy and sad paths of both changes.

Confidence Score: 5/5

• Safe to merge — minimal, targeted changes with no logic errors found.
• No bugs or security regressions identified. Both changes (server-side recordFailure removal and client-side reconnect gating) are logically sound, the interaction between them is correctly designed, and the test suite covers the critical decision boundaries.
• No files require special attention.

_{Last reviewed commit: 0e7ce3a}

🔒 Aisle Security Analysis

We found 1 potential security issue(s) in this PR:

1. 🟡 Rate-limit bypass for missing gateway credentials enables unthrottled auth attempts (DoS)

Description

authorizeGatewayConnect() calls rateLimiter.check() but the limiter only tracks/blocks clients when recordFailure() is invoked. This change removed recordFailure() for missing token/password cases, so an attacker can repeatedly send unauthenticated connect attempts with absent credentials and never consume rate-limit capacity.

Why this is a practical bypass:

• authorizeGatewayConnect() performs limiter.check(ip, scope) before validating credentials.
• createAuthRateLimiter().check() is non-consuming; it only inspects stored failures.
• Without recordFailure() on token_missing / password_missing, the limiter state never changes, so check() keeps returning allowed: true.
• Result: unlimited connection/auth attempts that always fail with token_missing/password_missing but are never throttled, enabling request floods/log spam/CPU burn (DoS).

Vulnerable code (missing-credential early return without recording a failure):

if (!connectAuth?.token) {
  ​// ...
  return { ok: false, reason: "token_missing" };
}

A similar branch exists for password_missing.

Relevant limiter behavior:

• check() does not increment attempts.
• Only recordFailure() appends a timestamp and enforces lockout once maxAttempts is reached.

Recommendation

Treat missing credentials as rate-limit relevant events or introduce a separate limiter for "missing credential" floods.

Option A (simplest): record failures for missing credentials too:

if (!connectAuth?.token) {
  limiter?.recordFailure(ip, rateLimitScope);
  return { ok: false, reason: "token_missing" };
}

if (!password) {
  limiter?.recordFailure(ip, rateLimitScope);
  return { ok: false, reason: "password_missing" };
}

Option B (better UX): use a separate scope and/or higher threshold for missing credentials (to avoid locking out legitimate misconfigured clients while still preventing floods):

const missingScope = `${rateLimitScope}:missing`;
if (!connectAuth?.token) {
  limiter?.recordFailure(ip, missingScope);
  return { ok: false, reason: "token_missing" };
}

Also consider adding a lightweight per-IP connection attempt limiter at the WebSocket/HTTP entrypoint to protect CPU/logs regardless of auth result.

Analyzed PR: #38725 at commit b15e4ca

_{Last updated on: 2026-03-07T22:48:18Z}

@Takhoffman - this could use a review when you've got a moment.

Context: Fixes a browser rate-limit self-DoS where missing auth credentials trigger recordFailure() on every auto-reconnect, locking the user out for 5 minutes. S-sized PR, 4 files, all CI green.

The "Changes at a glance" table in the description has the exact lines to review.

Rebased onto latest main (70 commits forward). No content changes - same single commit, just synced with upstream.

The check CI failure is a pre-existing upstream issue (chatStreamSegments duplicate identifier in app-view-state.ts and type errors in client-fetch.loopback-auth.test.ts). Not related to this PR.

Valid concern, but the trade-off is intentional.

Missing credentials carry zero brute-force risk - there's nothing to guess. The real-world problem this PR fixes is more severe: a single misconfigured browser triggers recordFailure() on every auto-reconnect (~10 in 40 seconds), locking out ALL browser connections from that IP for 5 minutes, including properly-configured ones. That's a self-DoS from normal usage.

The existing isRateLimited() check still runs first (auth.ts:430-435), so any IP already locked out from credential mismatches stays blocked. We only remove the failure count for absent credentials, not wrong ones.

Your separate-scope suggestion is reasonable for defence in depth - worth exploring as a follow-up. The connection-level rate limiting in server.impl.ts (the browserRateLimiter with hardcoded exemptLoopback: false) still applies to all browser connections regardless of auth outcome, so there's still a connection-attempt cap independent of the auth rate limiter.

Landed on main in 3a74dc0.

Source commit from this PR: 533ff3e

What I kept:

• stop counting token_missing / password_missing as auth rate-limit failures
• stop auto-reconnecting Control UI clients on non-recoverable auth errors
• regression coverage in src/gateway/auth.test.ts and new src/gateway/reconnect-gating.test.ts
• changelog entry with contributor + PR reference

Gate run before landing:

• pnpm lint
• pnpm build
• pnpm test

Thanks @ademczuk.

Landed on main after maintainer squash/rebase.

• Landed commit: 3a74dc0
• Source commit: 533ff3e
• Kept the gateway auth and reconnect-gating fix, preserved tests, and added changelog credit for contributor PR fix(gateway): prevent browser rate-limit self-DoS on missing credentials #38725.

Thanks @ademczuk.

Thanks for landing this one. Glad the rate-limit exemption and reconnect gating made the cut.