fix(dashboard): short-circuit OPTIONS preflight in auth middleware for CORS - #59189
Conversation
AmirF194
left a comment
There was a problem hiding this comment.
The diagnosis is right and the fix is safe. CORSMiddleware is registered first, so it ends up innermost and runs after the auth middleware, which is why a tokenless OPTIONS preflight gets 401 before CORS can answer. Short-circuiting OPTIONS in the auth middleware lets the inner CORSMiddleware produce the preflight response.
I checked the smuggling angle since bypassing auth on a whole method is the scary part, and it holds up. No route processes OPTIONS as a mutating or data-returning action (handlers are all method-specific, no @app.options, no methods lists including OPTIONS). I confirmed by sending OPTIONS /api/gateway/drain with a real drain body and no token, and the drain handler never ran. A genuine preflight is intercepted by CORSMiddleware and returns "OK" with just CORS headers, so no handler output leaks, and the host-header (DNS-rebinding) check still runs on the OPTIONS path since it is inner to auth. CORS itself stays locked to localhost with no allow-credentials, and an evil Origin gets a 400 with nothing reflected.
Two things before merge, neither a security blocker. First, please add a regression test. This changes an auth-bypass branch and there is currently no coverage for it (a temporary scaffold I wrote in a clean 3.11 container confirmed preflight-to-protected-route returns CORS not 401, non-OPTIONS still 401, and OPTIONS triggers no handler side effect, and it fails-first on main). Something small along those lines is enough. Second, consider gating on a real preflight, e.g. also requiring the access-control-request-method header, so a bare OPTIONS with no Origin does not fall through to the router. Right now it returns 405 vs 404 by route, which is a minor path-existence oracle rather than a real leak, but the tighter guard matches intent and costs nothing.
|
Reopening — this was closed in error by the automated stale-PR check, which only inspected GitHub's |
|
Thanks for the thorough review, @AmirF194 — both points addressed:
|
teknium1
left a comment
There was a problem hiding this comment.
Thanks for adding the focused regression coverage; the current-main diagnosis is correct: auth_middleware token-gates protected /api/ paths at hermes_cli/web_server.py:590-595, while CORS is registered earlier at hermes_cli/web_server.py:299-304.
Problems
hermes_cli/web_server.py:584checksOPTIONSplusAccess-Control-Request-Method, but does not requireOrigin. Starlette CORS passes through requests without Origin before considering the preflight header (starlette/middleware/cors.py:85-93), so a no-Origin OPTIONS request with that header bypasses token auth but is not handled as a CORS preflight.tests/dashboard/test_auth_cors_preflight.pycovers a headerless bare OPTIONS request, not that no-Origin-plus-preflight-header case.
Suggested changes
- Require a nonempty
Originas well asAccess-Control-Request-Method, then add a test that the no-Origin variant remains 401. Cross-referenced PR #59422 uses this exact predicate and its test covers a genuine cross-port preflight.
Automated hermes-sweeper review.
|
Thanks @teknium1 — good catch, addressed in the pushed commit. The short-circuit now also requires a nonempty Added a regression test ( Fails-first on the previous guard (the no-Origin variant returned 405 from routing, i.e. it had bypassed auth), passes after requiring |
…r CORS CORS preflight (OPTIONS) requests to /api/* protected routes return 401 because the auth middleware (registered before CORSMiddleware in Starlette stack) checks the session token, which preflights never carry by design. Add a guard in auth_middleware to pass OPTIONS through without token validation, letting CORSMiddleware respond with the proper CORS headers. Closes NousResearch#59052
…header Address review on NousResearch#59052: add a regression test that a genuine OPTIONS preflight to a protected /api/ route is answered by CORSMiddleware (not 401), a tokenless GET still 401s, and a bare OPTIONS with no Access-Control-Request-Method no longer bypasses auth. Tighten the guard to require the preflight header so a bare OPTIONS cannot probe protected routes.
Per maintainer review on NousResearch#59189: the OPTIONS short-circuit checked only Access-Control-Request-Method, but Starlette CORSMiddleware passes Origin-less requests through without emitting a preflight response. A no-Origin OPTIONS carrying that header therefore skipped the session-token check while never being handled as a real preflight. Require a nonempty Origin as well; add a regression test that the no-Origin variant stays 401.
df9dfae to
b5bdb96
Compare
Closes #59052
CORS preflight (OPTIONS) to
/api/*protected routes return 401 becausethe auth middleware runs before CORSMiddleware in Starlette's middleware
stack, and OPTIONS requests carry no session token by design.
This short-circuits OPTIONS in
auth_middleware()before the token check,so CORSMiddleware handles the preflight response with proper CORS headers.
Only change:
hermes_cli/web_server.py:auth_middleware()