fix(plugins): rate limiter returns proper HTTP status codes and headers (#2668)#3449
Conversation
ec7c31d to
07fc1cd
Compare
|
Thanks @ja8zyjits — important fix for #2668. Enabling plugins to return proper HTTP status codes (429, 403, 422) is critical for client interop. The PR is substantial — make sure test coverage is thorough for all status code paths. LGTM on the approach. |
|
Hi @crivetimihai, |
a59a18b to
91f61eb
Compare
Lang-Akshay
left a comment
There was a problem hiding this comment.
Good work @ja8zyjits
Linked Issues — Completion Status (Ignore IP based anonymous request rate limiting as only authorized user have access )
| Issue | Requirement | Status | Notes |
|---|---|---|---|
| #2668 | Return HTTP 429 for rate-limited requests | ✅ Done | Plugin sets http_status_code=429, handler propagates it |
| #2668 | Add X-RateLimit-Limit header |
✅ Done | _make_headers() generates it |
| #2668 | Add X-RateLimit-Remaining header |
✅ Done | _make_headers() generates it |
| #2668 | Add X-RateLimit-Reset (unix epoch) header |
✅ Done | _make_headers() generates it |
| #2668 | Add Retry-After header |
✅ Done | Included on violation path |
| #2668 | Extend enforcement to MCP tools/endpoints | ✅ Done | tool_pre_invoke hook applies to all tool invocations (REST + MCP) |
| #2668 | Support daily unit (/d) in _parse_rate() |
❌ Missing | _parse_rate() still only supports /s, /m, /h — no /d |
| #2668 | IP-based limiting for anonymous requests | Explicitly deferred to #3349 |
Overall coverage: 6 of 8 requirements addressed (1 missing, 1 deferred)
Here are few finding to strengthen the code
| # | Location | Severity | CWE | Description |
|---|---|---|---|---|
| 1 | main.py:1449 |
🔴 High | CWE-644 | No header-name allowlist/denylist — _validate_http_headers() validates RFC 9110 syntax but does not restrict which headers a plugin may set. A malicious or buggy plugin can inject Set-Cookie (session fixation), Location (open redirect), Access-Control-Allow-Origin (CORS bypass), WWW-Authenticate (auth confusion), Content-Security-Policy (CSP override), etc. |
| 2 | main.py:1474 |
🟡 Medium | CWE-117 | Log injection via f-string — logger.warning(f"Invalid header name: {key}") interpolates plugin-supplied data, risking structured log corruption. |
| 3 | models.py:1363 |
🟡 Medium | CWE-644 | Dead PluginResult.http_headers field — declared on PluginResult and populated by rate limiter on success path, but no code reads it. Success-path headers silently dropped; future wiring without validation creates injection risk. |
| 4 | main.py:1573 |
🔵 Low | — | Typo: validatated_headers → should be validated_headers |
| 5 | constants.py:13 |
🔵 Low | — | Duplicate # Standard comment on consecutive lines |
Remediation Suggestions
| # | Suggestion | Effort |
|---|---|---|
| 1 | Add a header-name denylist (or preferably an allowlist: Retry-After, X-RateLimit-*, X-Plugin-*) in _validate_http_headers(). Ref: OWASP A05:2021, CWE-644. |
Low |
| 2 | Switch from f-strings to parameterized logging: logger.warning("Invalid header name: %s", key). Ref: CWE-117. |
Trivial |
| 3 | Remove http_headers from PluginResult until wired up, or add a clear TODO and ensure future wiring uses _validate_http_headers(). |
Trivial |
| 4 | Rename validatated_headers → validated_headers. |
Trivial |
| 5 | Remove duplicate # Standard comment at constants.py:13. |
Trivial |
91f61eb to
dae5c52
Compare
|
Hi @crivetimihai @msureshkumar88 @Lang-Akshay I have created a new child issue from #2668 and have made sure that this PR is only directed to wards the new bug issue. Regards, |
96a4d75 to
434e77a
Compare
…lations Add support for plugins to specify HTTP status codes (e.g., 429 for rate limiting) and custom headers (e.g., Retry-After) in PluginViolation responses. - Add http_status_code and http_headers fields to PluginViolation model - Implement PLUGIN_VIOLATION_CODE_MAPPING for common violation types - Update plugin_violation_exception_handler to use explicit status codes with fallback to code mapping, defaulting to 200 for JSON-RPC compliance - Add RFC 9110 header validation to prevent header injection - Enhance rate limiter plugin with multi-dimensional rate limiting, proper HTTP 429 responses, and X-RateLimit-* / Retry-After headers - Add comprehensive test coverage for status code precedence, header propagation, and header validation Fixes #2668 Signed-off-by: Jitesh Nair <jiteshnair@ibm.com> Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
- Fix typo: validatated_headers → validated_headers - Hoist RFC 9110 token regex to module-level compiled constant - Consolidate redundant CRLF check into unified CTL validation - Fix integration test docstring path (was unit test path) - Add tests for _parse_rate minute/hour/error branches - Add tests for unlimited (no-limit) prompt and tool paths - Achieve 100% differential test coverage on rate_limiter.py Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
434e77a to
d158a5e
Compare
Review — Rebased, reviewed, fixed, and pushedRebaseRebased cleanly onto Fixes applied
Design reviewThe three-tier HTTP status resolution (explicit Note for follow-up
|
After the restructure into independent crates (#3147), each crate has its own target/ directory. The existing pattern only covered the workspace-level plugins_rust/target/. Signed-off-by: Mihai Criveti <crivetimihai@gmail.com>
crivetimihai
left a comment
There was a problem hiding this comment.
Reviewed, rebased, and fixed. 100% differential test coverage on rate_limiter.py. Design is sound — three-tier HTTP status resolution is backward compatible, header validation prevents injection per RFC 9110. Minor fixups applied (typo, regex hoist, redundant check consolidation, docstring path). Note for follow-up: PluginResult.http_headers on successful responses is not yet propagated by the PluginManager/service layer.
|
@ja8zyjits - created [FEATURE]: Propagate PluginResult.http_headers to HTTP responses on successful requests #3576 as a follow-up. |
🐛 Bug-fix PR
📌 Summary
Fixes #3560
Rate-limiting and other policy enforcement plugins needed the ability to return proper HTTP status codes (e.g., 429 Too Many Requests) and standard headers (e.g., Retry-After, X-RateLimit-*) when violations occur. Previously, all plugin violations returned HTTP 200 with JSON-RPC error payloads, making it impossible for:
This created a mismatch between HTTP-native clients expecting standard status codes and the gateway's JSON-RPC-centric error handling.
🔁 Reproduction Steps
Start the gateway with config.yaml
(e.g. make dev or python -m mcpgateway.main)
Repeatedly call a REST tool until rate limits are exceeded
Observe:
🐞 Root Cause
Improper error code handling in plugin violation.
💡 Fix Description
This PR introduces a three-tier approach for determining HTTP status codes in plugin violation responses:
Key Changes
1. Enhanced PluginViolation Model
2. Violation Code Mapping (mcpgateway/plugins/framework/constants.py)
3. Updated Exception Handler (mcpgateway/main.py)
🧪 Testing
Added 8 comprehensive test cases covering:
✅ Explicit http_status_code takes precedence over mapping
✅ Code mapping fallback (e.g., RATE_LIMIT → 429)
✅ Default to 200 for unknown codes (JSON-RPC compliance)
✅ HTTP headers properly propagated to response
✅ Multiple headers (Retry-After, X-RateLimit-*) included correctly
✅ Graceful handling when headers is None
✅ Backward compatibility with existing violations (no fields set)
✅ Doctest examples updated to reflect new behavior
Test Coverage: All new code paths covered, including edge cases.
End to End test
.envfile update:plugins/config.yamlupdate:TOKENenvironment variablemake devdeny 422responses with all the required headers🔄 Backward Compatibility
✅ Fully Backward Compatible
📝 Example Usage
Rate-Limiting Plugin
Response:
🧪 Verification
make lintmake testmake coverage📐 MCP Compliance (if relevant)
✅ Checklist
make black isort pre-commit)This PR enables plugins to implement proper HTTP semantics for policy enforcement while maintaining full backward compatibility with the existing JSON-RPC error model.