feat(6838): read the client address and HTTPS from the trusted proxy hop
CI & Build / Python lint (push) Successful in 2s
CI & Build / Plugin hooks (push) Successful in 11s
CI & Build / TypeScript typecheck (push) Successful in 37s
CI & Build / Python tests (push) Failing after 1m20s
CI & Build / integration (push) Successful in 2m27s
CI & Build / Build & push image (push) Skipped

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 <noreply@anthropic.com>
This commit is contained in:
2026-11-04 19:32:20 -05:00
co-authored by Claude Opus 5.5
parent b87178d261
commit dbd6d5b8b5
12 changed files with 369 additions and 82 deletions
+13 -10
View File
@@ -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")
+28 -6
View File
@@ -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))
+80
View File
@@ -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)
+3 -3
View File
@@ -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
+15 -14
View File
@@ -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"})
+20 -28
View File
@@ -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("/")