From dbd6d5b8b522a240652e3f601a0fe9b4947ac978 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 4 Nov 2026 19:32:20 -0500 Subject: [PATCH] feat(6838): read the client address and HTTPS from the trusted proxy hop grimoire/proxy.py is the one reader of X-Forwarded-For and X-Forwarded-Proto. It takes the entry TRUSTED_PROXY_HOPS in from the right (default 1; a shorter header reads its leftmost entry; 0 trusts no proxy), so an entry a client puts left of the proxy's is never believed. Before this, with TRUST_PROXY_HEADERS on, the leftmost X-Forwarded-For entry keyed the login, register, forgot and reset rate limits and the audit log, and a client could choose it. The session cookie is Secure, and HSTS (max-age=31536000) is sent, exactly when the request is HTTPS by the trusted proxy's word. Plain HTTP is untouched and nothing redirects. SECURE_COOKIES is gone (a startup warning says so when it is still set); TRUST_PROXY_HEADERS=false still maps to 0 hops, with a warning to move to TRUSTED_PROXY_HOPS. Admin audit lines now use the same client address instead of the socket peer. The placeholder-key refusal keyed on SECURE_COOKIES, so it now refuses either placeholder key outright; the instance makes its own key when none is set. Family idea #5004 (IsHTTPS, trusted-hop X-Forwarded-Proto), canon v2. Co-Authored-By: Claude Opus 5.5 --- README.md | 11 +-- docker-compose.prod.yml | 5 +- docker-compose.yml | 6 +- docs/architecture.md | 4 +- docs/configuration.md | 18 ++-- src/grimoire/app.py | 23 +++-- src/grimoire/config.py | 34 +++++-- src/grimoire/proxy.py | 80 +++++++++++++++ src/grimoire/rate_limit.py | 6 +- src/grimoire/routes/admin.py | 29 +++--- src/grimoire/routes/auth.py | 48 ++++----- tests/test_proxy.py | 187 +++++++++++++++++++++++++++++++++++ 12 files changed, 369 insertions(+), 82 deletions(-) create mode 100644 src/grimoire/proxy.py create mode 100644 tests/test_proxy.py diff --git a/README.md b/README.md index 388d47d8..6d449355 100644 --- a/README.md +++ b/README.md @@ -28,10 +28,10 @@ services: environment: DATABASE_URL: "postgresql+asyncpg://${POSTGRES_USER:-grimoire}:${POSTGRES_PASSWORD:-grimoire}@db:5432/${POSTGRES_DB:-grimoire}" LOG_LEVEL: "${LOG_LEVEL:-INFO}" - # Behind an HTTPS reverse proxy: + # Port 5000 is published straight to clients, so no proxy is trusted. + # Behind a reverse proxy, set 1 (one per proxy) and BASE_URL: + TRUSTED_PROXY_HOPS: "0" # BASE_URL: "https://grimoire.example.com" - # SECURE_COOKIES: "true" - # TRUST_PROXY_HEADERS: "true" volumes: - app_data:/data depends_on: @@ -75,8 +75,7 @@ Everything is optional. Set these in the environment or in a `.env` file beside | Variable | When | What it does | |----------|------|--------------| | `BASE_URL` | Behind a proxy | The public URL, used in email links and SSO redirects | -| `SECURE_COOKIES` | Over HTTPS | `true` when Grimoire is served over HTTPS | -| `TRUST_PROXY_HEADERS` | Behind a proxy | `true` to read the client address from the proxy's headers | +| `TRUSTED_PROXY_HOPS` | Behind a proxy | How many reverse proxies stand in front of Grimoire (default `1`; the compose file above sets `0` because it publishes the port directly). The client address and HTTPS are read from the proxy's headers by it | | `POSTGRES_PASSWORD` | Before the first start | The database password (defaults to `grimoire`; the database is reachable only inside the compose network). Postgres takes it once, when the volume is created | | `SECRET_KEY` | Rarely | A key you manage yourself, instead of the one Grimoire keeps. It wins over the kept key; remove it to go back | @@ -132,7 +131,7 @@ The full list is in [Features](docs/features.md). | What you see | Why | Fix | |--------------|-----|-----| -| `SECRET_KEY is set to the public placeholder` at startup | `SECURE_COOKIES=true` while `SECRET_KEY` is `dev-secret-change-me` | Remove `SECRET_KEY`, or set a real one | +| `SECRET_KEY is set to the public placeholder` at startup | `SECRET_KEY` is `dev-secret-change-me` or `change-me-in-production` | Remove `SECRET_KEY`, or set a real one | | Settings asks for forge tokens or the backup secret again | The instance's key changed: the `app_data` volume was replaced, or `SECRET_KEY` was set or removed | Enter them again, or restore the previous key | | `Can't locate revision identified by '…'` at startup | An older image is running against a database a newer one has already migrated | Run the newer image; migrations only move forward | | The plugin's tools are missing in Claude Code | The plugin isn't installed, or Claude Code hasn't reloaded it | `/plugin` to install or update it, then `/reload-plugins` | diff --git a/docker-compose.prod.yml b/docker-compose.prod.yml index a575577b..382fe772 100644 --- a/docker-compose.prod.yml +++ b/docker-compose.prod.yml @@ -8,8 +8,9 @@ services: # a Docker secret and sets SECRET_KEY_FILE to its path). SECRET_KEY: "${SECRET_KEY}" LOG_LEVEL: "${LOG_LEVEL:-INFO}" - TRUST_PROXY_HEADERS: "true" - SECURE_COOKIES: "true" + # Proxies in front of the app (Traefik = 1). The client address and + # HTTPS are read from that far in from the right of X-Forwarded-*. + TRUSTED_PROXY_HOPS: "1" volumes: - app_data:/data networks: diff --git a/docker-compose.yml b/docker-compose.yml index 0a231d78..666ec150 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -13,10 +13,10 @@ services: environment: DATABASE_URL: "postgresql+asyncpg://${POSTGRES_USER:-grimoire}:${POSTGRES_PASSWORD:-grimoire}@db:5432/${POSTGRES_DB:-grimoire}" LOG_LEVEL: "${LOG_LEVEL:-INFO}" - # Behind an HTTPS reverse proxy: + # Port 5000 is published straight to clients, so no proxy is trusted. + # Behind a reverse proxy, set 1 (one per proxy) and BASE_URL: + TRUSTED_PROXY_HOPS: "0" # BASE_URL: "https://grimoire.example.com" - # SECURE_COOKIES: "true" - # TRUST_PROXY_HEADERS: "true" volumes: - app_data:/data depends_on: diff --git a/docs/architecture.md b/docs/architecture.md index 2ecc9e19..4611f3bc 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -69,10 +69,10 @@ Every database wait on startup has a deadline. Migrations wait for the database ## Authentication -- **Sessions**: `HttpOnly`, `SameSite=Lax` cookies, `Secure` when `SECURE_COOKIES=true`. Changing a password bumps the user's session version and signs out their other sessions. +- **Sessions**: `HttpOnly`, `SameSite=Lax` cookies, `Secure` on a request the trusted proxy reports as HTTPS, which also gets HSTS (`grimoire/proxy.py`). Changing a password bumps the user's session version and signs out their other sessions. - **API keys**: `Authorization: Bearer fmcp_...`, stored hashed. A `read` key may only call read endpoints and read MCP tools. - **OIDC** single sign-on with PKCE, optionally replacing local logins ([SSO / OAuth](sso-oauth.md)). -- **Rate limits** on register, login and forgot-password, per client IP (`TRUST_PROXY_HEADERS` reads the IP from the proxy's headers). +- **Rate limits** on register, login and forgot-password, per client IP, read from `X-Forwarded-For` by `TRUSTED_PROXY_HOPS`. Credentials Grimoire stores for outside services (forge tokens, the backup destination's S3 secret) are sealed under a key derived from `SECRET_KEY`, which the instance makes on first start and keeps in `/data/secret_key` unless one is set. diff --git a/docs/configuration.md b/docs/configuration.md index 5c437bae..ab527a60 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -14,8 +14,7 @@ Any variable marked *secret* can also be read from a file: set `_FILE` to | `SECRET_KEY` *(secret)* | kept in `DATA_DIR` | Signs sessions and seals stored credentials. Unset, the instance makes its own on first start and keeps it in `DATA_DIR/secret_key`. Set it only to manage the key yourself; it wins over the kept one (see below) | | `DATA_DIR` | `/data` | Where the instance keeps its own `SECRET_KEY`. Mount a volume here | | `BASE_URL` | `http://localhost:5000` | Public URL, used in email links and OIDC redirects | -| `SECURE_COOKIES` | `false` | `true` when Grimoire is served over HTTPS | -| `TRUST_PROXY_HEADERS` | `false` | `true` behind a reverse proxy, to read the client IP from `X-Forwarded-For` / `X-Real-IP` | +| `TRUSTED_PROXY_HOPS` | `1` | How many reverse proxies stand in front of Grimoire. The client address and the scheme are read that many entries in from the right of `X-Forwarded-For` and `X-Forwarded-Proto`; `0` ignores both headers. See [Behind a reverse proxy](#behind-a-reverse-proxy) | | `LOG_LEVEL` | `INFO` | `DEBUG`, `INFO`, `WARNING` or `ERROR` | | `LOG_RETENTION_DAYS` | `90` | Days of app logs to keep | | `FASTEMBED_CACHE_DIR` | `/data/fastembed-cache` | Where the embedding model is stored after its first download | @@ -77,7 +76,7 @@ docker compose -f docker-compose.dev.yml up --build ### Docker Swarm -[`docker-compose.prod.yml`](../docker-compose.prod.yml) is a stack file: the app and database on a private overlay network, with `SECURE_COOKIES` and `TRUST_PROXY_HEADERS` on. A Swarm can move the app to another node, away from a node-local `/data` volume, so the stack sets its own key: provide `DB_PASSWORD` and `SECRET_KEY` in the environment (or mount a Docker secret and set `SECRET_KEY_FILE`): +[`docker-compose.prod.yml`](../docker-compose.prod.yml) is a stack file: the app and database on a private overlay network, trusting one proxy (`TRUSTED_PROXY_HOPS: "1"`). A Swarm can move the app to another node, away from a node-local `/data` volume, so the stack sets its own key: provide `DB_PASSWORD` and `SECRET_KEY` in the environment (or mount a Docker secret and set `SECRET_KEY_FILE`): ```bash DB_PASSWORD=... SECRET_KEY=... docker stack deploy -c docker-compose.prod.yml grimoire @@ -96,13 +95,12 @@ Back up the database with **Settings → Data → Scheduled backups** ([Backups] ## Behind a reverse proxy -Grimoire doesn't terminate TLS. Put it behind Nginx, Traefik or Caddy, forward to port 5000, and keep port 5000 off the internet. Set `SECURE_COOKIES=true`, `TRUST_PROXY_HEADERS=true` and `BASE_URL`. +Grimoire doesn't terminate TLS. Put it behind Nginx, Traefik or Caddy, forward to port 5000, and keep port 5000 off the internet. Set `BASE_URL`, and `TRUSTED_PROXY_HOPS` to the number of proxies a request passes through (one proxy is `1`, the default; Cloudflare in front of Traefik is `2`). ```nginx location / { proxy_pass http://127.0.0.1:5000; proxy_set_header Host $host; - proxy_set_header X-Real-IP $remote_addr; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; @@ -112,12 +110,16 @@ location / { } ``` -With `TRUST_PROXY_HEADERS=true`, make sure the proxy replaces any `X-Forwarded-For` the client sends. +Each proxy appends its entry to `X-Forwarded-For` and `X-Forwarded-Proto`, so Grimoire reads the entry `TRUSTED_PROXY_HOPS` in from the right: the one your own proxy wrote. Whatever a client puts to the left of it is ignored. A header with fewer entries than that (a LAN road with one proxy beside a Cloudflare road with two) is read at its leftmost entry, the one the first proxy wrote. + +When that entry says `https`, the session cookie is marked `Secure` and the response carries `Strict-Transport-Security: max-age=31536000`. Over plain HTTP neither is sent, and Grimoire never redirects to HTTPS, so a LAN install keeps working. + +A hop count higher than the real number of proxies lets a client choose its own address, so count them. With `0`, the headers are ignored and the socket peer is the client: use it when clients reach port 5000 directly. ## Security checklist -- **Keep the key.** Forge tokens and the backup destination's S3 secret are sealed under a key derived from `SECRET_KEY`, so a copy of the database alone can't read them. Unset, the instance makes a random key and keeps it in `/data/secret_key`, readable by its owner only; keep that volume. If the key changes (say, a restore onto an instance with a different key), Settings marks those credentials and asks for them again; nothing else breaks. The app warns at startup when `SECRET_KEY` is a public placeholder or `/data` is not a mounted volume, and refuses to start with `dev-secret-change-me` when `SECURE_COOKIES=true`. -- **`SECURE_COOKIES=true`** whenever Grimoire is served over HTTPS. +- **Keep the key.** Forge tokens and the backup destination's S3 secret are sealed under a key derived from `SECRET_KEY`, so a copy of the database alone can't read them. Unset, the instance makes a random key and keeps it in `/data/secret_key`, readable by its owner only; keep that volume. If the key changes (say, a restore onto an instance with a different key), Settings marks those credentials and asks for them again; nothing else breaks. The app warns at startup when `SECRET_KEY` is a public placeholder or `/data` is not a mounted volume, and refuses to start with a placeholder key (`dev-secret-change-me`, `change-me-in-production`). +- **`TRUSTED_PROXY_HOPS`** matches the number of proxies in front of Grimoire, so rate limits and the audit log see the real client and the cookie is `Secure` over HTTPS. - **Registration** is open only for the first account. Invite people, or open it briefly from Settings. - **Signing everyone else out**: changing a password ends that user's other sessions; Settings → Account has a button for it too. - **API keys** are per user, shown once, and can be read-only. Revoke any you no longer use under Settings → MCP Access. diff --git a/src/grimoire/app.py b/src/grimoire/app.py index 6d278ede..2b840635 100644 --- a/src/grimoire/app.py +++ b/src/grimoire/app.py @@ -6,7 +6,8 @@ from pathlib import Path from quart import Quart, g, jsonify, make_response, request, send_from_directory -from grimoire.config import PLACEHOLDER_SECRET_KEYS, SECRET_KEY_FILENAME, Config +from grimoire.config import SECRET_KEY_FILENAME, Config +from grimoire.proxy import PerRequestSecureSession, apply_https_headers from grimoire.routes.admin import admin_bp from grimoire.routes.api import api from grimoire.routes.auth import auth_bp @@ -61,13 +62,13 @@ def create_app() -> Quart: app.secret_key = Config.SECRET_KEY app.config["SESSION_COOKIE_HTTPONLY"] = True app.config["SESSION_COOKIE_SAMESITE"] = "Lax" - app.config["SESSION_COOKIE_SECURE"] = Config.SECURE_COOKIES + # Secure is decided per request, from the trusted proxy's X-Forwarded-Proto. + app.session_interface = PerRequestSecureSession() - if Config.SECRET_KEY in PLACEHOLDER_SECRET_KEYS: + if os.environ.get("SECURE_COOKIES"): logger.warning( - "SECRET_KEY is set to a public placeholder, so session cookies are insecure. " - "Remove it to let the instance keep its own key in %s, or set a real one.", - Config.DATA_DIR, + "SECURE_COOKIES is not used: the session cookie is Secure on every request " + "the trusted proxy reports as HTTPS. Remove it from the environment." ) if Config.SECRET_KEY_SOURCE == "ephemeral": logger.warning( @@ -82,11 +83,12 @@ def create_app() -> Quart: Config.DATA_DIR, SECRET_KEY_FILENAME, ) - if not Config.TRUST_PROXY_HEADERS: + if os.environ.get("TRUST_PROXY_HEADERS") and not os.environ.get("TRUSTED_PROXY_HOPS"): logger.warning( - "TRUST_PROXY_HEADERS is not set. If this instance is behind a reverse proxy " - "(nginx, Caddy, Traefik) set TRUST_PROXY_HEADERS=true so rate limiting uses " - "real client IPs rather than the proxy IP." + "TRUST_PROXY_HEADERS is replaced by TRUSTED_PROXY_HOPS (now %d). Set " + "TRUSTED_PROXY_HOPS to the number of proxies in front of Grimoire and " + "remove TRUST_PROXY_HEADERS.", + Config.TRUSTED_PROXY_HOPS, ) app.register_blueprint(admin_bp) @@ -159,6 +161,7 @@ def create_app() -> Quart: except Exception: logger.debug("Failed to log usage", exc_info=True) + apply_https_headers(response) response.headers.setdefault("X-Content-Type-Options", "nosniff") response.headers.setdefault("X-Frame-Options", "DENY") response.headers.setdefault("Referrer-Policy", "strict-origin-when-cross-origin") diff --git a/src/grimoire/config.py b/src/grimoire/config.py index d0a11a62..5b409ad0 100644 --- a/src/grimoire/config.py +++ b/src/grimoire/config.py @@ -85,6 +85,21 @@ def _secret_key() -> tuple[str, str]: return secrets.token_hex(32), "ephemeral" +def _trusted_proxy_hops() -> int: + """TRUSTED_PROXY_HOPS, default 1. The older TRUST_PROXY_HEADERS switch is + still read when the hop count is unset: false means 0 hops. A value that + is not a whole number reads as -1, which validate() refuses.""" + raw = os.environ.get("TRUSTED_PROXY_HOPS", "").strip() + if raw: + try: + return int(raw) + except ValueError: + return -1 + if os.environ.get("TRUST_PROXY_HEADERS", "").strip().lower() in ("0", "false", "no"): + return 0 + return 1 + + class Config: DATABASE_URL: str = _read_secret( "DATABASE_URL", @@ -95,7 +110,6 @@ class Config: # shipped compose file, beside the model cache and diagnostics. DATA_DIR: str = os.environ.get("DATA_DIR", "/data") SECRET_KEY, SECRET_KEY_SOURCE = _secret_key() - SECURE_COOKIES: bool = os.environ.get("SECURE_COOKIES", "").lower() in ("1", "true", "yes") LOG_LEVEL: str = os.environ.get("LOG_LEVEL", "INFO") LOG_RETENTION_DAYS: int = int(os.environ.get("LOG_RETENTION_DAYS", "90")) @@ -108,7 +122,10 @@ class Config: SMTP_FROM_NAME: str = os.environ.get("SMTP_FROM_NAME", "Fabled Grimoire") SMTP_USE_TLS: bool = os.environ.get("SMTP_USE_TLS", "true").lower() in ("1", "true", "yes") BASE_URL: str = os.environ.get("BASE_URL", "http://localhost:5000").rstrip("/") - TRUST_PROXY_HEADERS: bool = os.environ.get("TRUST_PROXY_HEADERS", "").lower() in ("1", "true", "yes") + # How many reverse proxies stand in front of the app; the client address + # and scheme are read that many entries in from the right of the + # forwarding headers (grimoire/proxy.py). 0 trusts no proxy. + TRUSTED_PROXY_HOPS: int = _trusted_proxy_hops() # Git URL of the repo that ships this Grimoire plugin. There is one correct # value per deployment (the repo serving the plugin), so it's a config @@ -163,11 +180,16 @@ class Config: errors.append(f"SMTP_PORT={cls.SMTP_PORT} must be between 1 and 65535") if cls.oidc_enabled() and not cls.BASE_URL.startswith(("http://", "https://")): errors.append(f"BASE_URL='{cls.BASE_URL}' must start with http:// or https:// when OIDC is enabled") - if cls.SECRET_KEY == "dev-secret-change-me" and cls.SECURE_COOKIES: + if cls.TRUSTED_PROXY_HOPS < 0: errors.append( - "SECRET_KEY is set to the public placeholder 'dev-secret-change-me' but " - "SECURE_COOKIES=true indicates a production deployment. Remove SECRET_KEY " - "to let the instance keep its own key in DATA_DIR, or set a real one." + f"TRUSTED_PROXY_HOPS={os.environ.get('TRUSTED_PROXY_HOPS')!r} must be a whole " + "number of proxies, 0 or more" + ) + if cls.SECRET_KEY in PLACEHOLDER_SECRET_KEYS: + errors.append( + f"SECRET_KEY is set to the public placeholder {cls.SECRET_KEY!r}, so anyone " + "could forge a session. Remove SECRET_KEY to let the instance keep its own " + "key in DATA_DIR, or set a real one." ) if errors: raise ValueError("Configuration errors:\n" + "\n".join(f" - {e}" for e in errors)) diff --git a/src/grimoire/proxy.py b/src/grimoire/proxy.py new file mode 100644 index 00000000..1c3082ae --- /dev/null +++ b/src/grimoire/proxy.py @@ -0,0 +1,80 @@ +"""What the trusted reverse proxy says about a request: who sent it, and +whether it arrived over HTTPS. + +Grimoire serves plain HTTP; TLS ends at the operator's proxy. The proxy +reports the client's address in X-Forwarded-For and the scheme in +X-Forwarded-Proto, and each proxy on the road APPENDS its entry, so only the +right-hand end of either header was written by a proxy we trust. Anything to +the left of that is whatever the client chose to send. + +`TRUSTED_PROXY_HOPS` says how many proxies stand in front of the app (default +1). The entry read is that many in from the right. Zero trusts no proxy: the +headers are ignored and the socket peer is the client. + +Never read X-Forwarded-For or X-Forwarded-Proto anywhere else. The client +address keys the rate limits and the audit log, and the scheme decides the +session cookie's Secure flag and HSTS. A second reader is a second trust rule. +""" +from __future__ import annotations + +from quart import has_request_context, request +from quart.sessions import SecureCookieSessionInterface + +from grimoire.config import Config + +# HSTS for a year, sent only on a request the trusted proxy says is HTTPS. +# No includeSubDomains or preload: the app does not own the operator's domain. +HSTS_VALUE = "max-age=31536000" + + +def trusted_entry(header: str, hops: int) -> str | None: + """The entry `hops` in from the right of a forwarding header. + + A header shorter than that is read at its leftmost entry, the one the + first proxy wrote. A road with fewer proxies than the count (a LAN road + beside a Cloudflare one) still reads right that way. None when no proxy + is trusted or there is no header. Pure, so the trust boundary is + testable without a request.""" + if hops <= 0: + return None + entries = [part.strip() for part in header.split(",") if part.strip()] + if not entries: + return None + return entries[-min(hops, len(entries))] + + +def _forwarded(name: str) -> str | None: + # Joined, because a header sent twice arrives as two values, and each + # proxy's append may land on either one. + return trusted_entry(",".join(request.headers.getlist(name)), Config.TRUSTED_PROXY_HOPS) + + +def client_ip() -> str: + """The caller's address, as the trusted proxy saw it.""" + if not has_request_context(): + return "" + return (_forwarded("X-Forwarded-For") or request.remote_addr or "")[:64] + + +def is_https() -> bool: + """Whether this request arrived over HTTPS, by the trusted proxy's word.""" + if not has_request_context(): + return False + if request.scheme == "https": + return True + return (_forwarded("X-Forwarded-Proto") or "").lower() == "https" + + +class PerRequestSecureSession(SecureCookieSessionInterface): + """The session cookie is Secure exactly when the request is HTTPS, so a + public deployment gets it and a plain-HTTP one keeps working.""" + + def get_cookie_secure(self, app) -> bool: + return is_https() + + +def apply_https_headers(response) -> None: + """HSTS on an HTTPS request, and only then. The app never redirects to + HTTPS; plain HTTP is a supported way to run it.""" + if is_https(): + response.headers.setdefault("Strict-Transport-Security", HSTS_VALUE) diff --git a/src/grimoire/rate_limit.py b/src/grimoire/rate_limit.py index 91e0c627..daad041d 100644 --- a/src/grimoire/rate_limit.py +++ b/src/grimoire/rate_limit.py @@ -2,9 +2,9 @@ IMPORTANT — deployment note: Rate limit counters are stored in memory and are lost on process restart. - When deployed behind a reverse proxy (nginx, Caddy, Traefik) you MUST set - TRUST_PROXY_HEADERS=true so that the real client IP is used as the bucket key - rather than the proxy's IP (which would cause all users to share one bucket). + The bucket key is the client address from grimoire/proxy.py, read by + TRUSTED_PROXY_HOPS. A hop count that is too low keys every caller on the + proxy's address, so they all share one bucket. """ import asyncio diff --git a/src/grimoire/routes/admin.py b/src/grimoire/routes/admin.py index e1baead5..e09a5f48 100644 --- a/src/grimoire/routes/admin.py +++ b/src/grimoire/routes/admin.py @@ -4,6 +4,7 @@ import json from quart import Blueprint, Response, g, jsonify, request from grimoire.auth import admin_required, login_required, get_current_user_id +from grimoire.proxy import client_ip from grimoire.services.auth import ( create_invitation, delete_user, @@ -46,7 +47,7 @@ async def backup(): else: data = await export_user_backup(uid) - await log_audit("backup", user_id=uid, username=g.user.username, ip_address=request.remote_addr, details={"scope": scope}) + await log_audit("backup", user_id=uid, username=g.user.username, ip_address=client_ip(), details={"scope": scope}) return Response( json.dumps(data, indent=2, default=str), content_type="application/json", @@ -79,7 +80,7 @@ async def restore(): stats = await restore_full_backup(data) uid = get_current_user_id() - await log_audit("restore", user_id=uid, username=g.user.username, ip_address=request.remote_addr, details={"stats": stats}) + await log_audit("restore", user_id=uid, username=g.user.username, ip_address=client_ip(), details={"stats": stats}) return jsonify({"status": "ok", "stats": stats}) @@ -100,7 +101,7 @@ async def remove_user(user_id: int): deleted = await delete_user(user_id) if not deleted: return jsonify({"error": "User not found"}), 404 - await log_audit("user_delete", user_id=current_uid, username=g.user.username, ip_address=request.remote_addr, details={"deleted_user_id": user_id}) + await log_audit("user_delete", user_id=current_uid, username=g.user.username, ip_address=client_ip(), details={"deleted_user_id": user_id}) return jsonify({"status": "ok"}) @@ -121,7 +122,7 @@ async def toggle_registration(): uid = get_current_user_id() await set_registration_open(uid, bool(open_val)) - await log_audit("registration_toggle", user_id=uid, username=g.user.username, ip_address=request.remote_addr, details={"open": bool(open_val)}) + await log_audit("registration_toggle", user_id=uid, username=g.user.username, ip_address=client_ip(), details={"open": bool(open_val)}) return jsonify({"status": "ok", "open": bool(open_val)}) @@ -151,7 +152,7 @@ async def update_smtp(): if settings_to_save: await set_settings_batch(uid, settings_to_save) - await log_audit("smtp_config", user_id=uid, username=g.user.username, ip_address=request.remote_addr) + await log_audit("smtp_config", user_id=uid, username=g.user.username, ip_address=client_ip()) return jsonify({"status": "ok"}) @@ -166,7 +167,7 @@ async def test_smtp(): uid = get_current_user_id() try: await send_test_email(recipient) - await log_audit("smtp_test", user_id=uid, username=g.user.username, ip_address=request.remote_addr, details={"recipient": recipient}) + await log_audit("smtp_test", user_id=uid, username=g.user.username, ip_address=client_ip(), details={"recipient": recipient}) return jsonify({"status": "ok"}) except Exception as e: return jsonify({"error": str(e)}), 500 @@ -209,7 +210,7 @@ async def update_forge_webhook_settings(): # The secret is deliberately absent from the audit detail. await log_audit( "forge_webhook_config", user_id=uid, username=g.user.username, - ip_address=request.remote_addr, details={}, + ip_address=client_ip(), details={}, ) return jsonify({"status": "ok"}) @@ -262,7 +263,7 @@ async def update_base_url(): return jsonify({"error": "Base URL must use http or https"}), 400 uid = get_current_user_id() await set_setting(uid, "base_url", url) - await log_audit("base_url_config", user_id=uid, username=g.user.username, ip_address=request.remote_addr, details={"base_url": url}) + await log_audit("base_url_config", user_id=uid, username=g.user.username, ip_address=client_ip(), details={"base_url": url}) return jsonify({"status": "ok"}) @@ -310,7 +311,7 @@ async def update_db_maintenance(): uid = get_current_user_id() await log_audit( "db_maintenance_config", user_id=uid, username=g.user.username, - ip_address=request.remote_addr, details={"enabled": enabled, "hour": hour}, + ip_address=client_ip(), details={"enabled": enabled, "hour": hour}, ) return jsonify({"status": "ok", "enabled": enabled, "hour": hour}) @@ -323,7 +324,7 @@ async def run_db_maintenance_now(): uid = get_current_user_id() await log_audit( "db_maintenance_run", user_id=uid, username=g.user.username, - ip_address=request.remote_addr, + ip_address=client_ip(), ) summary = await run_maintenance() return jsonify(summary) @@ -389,7 +390,7 @@ async def update_backups(): uid = get_current_user_id() await log_audit( "backup_config", user_id=uid, username=g.user.username, - ip_address=request.remote_addr, + ip_address=client_ip(), details={**config.to_dict(), "secret_changed": bool( data.get("clear_s3_secret") or (isinstance(secret, str) and secret and secret != SECRET_MASK) )}, @@ -439,7 +440,7 @@ async def run_backup_now(): return jsonify({"error": "A backup is already running."}), 409 uid = get_current_user_id() await log_audit("backup_run", user_id=uid, username=g.user.username, - ip_address=request.remote_addr, details={}) + ip_address=client_ip(), details={}) return jsonify({"status": "started"}), 202 @@ -463,7 +464,7 @@ async def create_invite(): "invitation_created", user_id=uid, username=g.user.username, - ip_address=request.remote_addr, + ip_address=client_ip(), details={"invited_email": email}, ) return jsonify({"status": "ok"}), 201 @@ -497,7 +498,7 @@ async def delete_invitation(invitation_id: int): "invitation_revoked", user_id=uid, username=g.user.username, - ip_address=request.remote_addr, + ip_address=client_ip(), details={"invitation_id": invitation_id}, ) return jsonify({"status": "ok"}) diff --git a/src/grimoire/routes/auth.py b/src/grimoire/routes/auth.py index 35ac6313..37a63f17 100644 --- a/src/grimoire/routes/auth.py +++ b/src/grimoire/routes/auth.py @@ -8,6 +8,7 @@ from quart import Blueprint, g, jsonify, redirect, request, session from grimoire.auth import login_required, get_current_user_id from grimoire.config import Config +from grimoire.proxy import client_ip from grimoire.rate_limit import is_rate_limited from grimoire.services.auth import ( authenticate, @@ -43,20 +44,11 @@ from grimoire.services.oauth import ( auth_bp = Blueprint("auth", __name__, url_prefix="/api/auth") -def _client_ip() -> str: - if Config.TRUST_PROXY_HEADERS: - for header in ("X-Forwarded-For", "X-Real-IP"): - val = request.headers.get(header, "").split(",")[0].strip() - if val: - return val - return request.remote_addr or "" - - @auth_bp.route("/register", methods=["POST"]) async def register(): if not Config.LOCAL_AUTH_ENABLED: return jsonify({"error": "Local authentication is disabled. Please use SSO."}), 403 - if await is_rate_limited(f"register:{_client_ip()}", max_requests=5, window_seconds=300): + if await is_rate_limited(f"register:{client_ip()}", max_requests=5, window_seconds=300): return jsonify({"error": "Too many registration attempts. Try again later."}), 429 if not await is_registration_open(): return jsonify({"error": "Registration is closed"}), 403 @@ -78,7 +70,7 @@ async def register(): session["user_id"] = user.id session["session_version"] = user.session_version - await log_audit("register", user_id=user.id, username=user.username, ip_address=_client_ip()) + await log_audit("register", user_id=user.id, username=user.username, ip_address=client_ip()) return jsonify(user.to_dict()), 201 @@ -86,7 +78,7 @@ async def register(): async def login(): if not Config.LOCAL_AUTH_ENABLED: return jsonify({"error": "Local authentication is disabled. Please use SSO."}), 403 - if await is_rate_limited(f"login:{_client_ip()}", max_requests=10, window_seconds=60): + if await is_rate_limited(f"login:{client_ip()}", max_requests=10, window_seconds=60): return jsonify({"error": "Too many login attempts. Try again later."}), 429 data = await request.get_json() username = (data.get("username") or "").strip() @@ -97,22 +89,22 @@ async def login(): user = await authenticate(username, password) if not user: - await log_audit("login_failed", username=username, ip_address=_client_ip()) + await log_audit("login_failed", username=username, ip_address=client_ip()) # Try to notify the actual user about the failed attempt target_user = await get_user_by_username(username) if target_user: asyncio.create_task(notify_security_event( target_user.id, "login_failed", - {"ip_address": _client_ip(), "username": username}, + {"ip_address": client_ip(), "username": username}, )) return jsonify({"error": "Invalid username or password"}), 401 session["user_id"] = user.id session["session_version"] = user.session_version - await log_audit("login", user_id=user.id, username=user.username, ip_address=_client_ip()) + await log_audit("login", user_id=user.id, username=user.username, ip_address=client_ip()) asyncio.create_task(notify_security_event( user.id, "login", - {"ip_address": _client_ip()}, + {"ip_address": client_ip()}, )) return jsonify(user.to_dict()) @@ -122,10 +114,10 @@ async def logout(): uid = session.get("user_id") if uid: user = await get_user_by_id(uid) - await log_audit("logout", user_id=uid, username=user.username if user else None, ip_address=_client_ip()) + await log_audit("logout", user_id=uid, username=user.username if user else None, ip_address=client_ip()) asyncio.create_task(notify_security_event( uid, "logout", - {"ip_address": _client_ip()}, + {"ip_address": client_ip()}, )) session.clear() return jsonify({"status": "ok"}) @@ -159,10 +151,10 @@ async def update_password(): # Keep the current session alive with the new version (other sessions are now invalidated) session["session_version"] = new_version - await log_audit("password_change", user_id=uid, username=g.user.username, ip_address=_client_ip()) + await log_audit("password_change", user_id=uid, username=g.user.username, ip_address=client_ip()) asyncio.create_task(notify_security_event( uid, "password_change", - {"ip_address": _client_ip()}, + {"ip_address": client_ip()}, )) return jsonify({"status": "ok"}) @@ -178,7 +170,7 @@ async def invalidate_sessions_route(): uid = get_current_user_id() new_version = await invalidate_other_sessions(uid) session["session_version"] = new_version - await log_audit("sessions_invalidated", user_id=uid, username=g.user.username, ip_address=_client_ip()) + await log_audit("sessions_invalidated", user_id=uid, username=g.user.username, ip_address=client_ip()) return jsonify({"status": "ok"}) @@ -208,14 +200,14 @@ async def update_email(): return jsonify({"error": "Email already in use by another account"}), 409 await update_user_email(uid, new_email) - await log_audit("email_change", user_id=uid, username=user.username, ip_address=_client_ip()) + await log_audit("email_change", user_id=uid, username=user.username, ip_address=client_ip()) updated = await get_user_by_id(uid) return jsonify(updated.to_dict()) @auth_bp.route("/forgot-password", methods=["POST"]) async def forgot_password(): - if await is_rate_limited(f"forgot:{_client_ip()}", max_requests=5, window_seconds=300): + if await is_rate_limited(f"forgot:{client_ip()}", max_requests=5, window_seconds=300): return jsonify({"status": "ok"}) data = await request.get_json() email = (data.get("email") or "").strip().lower() @@ -234,7 +226,7 @@ async def forgot_password(): "password_reset_requested", user_id=user.id, username=user.username, - ip_address=_client_ip(), + ip_address=client_ip(), ) return jsonify({"status": "ok"}) @@ -242,7 +234,7 @@ async def forgot_password(): @auth_bp.route("/reset-password", methods=["POST"]) async def reset_password(): - if await is_rate_limited(f"reset:{_client_ip()}", max_requests=10, window_seconds=60): + if await is_rate_limited(f"reset:{client_ip()}", max_requests=10, window_seconds=60): return jsonify({"error": "Too many attempts. Try again later."}), 429 data = await request.get_json() token = (data.get("token") or "").strip() @@ -261,7 +253,7 @@ async def reset_password(): if user: await log_audit( "password_reset_completed", - user_id=user.id, username=user.username, ip_address=_client_ip(), + user_id=user.id, username=user.username, ip_address=client_ip(), ) if user.email: asyncio.create_task(send_password_reset_success_email(user.email)) @@ -304,7 +296,7 @@ async def register_with_invite(): "register_with_invite", user_id=user.id, username=user.username, - ip_address=_client_ip(), + ip_address=client_ip(), ) return jsonify(user.to_dict()), 201 @@ -371,5 +363,5 @@ async def oauth_callback(): user = await find_or_create_oauth_user(sub, email, preferred_username) session["user_id"] = user.id session["session_version"] = user.session_version - await log_audit("oauth_login", user_id=user.id, username=user.username, ip_address=_client_ip()) + await log_audit("oauth_login", user_id=user.id, username=user.username, ip_address=client_ip()) return redirect("/") diff --git a/tests/test_proxy.py b/tests/test_proxy.py new file mode 100644 index 00000000..63905783 --- /dev/null +++ b/tests/test_proxy.py @@ -0,0 +1,187 @@ +"""The trusted proxy decides the client address and whether a request is HTTPS. + +Only the entry TRUSTED_PROXY_HOPS in from the right of a forwarding header was +written by a proxy the app trusts; a client can put anything to its left. The +session cookie's Secure flag and HSTS follow the trusted X-Forwarded-Proto, and +plain HTTP is left untouched. +""" +import pytest +from quart import Quart, Response, session + +from grimoire import config, proxy +from grimoire.config import Config +from grimoire.proxy import ( + HSTS_VALUE, PerRequestSecureSession, apply_https_headers, client_ip, is_https, + trusted_entry, +) + +CLIENT = "203.0.113.7" +FORGED = "198.51.100.1" +PEER = "192.0.2.10" + + +@pytest.mark.parametrize("header, hops, expected", [ + ("", 1, None), + (CLIENT, 0, None), + (CLIENT, 1, CLIENT), + (f"{FORGED}, {CLIENT}", 1, CLIENT), + (f"{FORGED}, {CLIENT}, {PEER}", 2, CLIENT), + # Shorter than the hop count: the leftmost entry, written by the first proxy. + (CLIENT, 2, CLIENT), + (" , ", 1, None), +]) +def test_trusted_entry_counts_in_from_the_right(header, hops, expected): + assert trusted_entry(header, hops) == expected + + +def _app() -> Quart: + app = Quart(__name__) + app.secret_key = "test-only-key" + app.session_interface = PerRequestSecureSession() + + @app.route("/") + async def index(): + session["seen"] = True + response = Response("ok") + apply_https_headers(response) + return response + + return app + + +def _context(app, headers=None, scheme="http"): + return app.test_request_context( + "/", headers=headers or {}, scheme=scheme, + scope_base={"client": (PEER, 40000)}, + ) + + +async def test_a_client_forged_address_left_of_the_proxys_is_ignored(monkeypatch): + monkeypatch.setattr(Config, "TRUSTED_PROXY_HOPS", 1) + async with _context(_app(), {"X-Forwarded-For": f"{FORGED}, {CLIENT}"}): + assert client_ip() == CLIENT + + +async def test_zero_hops_ignores_the_header_and_reads_the_socket_peer(monkeypatch): + monkeypatch.setattr(Config, "TRUSTED_PROXY_HOPS", 0) + async with _context(_app(), {"X-Forwarded-For": CLIENT}): + assert client_ip() == PEER + + +async def test_no_header_reads_the_socket_peer(monkeypatch): + monkeypatch.setattr(Config, "TRUSTED_PROXY_HOPS", 1) + async with _context(_app()): + assert client_ip() == PEER + + +async def test_a_header_sent_twice_is_read_as_one_chain(monkeypatch): + monkeypatch.setattr(Config, "TRUSTED_PROXY_HOPS", 1) + headers = [("X-Forwarded-For", FORGED), ("X-Forwarded-For", CLIENT)] + async with _app().test_request_context( + "/", headers=headers, scope_base={"client": (PEER, 40000)}, + ): + assert client_ip() == CLIENT + + +async def test_a_short_proto_header_is_read_where_the_first_proxy_wrote_it(monkeypatch): + monkeypatch.setattr(Config, "TRUSTED_PROXY_HOPS", 2) + async with _context(_app(), {"X-Forwarded-Proto": "https"}): + assert is_https() + + +async def test_a_forged_https_left_of_the_proxys_http_is_ignored(monkeypatch): + monkeypatch.setattr(Config, "TRUSTED_PROXY_HOPS", 1) + async with _context(_app(), {"X-Forwarded-Proto": "https, http"}): + assert not is_https() + + +async def test_a_direct_https_request_is_https_at_any_hop_count(monkeypatch): + monkeypatch.setattr(Config, "TRUSTED_PROXY_HOPS", 0) + async with _context(_app(), scheme="https"): + assert is_https() + + +def test_outside_a_request_nothing_is_trusted(): + assert client_ip() == "" + assert not is_https() + + +async def _get(monkeypatch, hops, headers): + monkeypatch.setattr(Config, "TRUSTED_PROXY_HOPS", hops) + response = await _app().test_client().get("/", headers=headers) + cookie = " ".join(response.headers.getlist("Set-Cookie")) + assert "session=" in cookie + return cookie, response.headers.get("Strict-Transport-Security") + + +async def test_hops_zero_with_https_header_sends_no_secure_flag_and_no_hsts(monkeypatch): + cookie, hsts = await _get(monkeypatch, 0, {"X-Forwarded-Proto": "https"}) + assert "Secure" not in cookie + assert hsts is None + + +async def test_hops_one_with_https_header_sends_secure_flag_and_hsts(monkeypatch): + cookie, hsts = await _get(monkeypatch, 1, {"X-Forwarded-Proto": "https"}) + assert "Secure" in cookie + assert hsts == HSTS_VALUE + + +async def test_a_plain_request_sends_neither(monkeypatch): + cookie, hsts = await _get(monkeypatch, 1, {}) + assert "Secure" not in cookie + assert hsts is None + + +def test_the_app_decides_secure_per_request(): + from grimoire.app import create_app + assert isinstance(create_app().session_interface, PerRequestSecureSession) + + +def test_the_app_reads_forwarding_headers_only_through_this_module(): + """A second reader of X-Forwarded-* or the socket peer would be a second + trust rule. Matches the reads themselves, not prose that names a header.""" + import pathlib + import re + read = re.compile( + r"""headers\.(?:get|getlist|__getitem__)\(\s*["']X-(?:Forwarded|Real)""" + r"""|headers\[\s*["']X-(?:Forwarded|Real)""" + r"""|\.remote_addr\b""", + re.IGNORECASE, + ) + src = pathlib.Path(proxy.__file__).parent + readers = sorted( + str(path.relative_to(src)) for path in src.rglob("*.py") + if path.name != "proxy.py" and read.search(path.read_text()) + ) + assert readers == [] + assert read.search((src / "proxy.py").read_text()), "the pattern no longer finds the real reader" + + +@pytest.mark.parametrize("env, expected", [ + ({}, 1), + ({"TRUSTED_PROXY_HOPS": "2"}, 2), + ({"TRUSTED_PROXY_HOPS": "0"}, 0), + ({"TRUSTED_PROXY_HOPS": "two"}, -1), + ({"TRUST_PROXY_HEADERS": "false"}, 0), + ({"TRUST_PROXY_HEADERS": "true"}, 1), + ({"TRUST_PROXY_HEADERS": "false", "TRUSTED_PROXY_HOPS": "1"}, 1), +]) +def test_the_hop_count_from_the_environment(monkeypatch, env, expected): + monkeypatch.delenv("TRUSTED_PROXY_HOPS", raising=False) + monkeypatch.delenv("TRUST_PROXY_HEADERS", raising=False) + for name, value in env.items(): + monkeypatch.setenv(name, value) + assert config._trusted_proxy_hops() == expected + + +def test_a_hop_count_that_is_not_a_number_refuses_to_start(monkeypatch): + monkeypatch.setattr(Config, "TRUSTED_PROXY_HOPS", -1) + with pytest.raises(ValueError, match="TRUSTED_PROXY_HOPS"): + Config.validate() + + +@pytest.mark.parametrize("key", sorted(config.PLACEHOLDER_SECRET_KEYS)) +def test_a_placeholder_secret_key_refuses_to_start(monkeypatch, key): + monkeypatch.setattr(Config, "SECRET_KEY", key) + with pytest.raises(ValueError, match="public placeholder"): + Config.validate()