fix(auth): return 405 Method Not Allowed instead of 404 for wrong HTTP methods#38
fix(auth): return 405 Method Not Allowed instead of 404 for wrong HTTP methods#38ojaswa072 wants to merge 6 commits intoOWASP-BLT:mainfrom
Conversation
|
👋 Thanks for opening this pull request, @ojaswa072! Before your PR is reviewed, please ensure:
🔍 Our team will review your PR shortly. If you have questions, feel free to ask in the comments. 🚀 Keep up the great work! — OWASP BLT |
📊 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 |
WalkthroughAuth handlers now validate HTTP method early (returning 405 + Allow) and reorder secret/DB access to occur after method checks where applicable; Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as AuthHandler
participant JWT as JWT (env.JWT_SECRET)
participant DB as Database
Client->>Handler: HTTP request (method + payload)
Handler->>Handler: validate HTTP method
alt method not allowed
Handler-->>Client: 405 Method Not Allowed\nHeader: Allow
else method allowed
Handler->>JWT: read/validate JWT secret (if required)
JWT-->>Handler: secret present / error
Handler->>DB: get_db_safe / queries (if required)
DB-->>Handler: data / error
Handler-->>Client: 200 / appropriate response or error
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)
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/handlers/auth.py (1)
159-164:⚠️ Potential issue | 🟠 MajorMove the method guard ahead of JWT/DB setup.
handle_signinandhandle_verify_emailstill do config/DB work before rejecting unsupported verbs. IfJWT_SECRETis missing or the DB is unavailable, a wrong-method request can still return500instead of405, so this PR does not fully guarantee the behavior it intends to fix.💡 Suggested change
async def handle_signin(request: Any, env: Any, path_params: Dict[str, str], query_params: Dict[str, str], path: str) -> Any: logger = logging.getLogger(__name__) try: + method = str(request.method).upper() + if method != "POST": + return error_response("Method Not Allowed", 405) + jwt_secret = env.JWT_SECRET if not jwt_secret: return error_response("JWT secret not configured, please configure it using `wrangler secret put JWT_SECRET`", 500) - method = str(request.method).upper() - if method != "POST": - return error_response("Method Not Allowed", 405) body = await parse_json_body(request) async def handle_verify_email(request: Any, env: Any, path_params: Dict[str, str], query_params: Dict[str, str], path: str) -> Any: logger = logging.getLogger(__name__) try: - db= await get_db_safe(env) - jwt_secret = env.JWT_SECRET + method = str(request.method).upper() + if method != "GET": + return error_response("Method Not Allowed", 405) + + db = await get_db_safe(env) + jwt_secret = env.JWT_SECRET if not jwt_secret: return error_response("JWT secret not configured, please configure it using `wrangler secret put JWT_SECRET`", 500) - - method = str(request.method).upper() - if method != "GET": - return error_response("Method Not Allowed", 405)Also applies to: 231-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/handlers/auth.py` around lines 159 - 164, The method guard must run before any config/DB initialization in handlers so wrong-method requests return 405 not 500; move the HTTP verb checks to the top of handle_signin and handle_verify_email (and the other handler block referenced around lines 231-240) so that the method is validated before reading env.JWT_SECRET or opening DB connections, i.e., check request.method and return error_response("Method Not Allowed", 405) immediately in each handler before accessing jwt_secret, DB clients, or other setup.
🤖 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/handlers/auth.py`:
- Around line 71-72: The 405 responses created by the handlers use
error_response(...) without an Allow header; update each handler to return a 405
with the appropriate Allow header by passing headers={"Allow": "<METHOD>"}
through the response helper. Specifically, in the signup handler (where it
currently checks if method != "POST"), change the error return to include
headers={"Allow":"POST"}; do the same in the signin handler (use "POST") and in
the verify_email handler (use "GET"). Use the json_response-compatible headers
parameter (i.e., have error_response forward headers to json_response) or add
the headers at each call site so the returned response includes Allow.
---
Outside diff comments:
In `@src/handlers/auth.py`:
- Around line 159-164: The method guard must run before any config/DB
initialization in handlers so wrong-method requests return 405 not 500; move the
HTTP verb checks to the top of handle_signin and handle_verify_email (and the
other handler block referenced around lines 231-240) so that the method is
validated before reading env.JWT_SECRET or opening DB connections, i.e., check
request.method and return error_response("Method Not Allowed", 405) immediately
in each handler before accessing jwt_secret, DB clients, or other setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: OWASP-BLT/coderabbit/.coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 3f0f0bf4-b243-4cef-9a61-28012d65ca3c
📒 Files selected for processing (1)
src/handlers/auth.py
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
👋 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!
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_auth.py (2)
10-13: Remove duplicatesys.pathsetup; rely on sharedconftest.pyconfiguration.
tests/conftest.pyalready insertssrcintosys.path(line 10). Adding it again here is redundant. Additionally, insertinghandlers_pathwithinsert(0, ...)elevates it abovesrc, which can shadow imports and make resolution fragile.Instead, import
authdirectly after ensuringconftest.pyis loaded. Ifhandlers/auth.pyrequires special module stubs (likeworkers), add those stubs toconftest.pyor a shared fixture rather than per-test-file imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth.py` around lines 10 - 13, Remove the duplicate sys.path manipulation in tests/test_auth.py: delete the sys.path.insert calls that add src_path and handlers_path, and instead rely on tests/conftest.py to set up sys.path; then import the auth module directly (e.g., from handlers import auth or import auth) in tests/test_auth.py. If handlers/auth.py needs module stubs (like workers), add those stubs centrally in tests/conftest.py rather than inserting handlers_path per test file, and ensure no insertion elevates handlers above src to avoid import shadowing.
23-25:sys.modulespatching at import time can leak across test files.Stubbing
workersglobally during module import is brittle for test isolation. Prefer a fixture (monkeypatch.setitem(sys.modules, ...)) plus controlled import/reload so cleanup is automatic.#!/bin/bash set -euo pipefail # Check whether other tests/modules rely on `workers` and may be affected by global stubs rg -n -C2 'from workers import|import workers|sys\.modules\.setdefault\("workers"|sys\.modules\["workers"\]' tests src🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_auth.py` around lines 23 - 25, The test currently patches sys.modules at import time via sys.modules.setdefault("workers", types.SimpleNamespace(Response=_MockWorkersResponse)) which can leak across tests; change to use a fixture that calls monkeypatch.setitem(sys.modules, "workers", types.SimpleNamespace(Response=_MockWorkersResponse)) before importing the module under test and then import/reload auth (e.g., use importlib.import_module or importlib.reload on auth_handler) so the stub is only present during the test and is automatically cleaned up; update references to the current import statement (import auth as auth_handler) to happen after the monkeypatch is applied.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/test_auth.py`:
- Around line 41-63: Add a new test in tests/test_auth.py that mirrors the
signin wrong-method test: call auth_handler.handle_signup with a
MockRequest(method="GET") (URL "/auth/signup"), pass EmptyEnv() and the same
empty dicts, and assert the response.status is 405 and
response.headers.get("Allow") == "POST"; to ensure the method-guard runs before
DB/JWT work, monkeypatch auth_handler.get_db_safe to a coroutine that raises
(e.g., AssertionError) so the test fails if get_db_safe is invoked. Reference:
handle_signup, get_db_safe, MockRequest, EmptyEnv, auth_handler.
---
Nitpick comments:
In `@tests/test_auth.py`:
- Around line 10-13: Remove the duplicate sys.path manipulation in
tests/test_auth.py: delete the sys.path.insert calls that add src_path and
handlers_path, and instead rely on tests/conftest.py to set up sys.path; then
import the auth module directly (e.g., from handlers import auth or import auth)
in tests/test_auth.py. If handlers/auth.py needs module stubs (like workers),
add those stubs centrally in tests/conftest.py rather than inserting
handlers_path per test file, and ensure no insertion elevates handlers above src
to avoid import shadowing.
- Around line 23-25: The test currently patches sys.modules at import time via
sys.modules.setdefault("workers",
types.SimpleNamespace(Response=_MockWorkersResponse)) which can leak across
tests; change to use a fixture that calls monkeypatch.setitem(sys.modules,
"workers", types.SimpleNamespace(Response=_MockWorkersResponse)) before
importing the module under test and then import/reload auth (e.g., use
importlib.import_module or importlib.reload on auth_handler) so the stub is only
present during the test and is automatically cleaned up; update references to
the current import statement (import auth as auth_handler) to happen after the
monkeypatch is applied.
🪄 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: b871a6a6-3f30-4359-a5f6-69b1c555d69f
📒 Files selected for processing (2)
src/handlers/auth.pytests/test_auth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/handlers/auth.py
…-allowed-status-code
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
|
@coderabbitai Can this PR be merged? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me pull the full PR details before giving you an assessment. 🏁 Script executed: Length of output: 50787
✅ What's in good shape
All actionable issues from previous reviews have been resolved. The code is technically sound. ❌ Blocking item — Human peer review requiredThe repository enforces peer review via the
You'll need to request a review from a human maintainer or team member to satisfy this requirement.
|
owasp-blt
bot
added
has-peer-review
2 participants
Summary
Fixes auth endpoint method handling so unsupported methods return 405 Method Not Allowed (with Allow) instead of 404, and adds focused regression tests.
Problem
Auth endpoints returned 404 Not Found for non-allowed methods, which is semantically incorrect and inconsistent with HTTP method handling.
Fix
Files Changed
Testing