refactor(#5534): move CORS preflight header logic into api/routes (keep server.py thin)
The initial fix added ~16 lines to server.py, tripping test_server_py_under_750_lines (server.py must stay a thin dispatcher; logic belongs in api/). Moved the whole preflight-response into apply_cors_preflight_headers(handler) in api/routes.py; server.py's do_OPTIONS is now a 4-line dispatcher. server.py 764 -> 746 lines. Same security behavior (same-origin/allowlist echo, Vary: Origin, no wildcard, header-less denial); test rewritten to the header-emitting API + added Vary and no-header-on-denial assertions and a routes.py wildcard regression guard.
This commit is contained in:
1 parent
4e8978f04c
commit
8da96a84d3
3 files changed
+66
-36
No files matched your search
+17
-11
@@ -5127,20 +5127,26 @@ def _check_same_origin_browser_request(handler, *, require_provenance: bool = Fa
|
|||||||
return True
|
return True
|
||||||
|
|
||||||
|
|
||||||
def preflight_allow_origin(handler) -> str:
|
def apply_cors_preflight_headers(handler) -> None:
|
||||||
"""CORS preflight: return the Origin value to echo back, or '' to omit.
|
"""Emit CORS preflight headers on ``handler`` for a same-origin/allowlisted
|
||||||
|
request; emit nothing for a disallowed origin (browser treats the header-less
|
||||||
|
200 as a preflight denial).
|
||||||
|
|
||||||
Echo the request Origin only when it is same-origin or explicitly
|
Echoes the request Origin only when it is same-origin or explicitly
|
||||||
allowlisted via HERMES_WEBUI_ALLOWED_ORIGINS — the exact policy the CSRF
|
allowlisted via HERMES_WEBUI_ALLOWED_ORIGINS — the exact policy the CSRF gate
|
||||||
gate enforces for real requests. Reuses _check_same_origin_browser_request
|
enforces for real requests. Reuses _check_same_origin_browser_request so the
|
||||||
so the preflight can never advertise wider access (`*`) than an actual
|
preflight can never advertise wider access (`*`) than an actual request would
|
||||||
request would be granted. A wildcard here would let any site read
|
be granted. A wildcard here would let any site read authenticated responses
|
||||||
authenticated responses on a deployment with no password set.
|
on a deployment with no password set. Kept in api/ so server.py stays a thin
|
||||||
|
dispatcher.
|
||||||
"""
|
"""
|
||||||
origin = handler.headers.get("Origin", "").strip()
|
origin = handler.headers.get("Origin", "").strip()
|
||||||
if not origin:
|
if not origin or not _check_same_origin_browser_request(handler):
|
||||||
return ""
|
return
|
||||||
return origin if _check_same_origin_browser_request(handler) else ""
|
handler.send_header("Access-Control-Allow-Origin", origin)
|
||||||
|
handler.send_header("Vary", "Origin")
|
||||||
|
handler.send_header("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS")
|
||||||
|
handler.send_header("Access-Control-Allow-Headers", "Content-Type, Authorization")
|
||||||
|
|
||||||
|
|
||||||
def _csrf_exempt_path(path: str) -> bool:
|
def _csrf_exempt_path(path: str) -> bool:
|
||||||
|
|||||||
@@ -109,14 +109,7 @@ from api.helpers import (
|
|||||||
_CLIENT_DISCONNECT_ERRORS,
|
_CLIENT_DISCONNECT_ERRORS,
|
||||||
)
|
)
|
||||||
from api.profiles import set_request_profile, clear_request_profile
|
from api.profiles import set_request_profile, clear_request_profile
|
||||||
from api.routes import (
|
from api.routes import handle_delete, handle_get, handle_patch, handle_post, handle_put, apply_cors_preflight_headers
|
||||||
handle_delete,
|
|
||||||
handle_get,
|
|
||||||
handle_patch,
|
|
||||||
handle_post,
|
|
||||||
handle_put,
|
|
||||||
preflight_allow_origin,
|
|
||||||
)
|
|
||||||
from api.startup import auto_install_agent_deps, fix_credential_permissions
|
from api.startup import auto_install_agent_deps, fix_credential_permissions
|
||||||
from api.updates import WEBUI_VERSION
|
from api.updates import WEBUI_VERSION
|
||||||
from api.crash_visibility import install_crash_visibility
|
from api.crash_visibility import install_crash_visibility
|
||||||
@@ -442,21 +435,10 @@ class Handler(BaseHTTPRequestHandler):
|
|||||||
self._handle_write(handle_patch)
|
self._handle_write(handle_patch)
|
||||||
|
|
||||||
def do_OPTIONS(self) -> None:
|
def do_OPTIONS(self) -> None:
|
||||||
"""Handle CORS preflight requests.
|
"""Handle CORS preflight requests (headers emitted by api.routes)."""
|
||||||
|
|
||||||
Echo the request Origin only when it is same-origin or allowlisted
|
|
||||||
via HERMES_WEBUI_ALLOWED_ORIGINS (same policy as the CSRF gate) —
|
|
||||||
never advertise `*`. A disallowed origin gets a 200 with no CORS
|
|
||||||
headers, which the browser treats as a preflight denial.
|
|
||||||
"""
|
|
||||||
self._req_t0 = time.time()
|
self._req_t0 = time.time()
|
||||||
allow_origin = preflight_allow_origin(self)
|
|
||||||
self.send_response(200)
|
self.send_response(200)
|
||||||
if allow_origin:
|
apply_cors_preflight_headers(self)
|
||||||
self.send_header("Access-Control-Allow-Origin", allow_origin)
|
|
||||||
self.send_header("Vary", "Origin")
|
|
||||||
self.send_header("Access-Control-Allow-Methods", "GET, POST, PUT, PATCH, DELETE, OPTIONS")
|
|
||||||
self.send_header("Access-Control-Allow-Headers", "Content-Type, Authorization")
|
|
||||||
self.end_headers()
|
self.end_headers()
|
||||||
|
|
||||||
def do_DELETE(self) -> None:
|
def do_DELETE(self) -> None:
|
||||||
|
|||||||
@@ -8,15 +8,35 @@ the same policy enforced for real requests.
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
from pathlib import Path
|
from pathlib import Path
|
||||||
from types import SimpleNamespace
|
|
||||||
|
|
||||||
from api.routes import preflight_allow_origin
|
from api.routes import apply_cors_preflight_headers
|
||||||
|
|
||||||
ROOT = Path(__file__).resolve().parent.parent
|
ROOT = Path(__file__).resolve().parent.parent
|
||||||
|
|
||||||
|
|
||||||
|
class _FakeHandler:
|
||||||
|
"""Captures send_header() calls so tests can assert emitted CORS headers."""
|
||||||
|
|
||||||
|
def __init__(self, headers):
|
||||||
|
self.headers = headers
|
||||||
|
self.sent = {}
|
||||||
|
|
||||||
|
def send_header(self, key, value):
|
||||||
|
self.sent[key] = value
|
||||||
|
|
||||||
|
|
||||||
def _preflight(headers):
|
def _preflight(headers):
|
||||||
return preflight_allow_origin(SimpleNamespace(headers=headers))
|
"""Return the Access-Control-Allow-Origin value the preflight would emit,
|
||||||
|
or '' when no CORS headers are emitted (disallowed / no-origin)."""
|
||||||
|
h = _FakeHandler(headers)
|
||||||
|
apply_cors_preflight_headers(h)
|
||||||
|
return h.sent.get("Access-Control-Allow-Origin", "")
|
||||||
|
|
||||||
|
|
||||||
|
def _preflight_headers(headers):
|
||||||
|
h = _FakeHandler(headers)
|
||||||
|
apply_cors_preflight_headers(h)
|
||||||
|
return h.sent
|
||||||
|
|
||||||
|
|
||||||
class TestPreflightAllowOrigin:
|
class TestPreflightAllowOrigin:
|
||||||
@@ -92,5 +112,27 @@ class TestServerNoWildcard:
|
|||||||
def test_server_source_has_no_wildcard_allow_origin(self):
|
def test_server_source_has_no_wildcard_allow_origin(self):
|
||||||
"""Regression guard: the literal `*` Allow-Origin must not come back."""
|
"""Regression guard: the literal `*` Allow-Origin must not come back."""
|
||||||
src = (ROOT / "server.py").read_text(encoding="utf-8")
|
src = (ROOT / "server.py").read_text(encoding="utf-8")
|
||||||
|
routes_src = (ROOT / "api" / "routes.py").read_text(encoding="utf-8")
|
||||||
assert '"Access-Control-Allow-Origin", "*"' not in src
|
assert '"Access-Control-Allow-Origin", "*"' not in src
|
||||||
assert "preflight_allow_origin" in src
|
assert '"Access-Control-Allow-Origin", "*"' not in routes_src
|
||||||
|
# server.py stays a thin dispatcher: the preflight logic lives in api/routes.
|
||||||
|
assert "apply_cors_preflight_headers" in src
|
||||||
|
assert "def apply_cors_preflight_headers" in routes_src
|
||||||
|
|
||||||
|
def test_allowed_preflight_sets_vary_origin(self):
|
||||||
|
"""An allowed preflight must set Vary: Origin (correct for caching an
|
||||||
|
origin-conditional response)."""
|
||||||
|
sent = _preflight_headers({
|
||||||
|
"Origin": "http://127.0.0.1:8787",
|
||||||
|
"Host": "127.0.0.1:8787",
|
||||||
|
})
|
||||||
|
assert sent.get("Vary") == "Origin"
|
||||||
|
assert sent.get("Access-Control-Allow-Origin") == "http://127.0.0.1:8787"
|
||||||
|
|
||||||
|
def test_denied_preflight_emits_no_cors_headers(self):
|
||||||
|
"""A disallowed origin gets zero CORS headers (browser preflight denial)."""
|
||||||
|
sent = _preflight_headers({
|
||||||
|
"Origin": "http://evil.com",
|
||||||
|
"Host": "127.0.0.1:8787",
|
||||||
|
})
|
||||||
|
assert sent == {}
|
||||||
Reference in new issue
Block a user