From 3c95502979b55901087ef8ad20cbb09d75585580 Mon Sep 17 00:00:00 2001
From: Nathan Esquenazi <nesquena@gmail.com>
Date: Fri, 3 Apr 2026 13:05:41 +0000
Subject: [PATCH] fix(auth): harden password_hash handling in settings API

Three security issues found during review:

1. password_hash exposed via GET /api/settings
   load_settings() returned all fields including the stored hash.
   Fix: strip password_hash from the response in routes.py.

2. password_hash directly settable via POST /api/settings
   'password_hash' was in _SETTINGS_ALLOWED_KEYS, so an attacker
   could POST {password_hash: 'X'} to hijack auth without knowing
   the current password.
   Fix: exclude password_hash from _SETTINGS_ALLOWED_KEYS.
   (Use _set_password for the legitimate hash-and-store path.)

3. Security headers missing from /api/auth/login and /api/auth/logout
   These endpoints built their responses manually (bypassing j()),
   so they omitted X-Content-Type-Options etc.
   Fix: call _security_headers() before end_headers() on both.

Tests updated: renamed test to assert key absent (not just None),
added new test verifying direct password_hash POST is blocked.
---
 api/config.py          |  2 +-
 api/routes.py          |  9 +++++++--
 tests/test_sprint19.py | 16 +++++++++++++---
 3 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/api/config.py b/api/config.py
index 0c77416..1f84f09 100644
--- a/api/config.py
+++ b/api/config.py
@@ -610,7 +610,7 @@ def load_settings() -> dict:
             pass
     return settings
 
-_SETTINGS_ALLOWED_KEYS = set(_SETTINGS_DEFAULTS.keys())
+_SETTINGS_ALLOWED_KEYS = set(_SETTINGS_DEFAULTS.keys()) - {'password_hash'}
 _SETTINGS_ENUM_VALUES = {
     'send_key': {'enter', 'ctrl+enter'},
 }
diff --git a/api/routes.py b/api/routes.py
index bb95de8..277ef30 100644
--- a/api/routes.py
+++ b/api/routes.py
@@ -19,7 +19,7 @@ from api.config import (
     IMAGE_EXTS, MD_EXTS, MIME_MAP, MAX_FILE_BYTES, MAX_UPLOAD_BYTES,
     CHAT_LOCK, load_settings, save_settings,
 )
-from api.helpers import require, bad, safe_resolve, j, t, read_body
+from api.helpers import require, bad, safe_resolve, j, t, read_body, _security_headers
 from api.models import (
     Session, get_session, new_session, all_sessions, title_from,
     _write_session_index, SESSION_INDEX_FILE,
@@ -139,7 +139,10 @@ def handle_get(handler, parsed):
         return j(handler, get_available_models())
 
     if parsed.path == '/api/settings':
-        return j(handler, load_settings())
+        settings = load_settings()
+        # Never expose the stored password hash to clients
+        settings.pop('password_hash', None)
+        return j(handler, settings)
 
     if parsed.path.startswith('/static/'):
         return _serve_static(handler, parsed)
@@ -475,6 +478,7 @@ def handle_post(handler, parsed):
         handler.send_response(200)
         handler.send_header('Content-Type', 'application/json')
         handler.send_header('Cache-Control', 'no-store')
+        _security_headers(handler)
         set_auth_cookie(handler, cookie_val)
         handler.end_headers()
         handler.wfile.write(json.dumps({'ok': True}).encode())
@@ -488,6 +492,7 @@ def handle_post(handler, parsed):
         handler.send_response(200)
         handler.send_header('Content-Type', 'application/json')
         handler.send_header('Cache-Control', 'no-store')
+        _security_headers(handler)
         clear_auth_cookie(handler)
         handler.end_headers()
         handler.wfile.write(json.dumps({'ok': True}).encode())
diff --git a/tests/test_sprint19.py b/tests/test_sprint19.py
index 6c17d5e..49440bd 100644
--- a/tests/test_sprint19.py
+++ b/tests/test_sprint19.py
@@ -88,11 +88,11 @@ def test_cache_control_no_store():
 
 # ── Settings password field ──────────────────────────────────────────────
 
-def test_settings_password_hash_default_null():
-    """Default settings should have password_hash as None."""
+def test_settings_password_hash_not_exposed():
+    """GET /api/settings must never expose the stored password hash."""
     d, status, _ = get("/api/settings")
     assert status == 200
-    assert d.get("password_hash") is None
+    assert "password_hash" not in d  # security: never send hash to client
 
 
 def test_settings_save_preserves_other_fields():
@@ -106,3 +106,13 @@ def test_settings_save_preserves_other_fields():
     updated, _, _ = get("/api/settings")
     assert "default_model" in updated
     assert "default_workspace" in updated
+
+
+def test_settings_password_hash_not_directly_settable():
+    """POST /api/settings with password_hash must not overwrite the stored hash."""
+    # Attempt to set a raw hash directly (attack vector)
+    post("/api/settings", {"password_hash": "deadbeef" * 8})
+    # Settings response must not expose it regardless
+    updated, status, _ = get("/api/settings")
+    assert status == 200
+    assert "password_hash" not in updated