fix: security review fixes, cookie sessions and Swiss brackets

.github/workflows/static-checks.yml

@@ -14,6 +14,24 @@ jobs:
     defaults:
       run:
         working-directory: backend
+    services:
+      postgres:
+        image: postgres:18
+        env:
+          POSTGRES_USER: bracket
+          POSTGRES_PASSWORD: bracket
+          POSTGRES_DB: bracket_test
+        ports:
+          - 5432:5432
+        options: >-
+          --health-cmd "pg_isready --username bracket"
+          --health-interval 3s
+          --health-timeout 5s
+          --health-retries 10
+    env:
+      ENVIRONMENT: DEVELOPMENT
+      JWT_SECRET: only-for-the-tests-and-at-least-32-characters-long
+      PG_DSN: postgresql://bracket:bracket@localhost:5432/bracket_test
     steps:
       - uses: actions/checkout@v7
 
@@ -34,8 +52,18 @@ jobs:
       - name: Check Ruff formatting
         run: uv run ruff format --check .
 
+      - name: Run Ruff
+        run: uv run ruff check .
+
       - name: Run mypy
-        run: uv run python -m mypy bracket
+        run: uv run python -m mypy bracket tests
+
+      - name: Run tests
+        # The tests create the tables from the metadata. Stamping the empty database at the latest
+        # migration first keeps the app from replaying every migration on top of them at startup.
+        run: |
+          uv run alembic stamp head
+          uv run pytest
 
   frontend:
     name: Frontend
@@ -49,7 +77,8 @@ jobs:
       - name: Set up pnpm
         uses: pnpm/action-setup@v6
         with:
-          version: 11
+          # Its `packageManager` field pins the version.
+          package_json_file: frontend/package.json
 
       - name: Set up Node.js
         uses: actions/setup-node@v6
@@ -63,3 +92,6 @@ jobs:
 
       - name: Run static checks
         run: pnpm run typecheck && pnpm run format:check
+
+      - name: Build
+        run: pnpm run build

.gitignore

@@ -13,7 +13,7 @@ node_modules
 coverage.xml
 coverage.json
 
-backend/static
+backend/uploads
 
 /process-compose.yml
 

README.md

@@ -37,7 +37,30 @@ POSTGRES_DB=... POSTGRES_USER=... POSTGRES_PASSWORD=... JWT_SECRET=... \
 On startup the backend connects to the database, runs Alembic migrations
 (`AUTO_RUN_MIGRATIONS`, on by default), and — if the database is empty and
 `ADMIN_EMAIL`/`ADMIN_PASSWORD` are set — creates the initial admin user.
-Health check: `GET /api/ping`.
+Health check: `GET /api/ping`. Uploaded logos are kept in the `bracket_uploads`
+volume.
+
+### Reverse proxy
+
+Both containers listen on `127.0.0.1` only; a reverse proxy on the host
+terminates TLS and routes `/api` to the backend (port 8400) and everything else
+to the frontend (port 3000).
+
+- **Client addresses.** Login and registration are rate limited per client IP.
+  The backend only trusts `X-Forwarded-For` from the addresses in
+  `FORWARDED_ALLOW_IPS`; otherwise every request seems to come from the proxy,
+  and all visitors share one limit. Docker forwards the published port from the
+  network's gateway, so set it to that address (the network is named
+  `<compose project>_bracket_lan`):
+
+  ```bash
+  docker network inspect bracket_bracket_lan -f '{{(index .IPAM.Config 0).Gateway}}'
+  ```
+
+  Don't use `*`: uvicorn then takes the left-most `X-Forwarded-For` entry, which
+  the client chooses.
+- **Request size.** Limit request bodies at the proxy (for example nginx's
+  `client_max_body_size 4m`); logos are at most `UPLOAD_MAX_BYTES`.
 
 ## Configuration
 
@@ -53,11 +76,13 @@ the `PRODUCTION` or `DEVELOPMENT` config class, which picks up `prod.env` or
 | `BASE_URL` | Public URL; also the default JWT issuer |
 | `API_PREFIX` | `/api` in our deployment; affects every route and the static mount |
 | `CORS_ORIGINS` | Comma-separated allowlist. `*` is rejected |
+| `CORS_ALLOW_CREDENTIALS` | Only for a frontend on another origin (see Sessions) |
 | `RATE_LIMIT_STORAGE_URI` | Must not be `memory://` in production (see below) |
 | `ALLOW_USER_REGISTRATION` | Self-service signup, enabled for our event |
 | `ADMIN_EMAIL` / `ADMIN_PASSWORD` | Only used to bootstrap an empty database |
 | `ACCESS_TOKEN_EXPIRE_MINUTES` | Minimum 5 |
 | `UPLOAD_DIR` / `UPLOAD_MAX_BYTES` | Team and tournament logo uploads |
+| `FORWARDED_ALLOW_IPS` | The reverse proxy's address (see above); read by uvicorn |
 
 ## Local development
 
@@ -67,11 +92,21 @@ Backend (needs [uv](https://docs.astral.sh/uv/) and a PostgreSQL instance):
 cd backend
 uv sync
 uv run ./cli.py create-dev-db     # seeds a database with dummy data
-uv run uvicorn bracket.app:app --reload --port 8400
+API_PREFIX=/api uv run uvicorn bracket.app:app --reload --port 8400
 ```
 
 `DevelopmentConfig` reads `backend/dev.env`, which is not committed. At minimum
-it needs `JWT_SECRET` and `PG_DSN`.
+it needs `JWT_SECRET` and `PG_DSN`. The interactive API docs (`/docs`, `/redoc`)
+are only served in development.
+
+Tests need a PostgreSQL database; stamp it first so that the app doesn't replay
+the migrations on top of the tables the tests create:
+
+```bash
+cd backend
+ENVIRONMENT=DEVELOPMENT JWT_SECRET=... PG_DSN=... uv run alembic stamp head
+ENVIRONMENT=DEVELOPMENT JWT_SECRET=... PG_DSN=... uv run pytest
+```
 
 Frontend:
 
@@ -82,9 +117,23 @@ pnpm install
 pnpm dev
 ```
 
+The dev server forwards `/api` to the backend on port 8400, so the page and the
+API share an origin, as they do in production.
+
 The SPA resolves its API base URL from `window.__BRACKET_RUNTIME_CONFIG__`
 (injected by the container entrypoint from `API_BASE_URL`), then
-`VITE_API_BASE_URL`, then `http://localhost:8400`. That indirection is what lets
-one frontend image be deployed against any backend URL without a rebuild.
-Set it to the backend **origin only** — the client appends `/api/...` itself, so
-a trailing `/api` or `/` is stripped before use.
+`VITE_API_BASE_URL`, and otherwise uses its own origin. Leave `API_BASE_URL`
+empty when the reverse proxy serves both from one origin. Otherwise set it to
+the backend **origin only** — the client appends `/api/...` itself, so a
+trailing `/api` or `/` is stripped before use.
+
+### Sessions
+
+Logging in sets an HttpOnly session cookie, so scripts on the page can't read
+the token; API clients can still send it as a bearer token. The cookie is
+`SameSite=Strict`, so the API must be on the same site as the frontend (for
+example the same domain). If it is on another origin of that site, also set
+`CORS_ALLOW_CREDENTIALS=true` next to `CORS_ORIGINS`. Requests that change
+something with the cookie need an `X-Requested-With: XMLHttpRequest` header,
+which other sites can't send (CSRF protection). Changing the password signs out
+every other session.

backend/Dockerfile

@@ -19,14 +19,15 @@ RUN uv export --frozen --no-dev --no-emit-project --output-file /tmp/requirement
     && python -m pip uninstall --yes uv \
     && rm -rf /root/.cache /tmp/requirements.txt
 
-COPY --chown=10001:10001 alembic.ini ./
-COPY --chown=10001:10001 cli.py ./
-COPY --chown=10001:10001 alembic ./alembic
-COPY --chown=10001:10001 bracket ./bracket
-COPY --chown=10001:10001 openapi ./openapi
-COPY --chown=10001:10001 static ./static
+# The code stays owned by root, so the app can't modify it. Uploaded logos are its only writes.
+COPY alembic.ini cli.py ./
+COPY alembic ./alembic
+COPY bracket ./bracket
+COPY openapi ./openapi
+RUN install --directory --owner=10001 --group=10001 uploads
 
 USER 10001:10001
 EXPOSE 8400
 
-CMD ["gunicorn", "-k", "uvicorn.workers.UvicornWorker", "bracket.app:app", "--bind", "0.0.0.0:8400", "--workers", "1", "--no-control-socket"]
+# uvicorn trusts the `X-Forwarded-For` of the proxies listed in `FORWARDED_ALLOW_IPS`.
+CMD ["uvicorn", "bracket.app:app", "--host", "0.0.0.0", "--port", "8400", "--no-server-header"]

backend/bracket/app.py

@@ -1,7 +1,7 @@
 from collections.abc import AsyncGenerator
 from contextlib import asynccontextmanager
 
-from fastapi import FastAPI, Request
+from fastapi import Depends, FastAPI, Request
 from secure import (
     ContentSecurityPolicy,
     PermissionsPolicy,
@@ -14,10 +14,8 @@
 from secure.middleware import SecureASGIMiddleware
 from slowapi.errors import RateLimitExceeded
 from slowapi.extension import _rate_limit_exceeded_handler
-from starlette.exceptions import HTTPException
 from starlette.middleware.cors import CORSMiddleware
 from starlette.responses import JSONResponse, Response
-from starlette.staticfiles import StaticFiles
 
 from bracket.config import Environment, config, environment
 from bracket.database import engine
@@ -74,44 +72,22 @@ async def lifespan(_: FastAPI) -> AsyncGenerator[None]:
     "Users": users.router,
 }
 
-table_of_contents = "\n\n".join(
-    [f"- [{tag}](#tag/{tag.replace(' ', '-')})" for tag in routers.keys()]
-)
-
-
-description = f"""
-### Description
-This API allows you to do everything the frontend of [Bracket](https://github.com/evroon/bracket)
-allows you to do (the frontend uses this API as well).
-
-Fore more information, see the [documentation](https://docs.bracketapp.nl).
-
-### Table of Contents
-*(links only work for [ReDoc](https://api.bracketapp.nl/redoc), not for Swagger UI)*
-
-{table_of_contents}
-
-### Links
-GitHub: <https://github.com/evroon/bracket>
-
-Docs: <https://docs.bracketapp.nl>
-
-API docs (Redoc): <https://api.bracketapp.nl/redoc>
-
-API docs (Swagger UI): <https://api.bracketapp.nl/docs>
-"""
+is_development = environment is Environment.DEVELOPMENT
 
 app = FastAPI(
     title="Bracket API",
-    docs_url="/docs",
     version="1.0.0",
     lifespan=lifespan,
-    summary="API for Bracket, an open source tournament system.",
-    description=description,
+    summary="API of the Ctrl-Alt-GG fork of Bracket, an open source tournament system.",
+    description="Everything the Bracket frontend does, it does through this API.",
     license_info={
         "name": "AGPL-3.0",
         "url": "https://www.gnu.org/licenses/agpl-3.0.en.html",
     },
+    # The interactive docs load Swagger UI and ReDoc from a CDN, which the production security
+    # headers don't allow, so only development serves them.
+    openapi_url="/openapi.json" if is_development else None,
+    dependencies=[Depends(auth.protect_session_from_csrf)],
 )
 
 
@@ -136,18 +112,17 @@ async def handle_rate_limit_exceeded(request: Request, exc: Exception) -> Respon
 app.add_middleware(
     SecureASGIMiddleware,
     secure=Secure(
-        csp=ContentSecurityPolicy()
-        .default_src("'self'")
-        .base_uri("'self'")
-        .frame_ancestors("'none'")
-        .form_action("'self'")
-        .img_src("'self'", "data:", "https:")
-        .style_src("'self'", "'unsafe-inline'")
-        .script_src("'self'", "'unsafe-inline'"),
+        # The API only answers with JSON and uploaded images, so nothing may load or frame it.
+        # Development leaves it out, so that the interactive docs work.
+        csp=(
+            None
+            if is_development
+            else ContentSecurityPolicy().default_src("'none'").frame_ancestors("'none'")
+        ),
         hsts=(
-            StrictTransportSecurity().max_age(31536000).include_subdomains()
-            if environment is Environment.PRODUCTION
-            else None
+            None
+            if is_development
+            else StrictTransportSecurity().max_age(31536000).include_subdomains()
         ),
         permissions=PermissionsPolicy().camera().geolocation().microphone(),
         referrer=ReferrerPolicy().no_referrer(),
@@ -157,11 +132,6 @@ async def handle_rate_limit_exceeded(request: Request, exc: Exception) -> Respon
 )
 
 
-@app.exception_handler(HTTPException)
-async def validation_exception_handler(request: Request, exc: HTTPException) -> JSONResponse:
-    return JSONResponse({"detail": exc.detail}, status_code=exc.status_code)
-
-
 @app.exception_handler(Exception)
 async def generic_exception_handler(request: Request, exc: Exception) -> JSONResponse:
     logger.exception(
@@ -170,8 +140,6 @@ async def generic_exception_handler(request: Request, exc: Exception) -> JSONRes
     return JSONResponse({"detail": "Internal server error"}, status_code=500)
 
 
-app.mount(f"{config.api_prefix}/static", StaticFiles(directory="static"), name="static")
-
 for tag, router in routers.items():
     assert router.prefix == config.api_prefix, f"Prefix not set on router with tag `{tag}`"
     app.include_router(router, tags=[tag])

backend/bracket/config.py

@@ -29,7 +29,6 @@ class Config(BaseSettings):
 
     admin_email: str | None = None
     admin_password: str | None = None
-    allow_insecure_http_sso: bool = False
     allow_user_registration: bool = True
     base_url: str = "http://localhost:8400"
     cors_origin_regex: str = ""
@@ -96,7 +95,6 @@ def validate_security_settings(self) -> "Config":
 
 class DevelopmentConfig(Config):
     admin_email: Annotated[str | None, Field("test@example.org")]
-    allow_insecure_http_sso: Annotated[bool, Field(True)]
     cors_origin_regex: Annotated[str, Field(r"^https?://(localhost|127\.0\.0\.1)(:\d+)?$")]
 
     model_config = SettingsConfigDict(env_file="dev.env")