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.
This commit is contained in:
@@ -610,7 +610,7 @@ def load_settings() -> dict:
|
|||||||
pass
|
pass
|
||||||
return settings
|
return settings
|
||||||
|
|
||||||
_SETTINGS_ALLOWED_KEYS = set(_SETTINGS_DEFAULTS.keys())
|
_SETTINGS_ALLOWED_KEYS = set(_SETTINGS_DEFAULTS.keys()) - {'password_hash'}
|
||||||
_SETTINGS_ENUM_VALUES = {
|
_SETTINGS_ENUM_VALUES = {
|
||||||
'send_key': {'enter', 'ctrl+enter'},
|
'send_key': {'enter', 'ctrl+enter'},
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -19,7 +19,7 @@ from api.config import (
|
|||||||
IMAGE_EXTS, MD_EXTS, MIME_MAP, MAX_FILE_BYTES, MAX_UPLOAD_BYTES,
|
IMAGE_EXTS, MD_EXTS, MIME_MAP, MAX_FILE_BYTES, MAX_UPLOAD_BYTES,
|
||||||
CHAT_LOCK, load_settings, save_settings,
|
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 (
|
from api.models import (
|
||||||
Session, get_session, new_session, all_sessions, title_from,
|
Session, get_session, new_session, all_sessions, title_from,
|
||||||
_write_session_index, SESSION_INDEX_FILE,
|
_write_session_index, SESSION_INDEX_FILE,
|
||||||
@@ -139,7 +139,10 @@ def handle_get(handler, parsed):
|
|||||||
return j(handler, get_available_models())
|
return j(handler, get_available_models())
|
||||||
|
|
||||||
if parsed.path == '/api/settings':
|
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/'):
|
if parsed.path.startswith('/static/'):
|
||||||
return _serve_static(handler, parsed)
|
return _serve_static(handler, parsed)
|
||||||
@@ -475,6 +478,7 @@ def handle_post(handler, parsed):
|
|||||||
handler.send_response(200)
|
handler.send_response(200)
|
||||||
handler.send_header('Content-Type', 'application/json')
|
handler.send_header('Content-Type', 'application/json')
|
||||||
handler.send_header('Cache-Control', 'no-store')
|
handler.send_header('Cache-Control', 'no-store')
|
||||||
|
_security_headers(handler)
|
||||||
set_auth_cookie(handler, cookie_val)
|
set_auth_cookie(handler, cookie_val)
|
||||||
handler.end_headers()
|
handler.end_headers()
|
||||||
handler.wfile.write(json.dumps({'ok': True}).encode())
|
handler.wfile.write(json.dumps({'ok': True}).encode())
|
||||||
@@ -488,6 +492,7 @@ def handle_post(handler, parsed):
|
|||||||
handler.send_response(200)
|
handler.send_response(200)
|
||||||
handler.send_header('Content-Type', 'application/json')
|
handler.send_header('Content-Type', 'application/json')
|
||||||
handler.send_header('Cache-Control', 'no-store')
|
handler.send_header('Cache-Control', 'no-store')
|
||||||
|
_security_headers(handler)
|
||||||
clear_auth_cookie(handler)
|
clear_auth_cookie(handler)
|
||||||
handler.end_headers()
|
handler.end_headers()
|
||||||
handler.wfile.write(json.dumps({'ok': True}).encode())
|
handler.wfile.write(json.dumps({'ok': True}).encode())
|
||||||
|
|||||||
@@ -88,11 +88,11 @@ def test_cache_control_no_store():
|
|||||||
|
|
||||||
# ── Settings password field ──────────────────────────────────────────────
|
# ── Settings password field ──────────────────────────────────────────────
|
||||||
|
|
||||||
def test_settings_password_hash_default_null():
|
def test_settings_password_hash_not_exposed():
|
||||||
"""Default settings should have password_hash as None."""
|
"""GET /api/settings must never expose the stored password hash."""
|
||||||
d, status, _ = get("/api/settings")
|
d, status, _ = get("/api/settings")
|
||||||
assert status == 200
|
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():
|
def test_settings_save_preserves_other_fields():
|
||||||
@@ -106,3 +106,13 @@ def test_settings_save_preserves_other_fields():
|
|||||||
updated, _, _ = get("/api/settings")
|
updated, _, _ = get("/api/settings")
|
||||||
assert "default_model" in updated
|
assert "default_model" in updated
|
||||||
assert "default_workspace" 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
|
||||||
|
|||||||
Reference in New Issue
Block a user