[ajitpratap0]

Summary

Follow-ups from the reviews of #831 (gosqlx-mcp HTTP hardening), in pkg/mcp, cmd/gosqlx-mcp and the MCP docs.

• Stream-cap rejections are free. A GET /mcp now reserves a stream slot before the rate limiter charges it. A GET refused by the 4-streams-per-client cap costs no sustained budget or burst tokens. A GET that the rate limiter then denies gives its slot back through the deferred release.
• Startup warning without a trusted proxy. When neither GOSQLX_MCP_TRUSTED_PROXY_HEADER nor GOSQLX_MCP_TRUSTED_PROXY_HOPS is set, gosqlx-mcp logs one warning line at startup: behind a proxy, every client shares one rate-limit bucket and one stream cap. The same is documented in docs/MCP_GUIDE.md.
• Read-deadline errors are handled. Failures to set or clear the 30 s body read deadline are counted on the limiter (deadlineErrs). Only the first is logged, with a fixed message. If the writer lacks deadline support (http.ErrNotSupported) the message says so; any other error is reported by its Go type only (%T), because its text can contain connection addresses. Session IDs, request bodies and client addresses are never logged.
• Opt-in proxy secret (CF-Connecting-IP residual risk). New GOSQLX_MCP_TRUSTED_PROXY_SECRET_HEADER and GOSQLX_MCP_TRUSTED_PROXY_SECRET settings, also available as Config.TrustedProxySecretHeader and Config.TrustedProxySecret. When both are set, the trusted header and hops apply only to requests whose secret header, last value, matches the secret. The comparison is SHA-256 followed by subtle.ConstantTimeCompare, so it runs in constant time and does not leak the secret's length. Requests with a missing or wrong secret fall back to RemoteAddr; they are not rejected. The secret header is stripped before the request reaches the MCP handler. With neither setting, behaviour is unchanged.
• Minor fixes.
  • A non-empty POST body that is not valid JSON-RPC, and an empty batch [], now cost minUnparsedWeight (1) instead of 0. Body-less requests such as DELETE stay free.
  • A batch containing initialize gets HTTP 200 on denial, the same as a single initialize.
• Docs.
  • The Server.Start() and Server.Handler() doc comments now state that they skip RateLimitMiddleware, and the MCP_GUIDE API table says the same.
  • docs/CHATGPT_APP.md explains that ChatGPT traffic arrives from OpenAI egress IPs, so ChatGPT users share one per-IP limit.
  • docs/MCP_GUIDE.md documents the Cloudflare Transform Rule setup for the secret header.

Root cause / design

• Stream cap ordering. fix(mcp): harden gosqlx-mcp HTTP server (rate limiting, timeouts, auth, tool errors) #831 ran rl.allow before streams.acquire, so the charge happened before the cap check. Swapping the order means a stream-cap rejection skips the limiter entirely, so those 429s do not carry X-RateLimit-* headers. The docs now say this. These rejections are cheap (no body read), and a client can only reach them while it already holds 4 open streams.

• Startup warning placement. The warning lives in cmd/gosqlx-mcp (sharedClientIPWarning), not in pkg/mcp. Library users who call NewRateLimitMiddleware(nil, …) keep silent behaviour. It also warns on loopback binds, because a reverse proxy on the same host is a common setup.

• Secret validation. LoadConfig validates the secret settings and fails fast in these cases:

  • only one of the two variables is set;
  • the header name is invalid;
  • the header is X-Forwarded-For, Authorization (which would clash with bearer auth, since the header is stripped) or the client-IP header itself;
  • the secret is shorter than 16 characters;
  • no trusted header and no hops are configured.

  Error messages never include the secret. If a programmatic Config sets only one of the two fields, the resolver fails closed and never trusts forwarded values.

• Why the secret is a fallback, not a rejection. A mismatched secret falls back to RemoteAddr rather than returning 403. Rotating the secret, or a Transform Rule that does not cover every hostname, therefore degrades to the shared bucket instead of an outage. Traffic that bypasses the proxy (for example via *.onrender.com) shares one bucket instead of choosing its own.

• Infra untouched. render.yaml, secrets and DNS are not changed. Turning the feature on in production means deploying the Transform Rule first, then setting the two variables in the Render dashboard (steps are in docs/MCP_GUIDE.md).

Test Plan

Run in the worktree on fix/mcp-ratelimit-followups:

Command	Result
gofmt -l pkg/mcp cmd/gosqlx-mcp	no output
go vet ./pkg/mcp/... ./cmd/gosqlx-mcp/...	clean
golangci-lint run ./pkg/mcp/... ./cmd/gosqlx-mcp/...	0 issues.
go test -race -count=1 -timeout 180s ./pkg/mcp/... ./cmd/gosqlx-mcp/... ./cmd/gosqlx/cmd/	all ok (the last package covers the docs dialect-sync check)
pre-commit hook (go fmt, go vet, go test -short ./...)	passed

These packages have no golden tests. No benchmark was run: the changes are in the HTTP middleware, not in the tokenizer, parser or AST hot paths. The only new per-request work is one SHA-256, and only when a secret is configured.

New behaviour tests:

• TestRateLimitMiddleware_StreamCapRejectionIsFree: with 4 streams open, 20 refused GETs (more than the burst limit) leave weightedCount at 4. A following POST returns 200 with X-RateLimit-Remaining = 115.
• TestRateLimitMiddleware_RateLimitedGETReleasesStreamSlot: GETs denied by the rate limiter leave 0 stream slots held.
• TestRateLimitMiddleware_ReadDeadlineErrors:
  • an unsupported writer records 6 counted errors (3 requests, set and clear) and exactly 1 log line, which contains no session ID, SQL or client IP;
  • other errors are logged with their type only (address not logged);
  • a real connection records 0 errors.
• TestRateLimitMiddleware_UnparseableBodyCostsBudget: malformed bodies, [] and invalid batches cost 1 each; DELETE stays free.
• TestClientKey_ProxySecret (13 cases): matching, missing, wrong, prefix, suffix, empty, forged-first and forged-last secrets; gating of hops; fail-closed partial config; behaviour unchanged when no secret is configured.
• TestRateLimitMiddleware_ProxySecret: rotating CF-Connecting-IP without the secret still gets 429. With the secret, each client gets its own bucket. The secret header never reaches the MCP handler.
• TestRateLimitMiddleware_Denials: a batched initialize gets 200; a batch without initialize gets 429.
• TestReadRequestInfo: new cases for minimum weight and for the initialize flag.
• TestLoadConfig_TrustedProxySecret (11 cases), including a check that errors never contain the secret.
• TestSharedClientIPWarning and TestClientIPSource (secret-gated case, secret never printed).

Unchecked: "Confirm Render forwards CF-Connecting-IP" cannot be verified from code. It needs a request through the live mcp.gosqlx.dev → Render path with the received headers inspected, so it is left open for the maintainer.

Breaking Changes

None for valid configurations.

• Behaviour changes for clients:
  • malformed non-empty POST bodies now cost 1 unit of budget;
  • stream-cap 429s no longer carry X-RateLimit-* headers;
  • a denied batched initialize now gets HTTP 200 instead of 429.
• Configuration changes:
  • LoadConfig now returns an error for invalid values of the two new variables. They were previously unused.
  • Config gains two string fields. This is additive, though unkeyed struct literals of Config (unlikely) would need updating.

Release note

### Fixed
- **gosqlx-mcp**: a `GET /mcp` stream refused by the 4-streams-per-client cap no longer counts against the client's rate limit; malformed non-empty request bodies now cost 1 unit; a denied batched `initialize` gets HTTP 200 like a single `initialize`. Failures to set the body read deadline are counted and logged once, without session IDs, bodies or addresses (#838).

### Added
- **gosqlx-mcp**: opt-in `GOSQLX_MCP_TRUSTED_PROXY_SECRET_HEADER` / `GOSQLX_MCP_TRUSTED_PROXY_SECRET`. When set, the trusted client-IP header or `X-Forwarded-For` hops are used only on requests carrying the shared secret (constant-time compare); other requests are keyed by `RemoteAddr`. See "Requiring a proxy secret" in `docs/MCP_GUIDE.md` for the Cloudflare Transform Rule setup (#838).
- **gosqlx-mcp**: startup warning when no trusted proxy is configured, since every client behind a proxy would share one rate-limit bucket and stream cap (#838).

### Documentation
- `Server.Start` and `Server.Handler` document that they do not apply `RateLimitMiddleware`; `docs/CHATGPT_APP.md` explains that ChatGPT users share OpenAI's per-IP limit (#838).

Closes #838

🤖 Generated with Claude Code

https://claude.ai/code/session_01RkzVu3JY9Y6BRWfjesWts9

[github-actions]

Claude finished @ajitpratap0's task in 39s —— View job

---

Review of PR #841: rate-limit follow-ups

I read the diff for pkg/mcp, cmd/gosqlx-mcp and the docs. I did not run the tests or the linter. This is a read-through only, and I did not review the test files in depth.

Verdict: The change is sound and I found no blocking issues. The secret handling is careful and the stream-cap reordering is correct. The points below are minor.

What looks good

• Stream-cap ordering (ratelimit.go): acquire now runs before allow, and defer release also covers a GET the rate limiter then denies, so no slot leaks. The 429 from the cap path no longer carries X-RateLimit-* headers, and the PR description and docs say so.
• Secret comparison: SHA-256 followed by subtle.ConstantTimeCompare is constant-time and does not leak the secret's length. Using the last header value is consistent with the existing trusted-header logic.
• Fail-closed partial config: secretGated with !secretOK never trusts forwarded values.
• Deadline error logging: the first error is logged with a fixed message and later ones are only counted. It logs %T instead of the error text, which avoids leaking addresses.
• LoadConfig validation: the error messages never include the secret. The cfg.TrustedProxyHeader clash check works, because that header is canonicalised on load (config.go:150).

Suggestions (non-blocking)

1. Secret stripped after clientKey, but only on the middleware path. resolver.stripSecret(r) mutates the incoming request's headers. That is fine here. Anyone who wraps the handler with their own middleware placed before this one would still see the secret. This is worth a doc note, not a code change.
2. Stream-cap refusals are now unmetered. A client holding 4 streams can send unlimited GETs that are refused at no cost. They are cheap because there is no body read. Still, logging or counting them would help spot abuse. activeReqs is also not incremented on the GET path, which matches the existing behaviour.
3. Whitespace-only POST bodies. readRequestInfo returns weight 0 only for NoBody or a nil body. Check that a whitespace-only body takes the unparseable path (weight 1) as intended. The unit test TestRateLimitMiddleware_UnparseableBodyCostsBudget should cover that case explicitly if it does not already.
4. deadlineLogged and deadlineErrs are per limiter. If NewRateLimitMiddleware is called more than once, you get one log line per instance. That is acceptable, but a comment would help.
5. Warning on loopback binds. The startup warning in sharedClientIPWarning also fires for direct, non-proxied deployments, such as local development. It is one line, so this is only noise. You could downgrade the wording to "if this server runs behind a proxy…". The text already says that, so no change is needed.
6. Unkeyed Config literals. The PR description already notes this in the breaking-changes section.

Security notes

• Bypass traffic. Requests that skip the proxy and have no valid secret all fall back to RemoteAddr. For Render's *.onrender.com hostname, that is every such client sharing one bucket. This is documented and is a deliberate trade-off against returning 403.
• Secret strength. The 16-character minimum is reasonable. The docs should recommend a random value, for example openssl rand -hex 32.

Open item

"Confirm Render forwards CF-Connecting-IP" can't be checked from code. It needs a request through the live mcp.gosqlx.dev → Render path, and the maintainer should do it before turning the feature on.
· branch fix/mcp-ratelimit-followups

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 21, 2026 3:40pm UTC