Skip to content

Fix 14 deep-review findings across security, concurrency, and API#7

Merged
kriszyp merged 4 commits into
mainfrom
claude/musing-tharp-a880b0
May 21, 2026
Merged

Fix 14 deep-review findings across security, concurrency, and API#7
kriszyp merged 4 commits into
mainfrom
claude/musing-tharp-a880b0

Conversation

@kriszyp
Copy link
Copy Markdown
Member

@kriszyp kriszyp commented May 14, 2026

Summary

Addresses all 14 findings from a deep code review of the symphony proxy. No new features — correctness and security fixes only.

Changes

Security / TLS

  • Private key now included in TLS config cache key — routes sharing a cert chain but different keys (mid-rotation) no longer silently share a ServerConfig (src/tls.rs)
  • Private key PEM bytes wrapped in zeroize::Zeroizing<Vec<u8>> so key material is zeroed on drop (src/tls.rs, Cargo.toml)
  • requireClientCert default changed true → false; omitting the field from an mtls block no longer silently enforces mandatory client certs — breaking change for callers that relied on the implicit default (src/proxy.rs, ts/types.ts)

Concurrency

  • Per-IP concurrency limit: replaced read+separate-write with fetch_update atomic test-and-increment to close the TOCTOU window (src/protection.rs)
  • Token bucket refill: replaced store on last_refill_ns with compare_exchange so concurrent threads cannot double-credit the same epoch (src/protection.rs, src/router.rs)
  • AllowBypassed Decision variant — allowlisted connections no longer call release() on a counter that was never incremented (src/protection.rs, src/proxy_conn.rs)

Reliability / API

  • Upstream connect (TCP + UDS) now has a timeout (defaults to TLS handshake timeout or 30 s); previously a firewall black-hole would block a tokio task indefinitely (src/upstream.rs)
  • idleTimeoutMs: 0 now means "no timeout" instead of dropping every connection instantly (src/proxy.rs, src/proxy_conn.rs)
  • Wildcard SNI routing now enforces exactly one left-most label: *.example.com no longer matches a.b.example.com (src/router.rs)
  • Invalid CIDR strings in allowlist/blocklist are now config errors surfaced to JS, not silently dropped rules (src/proxy.rs)
  • SNI normalized: trailing dot trimmed, empty/oversized/malformed values return None (src/sni.rs)
  • PROXY protocol v1: IPv4-mapped IPv6 addresses (::ffff:1.2.3.4) now emit TCP4 header so HAProxy/nginx don't reject the connection (src/upstream.rs)
  • toJsRoute now passes maxConnectionsPerSecond and burst; previously per-route rate limiting was silently dead (ts/proxy.ts)
  • ResolveRoute.upstream changed from Upstream[] to single Upstream to match actual semantics (ts/types.ts, ts/proxy.ts, src/proxy.rs)

Attention areas

  • The requireClientCert default flip is the one change most likely to affect existing callers. Anyone with mtls: { clientCaCert: "..." } without an explicit requireClientCert: true will now get optional client auth instead of mandatory. Review call sites.
  • The wildcard single-label fix could route traffic differently if any existing wildcard routes were intentionally matching multi-label subdomains (non-RFC-compliant but previously accepted). Unlikely but worth checking.
  • All 11 existing tests pass; the suspended.spec.ts test updated for the upstream rename.

🤖 Generated by Claude Sonnet 4.6

Security / TLS:
- Add private key to TLS cache deduplication key (finding 7)
- Zeroize private key PEM bytes on drop via zeroize::Zeroizing<Vec<u8>> (finding 13)
- Change mTLS requireClientCert default from true→false so omitting the field
  does not silently lock out clients without certificates (finding 9)

Concurrency:
- Fix TOCTOU in per-IP concurrency check: use fetch_update atomic test-and-increment
  so concurrent threads cannot race past the limit (finding 3)
- Fix rate-limit refill race: use CAS on last_refill_ns so only the first winner
  advances the refill window, preventing double-credits (finding 4); applied to
  both protection.rs and router.rs RouteTokenBucket
- Add AllowBypassed Decision variant so allowlisted IPs do not trigger release()
  on a counter that was never incremented (finding 6)

API / routing:
- Add upstream connect timeout (defaults to TLS handshake timeout / 30s) to
  prevent a black-hole upstream from blocking tokio tasks indefinitely (finding 1)
- Treat idleTimeoutMs: 0 as "no timeout" rather than firing instantly (finding 2)
- Wildcard route matching now requires exactly one left-most label, preventing
  *.example.com from matching sub.sub.example.com (finding 5)
- Surface invalid CIDR strings in allowlist/blocklist as config errors instead
  of silently dropping the rule (finding 10)
- Normalize SNI: trim trailing dot, reject if empty, >253 chars, or contains
  ':' or '/' (finding 11)
- PROXY v1 header: unwrap IPv4-mapped IPv6 addresses to TCP4 for backend compat
  (finding 12)
- toJsRoute now forwards maxConnectionsPerSecond and burst so per-route rate
  limiting is no longer silently disabled (finding 8)
- ResolveRoute.upstream changed from array to single Upstream; resolveConnection
  never supported multi-upstream balancing so the type now matches the semantics
  (finding 14)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kriszyp kriszyp requested a review from a team May 14, 2026 00:35
kriszyp and others added 3 commits May 13, 2026 18:39
…2 features

Conflicts in proxy.rs, proxy_conn.rs, upstream.rs, ts/proxy.ts.

In all cases: keep both sets of changes.
- proxy_conn.rs: apply_source_header() (main) + copy_with_idle_timeout() (ours)
  now run in sequence, so source address forwarding and zero-idle-timeout
  both work correctly together.
- proxy.rs: requireClientCert default=false (ours) + source_address_mode/http2
  fields (main) both preserved in parse_resolve_spec.
- upstream.rs: Duration + timeout imports (ours) merged with AsyncReadExt (main).
- ts/proxy.ts: maxConnectionsPerSecond + burst (ours) + sourceAddressHeader (main)
  all forwarded in toJsRoute.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Newer npm versions require bufferutil, utf-8-validate, and node-gyp-build
to be present in the lock file. Regenerated with current npm to fix CI.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
npm install -g npm@latest is broken on Node 22.22.2 (missing promise-retry
in the newly installed npm). The lock file was already regenerated with a
current npm, so npm ci runs cleanly without the upgrade step.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@kriszyp kriszyp requested review from Devin-Holland and heskew and removed request for a team May 21, 2026 14:08
@kriszyp kriszyp merged commit c2608bf into main May 21, 2026
8 of 10 checks passed
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