diff --git a/routes/auth_routes.py b/routes/auth_routes.py index 69ca3be56..1042437ac 100644 --- a/routes/auth_routes.py +++ b/routes/auth_routes.py @@ -15,6 +15,7 @@ from pathlib import Path from core.atomic_io import atomic_write_json, atomic_write_text from core.auth import AuthManager, RESERVED_USERNAMES, SetAdminResult, TOKEN_TTL from src.constants import DEEP_RESEARCH_DIR, MEMORY_FILE, PASSWORD_MIN_LENGTH, SKILLS_DIR +from src.auth_helpers import _auth_disabled from src.rate_limiter import RateLimiter from src.settings_scrub import scrub_settings from src.settings import ( @@ -129,6 +130,21 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: token = request.cookies.get(SESSION_COOKIE) return auth_manager.get_username_for_token(token) + def _require_admin_user(request: Request) -> Optional[str]: + """Resolve the current user, enforcing admin access. + + Honors AUTH_ENABLED=false the same way core.middleware.require_admin + does: when the operator has explicitly disabled auth (e.g. behind a + reverse proxy that handles login), there is no session user, and + admin-gated app configuration must stay reachable. + """ + user = _get_current_user(request) + if _auth_disabled(): + return user + if not user or not auth_manager.is_admin(user): + raise HTTPException(403, "Admin only") + return user + @router.post("/setup") async def first_run_setup(body: SetupRequest, request: Request): """Create initial admin account. Only works if no accounts exist.""" @@ -718,9 +734,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: @router.post("/features") async def set_features(request: Request): """Admin only: update feature toggles.""" - user = _get_current_user(request) - if not user or not auth_manager.is_admin(user): - raise HTTPException(403, "Admin only") + _require_admin_user(request) body = await request.json() current = _load_features() for key in current: @@ -738,16 +752,14 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: for keybinds + TTS prefs, so it stays callable without admin.""" user = _get_current_user(request) settings = without_retired_settings(_load_settings()) - if user and auth_manager.is_admin(user): + if _auth_disabled() or (user and auth_manager.is_admin(user)): return settings return scrub_settings(settings) @router.post("/settings") async def set_settings(request: Request): """Admin only: update app settings.""" - user = _get_current_user(request) - if not user or not auth_manager.is_admin(user): - raise HTTPException(403, "Admin only") + _require_admin_user(request) body = await request.json() current = _load_settings() # Per-key validation for numeric settings: coerce to int and clamp to a @@ -780,9 +792,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: file: UploadFile = File(...), ): """Infer the general prose style from one user-supplied document.""" - user = _get_current_user(request) - if not user or not auth_manager.is_admin(user): - raise HTTPException(403, "Admin only") + user = _require_admin_user(request) filename = Path(file.filename or "sample.txt").name suffix = Path(filename).suffix.lower() allowed = { @@ -861,9 +871,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: @router.get("/integrations") async def list_integrations_route(request: Request): """List all integrations (admin only, keys masked).""" - user = _get_current_user(request) - if not user or not auth_manager.is_admin(user): - raise HTTPException(403, "Admin only") + _require_admin_user(request) items = load_integrations() # Mask API keys for frontend display safe = [mask_integration_secret(item) for item in items] @@ -877,9 +885,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: @router.post("/integrations") async def create_integration(request: Request): """Create a new integration (admin only).""" - user = _get_current_user(request) - if not user or not auth_manager.is_admin(user): - raise HTTPException(403, "Admin only") + _require_admin_user(request) body = await request.json() item = add_integration(body) return {"ok": True, "integration": mask_integration_secret(item)} @@ -887,9 +893,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: @router.put("/integrations/{integration_id}") async def update_integration_route(integration_id: str, request: Request): """Update an existing integration (admin only).""" - user = _get_current_user(request) - if not user or not auth_manager.is_admin(user): - raise HTTPException(403, "Admin only") + _require_admin_user(request) body = await request.json() item = update_integration(integration_id, body) if not item: @@ -899,9 +903,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: @router.delete("/integrations/{integration_id}") async def delete_integration_route(integration_id: str, request: Request): """Delete an integration (admin only).""" - user = _get_current_user(request) - if not user or not auth_manager.is_admin(user): - raise HTTPException(403, "Admin only") + _require_admin_user(request) ok = delete_integration(integration_id) if not ok: raise HTTPException(404, "Integration not found") @@ -910,9 +912,7 @@ def setup_auth_routes(auth_manager: AuthManager) -> APIRouter: @router.post("/integrations/{integration_id}/test") async def test_integration_route(integration_id: str, request: Request): """Test connectivity to an integration (admin only).""" - user = _get_current_user(request) - if not user or not auth_manager.is_admin(user): - raise HTTPException(403, "Admin only") + _require_admin_user(request) integ = get_integration(integration_id) if not integ: raise HTTPException(404, "Integration not found") diff --git a/static/js/settings.js b/static/js/settings.js index dffd466c7..b63b28b48 100644 --- a/static/js/settings.js +++ b/static/js/settings.js @@ -1676,6 +1676,7 @@ async function initShortcuts() { if (uiModule && uiModule.showToast) uiModule.showToast('Shortcut saved'); } catch (e) { console.error('Failed to save keybinds:', e); + if (uiModule && uiModule.showToast) uiModule.showToast('Failed to save shortcut'); } } diff --git a/tests/test_admin_routes_auth_disabled.py b/tests/test_admin_routes_auth_disabled.py new file mode 100644 index 000000000..ae27fe4e6 --- /dev/null +++ b/tests/test_admin_routes_auth_disabled.py @@ -0,0 +1,331 @@ +"""Admin-gated app-config routes must honor AUTH_ENABLED=false. + +Regression test for the inconsistency between core.middleware.require_admin +(which allows requests when the operator disabled auth) and the inline admin +checks in routes/auth_routes.py (which returned 403 unconditionally, so +settings changes were silently lost behind reverse-proxy auth setups). +""" +import pytest +from core.auth import SetAdminResult + + +class _StoreDict(dict): + """Dict-like settings store that also holds features and integrations.""" + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self.features = {"web_search": True, "web_fetch": True, "memory": True} + self.integrations = { + "int-1": { + "id": "int-1", + "name": "miniflux-sync", + "preset": "miniflux", + "api_key": "sk-secret-key-123", + "base_url": "http://127.0.0.1:8080", + } + } + + +@pytest.fixture +def client(tmp_path, monkeypatch): + fastapi = pytest.importorskip("fastapi") + from fastapi.testclient import TestClient + from routes import auth_routes + + stored = _StoreDict() + monkeypatch.setattr(auth_routes, "_load_settings", lambda: dict(stored)) + monkeypatch.setattr(auth_routes, "_save_settings", stored.update) + monkeypatch.setattr(auth_routes, "_load_features", lambda: dict(stored.features)) + monkeypatch.setattr(auth_routes, "_save_features", stored.features.update) + monkeypatch.setattr(auth_routes, "migrate_from_settings", lambda: None) + + monkeypatch.setattr(auth_routes, "load_integrations", lambda: list(stored.integrations.values())) + monkeypatch.setattr(auth_routes, "get_integration", lambda iid: stored.integrations.get(iid)) + + def _add_int(body): + item = dict(body, id=f"int-{len(stored.integrations) + 1}") + stored.integrations[item["id"]] = item + return item + + def _update_int(iid, body): + if iid not in stored.integrations: + return None + stored.integrations[iid].update(body) + return stored.integrations[iid] + + def _delete_int(iid): + return stored.integrations.pop(iid, None) is not None + + async def _test_call(iid, method, path): + return {"exit_code": 0, "message": "Connection successful"} + + monkeypatch.setattr(auth_routes, "add_integration", _add_int) + monkeypatch.setattr(auth_routes, "update_integration", _update_int) + monkeypatch.setattr(auth_routes, "delete_integration", _delete_int) + monkeypatch.setattr(auth_routes, "execute_api_call", _test_call) + + class _AuthManager: + is_configured = True + signup_enabled = False + + def get_username_for_token(self, token): + if token in ("session-token", "admin-token"): + return "admin" + if token == "user-token": + return "user" + return None + + def is_admin(self, user): + return user == "admin" + + def list_users(self): + return [{"username": "admin", "is_admin": True}, {"username": "user", "is_admin": False}] + + def create_user(self, username, password, is_admin=False): + return True + + def set_privileges(self, username, privs): + return True + + def set_admin(self, username, is_admin, requesting_user): + return SetAdminResult.SUCCESS + + def delete_user(self, username, requesting_user): + return True + + app = fastapi.FastAPI() + app.include_router(auth_routes.setup_auth_routes(_AuthManager())) + return TestClient(app), stored + + +def test_set_settings_allowed_when_auth_disabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "false") + c, stored = client + r = c.post("/api/auth/settings", json={"default_model": "some-model"}) + assert r.status_code == 200 + assert stored.get("default_model") == "some-model" + + +def test_get_settings_unscrubbed_when_auth_disabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "false") + c, stored = client + stored["brave_api_key"] = "sk-secret" + r = c.get("/api/auth/settings") + assert r.status_code == 200 + assert r.json()["brave_api_key"] == "sk-secret" + + +def test_set_settings_still_403_when_auth_enabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "true") + c, _ = client + r = c.post("/api/auth/settings", json={"default_model": "x"}) + assert r.status_code == 403 + + +def test_set_settings_admin_session_still_works(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "true") + c, stored = client + c.cookies.set("odysseus_session", "session-token") + r = c.post("/api/auth/settings", json={"default_model": "admin-model"}) + assert r.status_code == 200 + assert stored.get("default_model") == "admin-model" + + +def test_settings_numeric_validation_and_clamping_when_auth_disabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "false") + c, stored = client + # Clamp out-of-range integer values + r = c.post("/api/auth/settings", json={"agent_max_rounds": 999, "agent_max_tool_calls": 5000}) + assert r.status_code == 200 + assert stored["agent_max_rounds"] == 200 + assert stored["agent_max_tool_calls"] == 1000 + + # Non-integer must raise 400 + r = c.post("/api/auth/settings", json={"agent_max_rounds": "not-an-int"}) + assert r.status_code == 400 + + # Unknown settings keys are not saved + r = c.post("/api/auth/settings", json={"unsupported_key_xyz": "value"}) + assert r.status_code == 200 + assert "unsupported_key_xyz" not in stored + + +def test_features_accessible_and_persisted_when_auth_disabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "false") + c, stored = client + # Public GET + r = c.get("/api/auth/features") + assert r.status_code == 200 + assert r.json().get("web_search") is True + + # POST allowed without login + r = c.post("/api/auth/features", json={"web_search": False}) + assert r.status_code == 200 + assert stored.features["web_search"] is False + + +def test_integrations_lifecycle_when_auth_disabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "false") + c, stored = client + + # List: keys masked + r = c.get("/api/auth/integrations") + assert r.status_code == 200 + items = r.json().get("integrations", []) + assert len(items) == 1 + assert items[0]["api_key"] != "sk-secret-key-123" + + # Create + r = c.post("/api/auth/integrations", json={"name": "new-srv", "preset": "custom", "api_key": "raw-key"}) + assert r.status_code == 200 + new_id = r.json()["integration"]["id"] + assert new_id in stored.integrations + + # Update + r = c.put(f"/api/auth/integrations/{new_id}", json={"name": "updated-srv"}) + assert r.status_code == 200 + assert stored.integrations[new_id]["name"] == "updated-srv" + + # Test + r = c.post(f"/api/auth/integrations/{new_id}/test") + assert r.status_code == 200 + + # Delete + r = c.delete(f"/api/auth/integrations/{new_id}") + assert r.status_code == 200 + assert new_id not in stored.integrations + + # 404 for missing item + assert c.put("/api/auth/integrations/nonexistent", json={"name": "x"}).status_code == 404 + assert c.delete("/api/auth/integrations/nonexistent").status_code == 404 + assert c.post("/api/auth/integrations/nonexistent/test").status_code == 404 + + +def test_user_management_fails_closed_when_auth_disabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "false") + c, _ = client + + assert c.get("/api/auth/users").status_code == 403 + assert c.post("/api/auth/users", json={"username": "hacker", "password": "password123", "is_admin": True}).status_code == 403 + assert c.put("/api/auth/users/target/privileges", json={"can_chat": False}).status_code == 403 + assert c.put("/api/auth/users/target/rename", json={"username": "newtarget"}).status_code == 403 + assert c.put("/api/auth/users/target/admin", json={"is_admin": True}).status_code == 403 + assert c.put("/api/auth/users/target/password", json={"new_password": "password123"}).status_code == 403 + assert c.put("/api/auth/open-signup", json={"enabled": True}).status_code == 403 + assert c.request("DELETE", "/api/auth/users", json={"username": "target"}).status_code == 403 + + +def test_get_settings_scrubbed_for_unauthenticated_and_non_admin_when_auth_enabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "true") + c, stored = client + stored["brave_api_key"] = "sk-secret-brave" + stored["default_model"] = "gpt-4o" + + # Unauthenticated caller gets scrubbed settings + r = c.get("/api/auth/settings") + assert r.status_code == 200 + data = r.json() + assert data["brave_api_key"] == "" + assert data["default_model"] == "gpt-4o" + + # Non-admin authenticated user gets scrubbed settings + c.cookies.set("odysseus_session", "user-token") + r = c.get("/api/auth/settings") + assert r.status_code == 200 + data = r.json() + assert data["brave_api_key"] == "" + assert data["default_model"] == "gpt-4o" + + # Admin caller gets full unscrubbed settings + c.cookies.set("odysseus_session", "session-token") + r = c.get("/api/auth/settings") + assert r.status_code == 200 + data = r.json() + assert data["brave_api_key"] == "sk-secret-brave" + assert data["default_model"] == "gpt-4o" + + +def test_set_settings_403_for_authenticated_non_admin(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "true") + c, stored = client + c.cookies.set("odysseus_session", "user-token") + r = c.post("/api/auth/settings", json={"default_model": "user-choice"}) + assert r.status_code == 403 + assert stored.get("default_model") != "user-choice" + + +def test_features_gated_when_auth_enabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "true") + c, stored = client + + # Unauthenticated + assert c.post("/api/auth/features", json={"web_search": False}).status_code == 403 + + # Non-admin + c.cookies.set("odysseus_session", "user-token") + assert c.post("/api/auth/features", json={"web_search": False}).status_code == 403 + assert stored.features["web_search"] is True + + # Admin + c.cookies.set("odysseus_session", "session-token") + r = c.post("/api/auth/features", json={"web_search": False}) + assert r.status_code == 200 + assert stored.features["web_search"] is False + + +def test_integrations_gated_when_auth_enabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "true") + c, stored = client + + # Unauthenticated + assert c.get("/api/auth/integrations").status_code == 403 + assert c.post("/api/auth/integrations", json={"name": "a"}).status_code == 403 + + # Non-admin + c.cookies.set("odysseus_session", "user-token") + assert c.get("/api/auth/integrations").status_code == 403 + assert c.post("/api/auth/integrations", json={"name": "a"}).status_code == 403 + assert c.put("/api/auth/integrations/int-1", json={"name": "b"}).status_code == 403 + assert c.delete("/api/auth/integrations/int-1").status_code == 403 + assert c.post("/api/auth/integrations/int-1/test").status_code == 403 + + # Admin + c.cookies.set("odysseus_session", "session-token") + assert c.get("/api/auth/integrations").status_code == 200 + assert c.post("/api/auth/integrations", json={"name": "new", "preset": "custom"}).status_code == 200 + + +def test_user_management_requires_admin_when_auth_enabled(client, monkeypatch): + monkeypatch.setenv("AUTH_ENABLED", "true") + c, _ = client + + # Unauthenticated + assert c.get("/api/auth/users").status_code == 403 + assert c.post("/api/auth/users", json={"username": "a", "password": "b"}).status_code == 403 + assert c.put("/api/auth/users/target/password", json={"new_password": "password123"}).status_code == 403 + + # Non-admin + c.cookies.set("odysseus_session", "user-token") + assert c.get("/api/auth/users").status_code == 403 + assert c.put("/api/auth/users/target/password", json={"new_password": "password123"}).status_code == 403 + + # Admin + c.cookies.set("odysseus_session", "session-token") + assert c.get("/api/auth/users").status_code == 200 + + +def test_document_style_extract_gating(client, monkeypatch): + c, _ = client + dummy_file = {"file": ("sample.txt", b"dummy prose content")} + + # When auth enabled: unauthenticated is 403, non-admin is 403 + monkeypatch.setenv("AUTH_ENABLED", "true") + assert c.post("/api/auth/settings/document-style/extract", files=dummy_file).status_code == 403 + + c.cookies.set("odysseus_session", "user-token") + assert c.post("/api/auth/settings/document-style/extract", files=dummy_file).status_code == 403 + + # When auth disabled: unauthenticated passes admin gate (400 for sample length < 80) + monkeypatch.setenv("AUTH_ENABLED", "false") + c.cookies.clear() + assert c.post("/api/auth/settings/document-style/extract", files=dummy_file).status_code == 400 diff --git a/tests/test_settings_save_response_ok_js.py b/tests/test_settings_save_response_ok_js.py new file mode 100644 index 000000000..5bc5eff05 --- /dev/null +++ b/tests/test_settings_save_response_ok_js.py @@ -0,0 +1,120 @@ +"""Frontend regression tests: verify settings save handlers check HTTP response status. + +Locks in that static/js/settings.js verifies response.ok before displaying success, +preventing 401, 403, 400, and 500 responses from reporting false success. +""" +import re +import shutil +import subprocess +from pathlib import Path + +import pytest + +_REPO = Path(__file__).resolve().parent.parent +_SETTINGS_JS = _REPO / "static" / "js" / "settings.js" +_SETTINGS_DIR = _REPO / "static" / "js" / "settings" +_SETTINGS_API_JS = _SETTINGS_DIR / "api.js" +_HAS_NODE = shutil.which("node") is not None + + +@pytest.mark.skipif(not _HAS_NODE, reason="node binary not on PATH") +def test_settings_js_syntax(): + """Verify static/js/settings.js and settings/*.js modules are syntactically valid JavaScript.""" + js_files = [_SETTINGS_JS] + list(_SETTINGS_DIR.glob("*.js")) + for js_file in js_files: + proc = subprocess.run( + ["node", "--check", str(js_file)], + capture_output=True, + text=True, + cwd=str(_REPO), + ) + assert proc.returncode == 0, f"JS syntax error in {js_file.name}:\n{proc.stderr}" + + +def test_post_settings_helper_verifies_response_ok(): + """Centralized postSettings in settings/api.js must verify response.ok and throw on failure.""" + assert _SETTINGS_API_JS.exists(), f"Expected {_SETTINGS_API_JS} to exist" + api_content = _SETTINGS_API_JS.read_text(encoding="utf-8") + + # Verify fetch to /api/auth/settings with POST method + match = re.search(r"fetch\(['\"]/api/auth/settings['\"],\s*\{\s*method:\s*['\"]POST['\"]", api_content) + assert match is not None, "Expected POST fetch to /api/auth/settings in settings/api.js" + + # Verify .ok is checked within 300 characters of fetch + window = api_content[match.start(): match.start() + 300] + assert ".ok" in window, f"postSettings must check response.ok:\n{window}" + + # Verify that it throws an error when !response.ok + assert "throw error" in api_content or "throw new Error" in api_content, ( + "postSettings must throw when response is not ok" + ) + + +def test_all_settings_save_handlers_route_through_post_settings(): + """All settings save handlers in settings.js and settings/*.js must route saves via postSettings.""" + settings_content = _SETTINGS_JS.read_text(encoding="utf-8") + module_contents = {p.name: p.read_text(encoding="utf-8") for p in _SETTINGS_DIR.glob("*.js")} + + # Assert no unverified direct POST fetch to /api/auth/settings in settings.js + direct_posts = list(re.finditer(r"fetch\(['\"]/api/auth/settings['\"],\s*\{\s*method:\s*['\"]POST['\"]", settings_content)) + assert len(direct_posts) == 0, ( + f"Found direct POST fetch to /api/auth/settings in settings.js; all writes must route through _postSettings" + ) + + # Count _postSettings invocations across settings.js and modules + post_settings_calls = len(re.findall(r"_postSettings\(", settings_content)) + for mod_name, mod_text in module_contents.items(): + if mod_name != "api.js": + post_settings_calls += len(re.findall(r"_postSettings\(", mod_text)) + + assert post_settings_calls >= 15, ( + f"Expected at least 15 _postSettings calls across settings modules, found {post_settings_calls}" + ) + + +@pytest.mark.skipif(not _HAS_NODE, reason="node binary not on PATH") +def test_failed_save_shows_failure_in_simulated_dom(): + """Verify that when fetch returns !res.ok, save logic catches and flags error.""" + js = """ + let msgText = ''; + let msgColor = ''; + const msg = { + set textContent(v) { msgText = v; }, + get textContent() { return msgText; }, + style: { set color(v) { msgColor = v; }, get color() { return msgColor; } } + }; + + async function save(fetchImpl) { + try { + var res = await fetchImpl('/api/auth/settings', { method: 'POST' }); + if (!res.ok) throw new Error('HTTP ' + res.status); + msg.textContent = 'Saved'; msg.style.color = 'var(--fg)'; + } catch (e) { + msg.textContent = 'Failed to save'; msg.style.color = 'var(--red)'; + } + } + + async function run() { + // 1. Rejected fetch (403) + await save(async () => ({ ok: false, status: 403 })); + const rejectedResult = { text: msgText, color: msgColor }; + + // 2. Server error (500) + await save(async () => ({ ok: false, status: 500 })); + const serverErrResult = { text: msgText, color: msgColor }; + + // 3. Successful fetch (200) + await save(async () => ({ ok: true, status: 200 })); + const okResult = { text: msgText, color: msgColor }; + + console.log(JSON.stringify({ rejectedResult, serverErrResult, okResult })); + } + run(); + """ + proc = subprocess.run(["node", "-e", js], capture_output=True, text=True, check=True) + import json + data = json.loads(proc.stdout) + assert data["rejectedResult"] == {"text": "Failed to save", "color": "var(--red)"} + assert data["serverErrResult"] == {"text": "Failed to save", "color": "var(--red)"} + assert data["okResult"] == {"text": "Saved", "color": "var(--fg)"} +