chore: code review.

Author: PttCodingMan

CLAUDE.md

@@ -128,4 +128,4 @@ After completing any development task, always run the following before reporting
 
 1. **Tests** — run `make test` (or the relevant subset: `make test-backend` / `make test-frontend`) and make sure everything passes.
 2. **Lint** — run `make lint` for frontend changes.
-3. **Code review** — perform a self-review of the diff, then run a second-opinion review with Gemini CLI for non-trivial changes.
+3. **Code review** — perform a self-review of the diff for non-trivial changes.

backend/app/auth.py

@@ -44,7 +44,7 @@ def create_token(user_id: int, username: str, role: str) -> str:
         "role": role,
         "exp": expire,
     }
-    return jwt.encode(payload, settings.SECRET_KEY, algorithm=ALGORITHM)
+    return jwt.encode(payload, settings.SECRET_KEY.get_secret_value(), algorithm=ALGORITHM)
 
 
 async def _resolve_api_token(token: str) -> dict | None:
@@ -98,55 +98,59 @@ async def _resolve_api_token(token: str) -> dict | None:
     }
 
 
-async def get_current_user(request: Request):
-    token = None
+async def _resolve_request_credentials(request: Request) -> dict | None:
+    """Decode whichever credential the request carries, or return None.
 
-    # Check Authorization header
+    Recognises both personal API tokens (prefixed with `jwk_`) and JWT
+    session tokens, read from `Authorization: Bearer` first and falling
+    back to the `token` cookie. Returns the resolved user dict on success
+    and None on any failure (no token, invalid token, missing user). The
+    caller decides whether to raise 401 or treat it as anonymous.
+    """
+    token = None
     auth_header = request.headers.get("Authorization")
     if auth_header and auth_header.startswith("Bearer "):
         token = auth_header[7:]
-
-    # Check cookie
     if not token:
         token = request.cookies.get("token")
-
     if not token:
-        raise HTTPException(
-            status_code=status.HTTP_401_UNAUTHORIZED,
-            detail="Not authenticated",
-        )
+        return None
 
-    # Personal API tokens are distinguished by a fixed prefix so we don't
-    # have to guess-and-fall-back. Anything else must parse as a JWT.
     if token.startswith(API_TOKEN_PREFIX):
-        user = await _resolve_api_token(token)
-        if user is None:
-            raise HTTPException(
-                status_code=status.HTTP_401_UNAUTHORIZED,
-                detail="Invalid token",
-            )
-        return user
+        return await _resolve_api_token(token)
 
     try:
-        payload = jwt.decode(token, settings.SECRET_KEY, algorithms=[ALGORITHM])
+        payload = jwt.decode(token, settings.SECRET_KEY.get_secret_value(), algorithms=[ALGORITHM])
         user_id = int(payload["sub"])
     except (JWTError, KeyError, ValueError):
-        raise HTTPException(
-            status_code=status.HTTP_401_UNAUTHORIZED,
-            detail="Invalid token",
-        )
+        return None
 
     db = await get_db()
     row = await db.execute_fetchall(
         "SELECT id, username, role, display_name, email FROM users WHERE id = ? AND deleted_at IS NULL",
         (user_id,),
     )
     if not row:
+        return None
+    return dict(row[0])
+
+
+async def get_current_user(request: Request):
+    user = await _resolve_request_credentials(request)
+    if user is None:
         raise HTTPException(
             status_code=status.HTTP_401_UNAUTHORIZED,
-            detail="User not found",
+            detail="Not authenticated",
         )
-    return dict(row[0])
+    return user
+
+
+async def get_optional_user(request: Request) -> dict | None:
+    """Same credential resolution as `get_current_user` but returns None
+    instead of raising on missing/invalid credentials. Use for endpoints
+    that serve both authenticated and anonymous traffic (e.g. media
+    files referenced by public pages)."""
+    return await _resolve_request_credentials(request)
 
 
 async def require_admin(user=Depends(get_current_user)):
@@ -159,7 +163,7 @@ async def ensure_admin_exists():
     db = await get_db()
     rows = await db.execute_fetchall("SELECT id FROM users WHERE role = 'admin'")
     if not rows:
-        pw_hash = hash_password(settings.ADMIN_PASS)
+        pw_hash = hash_password(settings.ADMIN_PASS.get_secret_value())
         await db.execute(
             "INSERT INTO users (username, password_hash, role) VALUES (?, ?, 'admin')",
             (settings.ADMIN_USER, pw_hash),

backend/app/config.py

@@ -1,11 +1,15 @@
+from pydantic import SecretStr
 from pydantic_settings import BaseSettings
 from pathlib import Path
 
 
 class Settings(BaseSettings):
-    SECRET_KEY: str = "change-me-to-random-string"
+    # Secrets use SecretStr so `repr(settings)` / a debug dump / a stray
+    # log line can't leak them. Read the plaintext at the point of use
+    # via `.get_secret_value()`.
+    SECRET_KEY: SecretStr = SecretStr("change-me-to-random-string")
     ADMIN_USER: str = "admin"
-    ADMIN_PASS: str = "admin"
+    ADMIN_PASS: SecretStr = SecretStr("admin")
 
     DATA_DIR: str = "./data"
     DB_PATH: str = "./data/just-wiki.db"
@@ -19,6 +23,14 @@ class Settings(BaseSettings):
     # in production, e.g. "https://wiki.example.com".
     ALLOWED_ORIGINS: str = ""
 
+    # When deployed behind a reverse proxy (nginx/Caddy/ALB/Traefik), the
+    # direct TCP peer is the proxy — rate limiters keyed on `request.client.host`
+    # then see every client as the same IP and either deny everyone or no-one.
+    # Setting this to True makes the rate limiters trust the left-most
+    # `X-Forwarded-For` entry. Only enable this when a trusted proxy is
+    # actually in front of the app; otherwise a client can spoof the header.
+    TRUST_PROXY: bool = False
+
     # Public base URL the browser can reach. Used to build OIDC redirect_uri
     # and SSO-error redirects. In dev leave as localhost:8000; in prod set to
     # e.g. "https://wiki.example.com".
@@ -37,27 +49,27 @@ class Settings(BaseSettings):
 
     # Google
     OIDC_GOOGLE_CLIENT_ID: str = ""
-    OIDC_GOOGLE_CLIENT_SECRET: str = ""
+    OIDC_GOOGLE_CLIENT_SECRET: SecretStr = SecretStr("")
     OIDC_GOOGLE_DISCOVERY: str = (
         "https://accounts.google.com/.well-known/openid-configuration"
     )
 
     # GitHub (non-OIDC OAuth2; no discovery URL)
     OIDC_GITHUB_CLIENT_ID: str = ""
-    OIDC_GITHUB_CLIENT_SECRET: str = ""
+    OIDC_GITHUB_CLIENT_SECRET: SecretStr = SecretStr("")
 
     # Generic OIDC (Keycloak / Authentik / Okta / self-hosted IdP)
     OIDC_GENERIC_NAME: str = "Company SSO"
     OIDC_GENERIC_CLIENT_ID: str = ""
-    OIDC_GENERIC_CLIENT_SECRET: str = ""
+    OIDC_GENERIC_CLIENT_SECRET: SecretStr = SecretStr("")
     OIDC_GENERIC_DISCOVERY: str = ""
 
     # ── LDAP / Active Directory (optional) ──
     LDAP_ENABLED: bool = False
     LDAP_SERVER: str = ""                # must be ldaps:// unless TLS disabled
     LDAP_TLS_VERIFY: bool = True
     LDAP_BIND_DN: str = ""
-    LDAP_BIND_PASSWORD: str = ""
+    LDAP_BIND_PASSWORD: SecretStr = SecretStr("")
     LDAP_USER_BASE: str = ""
     LDAP_USER_FILTER: str = "(&(objectClass=person)(uid={username}))"
     LDAP_ATTR_EMAIL: str = "mail"
@@ -74,14 +86,14 @@ class Settings(BaseSettings):
     # that speaks the same wire format works (OpenAI, Ollama, Groq, DeepSeek…).
     AI_ENABLED: bool = False
     AI_BASE_URL: str = "https://generativelanguage.googleapis.com/v1beta/openai"
-    AI_API_KEY: str = ""
+    AI_API_KEY: SecretStr = SecretStr("")
     AI_MODEL: str = "gemini-2.0-flash"
     AI_MAX_CONTEXT_PAGES: int = 5       # top-K pages stuffed into the prompt
     AI_EXCERPT_CHARS: int = 1500        # max chars per page content excerpt
     AI_RATE_LIMIT_PER_HOUR: int = 20    # per-user request cap
     # Legacy; kept as a read-only alias so existing deployments don't break.
     # Use AI_API_KEY instead.
-    GEMINI_API_KEY: str = ""
+    GEMINI_API_KEY: SecretStr = SecretStr("")
 
     # Dashboard / updates
     # Off by default so air-gapped installs never make an outbound call.

backend/app/database.py

@@ -1,3 +1,4 @@
+import asyncio
 import logging
 import re
 import sqlite3
@@ -13,6 +14,10 @@
 _TOKENIZE_RE = re.compile(r"""tokenize\s*=\s*['"](\w+)['"]""", re.IGNORECASE)
 
 _db: aiosqlite.Connection | None = None
+# Serialise the first-touch connection setup so two concurrent `get_db()`
+# calls (e.g. racing lifespan/boot-time tasks) can't both open a connection
+# and leak the loser.
+_db_lock = asyncio.Lock()
 
 SCHEMA_SQL = """
 CREATE TABLE IF NOT EXISTS users (
@@ -973,11 +978,17 @@ async def _ensure_fts5_index(db) -> bool:
 
 async def get_db() -> aiosqlite.Connection:
     global _db
-    if _db is None:
-        _db = await aiosqlite.connect(settings.DB_PATH)
-        _db.row_factory = aiosqlite.Row
-        await _db.execute("PRAGMA journal_mode=WAL")
-        await _db.execute("PRAGMA foreign_keys=ON")
+    if _db is not None:
+        return _db
+    async with _db_lock:
+        # Re-check after acquiring: another coroutine may have finished
+        # opening the connection while we were blocked on the lock.
+        if _db is None:
+            conn = await aiosqlite.connect(settings.DB_PATH)
+            conn.row_factory = aiosqlite.Row
+            await conn.execute("PRAGMA journal_mode=WAL")
+            await conn.execute("PRAGMA foreign_keys=ON")
+            _db = conn
     return _db
 
 
@@ -1065,9 +1076,10 @@ async def seed_welcome_page(db):
             shutil.copy2(logo_src, logo_dst)
             # Make sure it's readable
             os.chmod(logo_dst, 0o644)
-            print(f"Successfully seeded logo from {logo_src} to {logo_dst}")
-        except Exception as e:
-            print(f"Error seeding logo: {e}")
+            logger.info("Seeded logo from %s to %s", logo_src, logo_dst)
+        except OSError:
+            # Best-effort — missing/broken source file shouldn't block boot.
+            logger.warning("Logo seeding failed", exc_info=True)
 
     rows = await db.execute_fetchall("SELECT COUNT(*) as cnt FROM pages")
     if rows[0]["cnt"] > 0:

backend/app/main.py

@@ -25,7 +25,7 @@
 
 def _check_security():
     """Warn loudly about insecure defaults on startup."""
-    if settings.SECRET_KEY in _INSECURE_SECRETS:
+    if settings.SECRET_KEY.get_secret_value() in _INSECURE_SECRETS:
         safe_key = secrets.token_urlsafe(32)
         logger.critical(
             "\n"
@@ -37,11 +37,11 @@ def _check_security():
             "╚══════════════════════════════════════════════════════╝",
             safe_key[:44],
         )
-    if settings.ADMIN_PASS in {"admin", "password", "123456", ""}:
+    admin_pass = settings.ADMIN_PASS.get_secret_value()
+    if admin_pass in {"admin", "password", "123456", ""}:
         logger.warning(
-            "ADMIN_PASS is set to a weak default ('%s'). "
+            "ADMIN_PASS is set to a weak default. "
             "Change it in .env before deploying to production.",
-            settings.ADMIN_PASS,
         )
     # Either SSO path issues the session cookie (OIDC state/nonce/PKCE) over
     # the wire. Without TLS marking, a passive attacker on the path can
@@ -56,6 +56,38 @@ def _check_security():
                 "COOKIE_SECURE=true before exposing it."
             )
 
+    # Fail loudly if the SSO toggles are on but no usable provider is
+    # configured. Previously this surfaced only on the first login attempt
+    # as an opaque 500; catching it here saves operators a debugging loop.
+    if settings.OIDC_ENABLED:
+        providers = [p.strip() for p in settings.OIDC_PROVIDERS.split(",") if p.strip()]
+        if not providers:
+            logger.error(
+                "OIDC_ENABLED=true but OIDC_PROVIDERS is empty. "
+                "Login page will show no SSO buttons."
+            )
+        else:
+            any_configured = False
+            if "google" in providers and settings.OIDC_GOOGLE_CLIENT_ID and settings.OIDC_GOOGLE_CLIENT_SECRET.get_secret_value():
+                any_configured = True
+            if "github" in providers and settings.OIDC_GITHUB_CLIENT_ID and settings.OIDC_GITHUB_CLIENT_SECRET.get_secret_value():
+                any_configured = True
+            if "generic" in providers and settings.OIDC_GENERIC_CLIENT_ID and settings.OIDC_GENERIC_CLIENT_SECRET.get_secret_value() and settings.OIDC_GENERIC_DISCOVERY:
+                any_configured = True
+            if not any_configured:
+                logger.error(
+                    "OIDC_ENABLED=true but no provider is fully configured. "
+                    "Set the matching OIDC_{provider}_CLIENT_ID and _SECRET "
+                    "(and _DISCOVERY for 'generic')."
+                )
+
+    if settings.LDAP_ENABLED:
+        if not settings.LDAP_SERVER or not settings.LDAP_BIND_DN or not settings.LDAP_USER_BASE:
+            logger.error(
+                "LDAP_ENABLED=true but LDAP_SERVER/LDAP_BIND_DN/LDAP_USER_BASE "
+                "is missing. LDAP login will fail."
+            )
+
 
 @asynccontextmanager
 async def lifespan(app: FastAPI):
@@ -94,15 +126,17 @@ async def csrf_guard(request: Request, call_next):
     if request.url.path.rstrip("/") in _CSRF_EXEMPT_PATHS:
         return await call_next(request)
 
-    # Bearer-token requests don't ride on the browser-ambient cookie, so CSRF
-    # doesn't reach them. Skip the check.
-    auth_header = request.headers.get("Authorization", "")
-    if auth_header.startswith("Bearer "):
-        return await call_next(request)
-
     # If there's no session cookie, there's nothing to protect: any request
     # without credentials will be rejected by the route's auth dependency,
-    # and CSRF only matters for ambient-credential flows.
+    # and CSRF only matters for ambient-credential flows. This branch also
+    # covers legitimate Bearer-token clients (API tokens, tests), which never
+    # attach the session cookie.
+    #
+    # We deliberately do NOT exempt based on the Authorization header alone:
+    # `get_current_user` falls back to the cookie when a Bearer token is
+    # invalid, so a cross-origin attacker could otherwise pair a bogus
+    # `Authorization: Bearer xyz` with the victim's session cookie to skip
+    # CSRF protection on every mutating endpoint.
     if not request.cookies.get("token"):
         return await call_next(request)
 
@@ -149,7 +183,7 @@ def _origin_of(url: str | None) -> str | None:
 _session_https_only = settings.COOKIE_SECURE or settings.PUBLIC_BASE_URL.startswith("https://")
 app.add_middleware(
     SessionMiddleware,
-    secret_key=settings.SECRET_KEY,
+    secret_key=settings.SECRET_KEY.get_secret_value(),
     session_cookie="justwiki_oauth",
     same_site="lax",
     https_only=_session_https_only,

backend/app/migrations.py

@@ -98,6 +98,24 @@ async def _m006_auth_identities(db: aiosqlite.Connection) -> None:
     )
 
 
+async def _m007_groups_ldap_dn(db: aiosqlite.Connection) -> None:
+    """Add `ldap_dn` to `groups` so LDAP-mirrored groups can be reconciled.
+
+    A group with ldap_dn IS NOT NULL is considered fully managed by the LDAP
+    sync loop — user additions/removals during sync only touch those rows;
+    manually-created groups (ldap_dn IS NULL) are never pruned.
+    """
+    if not await _column_exists(db, "groups", "ldap_dn"):
+        await db.execute("ALTER TABLE groups ADD COLUMN ldap_dn TEXT")
+        # SQLite can't add UNIQUE via ALTER. A partial unique index is the
+        # idiomatic workaround and matches the semantics we want: only non-NULL
+        # DNs must be unique (multiple manual groups with NULL dn are fine).
+        await db.execute(
+            "CREATE UNIQUE INDEX IF NOT EXISTS idx_groups_ldap_dn "
+            "ON groups(ldap_dn) WHERE ldap_dn IS NOT NULL"
+        )
+
+
 async def _m008_api_tokens_extend(db: aiosqlite.Connection) -> None:
     """Extend api_tokens with prefix / expires_at / revoked_at.
 
@@ -128,24 +146,6 @@ async def _m009_page_type(db: aiosqlite.Connection) -> None:
         )
 
 
-async def _m007_groups_ldap_dn(db: aiosqlite.Connection) -> None:
-    """Add `ldap_dn` to `groups` so LDAP-mirrored groups can be reconciled.
-
-    A group with ldap_dn IS NOT NULL is considered fully managed by the LDAP
-    sync loop — user additions/removals during sync only touch those rows;
-    manually-created groups (ldap_dn IS NULL) are never pruned.
-    """
-    if not await _column_exists(db, "groups", "ldap_dn"):
-        await db.execute("ALTER TABLE groups ADD COLUMN ldap_dn TEXT")
-        # SQLite can't add UNIQUE via ALTER. A partial unique index is the
-        # idiomatic workaround and matches the semantics we want: only non-NULL
-        # DNs must be unique (multiple manual groups with NULL dn are fine).
-        await db.execute(
-            "CREATE UNIQUE INDEX IF NOT EXISTS idx_groups_ldap_dn "
-            "ON groups(ldap_dn) WHERE ldap_dn IS NOT NULL"
-        )
-
-
 MIGRATIONS: list[Migration] = [
     (1, "user_profile_columns", _m001_user_profile_columns),
     (2, "user_soft_delete", _m002_user_soft_delete),