From 1d0702b6e1516a9e6b65716484bde425ac7f0cb3 Mon Sep 17 00:00:00 2001 From: Amr Gaber Date: Sun, 28 Jun 2026 18:54:36 -0500 Subject: [PATCH] fix: harden Sentry budget, default Cache-Control to no-store, validate migrations on Postgres MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - sentry.py / logging.py: floor the metered Sentry Logs stream at WARNING (enable_logs=True was unfloored) and quiet httpx — its per-request INFO lines from OAuth calls were the firehose, and logs are a separately-billed budget. Pin transient upstream failures (OAuth provider / DB briefly unreachable) to one `upstream-unavailable` fingerprint so an outage is one rising-count issue rather than a storm (Sentry never samples errors). - main.py: add a default `no-store` Cache-Control middleware (caching is now opt-in) so per-user/token-bearing responses can't be cached by a proxy; the JWKS endpoint opts in to `public, max-age=3600` so verifiers keep caching keys. - ci.yml: add a `migrate` job that applies migrations against real Postgres 16 with a full downgrade->upgrade round-trip — the test suite builds its schema on SQLite and never runs Alembic. Validated locally against postgres:16. - add CLAUDE.md (conventions + production-hardening checklist). --- .github/workflows/ci.yml | 30 ++++++++++++++++ CLAUDE.md | 76 ++++++++++++++++++++++++++++++++++++++++ app/logging.py | 5 +++ app/main.py | 23 +++++++++++- app/sentry.py | 45 ++++++++++++++++++++++++ 5 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 CLAUDE.md diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2c32034..6b77018 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -24,3 +24,33 @@ jobs: - uses: astral-sh/setup-uv@v7 - run: uv sync - run: uv run pytest + + migrate: + # The test suite builds its schema on SQLite and never runs Alembic, so + # migrations are only exercised against real Postgres here: apply to head, + # then a full downgrade -> upgrade round-trip so an irreversible migration + # can't slip through. Validated locally against postgres:16. + runs-on: ubuntu-latest + services: + postgres: + image: postgres:16 + env: + POSTGRES_USER: postgres + POSTGRES_PASSWORD: postgres + POSTGRES_DB: criticalbit_auth + ports: + - 5432:5432 + options: >- + --health-cmd "pg_isready -U postgres" + --health-interval 10s + --health-timeout 5s + --health-retries 5 + env: + DATABASE_URL: postgresql+asyncpg://postgres:postgres@localhost:5432/criticalbit_auth + steps: + - uses: actions/checkout@v6 + - uses: astral-sh/setup-uv@v7 + - run: uv sync + - run: uv run alembic upgrade head + - run: uv run alembic downgrade base + - run: uv run alembic upgrade head diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 0000000..ce56454 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,76 @@ +# criticalbit-auth-api + +Shared authentication service for criticalbit.gg — email/password + OAuth (Google, +Steam), RS256 JWT SSO with a public JWKS, refresh-token rotation, RBAC, and +per-purpose consent. FastAPI + async Postgres (SQLAlchemy 2.0 + Alembic), +FastAPI-Users. See `README.md` for the full feature/endpoint tour. + +## How to run things + +```bash +uv sync +cp .env.example .env +uv run alembic upgrade head +uv run uvicorn app.main:app --reload + +# Checks (what CI runs) +uv run pytest # tests (async; in-memory SQLite) +uv run ruff check . # lint +uv run ruff format --check . # format (drop --check to apply) + +# Migrations +uv run alembic revision -m "describe change" # stub (review autogenerate) +uv run alembic upgrade head +``` + +CI (`.github/workflows/ci.yml`): `lint` + `test` + `migrate` (applies migrations +against real Postgres 16 — see hardening). `deploy.yml` ships to Cloud Run on +merge to `main` and prunes stale revisions. + +## Conventions + +- **Adding an identity provider is one file.** Drop a module in `app/providers/` + and register it in `app/providers/registry.py`; the unified provider router + mounts the login/associate routes. Don't add per-provider routes in `main.py`. +- **Caching is opt-in (default `no-store`).** `cache_control_middleware` + (`app/main.py`) defaults every response to `no-store` — this service returns + per-user / token-bearing data. Public, cacheable endpoints opt in explicitly; + the JWKS (`/auth/jwks`) sets `public, max-age=3600` so verifiers cache keys. +- **Rate limiting keys on the real client IP** via `client_ip` + (`app/limiting.py`) — `CF-Connecting-IP` validated against Cloudflare's edge + ranges, never the shared edge peer. +- **Tests build the schema on SQLite (`tests/conftest.py`) and never run + Alembic.** Migrations are validated against real Postgres only in the `migrate` + CI job. +- **Sentry**: `enable_logs` is on but the Logs stream is floored to WARNING and + `httpx` is quieted (logs are a separate metered budget); transient upstream + failures are pinned to one `upstream-unavailable` fingerprint (`app/sentry.py`). + +## Production hardening — gotchas learned under real live-event load + +> Provider-agnostic lessons from a sibling service's high-traffic launch. Several +> are already addressed here (checked); keep the rest in mind. + +- [x] **Prune stale Cloud Run revisions every deploy** (`deploy.yml`) — a + `minScale≥1` revision pins an instance + its DB pool even at 0 traffic; left + unpruned they saturate Postgres `max_connections`. +- [x] **Size the DB pool to autoscaling** (`app/config.py` `db_pool_*`) — total + connections ≈ (pool_size + max_overflow) × instances must stay under the cap. +- [x] **Rate-limit on the real client IP, not the edge peer** (`app/limiting.py`). +- [x] **Validate migrations against real Postgres** — tests are SQLite-only, so + the `migrate` CI job applies head plus a downgrade→upgrade round-trip on + postgres:16. +- [x] **Sentry meters errors / spans / logs independently** — `enable_logs=True` + drains the Logs budget unless floored; httpx INFO request-lines are the usual + firehose. Errors are never sampled, so transient upstream outages are + fingerprinted to avoid a storm (`app/sentry.py`, `app/logging.py`). +- [ ] **Schema changes under a rolling deploy: expand → transition → contract.** + A rolling deploy serves old + new revisions at once, so never drop/rename a + column the still-running revision reads. Three phases: add (dual-write) → move + reads (keep the old column) → drop. Aim for zero 5xx per rollover. +- [ ] **Log `type(e).__name__`, not just `str(e)`** — transport errors (httpx + timeouts/connect) stringify to `""`. The unhandled-exception handler already + records `exc_type`; keep that habit at other error sites. +- [ ] **Startup must not block on a remote dependency you redeploy through** — + this app binds without awaiting the DB (good); keep it that way so a degraded + DB can't prevent the deploy that fixes it. diff --git a/app/logging.py b/app/logging.py index ba0116a..0724886 100644 --- a/app/logging.py +++ b/app/logging.py @@ -51,6 +51,11 @@ def setup_logging() -> None: # Quiet noisy third-party loggers logging.getLogger("uvicorn.access").setLevel(logging.WARNING) + # httpx emits one INFO line per request; the OAuth providers (Google/Steam) + # are called per login, so at volume these dominate both Cloud Logging and + # the metered Sentry Logs stream. Keep warnings and above only. + logging.getLogger("httpx").setLevel(logging.WARNING) + logging.getLogger("httpcore").setLevel(logging.WARNING) def get_logger(name: str | None = None) -> structlog.stdlib.BoundLogger: diff --git a/app/main.py b/app/main.py index 65ba9f2..ff4f850 100644 --- a/app/main.py +++ b/app/main.py @@ -135,8 +135,13 @@ async def unhandled_exception_handler(request: Request, exc: Exception) -> JSONR @app.get("/auth/jwks", tags=["auth"]) -async def jwks(): +async def jwks(response: Response): """Public JWKS endpoint. Tool backends use this to verify JWTs.""" + # Public signing keys: safe and beneficial to cache. Opt in explicitly so + # the default `no-store` doesn't force verifiers to refetch keys on every + # token validation; a verifier re-fetches on an unknown `kid` when keys + # rotate. + response.headers["Cache-Control"] = "public, max-age=3600" return get_jwks() @@ -343,6 +348,22 @@ async def add_security_headers(request: Request, call_next) -> Response: return response +@app.middleware("http") +async def cache_control_middleware(request: Request, call_next) -> Response: + """Default every response to ``no-store``; caching is strictly opt-in. + + This is an auth service — almost everything it returns is per-user or + token-bearing, and a blanket cache on GETs would let shared/CDN caches and + browsers store sensitive responses. So the safe default is ``no-store``; an + endpoint that serves genuinely public data (e.g. the JWKS) opts in by + setting its own ``Cache-Control``, which this middleware leaves intact. + """ + response = await call_next(request) + if "cache-control" not in response.headers: + response.headers["Cache-Control"] = "no-store" + return response + + @app.middleware("http") async def request_id_middleware(request: Request, call_next) -> Response: """Assign a unique request ID to every request.""" diff --git a/app/sentry.py b/app/sentry.py index ac2e0da..c3a0cb9 100644 --- a/app/sentry.py +++ b/app/sentry.py @@ -4,10 +4,49 @@ StarletteIntegration can auto-instrument middleware. See app/main.py. """ +import logging + import sentry_sdk +from sentry_sdk.integrations.logging import LoggingIntegration from app.config import settings +# Transient infrastructure failures — an OAuth provider (Google/Steam, called +# over httpx) or the database briefly unreachable — are not application bugs. +# Sentry never samples errors, so without grouping, a short upstream outage +# under load floods the (un-sampled) errors budget with near-duplicate events. +# Pin them to one fingerprint so a blip is a single rising-count issue. Matched +# by exception class *name* to avoid importing httpx/asyncpg/sqlalchemy here. +_TRANSIENT_EXC_NAMES = frozenset( + { + # httpx (OAuth provider calls) + "ConnectError", + "ConnectTimeout", + "ReadTimeout", + "WriteTimeout", + "PoolTimeout", + "TimeoutException", + # asyncpg + "ConnectionDoesNotExistError", + "CannotConnectNowError", + # SQLAlchemy / DB driver + "OperationalError", + "InterfaceError", + "DBAPIError", + # stdlib socket-level + "ConnectionError", + "ConnectionResetError", + "ConnectionRefusedError", + } +) + + +def _before_send(event: dict, hint: dict) -> dict: + exc_info = hint.get("exc_info") + if exc_info and type(exc_info[1]).__name__ in _TRANSIENT_EXC_NAMES: + event["fingerprint"] = ["upstream-unavailable"] + return event + def init_sentry() -> None: if not settings.sentry_dsn: @@ -23,4 +62,10 @@ def init_sentry() -> None: profile_session_sample_rate=1.0, profile_lifecycle="trace", enable_logs=True, + # Floor the metered Sentry Logs stream at WARNING so high-volume INFO + # records (notably httpx request lines from OAuth calls) don't drain the + # separately-billed Logs budget. Full INFO still flows to stdout → + # Cloud Logging, and error capture (event_level) is unchanged. + integrations=[LoggingIntegration(sentry_logs_level=logging.WARNING)], + before_send=_before_send, )