Skip to content

fix: harden proxy security, account selection, and scheduler lifecycle#81

Merged
MagicalTux merged 2 commits into
masterfrom
fix/review-hardening
Jul 6, 2026
Merged

fix: harden proxy security, account selection, and scheduler lifecycle#81
MagicalTux merged 2 commits into
masterfrom
fix/review-hardening

Conversation

@MagicalTux

Copy link
Copy Markdown
Member

Full-codebase review (fanned out across subsystems; every finding verified against the code before fixing). Grouped by area.

🔒 Security — MITM/CONNECT path

  • Unauthenticated CONNECT → account-token theft / open relay (HIGH). The proxy-apiKey gate lived only on the HTTP handler. A remote client could CONNECT api.anthropic.com:443 and get one of your account tokens injected (curl -k --proxy victim:3456 …), or blind-tunnel to any host (open relay / SSRF to cloud metadata). Fix: gate the CONNECT handler with the same key check the HTTP path uses (loopback-exempt, else Proxy-Authorization).
  • Bind 127.0.0.1 by default (opt into a wider bind with TEAMCLAUDE_HOST / config.proxy.host) — defense-in-depth so the port isn't reachable off-box unless you ask for it.
  • Constant-time proxy-key comparison on both gates; chmod 0600 config/state on every save (mode is ignored when the file already exists).

🐞 Correctness

  • _selectNext fallback resurrected disabled/errored accounts — the "all unavailable → soonest reset" path force-cleared their state and routed a live request to an operator-disabled or dead-token account. Now skips hard-state accounts.
  • Model-ownership bypass — the probe and soonest-reset fallbacks ignored models ownership, so an owned model could be routed to a non-owner (which just rejects it). Both paths now honor ownership. _accountOwnsModel returns a real boolean; the match predicate is shared; addAccount sets throttledAt.
  • server: a non-transient error after headers hangs the client → now destroyed; a pinned request whose account is unavailable did a pointless fleet-wide wait → short-circuits; backpressure wait leaked a close listener per drain cycle on long SSE → mutual cleanup; the idle-timeout read could surface as an unhandledRejection → swallowed.
  • upstream-fetch: per-call sx Agent used keepAlive:true, leaking a tunneled socket per request with no reuse benefit → keepAlive:false.
  • mitm: a transient cert error poisoned the memoized promise and killed the MITM path until restart → reset on failure; tunnel sockets leaked on one-sided errors → both torn down on error/close (+ connect/idle timeout).

♻️ Lifecycle

  • No SIGTERM handler in TUI mode — a systemd/kill bypassed cleanup, losing up to ~1 min of unsaved quota state and orphaning schedulers. Now both modes register SIGINT/SIGTERM with a forced-exit backstop.
  • warmer: stop() now aborts an in-flight sweep and kills the claude child (no orphan, no shutdown hang); a signal-killed child is an error, not a success; an interval change no longer triggers an extra quota-spending sweep (only off→on does). Same off→on fix in prober.
  • status-renderer surfaces the keep-warm summary (was --json-only).

Tests

connect-auth.test.js (the CONNECT gate + safeKeyEqual), selection-hardening.test.js (resurrection + ownership-fallback guards), warmer stop/abort + no-extra-sweep. 194 tests pass, lint clean. Boot smoke-tested: binds 127.0.0.1, status carries warm, pin 404s.

Not fixed (noted, low/edge or deferred)

  • Unbounded request-body buffering (DoS) — much lower risk now that the default bind is loopback; skipped to avoid capping legitimately large requests.
  • AccountUuidPatcher 36-byte overwrite assumes a 36-char value (real Claude Code always sends one) — left the delicate state machine untouched.
  • warmer.jsprober.js share a scheduler skeleton — worth extracting a base class, but deferred (didn't want a structural refactor mixed into these fixes).
  • prober fires all accounts concurrently; _withTimeout doesn't abort the fetch — low, opt-in path.

🤖 Generated with Claude Code

MagicalTux and others added 2 commits July 6, 2026 17:49
Findings from a full-codebase review (fanned out across subsystems, each
finding verified against the code before fixing).

Security (MITM/CONNECT path):
- Gate the CONNECT handler with the proxy apiKey, mirroring the HTTP path
  (loopback-exempt, else require it via Proxy-Authorization). Without this a
  remote client could CONNECT api.anthropic.com and have an ACCOUNT TOKEN
  injected (token theft), or blind-tunnel to any host (open relay / SSRF).
- Bind the listener to 127.0.0.1 by default; opt into a wider bind with
  TEAMCLAUDE_HOST / config.proxy.host. Defense-in-depth for the above.
- Constant-time proxy-key comparison (safeKeyEqual) on both gates.
- chmod 0600 config/state on every save (mode is ignored when the file exists).

Correctness:
- account-manager: the soonest-reset fallback in _selectNext no longer
  resurrects a disabled/errored account (it was force-clearing their state and
  routing a live request to them). Both fallback paths (_selectNext + probe)
  now honor model-ownership so an owned model never lands on a non-owner.
  _accountOwnsModel returns a real boolean; the match predicate is shared.
  addAccount now sets throttledAt (drift vs the constructor).
- server: a non-transient error after headers are sent now destroys the socket
  instead of hanging the client; a pinned request whose account is unavailable
  short-circuits instead of doing a fleet-wide wait; the backpressure wait no
  longer leaks a 'close' listener per drain cycle on long SSE; the idle-timeout
  read can't surface as an unhandledRejection.
- upstream-fetch: the per-call sx Agent uses keepAlive:false (it was leaking one
  tunneled socket per request; the per-call agent gives no reuse anyway).
- mitm: reset the cert/server memo on failure so a transient error doesn't wedge
  the MITM path until restart; tear down both tunnel sockets on either side's
  error/close (+ a connect/idle timeout).

Lifecycle:
- Register SIGINT/SIGTERM in BOTH modes (TUI mode had no SIGTERM path, so a
  systemd/kill lost unsaved quota state and orphaned schedulers), with a
  forced-exit backstop.
- warmer: stop() aborts an in-flight sweep and kills the child (no orphaned
  `claude`, no shutdown hang); a signal-killed child is an error, not a success;
  an interval CHANGE no longer triggers an extra quota-spending sweep (only
  off->on does). Same off->on fix in prober.
- status-renderer surfaces the keep-warm summary (it was only in --json).

Tests: connect-auth (the CONNECT gate + safeKeyEqual), selection-hardening (the
resurrection + ownership-fallback guards), and warmer stop/abort + no-extra-sweep.
194 tests pass, lint clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@MagicalTux MagicalTux merged commit 4fc849a into master Jul 6, 2026
5 checks passed
@MagicalTux MagicalTux deleted the fix/review-hardening branch July 6, 2026 09:53
@MagicalTux MagicalTux mentioned this pull request Jul 6, 2026
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