From 8ae60ab8cf70cfea6c0fdd2e52f9dbef50a21f32 Mon Sep 17 00:00:00 2001 From: Mike Teehan Date: Tue, 28 Jul 2026 18:52:03 +0000 Subject: [PATCH] fix: harden auth and fix frontend issues - Add builtin admin user with full access, immutable permissions (lib/db.py, lib/auth_users.py, webui/static/pages/users.js) - Fix passkeys TypeError on string throws (webui/static/pages/passkeys.js) - Add zero-permission warning in create user modal (webui/static/pages/users.js) - Restore readonly on proxy paths textarea (webui/static/pages/proxy.js) - Mask credential ownership errors to prevent enumeration (lib/webauthn.py, tests/test_auth.py) --- lib/auth_users.py | 9 +++++++++ lib/db.py | 30 ++++++++++++++++++++++++++++++ lib/webauthn.py | 2 +- tests/test_auth.py | 27 +++++++++++++++++---------- webui/static/pages/passkeys.js | 14 ++++++++------ webui/static/pages/proxy.js | 2 +- webui/static/pages/users.js | 16 ++++++++++++---- 7 files changed, 78 insertions(+), 22 deletions(-) diff --git a/lib/auth_users.py b/lib/auth_users.py index ccdf54e..6702bde 100644 --- a/lib/auth_users.py +++ b/lib/auth_users.py @@ -31,6 +31,9 @@ from lib.password import hash_password, needs_rehash, verify_password logger = logging.getLogger(__name__) +# Builtin admin — hardcoded, full access, cannot be modified/deleted +BUILTIN_ADMIN_USERNAME = "admin" + # All subsystem names for default permission assignment ALL_SUBSYSTEMS = [ "firewall", @@ -211,7 +214,13 @@ def update_permissions(username: str, permissions: dict[str, str]) -> None: Args: username: The username. permissions: Dict mapping subsystem names to permission levels. + + Raises: + ValueError: If attempting to modify builtin admin. """ + if username == BUILTIN_ADMIN_USERNAME: + raise ValueError("Cannot modify permissions for builtin admin") + user = find_user(username) if user is None: raise ValueError(f"User {username!r} not found") diff --git a/lib/db.py b/lib/db.py index 2f36b36..62dd667 100644 --- a/lib/db.py +++ b/lib/db.py @@ -18,6 +18,7 @@ from __future__ import annotations import logging import os +import secrets from abc import ABC, abstractmethod from pathlib import Path from typing import Any, ClassVar @@ -271,11 +272,40 @@ def get_db() -> Database: _db_instance = SQLiteBackend(path) Path(path).parent.mkdir(parents=True, exist_ok=True) _db_instance.init_tables() + _seed_builtin_admin(_db_instance) else: raise ValueError(f"Unknown database backend: {backend!r}") return _db_instance +def _seed_builtin_admin(db: Database) -> None: + """Create the builtin admin user if they don't exist. + + The builtin admin has full (rw) access to all subsystems and cannot + be deleted or have permissions modified through the normal API. + """ + from lib.auth_users import ALL_SUBSYSTEMS, BUILTIN_ADMIN_USERNAME + from lib.password import hash_password + + # Check if admin already exists + rows = db.query(Q_SELECT_USER_BY_NAME, (BUILTIN_ADMIN_USERNAME,)) + if rows: + return + + # Create with a placeholder password that should be changed + placeholder_hash = hash_password("CHANGEME") + jwt_secret = secrets.token_urlsafe(32) + + with db.in_transaction() as tx: + tx.run_one( + Q_INSERT_USER, (BUILTIN_ADMIN_USERNAME, placeholder_hash, jwt_secret) + ) + for subsystem in ALL_SUBSYSTEMS: + tx.run(Q_UPSERT_PERMISSION, (BUILTIN_ADMIN_USERNAME, subsystem, "rw")) + + logger.info("Builtin admin user created with full access") + + def reset_db_for_test() -> None: """Reset the singleton — only for tests.""" global _db_instance diff --git a/lib/webauthn.py b/lib/webauthn.py index 1296b0e..228060b 100644 --- a/lib/webauthn.py +++ b/lib/webauthn.py @@ -337,7 +337,7 @@ def remove_credential(username: str, credential_id: str) -> bool: if not rows: raise ValueError(f"Credential {credential_id!r} not found") if rows[0]["username"] != username: - raise ValueError("Credential does not belong to this user") + raise ValueError(f"Credential {credential_id!r} not found") db.run(Q_DELETE_WEBAUTHN, (credential_id,)) logger.info("WebAuthn credential removed: %s, %s", username, credential_id) diff --git a/tests/test_auth.py b/tests/test_auth.py index ea0b1ef..d51ed12 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -189,7 +189,8 @@ class TestDBLayer: db.run(Q_INSERT_USER, ("aaa", "$argon2id$hash", "test-secret")) db.run(Q_INSERT_USER, ("bbb", "$argon2id$hash", "test-secret")) rows = db.query(Q_SELECT_ALL_USERS, ()) - assert len(rows) == 2 + # admin (builtin seed) + aaa + bbb, ordered by username + assert len(rows) == 3 assert rows[0]["username"] == "aaa" @@ -200,7 +201,6 @@ class TestJWT: def setup_method(self) -> None: reset_db_for_test() get_db() - create_user("admin", "secretpass", {"firewall": "rw"}) def test_generate_tokens(self): tokens = generate_tokens("admin", {"firewall": "rw"}) @@ -347,7 +347,8 @@ class TestUserManagement: create_user("listuser1", "pass1", {"firewall": "rw"}) create_user("listuser2", "pass2", {"network": "rw"}) users = list_users() - assert len(users) == 2 + # admin (builtin) + listuser1 + listuser2 + assert len(users) == 3 usernames = {u["username"] for u in users} assert "listuser1" in usernames @@ -368,13 +369,13 @@ class TestLoginFlow: def test_full_login_flow(self): """Create user → verify password → generate tokens → validate tokens.""" - user = create_user("admin", "secretpass", {"firewall": "rw", "auth": "rw"}) - assert verify_user_password("admin", "secretpass") is not None + user = create_user("testadmin", "secretpass", {"firewall": "rw", "auth": "rw"}) + assert verify_user_password("testadmin", "secretpass") is not None - tokens = generate_tokens("admin", user["permissions"]) + tokens = generate_tokens("testadmin", user["permissions"]) payload = validate_token(tokens["access_token"], "access") assert payload is not None - assert payload["sub"] == "admin" + assert payload["sub"] == "testadmin" assert payload["permissions"]["firewall"] == "rw" def test_logout_flow(self): @@ -695,11 +696,16 @@ class TestWebAuthnCredentials: remove_credential("testuser", "nonexistent") def test_remove_credential_wrong_user(self) -> None: + """Remove fails for another user's credential with same error as not-found. + + Both cases return 404 — no distinction leaked to prevent credential + enumeration. Internal ownership check still prevents cross-user deletion. + """ _insert_cred("otheruser", cred_id=_FAKE_CRED_ID) from lib.webauthn import remove_credential - with pytest.raises(ValueError, match="does not belong"): + with pytest.raises(ValueError, match="not found"): remove_credential("testuser", _FAKE_CRED_ID) @@ -844,7 +850,7 @@ class TestMultiUserAdmin: """ from daemon.handlers.auth import auth_delete_user - create_user("admin", "password123", {"auth": "rw"}) + create_user("tester", "password123", {"auth": "rw"}) create_user("target", "password123", {"firewall": "read"}) # Handler can delete other users @@ -927,7 +933,8 @@ class TestMultiUserAdmin: create_user("charlie", "pass3", {}) users = list_users() - assert len(users) == 3 + # admin (builtin) + alice + bob + charlie + assert len(users) == 4 by_name = {u["username"]: u for u in users} diff --git a/webui/static/pages/passkeys.js b/webui/static/pages/passkeys.js index 79b2119..9e533e1 100644 --- a/webui/static/pages/passkeys.js +++ b/webui/static/pages/passkeys.js @@ -108,7 +108,7 @@ function addCredentialModal() { }); if (!beginRes.ok) { - throw beginRes.error || 'Registration failed'; + throw new Error(beginRes.error || 'Registration failed'); } const options = beginRes.data; @@ -129,15 +129,16 @@ function addCredentialModal() { }); if (!finishRes.ok) { - throw finishRes.error || 'Registration verification failed'; + throw new Error(finishRes.error || 'Registration verification failed'); } toast('Passkey registered', 'success'); closeModal(); loadCredentials(); } catch (e) { - if (!e.message.toLowerCase().includes('cancelled')) { - toast(e.message || 'Registration failed', 'error'); + const msg = typeof e === 'string' ? e : (e.message || 'Registration failed'); + if (!msg.toLowerCase().includes('cancelled')) { + toast(msg, 'error'); } } finally { setModalProcessing(false); @@ -180,14 +181,15 @@ function confirmRemove(credentialId, credentialName) { }); if (!res.ok) { - throw res.error || 'Removal failed'; + throw new Error(res.error || 'Removal failed'); } toast('Passkey removed', 'success'); closeModal(); loadCredentials(); } catch (e) { - toast(e.message || 'Removal failed', 'error'); + const msg = typeof e === 'string' ? e : (e.message || 'Removal failed'); + toast(msg, 'error'); } finally { setModalProcessing(false); refreshModals(); diff --git a/webui/static/pages/proxy.js b/webui/static/pages/proxy.js index 2336542..be1ffd0 100644 --- a/webui/static/pages/proxy.js +++ b/webui/static/pages/proxy.js @@ -165,7 +165,7 @@ function editDomain(d, state) { postRender: (inner) => { const certSelect = inner.querySelector('#pe-cert'); if (certSelect) certSelect.value = selectedCert; - for (const id of ['pe-domain', 'pe-backend']) { + for (const id of ['pe-domain', 'pe-backend', 'pe-paths']) { const el = inner.querySelector('#' + id); if (el) { el.readOnly = true; el.style.background = '#f5f5f5'; } } diff --git a/webui/static/pages/users.js b/webui/static/pages/users.js index f4591b0..f554dc7 100644 --- a/webui/static/pages/users.js +++ b/webui/static/pages/users.js @@ -8,6 +8,8 @@ import { h, definePage, reactive } from '/static/hoover/index.js'; import { html, PageHeader, Table, Badge, ConfirmDelete, Empty, Card, openModal, closeModal, formModal, apiFetch, toast, esc } from '/static/hoover/index.js'; +const BUILTIN_ADMIN = 'admin'; + const SUBSYSTEMS = [ { key: 'firewall', label: 'Firewall' }, { key: 'network', label: 'Network' }, @@ -157,11 +159,16 @@ function openEditPermissionsModal(user) { for (const sub of SUBSYSTEMS) { const level = document.getElementById('edit-perm-' + sub.key).value; if (level && level !== '—') { - perms[sub.key] = level; + perms[sub.key] = level; + } } - } - const res = await apiFetch('/api/auth/users/' + encodeURIComponent(user.username), { + if (user.username === BUILTIN_ADMIN) { + toast('Cannot modify permissions for builtin admin', 'error'); + return; + } + + const res = await apiFetch('/api/auth/users/' + encodeURIComponent(user.username), { method: 'POST', body: { permissions: perms }, }); @@ -215,7 +222,8 @@ function UsersPage() { ${u.credCount || 0} ${permBadges.length ? permBadges.join(' ') : '—'} - + ${u.username === BUILTIN_ADMIN ? html`(builtin)` : + html``} ${isMe ? html`(you)` : html`<${ConfirmDelete} url=${'/api/auth/users/' + encodeURIComponent(u.username)}