fix(router): return 405 with Allow header for method mismatch#71
fix(router): return 405 with Allow header for method mismatch#71ojaswa072 wants to merge 3 commits intoOWASP-BLT:mainfrom
Conversation
📊 Monthly LeaderboardHi @ojaswa072! Here's how you rank for March 2026:
Scoring this month (across OWASP-BLT org): Open PRs (+1 each), Merged PRs (+10), Closed (not merged) (−2), Reviews (+5; first two per PR in-month), Comments (+2, excludes CodeRabbit). Run |
owasp-blt
bot
added
unresolved-conversations: 0
needs-peer-review
|
👋 Hi @ojaswa072! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you!
|
WalkthroughRouter dispatch now collects methods from routes whose regex matches the requested path and returns 405 Method Not Allowed with an Allow header when the path matches but the method does not. Handler param extraction uses Changes
Sequence DiagramsequenceDiagram
actor Client
participant Router
participant RouteCollection
participant Response
Client->>Router: HTTP request (method, path)
Router->>RouteCollection: iterate routes (use route.regex to match path)
loop per route
RouteCollection->>Router: regex matches? (yes/no)
alt regex matches
Router->>Router: collect route.method
end
end
alt path matched AND method matches
Router->>Response: invoke handler -> 200/handler response
Response->>Client: handler response
else path matched BUT method didn't match
Router->>Response: return 405 with Allow header (sorted methods)
Response->>Client: 405 Method Not Allowed + Allow
else no path matched
Router->>Response: return 404 Not Found
Response->>Client: 404 Not Found
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/router.py (1)
163-167: Avoid duplicate regex evaluation in the route loop.You can compute the path regex match once per route and reuse it for both
allowed_methodsand handler dispatch.♻️ Proposed simplification
- for route in self.routes: - if route.regex.match(path): - allowed_methods.add(route.method) - - path_params = route.match(method, path) - if path_params is not None: + for route in self.routes: + path_match = route.regex.match(path) + if not path_match: + continue + + allowed_methods.add(route.method) + if route.method == method: + path_params = path_match.groupdict() try: return await route.handler( request=request, env=env, path_params=path_params, query_params=query_params, path=path )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/router.py` around lines 163 - 167, Compute the regex result once per route and reuse it: call route.regex.match(path) into a local variable (e.g., match) and use that to decide adding to allowed_methods and to gate the handler dispatch; then only call route.match(method, path) when the stored match indicates a path match (to avoid re-evaluating the regex). Update the logic around allowed_methods, the path_params assignment, and any conditionals that currently call route.regex.match(path) or route.match(method, path) so they use the single stored match value.tests/test_router.py (1)
205-223: Add one test for multi-methodAllowaggregation.Consider adding a case where the same path is registered for multiple methods to verify the combined/sorted
Allowheader contract.🧪 Suggested test addition
class TestRouterHandleMethodBehavior: @@ async def test_handle_returns_405_with_allow_header_when_path_exists(self): router = Router() @@ assert body["status"] == 405 assert body["message"] == "Method Not Allowed" + + `@pytest.mark.asyncio` + async def test_handle_returns_405_with_all_allowed_methods_for_path(self): + router = Router() + + async def get_handler(request, env, path_params, query_params, path): + return {"ok": True} + + async def put_handler(request, env, path_params, query_params, path): + return {"ok": True} + + router.add_route("GET", "/bugs", get_handler) + router.add_route("PUT", "/bugs", put_handler) + + request = _MockRequest("DELETE", "https://example.com/bugs") + response = await router.handle(request, env={}) + + assert getattr(response, "status", None) == 405 + assert response.headers.get("Allow") == "GET, PUT"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_router.py` around lines 205 - 223, Add a new async test mirroring test_handle_returns_405_with_allow_header_when_path_exists that registers the same path with multiple methods using Router.add_route (e.g., "GET", "HEAD", "POST" handlers), then call Router.handle with a different method to trigger 405 and assert the response.headers["Allow"] contains the aggregated, sorted methods (e.g., "GET, HEAD, POST") and the body retains status 405 and message "Method Not Allowed"; reference Router, add_route and handle to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/router.py`:
- Around line 163-167: Compute the regex result once per route and reuse it:
call route.regex.match(path) into a local variable (e.g., match) and use that to
decide adding to allowed_methods and to gate the handler dispatch; then only
call route.match(method, path) when the stored match indicates a path match (to
avoid re-evaluating the regex). Update the logic around allowed_methods, the
path_params assignment, and any conditionals that currently call
route.regex.match(path) or route.match(method, path) so they use the single
stored match value.
In `@tests/test_router.py`:
- Around line 205-223: Add a new async test mirroring
test_handle_returns_405_with_allow_header_when_path_exists that registers the
same path with multiple methods using Router.add_route (e.g., "GET", "HEAD",
"POST" handlers), then call Router.handle with a different method to trigger 405
and assert the response.headers["Allow"] contains the aggregated, sorted methods
(e.g., "GET, HEAD, POST") and the body retains status 405 and message "Method
Not Allowed"; reference Router, add_route and handle to locate where to add the
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: c8d984c8-c51c-4408-b582-314ea2cdd20d
📒 Files selected for processing (2)
src/router.pytests/test_router.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/router.py`:
- Around line 181-184: The except block currently returns raw exception text to
clients; change it to log the full exception server-side (e.g., call
logger.exception or similar within the except Exception as e: handler) and
return a generic 500 error message via error_response (e.g., "Internal server
error" or "An unexpected error occurred") without including e!s; ensure you
still preserve the error for diagnostics by logging the exception and any
relevant context before returning the generic response.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: d39c05dc-9d2a-4525-ab72-7acd94e74936
📒 Files selected for processing (2)
src/router.pytests/test_router.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_router.py
|
@coderabbitai analyze this PR thoroughly and check its merge readiness |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me gather the actual code to analyze this PR thoroughly. 🏁 Script executed: Length of output: 25965 Good, I have the main files. Let me check a few more critical pieces. 🏁 Script executed: Length of output: 14167
✅ Overall Verdict: Ready to merge — with one minor fix recommendedThe core logic is correct, RFC-compliant, and well-tested. There is one minor issue worth fixing before merge, and a few low-priority nits. 🔍 Logic & Correctness
|
| Area | Status |
|---|---|
RFC 7231 §6.5.5 compliance (Allow header on 405) |
✅ |
Sorted Allow header values |
✅ |
| 404 preserved for truly unknown paths | ✅ |
| Exception details no longer leaked to client | ✅ |
Structured logging with logger.exception() |
✅ |
error_response(headers=...) signature confirmed supported |
✅ |
asyncio_mode = "auto" in pyproject.toml supports async tests |
✅ |
Mock response .headers.get("Allow") works on plain dict |
✅ |
| Test coverage for single-method 405, multi-method 405, and 404 | ✅ |
📋 Summary
Fix the unused json_response import (issue #1) and this PR is clean to merge. Issues #2–#4 are low-priority and can be addressed in follow-up PRs if desired.
|
@ojaswa072 Nice work on the RFC-compliant 405 handling! The two-phase matching is clean and the security improvement (not leaking exception details) is solid.
|
2 participants
Summary
Fixes router method handling so existing paths return 405 (with
Allow) instead of 404, while unknown paths still return 404.Changes
Allowheader for 405 responses.Allow(single and multi-method) and 404 fallback.Testing
Allowbehavior.