From 5986c4ad8691d2ac955073b6d9cfd0ee2154fc5c Mon Sep 17 00:00:00 2001 From: gramps Date: Sat, 27 Jun 2026 15:20:02 -0700 Subject: [PATCH] fix: close two CSRF origin-check security gaps - Extend origin check to all /api/ requests (not just state-changing methods), closing the GET/HEAD/OPTIONS bypass that allowed cross-origin reads - origin_allowed() now returns False when both Origin and Referer headers are absent, preventing script-initiated requests from bypassing the check - Update AGENTS.md and README.md to document the changes --- AGENTS.md | 1 + README.md | 2 ++ app.py | 2 +- security.py | 2 +- tests/test_auth_capabilities.py | 4 ++-- tests/test_chat_streaming_and_memory_paths.py | 4 ++-- tests/test_error_envelopes.py | 2 +- tests/test_ip_allowlist.py | 2 +- tests/test_rate_and_payload_guardrails.py | 8 ++++---- tests/test_skills_framework.py | 4 ++-- 10 files changed, 17 insertions(+), 14 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7babf72..18b2f00 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -53,6 +53,7 @@ The upstream request includes `"logprobs": true`. `parse_llama_stream_chunk()` e - Guest session by default (`POST /api/auth/guest`), admin unlock via 4-digit PIN (`POST /api/auth/login`) - Admin required for PUT/DELETE/PATCH + all POST except allowlist (`/api/chat`, `/api/search`, `/api/auth/*`) - IP allowlist, rate limiting, origin checking, payload size limits — all enforced in `app.py` middleware +- Origin check applies to **all** `/api/` requests (not just state-changing methods); `origin_allowed()` returns `False` when both `Origin` and `Referer` headers are absent, closing CSRF read gap - `JARVISCHAT_ADMIN_PIN` env var required on first boot (or `JARVISCHAT_ALLOW_DEFAULT_PIN=true`) ### Database diff --git a/README.md b/README.md index 3a1ce3e..efe07da 100644 --- a/README.md +++ b/README.md @@ -17,6 +17,8 @@ Developer wiki: [docs/wiki/Home.md](docs/wiki/Home.md) - **Secure SSE protocol** — raw search results are no longer leaked in the SSE event stream - **FTS5 query safety** — operator keywords (`AND`, `OR`, `NOT`, `NEAR`) are double-quoted to prevent parse errors - **All 8 test files fixed** — rewired imports after the modular refactor; all 26 tests pass +- **Origin check extended to all API methods** — GET/HEAD/OPTIONS requests no longer bypass origin checking (was limited to POST/PUT/DELETE/PATCH) +- **Missing headers now rejected** — `origin_allowed()` returns `False` when both `Origin` and `Referer` are absent, closing the CSRF read gap for script-initiated requests ## Features diff --git a/app.py b/app.py index 2f75024..52bf80c 100644 --- a/app.py +++ b/app.py @@ -102,7 +102,7 @@ async def session_auth_middleware(request: Request, call_next): "/api/auth/heartbeat", "/api/auth/guest", } - if path.startswith("/api/") and is_state_changing(request.method): + if path.startswith("/api/"): if not origin_allowed(request): audit_event("origin_check", "denied", ip=ip, role="none", details=f"{request.method} {path}", warning=True) diff --git a/security.py b/security.py index 4b16970..135397f 100644 --- a/security.py +++ b/security.py @@ -156,7 +156,7 @@ def origin_allowed(request: Request) -> bool: parsed = urlparse(referer) ref_origin = f"{parsed.scheme}://{parsed.netloc}".rstrip("/") return ref_origin == expected_origin or ref_origin in TRUSTED_ORIGINS - return True + return False def is_state_changing(method: str) -> bool: diff --git a/tests/test_auth_capabilities.py b/tests/test_auth_capabilities.py index d32a7a8..c9dfbb9 100644 --- a/tests/test_auth_capabilities.py +++ b/tests/test_auth_capabilities.py @@ -22,7 +22,7 @@ def test_guest_read_only_admin_write_blocked(tmp_path: Path): guest = client.post("/api/auth/guest", headers={"Origin": "http://testserver"}) assert guest.status_code == 200 sid = guest.json()["session_id"] - headers = {"X-Session-ID": sid} + headers = {"X-Session-ID": sid, "Origin": "http://testserver"} read_resp = client.get("/api/memories", headers=headers) assert read_resp.status_code == 200 @@ -76,5 +76,5 @@ def test_logout_revokes_session(tmp_path: Path): logout = client.post("/api/auth/logout", headers=headers) assert logout.status_code == 200 - after = client.get("/api/memories", headers={"X-Session-ID": sid}) + after = client.get("/api/memories", headers={"X-Session-ID": sid, "Origin": "http://testserver"}) assert after.status_code == 401 diff --git a/tests/test_chat_streaming_and_memory_paths.py b/tests/test_chat_streaming_and_memory_paths.py index 9696958..53c52ea 100644 --- a/tests/test_chat_streaming_and_memory_paths.py +++ b/tests/test_chat_streaming_and_memory_paths.py @@ -172,7 +172,7 @@ def test_memory_command_paths_remember_and_forget(tmp_path: Path, monkeypatch): remember_events = parse_sse_payloads(remember_resp.text) assert any("Remembered" in p.get("token", "") for p in remember_events) - memories_after_add = client.get("/api/memories", headers={"X-Session-ID": sid}) + memories_after_add = client.get("/api/memories", headers={"X-Session-ID": sid, "Origin": "http://testserver"}) assert memories_after_add.status_code == 200 assert memories_after_add.json().get("count", 0) >= 1 @@ -188,6 +188,6 @@ def test_memory_command_paths_remember_and_forget(tmp_path: Path, monkeypatch): forget_events = parse_sse_payloads(forget_resp.text) assert any("Forgot" in p.get("token", "") for p in forget_events) - memories_after_forget = client.get("/api/memories", headers={"X-Session-ID": sid}) + memories_after_forget = client.get("/api/memories", headers={"X-Session-ID": sid, "Origin": "http://testserver"}) assert memories_after_forget.status_code == 200 assert memories_after_forget.json().get("count", 0) == 0 diff --git a/tests/test_error_envelopes.py b/tests/test_error_envelopes.py index 26fdb99..45d89e2 100644 --- a/tests/test_error_envelopes.py +++ b/tests/test_error_envelopes.py @@ -28,7 +28,7 @@ def test_unhandled_api_exception_returns_friendly_error_with_incident_key( sid = client.post("/api/auth/guest", headers={"Origin": "http://testserver"}).json()[ "session_id" ] - headers = {"X-Session-ID": sid} + headers = {"X-Session-ID": sid, "Origin": "http://testserver"} def boom(_topic=None): raise RuntimeError("super secret db internals") diff --git a/tests/test_ip_allowlist.py b/tests/test_ip_allowlist.py index 910f848..9be5d7c 100644 --- a/tests/test_ip_allowlist.py +++ b/tests/test_ip_allowlist.py @@ -40,5 +40,5 @@ def test_middleware_blocks_disallowed_ip(tmp_path: Path, monkeypatch): def test_middleware_allows_local_ip(tmp_path: Path, monkeypatch): monkeypatch.setattr(app, "get_client_ip", lambda _req: "192.168.50.109") with make_client(tmp_path) as client: - resp = client.post("/api/auth/guest") + resp = client.post("/api/auth/guest", headers={"Origin": "http://testserver"}) assert resp.status_code == 200 diff --git a/tests/test_rate_and_payload_guardrails.py b/tests/test_rate_and_payload_guardrails.py index 8984ef4..df73ca1 100644 --- a/tests/test_rate_and_payload_guardrails.py +++ b/tests/test_rate_and_payload_guardrails.py @@ -28,8 +28,8 @@ def test_stats_rate_limit_hits_429(tmp_path: Path): app.RATE_WINDOW_SECONDS = 60 try: with make_client(tmp_path) as client: - sid = client.post("/api/auth/guest").json()["session_id"] - headers = {"X-Session-ID": sid} + sid = client.post("/api/auth/guest", headers={"Origin": "http://testserver"}).json()["session_id"] + headers = {"X-Session-ID": sid, "Origin": "http://testserver"} r1 = client.get("/api/stats", headers=headers) r2 = client.get("/api/stats", headers=headers) @@ -56,7 +56,7 @@ def test_large_login_payload_rejected_413(tmp_path: Path): def test_chat_message_length_rejected_413(tmp_path: Path): with make_client(tmp_path) as client: - sid = client.post("/api/auth/guest").json()["session_id"] + sid = client.post("/api/auth/guest", headers={"Origin": "http://testserver"}).json()["session_id"] headers = {"X-Session-ID": sid, "Origin": "http://testserver"} message = "x" * (config.MAX_CHAT_MESSAGE_CHARS + 1) resp = client.post( @@ -69,7 +69,7 @@ def test_chat_message_length_rejected_413(tmp_path: Path): def test_search_query_length_rejected_413(tmp_path: Path): with make_client(tmp_path) as client: - sid = client.post("/api/auth/guest").json()["session_id"] + sid = client.post("/api/auth/guest", headers={"Origin": "http://testserver"}).json()["session_id"] headers = {"X-Session-ID": sid, "Origin": "http://testserver"} query = "q" * (config.MAX_SEARCH_QUERY_CHARS + 1) resp = client.post( diff --git a/tests/test_skills_framework.py b/tests/test_skills_framework.py index c6860a5..7eb993a 100644 --- a/tests/test_skills_framework.py +++ b/tests/test_skills_framework.py @@ -25,7 +25,7 @@ def test_guest_can_list_skills(tmp_path: Path): sid = client.post("/api/auth/guest", headers={"Origin": "http://testserver"}).json()[ "session_id" ] - resp = client.get("/api/skills", headers={"X-Session-ID": sid}) + resp = client.get("/api/skills", headers={"X-Session-ID": sid, "Origin": "http://testserver"}) assert resp.status_code == 200 payload = resp.json() assert payload["count"] >= 1 @@ -50,7 +50,7 @@ def test_admin_can_toggle_skill_enabled_state(tmp_path: Path): assert disable.status_code == 200 assert disable.json()["skill"]["enabled"] is False - active = client.get("/api/skills/active", headers={"X-Session-ID": sid}) + active = client.get("/api/skills/active", headers={"X-Session-ID": sid, "Origin": "http://testserver"}) assert active.status_code == 200 assert all(skill["key"] != "search.web" for skill in active.json()["skills"])