fix: auth review fixes — token revocation, WS auth, seeding, and hardening

Refresh/logout and token robustness - drop the post-rotation refresh_tokens row delete in auth_refresh so logout blacklists the current (rotated) refresh token; remove the dead _clear_refresh_token_after_rotation helper and clear_active_refresh_token - reject non-object JWT payloads in _extract_unverified_sub so crafted Authorization headers return 401 instead of crashing with 500 SQLite user store - make builtin-admin seeding idempotent: on a concurrent first start the losing seeder re-checks, finds the winner, and returns instead of raising IntegrityError - per-thread sqlite connections + busy_timeout so Flask worker threads don't hit cross-thread ProgrammingError / SQLITE_BUSY - add LogsDirectory + /var/log/vacuum-wall to ReadWritePaths in both systemd units so the fallback admin password actually lands on disk Frontend - skip apiFetch 401-recovery for public auth endpoints so a failed login no longer logs out a valid session - add /passkeys to the nav (passkey registration was unreachable); remove the dead checkWebAuthnCapable export - drop the CSP-blocked inline WS-URL script and the __WS_URL_PLACEHOLDER__ plumbing; the WS URL is derived from location Daemon / WS - parse Sec-WebSocket-Protocol manually (web.Request.get_subprotocols does not exist in aiohttp 3.13); X-Auth-Token is a custom-nginx fallback only — docstring and security docs corrected Install / system - bootstrap_auth.py is now idempotent: preserves existing auth config and syncs the admin password on re-runs (new reset_password helper) - WebUI server block renders auth_basic off (the UI is JWT-protected) - install.sh chown/chmod skips .git to avoid git dubious-ownership breakage - tolerate unreadable /etc/wireguard during system import Contracts / docs - create_user returns 409 on duplicate username per docs/api.md - correct docs/api.md response shapes, docs/security.md blacklist cleanup wording + one-refresh-per-user caveat, stale WS-URL references, and the .htpasswd description Tests: +7 regression tests (rotation/logout revocation, crafted-token 401, concurrent seeding); placeholder-substitution tests replaced with serve-as-is SPA root tests.
2026-08-17 01:45:15 +00:00
parent 1980043afd
commit 0ed275835d
27 changed files with 442 additions and 128 deletions
@@ -28,6 +28,7 @@ from lib.auth_users import (
    delete_user,
    get_user,
    list_users,
+    reset_password,
    update_password,
    update_permissions,
    verify_user_password,
@@ -132,6 +133,62 @@ class TestDBLayer:
        conn2 = db.conn
        assert conn1 is conn2

+    def test_connections_are_thread_local(self, tmp_path):
+        """DB access from multiple threads must work (regression test).
+
+        The Flask WebUI validates JWTs in lib.db from worker threads while
+        the daemon uses its event-loop thread. A single shared connection
+        raises sqlite3.ProgrammingError ("SQLite objects created in a
+        thread can only be used in that same thread") on the first
+        cross-thread query.
+        """
+        import threading
+
+        reset_db_for_test()
+        db_path = str(tmp_path / "thread_local.db")
+        os.environ["VACUUM_WALL_DB_PATH"] = db_path
+        try:
+            db = get_db()
+            db.run(Q_INSERT_USER, ("touser", "$argon2id$hash", "test-secret"))
+
+            results: list = []
+            threads = [
+                threading.Thread(
+                    target=lambda: results.append(
+                        db.query(Q_SELECT_USER_BY_NAME, ("touser",))
+                    )
+                )
+                for _ in range(4)
+            ]
+            for t in threads:
+                t.start()
+            for t in threads:
+                t.join()
+
+        finally:
+            reset_db_for_test()
+
+        assert len(results) == 4
+        for rows in results:
+            assert isinstance(rows, list), f"query raised or returned {rows!r}"
+            assert len(rows) == 1
+            assert rows[0]["username"] == "touser"
+
+    def test_connections_are_distinct_per_thread(self, db):
+        """Each thread gets its own connection object."""
+        import threading
+
+        def conn_in_thread(result: list) -> None:
+            result.append(db.conn)
+
+        main_conn = db.conn
+        result: list = []
+        t = threading.Thread(target=conn_in_thread, args=(result,))
+        t.start()
+        t.join()
+        assert len(result) == 1
+        assert result[0] is not main_conn
+
    def test_insert_user(self, db):
        uid = db.run_one(Q_INSERT_USER, ("testuser", "$argon2id$hash", "test-secret"))
        assert isinstance(uid, int)
@@ -350,6 +407,17 @@ class TestUserManagement:
        with pytest.raises(ValueError, match="incorrect"):
            update_password("upwfail", "wrong_old", "newpass123")

+    def test_reset_password_without_old(self, db):
+        """Installer lockout recovery: reset works without knowing the old password."""
+        create_user("rstuser", "unknown-old-pass")
+        reset_password("rstuser", "freshpass123")
+        assert verify_user_password("rstuser", "freshpass123") is not None
+        assert verify_user_password("rstuser", "unknown-old-pass") is None
+
+    def test_reset_password_not_found(self, db):
+        with pytest.raises(ValueError, match="not found"):
+            reset_password("ghostuser", "newpass123")
+
    def test_update_permissions(self, db):
        create_user("permuser", "password123", {"firewall": "rw"})
        update_permissions("permuser", {"firewall": "read", "network": "rw"})
@@ -1191,3 +1259,158 @@ class TestPermissionMiddleware:
        assert _subsystem_from_path("/api/dhcp/leases/subpath") == "dhcp"
        assert _subsystem_from_path("/") is None
        assert _subsystem_from_path("/static/app.js") is None
+
+
+class TestRefreshRotationLogout:
+    """Refresh-rotation + logout interaction (regression for the reorder in
+    0889ef0: clearing the refresh_tokens row after rotation left logout with
+    nothing to blacklist, so the rotated token stayed valid)."""
+
+    def test_logout_revokes_rotated_refresh_token(self) -> None:
+        """Logout must blacklist the current refresh token after rotation."""
+        from daemon.handlers.auth import auth_logout, auth_refresh
+
+        create_user("rotuser", "password123", {"auth": "rw"})
+        tokens = generate_tokens("rotuser", {"auth": "rw"})
+
+        rotated = auth_refresh(
+            MagicMock(),
+            {
+                "refresh_token": tokens["refresh_token"],
+                "session_id": tokens["session_id"],
+            },
+        )["tokens"]
+
+        # The rotated token must be valid before logout (rotation works).
+        payload = validate_token(
+            rotated["refresh_token"], "refresh", session_id=rotated["session_id"]
+        )
+        assert payload is not None
+
+        auth_logout(MagicMock(), {"jti": None, "username": "rotuser"})
+
+        with pytest.raises(ValueError, match="Invalid or expired refresh token"):
+            auth_refresh(
+                MagicMock(),
+                {
+                    "refresh_token": rotated["refresh_token"],
+                    "session_id": rotated["session_id"],
+                },
+            )
+
+    def test_old_refresh_token_blacklisted_on_rotation(self) -> None:
+        """The pre-rotation refresh token must be blacklisted immediately."""
+        from daemon.handlers.auth import auth_refresh
+
+        create_user("rotuser2", "password123", {"auth": "rw"})
+        tokens = generate_tokens("rotuser2", {"auth": "rw"})
+
+        auth_refresh(
+            MagicMock(),
+            {
+                "refresh_token": tokens["refresh_token"],
+                "session_id": tokens["session_id"],
+            },
+        )
+
+        assert (
+            validate_token(
+                tokens["refresh_token"], "refresh", session_id=tokens["session_id"]
+            )
+            is None
+        )
+
+
+class TestMalformedTokenPayload:
+    """Malformed/untrusted JWT payloads must be rejected (401), not 500."""
+
+    def test_decode_non_object_payload_returns_none(self) -> None:
+        """A payload segment decoding to non-object JSON is rejected."""
+        hdr = (
+            base64.urlsafe_b64encode(b'{"alg":"HS256","typ":"JWT"}')
+            .decode()
+            .rstrip("=")
+        )
+        payload = base64.urlsafe_b64encode(b'"hello"').decode().rstrip("=")
+        token = f"{hdr}.{payload}.signature"
+        assert decode_token(token) is None
+
+    def test_middleware_crafted_token_returns_401(self) -> None:
+        """Crafted Bearer token on a protected route returns JSON 401, not 500."""
+        from webui.server import app
+
+        client = app.test_client()
+        hdr = (
+            base64.urlsafe_b64encode(b'{"alg":"HS256","typ":"JWT"}')
+            .decode()
+            .rstrip("=")
+        )
+        payload = base64.urlsafe_b64encode(b'"hello"').decode().rstrip("=")
+        token = f"{hdr}.{payload}.signature"
+
+        res = client.get(
+            "/api/auth/session",
+            headers={"Authorization": f"Bearer {token}", "X-Session-Id": "x"},
+        )
+        assert res.status_code == 401
+        assert res.get_json() == {"ok": False, "error": "unauthorized"}
+
+
+class TestBuiltinAdminSeeding:
+    """Fallback seeding of the builtin admin user (lib.db._seed_builtin_admin)."""
+
+    def test_seed_runs_and_creates_admin(self) -> None:
+        """A fresh (empty) DB gets the builtin admin with full permissions."""
+        from lib.auth_users import ALL_SUBSYSTEMS
+        from lib.db import _seed_builtin_admin
+
+        db = get_db()
+        _seed_builtin_admin(db)
+
+        user = get_user("admin")
+        assert user is not None
+        assert user["permissions"] == {s: "rw" for s in ALL_SUBSYSTEMS}
+
+    def test_seed_noop_when_admin_exists(self) -> None:
+        """Seeding is a no-op when the admin user already exists."""
+        from lib.db import Q_SELECT_USER_BY_NAME, _seed_builtin_admin
+
+        db = get_db()
+        _seed_builtin_admin(db)
+
+        real_query = db.query
+        calls = {"n": 0}
+
+        def counting_query(query_id, params=()):
+            if query_id == Q_SELECT_USER_BY_NAME:
+                calls["n"] += 1
+            return real_query(query_id, params)
+
+        with patch.object(db, "query", side_effect=counting_query):
+            _seed_builtin_admin(db)
+        # Early-return path: only the existence check runs.
+        assert calls["n"] >= 1
+
+    def test_seed_concurrent_lose_race(self) -> None:
+        """Concurrent seeding: if the insert loses a race, the loser re-checks,
+        finds the winner's admin, and returns instead of raising IntegrityError."""
+        from lib.db import Q_SELECT_USER_BY_NAME, _seed_builtin_admin
+
+        db = get_db()
+        # get_db() already seeded admin for this fresh in-memory DB.
+        assert get_user("admin") is not None
+
+        real_query = db.query
+        calls = {"n": 0}
+
+        def fake_query(query_id, params=()):
+            if query_id == Q_SELECT_USER_BY_NAME and params and params[0] == "admin":
+                calls["n"] += 1
+                if calls["n"] == 1:
+                    return []  # stale view: existence check misses concurrent seeder
+            return real_query(query_id, params)
+
+        with patch.object(db, "query", side_effect=fake_query):
+            _seed_builtin_admin(db)  # must not raise
+
+        assert get_user("admin") is not None
@@ -39,14 +39,17 @@ class TestSPARoutes:
        assert data.get("error") == "unauthorized"


-class TestWsUrlGeneration:
-    def test_ws_url_ipv4_host(self, client):
-        resp = client.get("/", headers={"Host": "192.168.1.1:9090"})
-        assert b"ws://192.168.1.1:9090/ws" in resp.data
+class TestSpaRoot:
+    def test_serves_index_html_as_is(self, client):
+        resp = client.get("/")
+        assert resp.status_code == 200
+        assert b"/static/app.js" in resp.data

-    def test_ws_url_ipv6_host(self, client):
-        resp = client.get("/", headers={"Host": "[::1]:9090"})
-        assert b"ws://[::1]:9090/ws" in resp.data
+    def test_no_ws_url_substitution(self, client):
+        """index.html is served verbatim — no WS URL placeholder substitution."""
+        resp = client.get("/", headers={"Host": "192.168.1.1:9090"})
+        assert b"__WS_URL_PLACEHOLDER__" not in resp.data
+        assert b"ws://" not in resp.data


 class TestBlueprintsRegistered: