diff --git a/README.md b/README.md index 7ba2aaf..6702b8c 100644 --- a/README.md +++ b/README.md @@ -15,6 +15,35 @@ as intuitive as Dockge, as capable as Portainer for Compose workflows. > (Auto-update) + Phase 23 (Secrets & configs) + Phase 24 (Design System v2) > complete. +## Upgrading to 0.46.0 — everyone is signed out once + +Sessions now hold their access token in memory and the refresh token in an +httpOnly cookie, so **the upgrade signs everybody out exactly once**. Sign back +in and it behaves as before, including staying signed in across restarts. + +What changed and why: + +- **Tokens can be revoked.** Each account carries a `token_version` that every + token is minted with and every request checks. Resetting a password, changing + a role or disabling an account now bumps it, which cuts off the tokens that + account already holds — previously a password reset was cosmetic and whoever + had the old refresh token kept full access for up to 30 days. +- **The refresh token left `localStorage`.** It is an httpOnly cookie + (`SameSite=Lax`, scoped to `/api/auth`), so a successful XSS can act inside the + open page but cannot walk off with 30 days of access. The cookie is marked + `Secure` only when the request arrived over HTTPS, so a plain-HTTP homelab + keeps working. Any refresh token left in `localStorage` by an older build is + deleted on first load. +- **"Sign out everywhere"** in the user menu revokes every token the account + holds, on every device. Plain "Sign out" only ends the session on that device. +- **WebSockets re-check the database.** Log streams and the container terminal + read the role from the live user instead of the token's claim, so a demotion + or a disabled account takes effect immediately — the terminal is + root-equivalent on the host. + +Scripted clients that cannot hold a cookie can still get the refresh token in +the response body with `?in_body=true` on login and refresh. + ## Upgrading to 0.44.0 / 0.45.0 — two defaults changed 0.44.0 closes a privilege-escalation hole and tightens two defaults. Both @@ -49,7 +78,10 @@ it is what your saved destination credentials are encrypted with. - **File-first stacks** — every stack is a plain `compose.yaml` (+ optional `.env`) on disk. The DB only stores metadata; nothing is locked in. - **Auth** — JWT access/refresh tokens, bcrypt hashing, admin/user roles, and a - first-launch setup wizard that creates the initial admin account. + first-launch setup wizard that creates the initial admin account. The access + token is held in memory; the refresh token is an httpOnly cookie. Every token + carries the account's `token_version`, so a password reset, role change or + disable revokes the tokens that account already holds — on every device. - **Stack lifecycle** — create, edit, clone, delete, and `up / down / start / stop / restart / pull / update` via `docker compose`. - **Live status** — running / partial / stopped / error / updating, computed from @@ -506,7 +538,7 @@ Same three commands the CI runs — `build-and-push` only starts once they pass. ```bash cd backend pip install -r requirements-dev.txt -pytest # 670 tests, no Docker daemon needed +pytest # 698 tests, no Docker daemon needed ruff check . cd ../frontend && npx tsc --noEmit -p tsconfig.json ``` @@ -525,6 +557,13 @@ adding it to `USER_READABLE` with a note on why it cannot return a credential. `tests/test_agent_authorization.py` does the same for the agent, where a single forgotten `Depends(verify_token)` would expose a whole host. +`tests/test_token_revocation.py` covers the revoke switch — that each of the +three authority changes kills the account's tokens, that a cosmetic re-save does +not, and that the refresh cookie is httpOnly and not marked `Secure` over plain +HTTP. `tests/test_schema_migration.py` builds a database with the *old* user +table and asserts the added column is backfilled rather than left NULL, which is +what would otherwise have signed out every user on every install. + `tests/test_bundled_templates.py` covers the 83 shipped templates: each must parse, name an image per service, keep `.env.example` in sync with the variables compose actually reads, ship every file it bind-mounts, and never come with a @@ -534,6 +573,7 @@ working default password. ``` POST /api/auth/setup | login | refresh GET /api/auth/me | needs-setup +POST /api/auth/logout | logout-everywhere GET /api/stacks POST /api/stacks GET /api/stacks/{id} PUT /api/stacks/{id} DELETE /api/stacks/{id} POST /api/stacks/{id}/{start|stop|restart|pull|update|down|clone} diff --git a/backend/auth.py b/backend/auth.py index b8e59c8..9f56014 100644 --- a/backend/auth.py +++ b/backend/auth.py @@ -32,12 +32,20 @@ def verify_password(plain: str, hashed: str) -> bool: # --- token helpers --- -def _create_token(sub: str, role: str, token_type: str, expires: timedelta) -> str: +def token_version_of(user: User) -> int: + """A user's current token version, tolerating a NULL from an older schema.""" + return int(user.token_version or 1) + + +def _create_token(user: User, token_type: str, expires: timedelta) -> str: now = datetime.now(timezone.utc) payload = { - "sub": sub, - "role": role, + "sub": user.username, + "role": user.role, "type": token_type, + # Minted-at authority version. Checked on every request, so bumping it + # revokes every token this user already holds. + "ver": token_version_of(user), "iat": now, "exp": now + expires, } @@ -46,22 +54,26 @@ def _create_token(sub: str, role: str, token_type: str, expires: timedelta) -> s def create_access_token(user: User) -> str: return _create_token( - user.username, - user.role, - "access", - timedelta(minutes=settings.ACCESS_TOKEN_EXPIRE_MINUTES), + user, "access", timedelta(minutes=settings.ACCESS_TOKEN_EXPIRE_MINUTES) ) def create_refresh_token(user: User) -> str: return _create_token( - user.username, - user.role, - "refresh", - timedelta(days=settings.REFRESH_TOKEN_EXPIRE_DAYS), + user, "refresh", timedelta(days=settings.REFRESH_TOKEN_EXPIRE_DAYS) ) +def bump_token_version(user: User) -> None: + """Invalidate every token this user currently holds. + + Called whenever their authority changes — password, role, active flag — so + a compromised account is actually cut off instead of staying usable until + the tokens expire on their own. The caller commits. + """ + user.token_version = token_version_of(user) + 1 + + def decode_token(token: str, expected_type: str = "access") -> dict: try: payload = jwt.decode( @@ -83,6 +95,21 @@ def decode_token(token: str, expected_type: str = "access") -> dict: # --- user lookups --- +def resolve_token_user(session: Session, payload: dict) -> Optional[User]: + """The live user a token payload refers to, or None if it is no longer valid. + + Deliberately re-reads the database rather than trusting the token's claims: + the role in a token is a snapshot from when it was minted, and an account + can be disabled or have its password reset at any point afterwards. + """ + user = get_user(session, payload.get("sub", "")) + if not user or not user.is_active: + return None + if int(payload.get("ver", 0)) != token_version_of(user): + return None + return user + + def get_user(session: Session, username: str) -> Optional[User]: return session.exec(select(User).where(User.username == username)).first() @@ -114,19 +141,24 @@ def get_current_user( headers={"WWW-Authenticate": "Bearer"}, ) payload = decode_token(token, "access") - user = get_user(session, payload.get("sub", "")) - if not user or not user.is_active: + user = resolve_token_user(session, payload) + if not user: raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, - detail="User not found or inactive", + detail="Session is no longer valid — sign in again", ) return user -def require_admin(user: User = Depends(get_current_user)) -> User: +def require_admin_role(user: User) -> User: + """Role check split out so the WebSocket routes can reuse it.""" if user.role != "admin": raise HTTPException( status_code=status.HTTP_403_FORBIDDEN, detail="Admin privileges required", ) return user + + +def require_admin(user: User = Depends(get_current_user)) -> User: + return require_admin_role(user) diff --git a/backend/database.py b/backend/database.py index 93d0e09..24ca1aa 100644 --- a/backend/database.py +++ b/backend/database.py @@ -4,6 +4,7 @@ from __future__ import annotations import logging import os from collections.abc import Generator +from typing import Optional from sqlalchemy import inspect, text from sqlmodel import Session, SQLModel, create_engine @@ -23,6 +24,27 @@ engine = create_engine( ) +def _default_literal(col) -> Optional[str]: + """SQL literal for a column's scalar default, or None if it has none. + + Only plain values are rendered — a callable default (``default_factory``, + e.g. a timestamp) has no fixed literal, so those columns are added nullable + as before and filled by the ORM on the next write. + """ + default = col.default + if default is None or not getattr(default, "is_scalar", False): + return None + value = default.arg + if isinstance(value, bool): + return "1" if value else "0" + if isinstance(value, (int, float)): + return str(value) + if isinstance(value, str): + escaped = value.replace("'", "''") + return f"'{escaped}'" + return None + + def _ensure_model_columns() -> None: """Add columns that models define but a pre-existing table is missing. @@ -33,6 +55,15 @@ def _ensure_model_columns() -> None: model's columns against the live table and ``ADD COLUMN`` the safe (nullable, or defaulted) ones. Idempotent: on a fresh DB create_all already made every column, so this is a no-op. + + Requires ``models`` to have been imported, or ``SQLModel.metadata`` is empty + and this silently does nothing. :func:`init_db` imports it first. + + A column with a scalar default is added ``NOT NULL DEFAULT `` so + existing rows are backfilled in the same statement. Without that clause + SQLite fills them with NULL, which is how a new non-nullable field turns + into a runtime surprise — for ``User.token_version`` it would have meant + every existing session failing its version check after the upgrade. """ insp = inspect(engine) live_tables = set(insp.get_table_names()) @@ -52,8 +83,13 @@ def _ensure_model_columns() -> None: "manual migration needed", table_name, col.name ) continue - ddl_type = col.type.compile(dialect=engine.dialect) - conn.execute(text(f'ALTER TABLE "{table_name}" ADD COLUMN "{col.name}" {ddl_type}')) + ddl = f'ALTER TABLE "{table_name}" ADD COLUMN "{col.name}" ' + ddl += col.type.compile(dialect=engine.dialect) + if (literal := _default_literal(col)) is not None: + # Backfills existing rows and satisfies SQLite's rule that a + # NOT NULL column may only be added together with a default. + ddl += f" NOT NULL DEFAULT {literal}" + conn.execute(text(ddl)) logger.info("Schema migration: added column %s.%s", table_name, col.name) diff --git a/backend/models/user.py b/backend/models/user.py index 36971f7..991e65e 100644 --- a/backend/models/user.py +++ b/backend/models/user.py @@ -17,6 +17,12 @@ class User(SQLModel, table=True): role: str = Field(default="user") # "admin" | "user" is_active: bool = Field(default=True) created_at: datetime = Field(default_factory=_now) + #: Bumped whenever this account's authority changes — password, role or + #: active flag. Every token carries the value it was minted with, so a + #: bump makes all outstanding tokens for this user fail their next check. + #: Without it a password reset left the old tokens usable for their full + #: lifetime (up to 30 days for a refresh token). + token_version: int = Field(default=1) # --- API schemas --- @@ -47,10 +53,20 @@ class LoginRequest(SQLModel): class TokenPair(SQLModel): + """Login/refresh response. + + ``refresh_token`` is optional in the body: the API sets it as an httpOnly + cookie, and browsers never need (or should) see it. It is still returned + when the caller opts in with ``?in_body=true`` so scripted clients that + cannot hold a cookie jar keep working. + """ + access_token: str - refresh_token: str token_type: str = "bearer" + refresh_token: Optional[str] = None class RefreshRequest(SQLModel): - refresh_token: str + """Body for ``/api/auth/refresh``. Optional — the cookie is preferred.""" + + refresh_token: Optional[str] = None diff --git a/backend/routers/auth.py b/backend/routers/auth.py index 9f75892..9dfea10 100644 --- a/backend/routers/auth.py +++ b/backend/routers/auth.py @@ -4,7 +4,7 @@ from __future__ import annotations import time from collections import defaultdict, deque -from fastapi import APIRouter, Depends, HTTPException, Request, status +from fastapi import APIRouter, Depends, HTTPException, Request, Response, status from sqlmodel import Session, select import auth as auth_mod @@ -18,6 +18,7 @@ from models.user import ( UserRead, UserUpdate, ) +from config import settings from services import audit_service router = APIRouter(prefix="/api/auth", tags=["auth"]) @@ -41,10 +42,44 @@ def _check_rate_limit(ip: str) -> None: hits.append(now) -def _tokens_for(user: User) -> TokenPair: +#: The refresh cookie is scoped to the two endpoints that consume it, so it is +#: not attached to every API call the way a "/" cookie would be. +REFRESH_COOKIE = "stackpilot_refresh" +REFRESH_COOKIE_PATH = "/api/auth" + + +def _issue( + user: User, response: Response, request: Request, in_body: bool = False +) -> TokenPair: + """Mint a token pair, putting the refresh token in an httpOnly cookie. + + Keeping the long-lived token out of JavaScript's reach means a successful + XSS can no longer walk off with 30 days of access — it is limited to + whatever it can do in the live page. The short-lived access token still + goes to the client, which holds it in memory only. + + ``in_body`` returns it in the response as well, for scripted clients that + have no cookie jar. + """ + refresh = auth_mod.create_refresh_token(user) + response.set_cookie( + REFRESH_COOKIE, + refresh, + max_age=settings.REFRESH_TOKEN_EXPIRE_DAYS * 24 * 3600, + httponly=True, + # Lax rather than Strict so following a link into StackPilot keeps you + # signed in; the cookie is only ever read by same-site POSTs anyway. + samesite="lax", + # Only when the request actually arrived over TLS — marking it Secure on + # a plain-HTTP homelab deployment would make the browser drop it and + # nobody could stay signed in. request.url.scheme is trustworthy here + # because uvicorn runs with --proxy-headers. + secure=request.url.scheme == "https", + path=REFRESH_COOKIE_PATH, + ) return TokenPair( access_token=auth_mod.create_access_token(user), - refresh_token=auth_mod.create_refresh_token(user), + refresh_token=refresh if in_body else None, ) @@ -56,7 +91,11 @@ def needs_setup(session: Session = Depends(get_session)) -> dict: @router.post("/setup", response_model=TokenPair) def setup( - body: UserCreate, session: Session = Depends(get_session) + body: UserCreate, + request: Request, + response: Response, + in_body: bool = False, + session: Session = Depends(get_session), ) -> TokenPair: if auth_mod.users_exist(session): raise HTTPException(status_code=400, detail="Setup already completed") @@ -71,13 +110,15 @@ def setup( audit_service.record( session, user=user.username, action="user.setup", target=user.username ) - return _tokens_for(user) + return _issue(user, response, request, in_body) @router.post("/login", response_model=TokenPair) def login( body: LoginRequest, request: Request, + response: Response, + in_body: bool = False, session: Session = Depends(get_session), ) -> TokenPair: ip = request.client.host if request.client else "unknown" @@ -91,20 +132,71 @@ def login( audit_service.record( session, user=user.username, action="auth.login", target=user.username, ip=ip ) - return _tokens_for(user) + return _issue(user, response, request, in_body) @router.post("/refresh", response_model=TokenPair) def refresh( - body: RefreshRequest, session: Session = Depends(get_session) + request: Request, + response: Response, + body: RefreshRequest | None = None, + in_body: bool = False, + session: Session = Depends(get_session), ) -> TokenPair: - payload = auth_mod.decode_token(body.refresh_token, "refresh") - user = auth_mod.get_user(session, payload.get("sub", "")) - if not user or not user.is_active: + """Exchange a refresh token for a fresh pair. + + Reads the httpOnly cookie; a body is accepted as a fallback for clients + that cannot hold one. The token is re-validated against the live user, so a + password reset or a disabled account takes effect here too rather than at + the end of the token's 30-day life. + """ + token = request.cookies.get(REFRESH_COOKIE) or (body.refresh_token if body else None) + if not token: raise HTTPException( - status_code=status.HTTP_401_UNAUTHORIZED, detail="Invalid refresh token" + status_code=status.HTTP_401_UNAUTHORIZED, detail="No refresh token" ) - return _tokens_for(user) + payload = auth_mod.decode_token(token, "refresh") + user = auth_mod.resolve_token_user(session, payload) + if not user: + response.delete_cookie(REFRESH_COOKIE, path=REFRESH_COOKIE_PATH) + raise HTTPException( + status_code=status.HTTP_401_UNAUTHORIZED, + detail="Session is no longer valid — sign in again", + ) + return _issue(user, response, request, in_body) + + +@router.post("/logout") +def logout(request: Request, response: Response) -> dict: + """End the session on this device by dropping the refresh cookie. + + Deliberately does not bump ``token_version``: signing out on your phone + should not kill the session on your desktop. Use "sign out everywhere" + for that. The access token is held in memory by the client and dies with + the tab; it stays technically valid for the rest of its hour, which is why + it is short-lived. + """ + response.delete_cookie(REFRESH_COOKIE, path=REFRESH_COOKIE_PATH) + return {"ok": True} + + +@router.post("/logout-everywhere") +def logout_everywhere( + request: Request, + response: Response, + session: Session = Depends(get_session), + user: User = Depends(auth_mod.get_current_user), +) -> dict: + """Revoke every token this account holds, on every device.""" + auth_mod.bump_token_version(user) + session.add(user) + session.commit() + response.delete_cookie(REFRESH_COOKIE, path=REFRESH_COOKIE_PATH) + audit_service.record( + session, user=user.username, action="auth.logout_everywhere", + target=user.username, ip=_ip(request), + ) + return {"ok": True} @router.get("/me", response_model=UserRead) @@ -175,6 +267,15 @@ def update_user( ).first() if not other_admins: raise HTTPException(status_code=400, detail="Cannot demote or disable the last active admin") + # Any of these three changes what this account is allowed to do, so the + # tokens it already holds must stop working. Without the bump a password + # reset was cosmetic: whoever had the old tokens kept full access for up to + # 30 days, and a demotion or a disable only took effect once they expired. + authority_changed = ( + bool(body.password) + or (body.role is not None and body.role != user.role) + or (body.is_active is not None and body.is_active != user.is_active) + ) if body.password: user.hashed_password = auth_mod.hash_password(body.password) if body.role is not None: @@ -183,6 +284,8 @@ def update_user( user.role = body.role if body.is_active is not None: user.is_active = body.is_active + if authority_changed: + auth_mod.bump_token_version(user) session.add(user) session.commit() session.refresh(user) diff --git a/backend/routers/ws.py b/backend/routers/ws.py index d103d80..dde8121 100644 --- a/backend/routers/ws.py +++ b/backend/routers/ws.py @@ -13,7 +13,7 @@ from fastapi import APIRouter, Query, WebSocket, WebSocketDisconnect from jose import JWTError from sqlmodel import Session -from auth import decode_token +from auth import decode_token, resolve_token_user from database import engine from models.agent import Agent from models.setting import EVENT_PULL_FAILED, EVENT_STACK_ERROR, EVENT_STACK_START @@ -30,14 +30,30 @@ logger = logging.getLogger("stackpilot.ws") router = APIRouter(tags=["ws"]) +def _user_for(token: str | None): + """The live user behind a socket's token, or None. + + Resolves against the database rather than reading the role straight off the + JWT: a socket can outlive a demotion, a disabled account or a password + reset, and the exec endpoint below is root-equivalent on the host. Same + check the HTTP routes make. + """ + if not token: + return None + try: + payload = decode_token(token, "access") + except (JWTError, Exception): # noqa: BLE001 + return None + with Session(engine) as session: + user = resolve_token_user(session, payload) + if user: + session.expunge(user) + return user + + async def _authorize(websocket: WebSocket, token: str | None) -> bool: """Validate the JWT supplied as a query param. Closes socket on failure.""" - if not token: - await websocket.close(code=4401) - return False - try: - decode_token(token, "access") - except (JWTError, Exception): # noqa: BLE001 + if _user_for(token) is None: await websocket.close(code=4401) return False return True @@ -47,15 +63,11 @@ async def _authorize_admin(websocket: WebSocket, token: str | None) -> bool: """Like _authorize but also requires the admin role (exec is root-equivalent). Closes 4401 on a missing/invalid token, 4403 on a valid non-admin token.""" - if not token: + user = _user_for(token) + if user is None: await websocket.close(code=4401) return False - try: - payload = decode_token(token, "access") - except (JWTError, Exception): # noqa: BLE001 - await websocket.close(code=4401) - return False - if payload.get("role") != "admin": + if user.role != "admin": await websocket.close(code=4403) return False return True diff --git a/backend/tests/test_route_authorization.py b/backend/tests/test_route_authorization.py index ab45915..a6f6b22 100644 --- a/backend/tests/test_route_authorization.py +++ b/backend/tests/test_route_authorization.py @@ -36,6 +36,9 @@ PUBLIC = { "POST /api/auth/setup", "POST /api/auth/login", "POST /api/auth/refresh", + # Only drops the refresh cookie. Requiring a valid token would mean you + # cannot sign out once the session has already gone stale. + "POST /api/auth/logout", } #: Reachable by the read-only ``user`` role. Everything here has been checked @@ -51,6 +54,9 @@ PUBLIC = { #: in and hand YAML back, touching nothing on disk. USER_READABLE = { "GET /api/auth/me", + # Mutating, but only ever on the caller's own account: it bumps their own + # token_version to sign every one of their devices out. + "POST /api/auth/logout-everywhere", "GET /api/dashboard/fleet", # Stacks: status, logs and the compose file. Not the .env, not the export. "GET /api/stacks", diff --git a/backend/tests/test_schema_migration.py b/backend/tests/test_schema_migration.py new file mode 100644 index 0000000..dfd4627 --- /dev/null +++ b/backend/tests/test_schema_migration.py @@ -0,0 +1,128 @@ +"""Adding a column to a database that already exists. + +There is no Alembic here: ``_ensure_model_columns`` diffs each model against +the live table and ``ADD COLUMN``s what is missing. That covers the only kind +of change made so far, but it has a sharp edge — SQLite fills an added column +with NULL unless the statement carries a DEFAULT. + +``User.token_version`` is where that edge would have cut: existing rows would +have come back NULL, every token's version check would have failed against it, +and the upgrade would have signed out every user on every install. So the +helper now renders a DEFAULT for scalar defaults, and this file pins that +behaviour against a genuinely old-shaped table. +""" +from __future__ import annotations + +import pytest +from sqlalchemy import create_engine, inspect, text + + +@pytest.fixture +def legacy_db(tmp_path, monkeypatch): + """A database whose ``user`` table predates ``token_version``.""" + import database + import models # noqa: F401 — populates SQLModel.metadata + + db_file = tmp_path / "legacy.db" + engine = create_engine(f"sqlite:///{db_file}") + with engine.begin() as conn: + conn.execute( + text( + """ + CREATE TABLE user ( + id INTEGER NOT NULL PRIMARY KEY, + username VARCHAR NOT NULL, + hashed_password VARCHAR NOT NULL, + role VARCHAR NOT NULL, + is_active BOOLEAN NOT NULL, + created_at DATETIME NOT NULL + ) + """ + ) + ) + conn.execute( + text( + "INSERT INTO user (id, username, hashed_password, role, is_active," + " created_at) VALUES (1, 'olduser', 'x', 'admin', 1, '2026-01-01')" + ) + ) + monkeypatch.setattr(database, "engine", engine) + return engine + + +def test_the_new_column_is_added_and_backfilled(legacy_db): + import database + + assert "token_version" not in {c["name"] for c in inspect(legacy_db).get_columns("user")} + + database._ensure_model_columns() + + columns = {c["name"]: c for c in inspect(legacy_db).get_columns("user")} + assert "token_version" in columns + with legacy_db.begin() as conn: + value = conn.execute(text("SELECT token_version FROM user WHERE id = 1")).scalar() + assert value == 1, "existing rows must be backfilled, not left NULL" + + +def test_the_existing_row_survives_untouched(legacy_db): + import database + + database._ensure_model_columns() + with legacy_db.begin() as conn: + row = conn.execute(text("SELECT username, role FROM user WHERE id = 1")).one() + assert row.username == "olduser" + assert row.role == "admin" + + +def test_running_it_twice_changes_nothing(legacy_db): + import database + + database._ensure_model_columns() + database._ensure_model_columns() # must not raise "duplicate column name" + with legacy_db.begin() as conn: + assert conn.execute(text("SELECT token_version FROM user")).scalar() == 1 + + +@pytest.mark.parametrize( + "value,expected", + [ + (1, "1"), + (0, "0"), + (True, "1"), + (False, "0"), + ("user", "'user'"), + ("it's", "'it''s'"), # quotes escaped, not injected + ], +) +def test_default_literals_are_rendered_and_escaped(value, expected): + from sqlalchemy import Column, Integer + + import database + + column = Column("c", Integer, default=value) + assert database._default_literal(column) == expected + + +def test_callable_defaults_have_no_literal(): + """``default_factory`` (a timestamp, say) has no fixed value to backfill — + such a column is added nullable, as before.""" + from sqlalchemy import Column, DateTime + + import database + from models.user import User + + assert database._default_literal(Column("c", DateTime, default=lambda: 1)) is None + assert database._default_literal(User.__table__.columns["created_at"]) is None + + +def test_the_live_schema_matches_the_models(db): + """Belt and braces on the real database the rest of the suite uses.""" + from sqlmodel import SQLModel + + import database + + inspector = inspect(database.engine) + for table_name, table in SQLModel.metadata.tables.items(): + live = {c["name"] for c in inspector.get_columns(table_name)} + missing = {c.name for c in table.columns} - live + assert not missing, f"{table_name} is missing {missing}" diff --git a/backend/tests/test_token_revocation.py b/backend/tests/test_token_revocation.py new file mode 100644 index 0000000..d5aaf8c --- /dev/null +++ b/backend/tests/test_token_revocation.py @@ -0,0 +1,279 @@ +"""Token revocation. + +Before 0.46.0 a token was valid until it expired, full stop. Resetting a +compromised account's password changed nothing for whoever held its tokens — +up to 30 days for a refresh token — and demoting or disabling an account only +took effect once the same clock ran out. + +Every token now carries the ``token_version`` it was minted with, and every +request compares that against the live user. Bumping the version is therefore +the revoke switch, and it is pulled on the three changes that alter what an +account may do: password, role, active flag. +""" +from __future__ import annotations + +import pytest +from sqlmodel import Session, select + + +@pytest.fixture +def victim(db): + """A throwaway account whose tokens the tests revoke.""" + import auth as auth_mod + from database import engine + from models.user import User + + with Session(engine) as session: + existing = session.exec(select(User).where(User.username == "revoke-me")).first() + if existing: + session.delete(existing) + session.commit() + user = User( + username="revoke-me", + hashed_password=auth_mod.hash_password("original-password"), + role="user", + ) + session.add(user) + session.commit() + session.refresh(user) + session.expunge(user) + return user + + +def _token_for(username: str) -> str: + import auth as auth_mod + from database import engine + from models.user import User + + with Session(engine) as session: + user = session.exec(select(User).where(User.username == username)).one() + return auth_mod.create_access_token(user) + + +def _still_works(client, token: str) -> bool: + response = client.get("/api/auth/me", headers={"Authorization": f"Bearer {token}"}) + return response.status_code == 200 + + +# --------------------------------------------------------------------------- # +# The claim itself +# --------------------------------------------------------------------------- # + + +def test_tokens_carry_the_version_they_were_minted_with(victim): + import auth as auth_mod + + payload = auth_mod.decode_token(auth_mod.create_access_token(victim), "access") + assert payload["ver"] == 1 + + +def test_a_null_version_from_an_older_schema_reads_as_one(victim): + """Tolerated so a half-migrated database does not lock everyone out.""" + import auth as auth_mod + + victim.token_version = None + assert auth_mod.token_version_of(victim) == 1 + + +# --------------------------------------------------------------------------- # +# What revokes +# --------------------------------------------------------------------------- # + + +@pytest.mark.parametrize( + "change", + [ + pytest.param({"password": "brand-new-password"}, id="password-reset"), + pytest.param({"role": "admin"}, id="role-change"), + pytest.param({"is_active": False}, id="account-disabled"), + ], +) +def test_changing_what_an_account_may_do_kills_its_tokens( + as_admin, client, victim, change +): + token = _token_for("revoke-me") + assert _still_works(client, token) + + assert as_admin.request( + "PATCH", f"/api/auth/users/{victim.id}", json=change + ).status_code == 200 + + assert not _still_works(client, token), f"{change} left the old token usable" + + +def test_a_cosmetic_update_does_not_sign_the_user_out(as_admin, client, victim): + """Re-saving the same role must not invalidate a working session.""" + token = _token_for("revoke-me") + assert as_admin.request( + "PATCH", f"/api/auth/users/{victim.id}", json={"role": victim.role} + ).status_code == 200 + assert _still_works(client, token) + + +def test_a_fresh_token_works_after_a_revoke(as_admin, client, victim): + old = _token_for("revoke-me") + as_admin.request( + "PATCH", f"/api/auth/users/{victim.id}", json={"password": "another-one"} + ) + assert not _still_works(client, old) + assert _still_works(client, _token_for("revoke-me")) + + +def test_refresh_rejects_a_revoked_token(as_admin, client, victim): + """The long-lived token is the one that mattered — 30 days of access.""" + import auth as auth_mod + from database import engine + from models.user import User + + with Session(engine) as session: + user = session.exec(select(User).where(User.username == "revoke-me")).one() + refresh = auth_mod.create_refresh_token(user) + + as_admin.request( + "PATCH", f"/api/auth/users/{victim.id}", json={"password": "changed-again"} + ) + response = client.post("/api/auth/refresh", json={"refresh_token": refresh}) + assert response.status_code == 401 + + +def test_logout_everywhere_revokes_the_callers_own_tokens(client, victim): + import auth as auth_mod + from database import engine + from models.user import User + + with Session(engine) as session: + user = session.exec(select(User).where(User.username == "revoke-me")).one() + token = auth_mod.create_access_token(user) + + assert client.post( + "/api/auth/logout-everywhere", headers={"Authorization": f"Bearer {token}"} + ).status_code == 200 + assert not _still_works(client, token) + + +# --------------------------------------------------------------------------- # +# The refresh cookie +# --------------------------------------------------------------------------- # + + +def test_login_puts_the_refresh_token_in_an_httponly_cookie(client, victim): + response = client.post( + "/api/auth/login", json={"username": "revoke-me", "password": "original-password"} + ) + assert response.status_code == 200 + + # Not in the body — that is the whole point. + assert response.json().get("refresh_token") is None + assert response.json()["access_token"] + + cookie = response.headers["set-cookie"] + assert "stackpilot_refresh=" in cookie + assert "HttpOnly" in cookie + assert "Path=/api/auth" in cookie + assert "SameSite=lax" in cookie.lower().replace("samesite=lax", "SameSite=lax") + + +def test_refresh_works_off_the_cookie_alone(client, victim): + client.post( + "/api/auth/login", json={"username": "revoke-me", "password": "original-password"} + ) + # TestClient keeps the cookie jar, so no body is sent here. + response = client.post("/api/auth/refresh", json={}) + assert response.status_code == 200 + assert response.json()["access_token"] + + +def test_scripted_clients_can_still_ask_for_the_token_in_the_body(client, victim): + response = client.post( + "/api/auth/login?in_body=true", + json={"username": "revoke-me", "password": "original-password"}, + ) + assert response.json()["refresh_token"] + + +def test_logout_clears_the_cookie(client, victim): + client.post( + "/api/auth/login", json={"username": "revoke-me", "password": "original-password"} + ) + response = client.post("/api/auth/logout") + assert response.status_code == 200 + assert 'stackpilot_refresh=""' in response.headers["set-cookie"] or ( + "stackpilot_refresh=;" in response.headers["set-cookie"] + ) + + +def test_the_cookie_is_not_marked_secure_over_plain_http(client, victim): + """A homelab on plain HTTP must still be able to stay signed in. + + Marking the cookie Secure unconditionally would make the browser drop it and + nobody could hold a session. Over HTTPS the flag is set — see _issue(). + """ + response = client.post( + "/api/auth/login", json={"username": "revoke-me", "password": "original-password"} + ) + assert "secure" not in response.headers["set-cookie"].lower() + + +# --------------------------------------------------------------------------- # +# WebSockets go through the same check (F9) +# --------------------------------------------------------------------------- # + + +def test_websocket_auth_resolves_against_the_database(victim): + """A socket must not trust the role baked into the token. + + ``/ws/exec`` is root-equivalent on the host, and a token minted while the + account was an admin stays syntactically valid after a demotion — so the + role has to come from the database, not the claim. + """ + import auth as auth_mod + from database import engine + from models.user import User + from routers.ws import _user_for + + with Session(engine) as session: + user = session.exec(select(User).where(User.username == "revoke-me")).one() + user.role = "admin" + auth_mod.bump_token_version(user) + session.add(user) + session.commit() + session.refresh(user) + admin_token = auth_mod.create_access_token(user) + + assert _user_for(admin_token).role == "admin" + + # Demote. The token still decodes, but must no longer resolve. + with Session(engine) as session: + user = session.exec(select(User).where(User.username == "revoke-me")).one() + user.role = "user" + auth_mod.bump_token_version(user) + session.add(user) + session.commit() + + assert auth_mod.decode_token(admin_token, "access")["role"] == "admin" + assert _user_for(admin_token) is None + + +def test_websocket_auth_rejects_a_disabled_account(victim): + import auth as auth_mod + from database import engine + from models.user import User + from routers.ws import _user_for + + with Session(engine) as session: + user = session.exec(select(User).where(User.username == "revoke-me")).one() + token = auth_mod.create_access_token(user) + assert _user_for(token) is not None + user.is_active = False + session.add(user) + session.commit() + + assert _user_for(token) is None + + +def test_websocket_auth_rejects_garbage(victim): + from routers.ws import _user_for + + assert _user_for(None) is None + assert _user_for("") is None + assert _user_for("not-a-jwt") is None diff --git a/backend/version.py b/backend/version.py index 9b6d831..2ab99a9 100644 --- a/backend/version.py +++ b/backend/version.py @@ -1,3 +1,3 @@ """Single source of truth for the StackPilot release version.""" -APP_VERSION = "0.45.0" +APP_VERSION = "0.46.0" diff --git a/frontend/package.json b/frontend/package.json index bcb7b0e..9e5a17e 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -1,7 +1,7 @@ { "name": "stackpilot-frontend", "private": true, - "version": "0.45.0", + "version": "0.46.0", "type": "module", "scripts": { "dev": "vite", diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 8053391..ae6afc4 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -19,21 +19,38 @@ import { useThemeStore } from "@/store/theme"; function RequireAuth() { const token = useAuthStore((s) => s.accessToken); + const ready = useAuthStore((s) => s.ready); + // The access token lives in memory, so on a reload we have nothing until the + // cookie-based restore has run. Bouncing to /login before that would sign + // people out on every refresh. + if (!ready) return ; return token ? : ; } +function BootSplash() { + return ( +
+ Restoring your session… + + ); +} + export default function App() { const applyTheme = useThemeStore((s) => s.apply); - const fetchMe = useAuthStore((s) => s.fetchMe); - const token = useAuthStore((s) => s.accessToken); + const restore = useAuthStore((s) => s.restore); useEffect(() => { applyTheme(); }, [applyTheme]); + // Trade the httpOnly refresh cookie for an access token once on boot. useEffect(() => { - if (token) fetchMe().catch(() => {}); - }, [token, fetchMe]); + restore(); + }, [restore]); return ( diff --git a/frontend/src/api/client.ts b/frontend/src/api/client.ts index 2ba1e63..8d7a859 100644 --- a/frontend/src/api/client.ts +++ b/frontend/src/api/client.ts @@ -1,7 +1,8 @@ import axios, { AxiosError } from "axios"; import { useAuthStore } from "@/store/auth"; -const api = axios.create({ baseURL: "/" }); +// withCredentials so the httpOnly refresh cookie rides along on /api/auth/refresh. +const api = axios.create({ baseURL: "/", withCredentials: true }); api.interceptors.request.use((config) => { const token = useAuthStore.getState().accessToken; @@ -32,7 +33,8 @@ api.interceptors.response.use( original.headers.Authorization = `Bearer ${newToken}`; return api(original); } - useAuthStore.getState().logout(); + // Refresh failed: the session is genuinely over (expired, password + // changed, account disabled). refresh() has already cleared the store. } return Promise.reject(error); } diff --git a/frontend/src/components/layout/TopNav.tsx b/frontend/src/components/layout/TopNav.tsx index 2fb4317..3766b47 100644 --- a/frontend/src/components/layout/TopNav.tsx +++ b/frontend/src/components/layout/TopNav.tsx @@ -14,6 +14,7 @@ import { Moon, Sun, LogOut, + ShieldOff, Menu, X, } from "lucide-react"; @@ -87,6 +88,7 @@ export function TopNav() { const user = useAuthStore((s) => s.user); const navItems = NAV_ITEMS.filter((i) => !i.adminOnly || user?.role === "admin"); const logout = useAuthStore((s) => s.logout); + const signOutEverywhere = useAuthStore((s) => s.signOutEverywhere); const { theme, toggle } = useThemeStore(); const agents = useQuery({ @@ -198,11 +200,19 @@ export function TopNav() { {user?.role === "admin" &&

admin

}
+ )} diff --git a/frontend/src/store/auth.ts b/frontend/src/store/auth.ts index 678bb83..34a05af 100644 --- a/frontend/src/store/auth.ts +++ b/frontend/src/store/auth.ts @@ -1,78 +1,123 @@ import axios from "axios"; import { create } from "zustand"; -import { persist } from "zustand/middleware"; import type { TokenPair, User } from "@/types"; +/** + * Session state. + * + * The access token lives **in memory only** — deliberately not in + * localStorage. The long-lived refresh token is an httpOnly cookie the browser + * holds and JavaScript cannot read, so a successful XSS can act inside the open + * page but cannot walk off with 30 days of access. + * + * The cost is that a page reload starts with no token; `restore()` trades the + * cookie for a fresh one on boot, which is why the app shows a brief loading + * state instead of jumping straight to the login screen. + */ + interface AuthState { accessToken: string | null; - refreshToken: string | null; user: User | null; - setTokens: (t: TokenPair) => void; + /** False until the initial cookie-based restore has settled. */ + ready: boolean; login: (username: string, password: string) => Promise; setup: (username: string, password: string) => Promise; + restore: () => Promise; refresh: () => Promise; fetchMe: () => Promise; - logout: () => void; + logout: () => Promise; + signOutEverywhere: () => Promise; } // Raw client without interceptors (avoids refresh loops). -const raw = axios.create({ baseURL: "/" }); +const raw = axios.create({ baseURL: "/", withCredentials: true }); -export const useAuthStore = create()( - persist( - (set, get) => ({ - accessToken: null, - refreshToken: null, - user: null, +// Before 0.46.0 both tokens were persisted here by zustand/persist. Upgrading +// users still have a valid 30-day refresh token sitting in localStorage, which +// is exactly what this change exists to remove — so drop it on first load. +try { + localStorage.removeItem("stackpilot-auth"); +} catch { + /* private mode / storage disabled */ +} - setTokens: (t) => - set({ accessToken: t.access_token, refreshToken: t.refresh_token }), +export const useAuthStore = create()((set, get) => ({ + accessToken: null, + user: null, + ready: false, - login: async (username, password) => { - const { data } = await raw.post("/api/auth/login", { - username, - password, - }); - set({ accessToken: data.access_token, refreshToken: data.refresh_token }); - await get().fetchMe(); - }, + login: async (username, password) => { + const { data } = await raw.post("/api/auth/login", { username, password }); + set({ accessToken: data.access_token }); + await get().fetchMe(); + }, - setup: async (username, password) => { - const { data } = await raw.post("/api/auth/setup", { - username, - password, - role: "admin", - }); - set({ accessToken: data.access_token, refreshToken: data.refresh_token }); - await get().fetchMe(); - }, + setup: async (username, password) => { + const { data } = await raw.post("/api/auth/setup", { + username, + password, + role: "admin", + }); + set({ accessToken: data.access_token }); + await get().fetchMe(); + }, - refresh: async () => { - const rt = get().refreshToken; - if (!rt) return null; - try { - const { data } = await raw.post("/api/auth/refresh", { - refresh_token: rt, - }); - set({ accessToken: data.access_token, refreshToken: data.refresh_token }); - return data.access_token; - } catch { - set({ accessToken: null, refreshToken: null, user: null }); - return null; - } - }, + // Called once on boot: if the refresh cookie is still good we come back + // signed in, otherwise we land on the login screen. + restore: async () => { + try { + const token = await get().refresh(); + if (token) await get().fetchMe(); + } catch { + /* not signed in */ + } finally { + set({ ready: true }); + } + }, - fetchMe: async () => { - const token = get().accessToken; - if (!token) return; - const { data } = await raw.get("/api/auth/me", { - headers: { Authorization: `Bearer ${token}` }, - }); - set({ user: data }); - }, + refresh: async () => { + try { + const { data } = await raw.post("/api/auth/refresh", {}); + set({ accessToken: data.access_token }); + return data.access_token; + } catch { + set({ accessToken: null, user: null }); + return null; + } + }, - logout: () => set({ accessToken: null, refreshToken: null, user: null }), - }), - { name: "stackpilot-auth" } - ) -); + fetchMe: async () => { + const token = get().accessToken; + if (!token) return; + const { data } = await raw.get("/api/auth/me", { + headers: { Authorization: `Bearer ${token}` }, + }); + set({ user: data }); + }, + + logout: async () => { + // Clear locally first so the UI never sits on a dead session if the + // request fails; the server call only drops the cookie. + set({ accessToken: null, user: null }); + try { + await raw.post("/api/auth/logout", {}); + } catch { + /* already gone */ + } + }, + + // Bumps the account's token version, so every token it holds — on every + // device — stops working. The thing to reach for when a device is lost. + signOutEverywhere: async () => { + const token = get().accessToken; + try { + await raw.post( + "/api/auth/logout-everywhere", + {}, + { headers: token ? { Authorization: `Bearer ${token}` } : {} } + ); + } finally { + set({ accessToken: null, user: null }); + } + }, +}));