[ajitpratap0]

Summary

Hardens the gosqlx-mcp HTTP server against the pre-existing security/robustness defects found by the PR #823 test run (all also present on main). Stacked on #823 — review/merge #823 first; this PR then retargets to main.

Description

Severity	Defect (pre-existing)	Fix
High	Per-IP rate limit bypassed by spoofing X-Forwarded-For	Key on RemoteAddr by default; opt-in GOSQLX_MCP_TRUSTED_PROXY_HEADER / GOSQLX_MCP_TRUSTED_PROXY_HOPS, values validated with net.ParseIP, IPv6 grouped by /64; render.yaml sets CF-Connecting-IP
High	Unbounded limiter memory (12 MB → 668 MB in 8 s)	Per-shard LRU cap with least-recently-seen eviction
Medium	90 idle SSE GET streams cut every user's limit 120 → 30	GET streams excluded from load metric; max 4 per IP
Medium	Slow bodies / idle keep-alives held forever	30 s body read deadline on every route (408 on /mcp), IdleTimeout 120 s, MaxHeaderBytes 32 KB; no global Read/WriteTimeout so SSE streams survive
Low	>64 KB body → misleading "not valid json"	HTTP 413 with a clear message (limit kept at 64 KB: 1 MB SQL allocates ~530 MB in analyze_sql)
Low	Non-constant-time bearer compare; case-sensitive scheme	SHA-256 digest + crypto/subtle; Bearer case-insensitive, token trimmed
Low	Denials were HTTP 200, id: null, no Retry-After	HTTP 429 + Retry-After + request id; accurate X-RateLimit-*; batches charged per tool (denied initialize stays 200 because mcp-go treats any 4xx there as a legacy SSE server)
Low	Input errors surfaced as JSON-RPC -32603 "internal error"; wrong-typed args silently coerced	isError tool results naming the argument; indent_size must be an integer 0–16

Type of Change

• Bug fix (non-breaking change which fixes an issue)
• Breaking change — see below

Test Plan

• task check (fmt, vet, golangci-lint 0 issues, race tests) passes
• New tests: client key resolution, spoofed-XFF still limited, 200k-key memory bound + LRU eviction, 429/Retry-After/id, body limits (413/408), read deadline cleared after body, GET stream cap, non-/mcp slow-body deadline (with a no-fix control case), bearer header variants, trusted-proxy config, handler isError cases, HTTP server limits
• Original repro scripts re-run against the fixed binary: XFF flood 300/300 allowed → 0 allowed; RSS 671 MB → 24 MB; SSE victim stays at 120/min; slow body closed at 30 s; idle keep-alive closed at 120 s; 90 KB body → 413
• Compatibility: 1,656-call stdio corpus vs pre-fix binary — all 1,557 successful calls byte-identical; the 98 former -32603 errors are now isError with the same text
• After deploy: confirm Render forwards CF-Connecting-IP (two clients on different networks should see independent X-RateLimit-Remaining); otherwise all users share one bucket

Breaking Changes

• Deployments behind a reverse proxy must set GOSQLX_MCP_TRUSTED_PROXY_HEADER or GOSQLX_MCP_TRUSTED_PROXY_HOPS, or every client shares the proxy's bucket.
• Rate-limit denials are HTTP 429 (was 200); clients that only parsed the JSON-RPC body still work.
• Tool input errors are isError results instead of JSON-RPC errors; indent_size outside 0–16 or non-integer is rejected.

Additional Notes

Residual risk: CF-Connecting-IP is trusted on any request, so a client that reaches the Render origin URL directly (bypassing Cloudflare) can still choose its rate-limit key. Memory stays bounded. A follow-up could trust the header only when Render's appended X-Forwarded-For hop is a Cloudflare IP, or require a secret header set by a Cloudflare Transform Rule.

🤖 Generated with Claude Code

https://claude.ai/code/session_01L2wnDjnjXs2u8baaf2PC6w

[vercel]

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

Project	Deployment	Actions	Updated
gosqlx-website	Ready	Preview	Oct 21, 2026 10:22am UTC

[github-actions]

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

---

Review of PR #831: gosqlx-mcp HTTP hardening

I read pkg/mcp/ratelimit.go in full and skimmed the config.go, middleware.go and main.go diffs. I did not review tools.go (the isError and indent_size changes) or the test files in depth, and I did not run the tests.

Overall: The change is well structured and the rate limiter is sound. The per-shard LRU bounds memory, cleanup only walks the stale tail, and clientKey falls back safely to RemoteAddr on bad input. The deadline handling is careful: it is cleared after a successful read and kept after a failed one, so net/http's drain of the unread body can't block. The note about mcp-go treating a 4xx on initialize as a legacy SSE server is a good catch.

Issues and suggestions

1. Stream-cap rejections still spend budget (ratelimit.go:598-625). allow() consumes a burst token and sustained weight before streams.acquire runs. A client at its 4-stream cap that keeps retrying GETs drains its own tools/call budget. It only affects that client, but acquiring the slot first, or refunding on rejection, would be cleaner.

2. Shared bucket and shared stream cap without proxy config (ratelimit.go:618). Behind an unconfigured proxy, all users share one bucket. They also share one 4-stream cap, so a fifth SSE client anywhere gets 429. That is worse than the rate-limit-only impact in the PR text. Consider logging a startup warning when X-Forwarded-For or CF-Connecting-IP is seen but no trusted-proxy setting is configured. That would make the breaking change much easier to diagnose.

3. CF-Connecting-IP residual risk. I agree with the PR's own note. Trusting it on any request lets a client that reaches the origin directly choose its key. Memory stays bounded, but the per-IP limit is effectively off for that attacker. Treat the follow-up (check that the last X-Forwarded-For hop is a Cloudflare IP, or require a shared-secret header) as more than optional.

4. Silent loss of the read deadline (ratelimit.go:586). rc.SetReadDeadline errors are discarded. If the writer doesn't support deadlines (a wrapped ResponseWriter or some HTTP/2 setups), slow-body protection quietly stops working. Consider logging once on http.ErrNotSupported, and add a test that the middleware's ResponseWriter chain supports deadlines.

5. Weight-0 and unparseable bodies (ratelimit.go:468-490). Malformed JSON, or a batch with a non-object element, gets weight 0 and is limited only by the 10/s burst. Downstream rejects it, so this is probably fine. tools/list and ping are also unmetered beyond the burst, which is how it worked before the PR. A small non-zero weight for unparseable bodies would close it cheaply.

6. Denied initialize in a batch (ratelimit.go:609). Only a single-message initialize is exempted from 429. A batched initialize would still get 429 and hit the legacy-SSE fallback. This is an edge case, but requestInfo could track "batch contains initialize".

7. IPv6 /64 grouping. This is reasonable. Hosts that share a /64, such as some cloud providers or small ISPs, will share a bucket. Worth a line in MCP_GUIDE.md.

Nits

• bucketFor evicts in a for loop, but one eviction is enough because it adds one entry at a time. This is harmless.
• ipv6KeyMask and normalizeIP are tidy. A test for IPv4-mapped IPv6 (::ffff:1.2.3.4) would confirm To4() handles it. It looks right.

Verdict

There are no blocking bugs in the parts I reviewed. Items 1, 2 and 4 are worth addressing before merge, and the rest can follow. Please complete the unchecked post-deploy item (confirm Render forwards CF-Connecting-IP) before relying on the per-IP limits in production.
· branch fix/mcp-server-hardening

[ajitpratap0]

Retargeted to main after #823 squash-merged. Resolved conflicts by merging origin/main (e80fef3): the branch still carried #823's pre-squash commits, so for the 10 conflicted files I took main's version and re-applied only this PR's own diff (48bda6e..3eebd5d). Net diff vs main is this PR's 15 files only. gofmt, vet, golangci-lint and go test -race on pkg/mcp and cmd/gosqlx-mcp pass.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RkzVu3JY9Y6BRWfjesWts9

[github-actions]

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

---

Review of PR #831: gosqlx-mcp HTTP hardening

I read the request path in pkg/mcp/ratelimit.go (lines 420-630). I did not run the tests or review tools.go and the test files in depth. This review builds on the earlier one on this PR, and I agree with its findings. The numbers below are my own reading of the code.

Overall: The design is sound. The default of keying on RemoteAddr fixes the XFF bypass. The body-read deadline is set before the read, cleared on success and kept on failure, which is correct. The 413, 408 and 400 mapping in writeBodyError is clean.

Issues

1. Stream-cap rejection still spends budget (ratelimit.go:598 vs :618). allow() runs before streams.acquire. A client at the 4-stream cap that keeps retrying GET drains its own burst and sustained budget. A GET has weight streamWeight, so check how much that costs. Acquiring the slot before allow, or refunding on rejection, fixes this.
2. Stream-cap 429 lacks accurate headers. The rejection at :619 sends a fixed Retry-After: 60. That value is arbitrary, and the X-RateLimit-* headers describe the rate bucket, not the stream cap. This is minor, but a comment would help.
3. Silent loss of the read deadline (:586, :593). The SetReadDeadline errors are discarded. If a wrapper ResponseWriter doesn't implement Unwrap, the slow-body protection quietly stops working. Log once on http.ErrNotSupported, and add a test that goes through the real middleware chain (middleware.go) to confirm a slow body gets 408.
4. Unparseable bodies get weight 0 (:473, :484). Malformed JSON, or a batch that fails to unmarshal, is charged nothing and is limited only by the burst. Downstream rejects these, but a small non-zero weight, such as 1, would close the gap cheaply. Note that ping and tools/list are also free beyond the burst.
5. Batched initialize still gets 429 (:609). The 200 exemption only applies when info.method == "initialize", and the batch path never sets method. This is an edge case, but it would trigger the legacy-SSE fallback you noted. Track "batch contains initialize" in requestInfo.
6. Batch memory cost. The body is parsed fully for the weight and then parsed again downstream. At the 64 KB cap this is fine. Large batches of tiny messages could still amplify allocations, so consider capping the batch length.
7. Shared bucket and shared stream cap behind an unconfigured proxy. All users share both. A fifth SSE client anywhere gets 429. A startup or first-request warning when X-Forwarded-For or CF-Connecting-IP is present but no trusted-proxy setting exists would make this breaking change easy to diagnose.
8. CF-Connecting-IP residual risk. I agree with your note. A client that hits the origin directly picks its own key. The follow-up should be treated as more than optional.
9. IPv6 /64 grouping. Hosts sharing a /64 share a bucket. Mention this in docs/MCP_GUIDE.md.

Nits

• bucketFor evicts in a loop where one eviction is enough. This is harmless.
• Add a test for IPv4-mapped IPv6 (::ffff:1.2.3.4).

Verdict

I found no blocking bugs. Please address items 1, 3 and 7 before merge, and complete the unchecked post-deploy item (confirm Render forwards CF-Connecting-IP).
· branch fix/mcp-server-hardening