fix(ui): CSRF guard must fail closed when Origin is absent
The state-changing-request guard only rejected a present-but-disallowed
Origin; a POST with no Origin header at all skipped the check entirely,
so /ui/n/{id}/approve (and every other POST/PUT/DELETE/PATCH route)
could be forged from a page that omits Origin. Now the guard requires
either an allowed Origin or a same-origin Referer, and refuses when
neither is present. Updated UI-flow tests to send an Origin header, as
a real browser does, instead of relying on the check being skipped.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0139vHnNpS2sLNELjrAHX5Cu
This commit is contained in:
1 parent
895829ea03
commit
59958f83ea
3 files changed
+80
-8
No files matched your search
+20
-2
@@ -89,8 +89,16 @@ def build_ui_app(svc: MemoryService) -> FastAPI: # noqa: C901 - one place, many
|
||||
is_open = path in OPEN_PATHS or path.startswith(OPEN_PREFIXES)
|
||||
|
||||
if request.method in ("POST", "PUT", "DELETE", "PATCH") and not is_open:
|
||||
origin = request.headers.get("origin")
|
||||
if origin is not None and not _origin_allowed(origin, settings, request):
|
||||
# Fail closed: a state-changing request must present an allowed Origin,
|
||||
# or — some browsers omit Origin on a same-origin POST — a Referer whose
|
||||
# origin is allowed. No header at all is refused, not waved through.
|
||||
# The session cookie is already SameSite=Lax, which stops it riding along
|
||||
# on a genuine cross-site form POST in a compliant browser; this check is
|
||||
# an independent second layer for clients that do not enforce SameSite
|
||||
# (older browsers, embedded webviews) or a browser bug, so it must not
|
||||
# itself degrade to "no Origin, so allow".
|
||||
candidate = request.headers.get("origin") or _origin_from_referer(request)
|
||||
if candidate is None or not _origin_allowed(candidate, settings, request):
|
||||
return JSONResponse({"error": "bad origin"}, status_code=403)
|
||||
|
||||
if not is_open:
|
||||
@@ -450,6 +458,16 @@ def build_ui_app(svc: MemoryService) -> FastAPI: # noqa: C901 - one place, many
|
||||
return app
|
||||
|
||||
|
||||
def _origin_from_referer(request: Request) -> str | None:
|
||||
referer = request.headers.get("referer")
|
||||
if not referer:
|
||||
return None
|
||||
parsed = urlparse(referer)
|
||||
if not parsed.scheme or not parsed.netloc:
|
||||
return None
|
||||
return f"{parsed.scheme}://{parsed.netloc}"
|
||||
|
||||
|
||||
def _origin_allowed(origin: str, settings: Any, request: Request) -> bool:
|
||||
allowed = set(settings.ui_allowed_origins)
|
||||
if settings.ui_base_url:
|
||||
|
||||
+57
-6
@@ -117,12 +117,8 @@ async def test_id_token_requires_exp_and_matching_nonce(auth_settings):
|
||||
with pytest.raises(AuthError, match="exp"):
|
||||
await oidc.claims({"id_token": token()}, "expected")
|
||||
with pytest.raises(AuthError, match="nonce"):
|
||||
await oidc.claims(
|
||||
{"id_token": token(exp=int(time.time()) + 300, nonce="wrong")}, "expected"
|
||||
)
|
||||
accepted = await oidc.claims(
|
||||
{"id_token": token(exp=int(time.time()) + 300)}, "expected"
|
||||
)
|
||||
await oidc.claims({"id_token": token(exp=int(time.time()) + 300, nonce="wrong")}, "expected")
|
||||
accepted = await oidc.claims({"id_token": token(exp=int(time.time()) + 300)}, "expected")
|
||||
assert accepted["sub"] == "u1"
|
||||
finally:
|
||||
await oidc.aclose()
|
||||
@@ -224,6 +220,61 @@ async def test_post_from_the_configured_origin_is_allowed(app_and_svc):
|
||||
assert svc.get_note(res["id"]).state == "active"
|
||||
|
||||
|
||||
async def test_post_without_origin_or_referer_is_refused(app_and_svc):
|
||||
"""A state-changing POST with neither header must fail closed (§10, §14.21).
|
||||
|
||||
SameSite=Lax on the session cookie already stops the cookie riding along on
|
||||
a genuine cross-site form POST in a compliant browser, but that is a client
|
||||
behaviour mdmem cannot verify from the server side. This check is an
|
||||
independent second layer for clients that do not enforce SameSite (old
|
||||
browsers, embedded webviews) or a browser bug — so "no Origin" must refuse,
|
||||
never quietly fall through to "allowed".
|
||||
"""
|
||||
app, svc = app_and_svc
|
||||
cookie = SessionCodec(svc.settings.session_secret, 12).issue(Principal("u1", "M", ""))
|
||||
res = await svc.write_note(title="T", body="b", type="note")
|
||||
async with _client(app) as c:
|
||||
r = await c.post(
|
||||
f"/ui/n/{res['id']}/approve",
|
||||
cookies={SESSION_COOKIE: cookie},
|
||||
data={},
|
||||
)
|
||||
assert r.status_code == 403
|
||||
assert svc.get_note(res["id"]).state == "inbox"
|
||||
|
||||
|
||||
async def test_post_with_a_same_origin_referer_and_no_origin_header_is_allowed(app_and_svc):
|
||||
"""No Origin header (some browsers omit it on same-origin POSTs) but a Referer
|
||||
that matches the configured origin must still be accepted."""
|
||||
app, svc = app_and_svc
|
||||
cookie = SessionCodec(svc.settings.session_secret, 12).issue(Principal("u1", "M", ""))
|
||||
res = await svc.write_note(title="T", body="b", type="note")
|
||||
async with _client(app) as c:
|
||||
r = await c.post(
|
||||
f"/ui/n/{res['id']}/approve",
|
||||
headers={"referer": "https://memory.cloud.traunseenet.com/ui/n/xyz/edit"},
|
||||
cookies={SESSION_COOKIE: cookie},
|
||||
data={},
|
||||
)
|
||||
assert r.status_code in (303, 302)
|
||||
assert svc.get_note(res["id"]).state == "active"
|
||||
|
||||
|
||||
async def test_post_with_a_foreign_referer_and_no_origin_is_refused(app_and_svc):
|
||||
app, svc = app_and_svc
|
||||
cookie = SessionCodec(svc.settings.session_secret, 12).issue(Principal("u1", "M", ""))
|
||||
res = await svc.write_note(title="T", body="b", type="note")
|
||||
async with _client(app) as c:
|
||||
r = await c.post(
|
||||
f"/ui/n/{res['id']}/approve",
|
||||
headers={"referer": "https://evil.example.com/attack"},
|
||||
cookies={SESSION_COOKIE: cookie},
|
||||
data={},
|
||||
)
|
||||
assert r.status_code == 403
|
||||
assert svc.get_note(res["id"]).state == "inbox"
|
||||
|
||||
|
||||
async def test_ui_refuses_to_start_without_a_session_secret(settings):
|
||||
svc = MemoryService.build(replace(settings, session_secret=""), embedder=FakeEmbedder(settings.embed_dim))
|
||||
try:
|
||||
|
||||
@@ -28,6 +28,9 @@ async def ui(settings):
|
||||
base_url="https://memory.test",
|
||||
cookies={SESSION_COOKIE: cookie},
|
||||
follow_redirects=False,
|
||||
# A real browser sends Origin on every state-changing fetch/XHR/form POST;
|
||||
# the CSRF guard now fails closed without it (§10, §14.21).
|
||||
headers={"origin": "https://memory.test"},
|
||||
)
|
||||
yield client, svc
|
||||
await client.aclose()
|
||||
|
||||
Reference in new issue
Block a user