Trust proxy headers by hop count, and log every credential event
CI & Build / Build now, or wait for Android? (push) Successful in 2s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 6s
CI & Build / Python tests (push) Successful in 9s
CI & Build / integration (push) Successful in 12s
CI & Build / Build & push image (push) Successful in 32s
CI & Build / Build now, or wait for Android? (push) Successful in 2s
CI & Build / Python lint (push) Successful in 3s
CI & Build / TypeScript typecheck (push) Successful in 6s
CI & Build / Python tests (push) Successful in 9s
CI & Build / integration (push) Successful in 12s
CI & Build / Build & push image (push) Successful in 32s
Operator, before exposing the instance: *"I'd expect that we should have a proxy hops setting for how many proxy hops we should trust a shared real-ip at… and is there any session logging."* Neither existed, and the first one was a real hole. **The address was forgeable.** `client_address()` read the LEFTMOST `X-Forwarded-For` entry — nominally "the original client", and precisely the one a caller controls, because anything they send arrives before what proxies append. So `curl -H "X-Forwarded-For: 1.2.3.4"`, rotated per request, minted a fresh rate-limit bucket every time. Concretely: stuffing ONE account stayed limited (the account key is unforgeable and that is why it exists), but spraying MANY accounts from one source was not — each account got its own budget, and the per-address cap meant to bound the total was defeated by a header. On a LAN that is nothing. It is not nothing on a public host. Now it counts in from the RIGHT by `THOUGHTSYNC_TRUSTED_PROXY_HOPS`, default 1. Each hop appends what it saw, so the rightmost entries are the ones our own infrastructure wrote and a forged prefix lands to the left of them where it can never be selected — proven for the honest, forged, padded, CDN and shorter-than-configured cases. 0 ignores the header entirely; 2 is Cloudflare in front of a proxy. Too high is the dangerous direction, so a header shorter than configured falls back to the socket address rather than reaching further left. `X-Forwarded-Proto` had the same bug and now shares the same rule. Both live in a new `proxy.py` rather than being written twice — two places holding one decision is how issue 2183 happened, and this is the same decision. Env rather than the Settings UI, against rule 25's usual pull: it is deployment topology rather than preference, and the limiter consults it BEFORE opening a database connection, which is the entire point of checking a throttle before doing expensive work. Easy to move if that reads wrong. **And there was no logging at all** — `auth.py` had no logger, and the only record of anything was `device_tokens.last_used_at`. Sign-ins, failures, throttle trips, new accounts and device-token issuance now all log, with the attempted email and the trusted address. Deliberately including the email: it is the operator's own server, and "somebody failed a login" without saying against which account is not actionable. `basicConfig` at INFO in `create_app`, because hypercorn configures its own loggers and leaves the root at WARNING — without it every line above would have gone nowhere, which is a worse failure than not writing them. This is the app log, not an audit table. Not queryable, not retained past log rotation. The table is task 2939; this is what makes the next few days observable.
This commit is contained in:
@@ -39,6 +39,24 @@ POSTGRES_PASSWORD=
|
|||||||
# 127.0.0.1 so only the proxy can talk to it.
|
# 127.0.0.1 so only the proxy can talk to it.
|
||||||
#THOUGHTSYNC_BIND=0.0.0.0
|
#THOUGHTSYNC_BIND=0.0.0.0
|
||||||
|
|
||||||
|
# How many proxies sit in front of this app. This is a SECURITY setting, not a
|
||||||
|
# preference: it decides which entry of X-Forwarded-For is believed, and therefore
|
||||||
|
# whether a caller can forge their own address and slip the per-address rate limit.
|
||||||
|
#
|
||||||
|
# 0 nothing in front — the app is directly exposed
|
||||||
|
# 1 one reverse proxy terminating TLS (the default, and the usual case)
|
||||||
|
# 2 a CDN in front of that proxy, e.g. Cloudflare
|
||||||
|
#
|
||||||
|
# Set it to the number you actually run. Too HIGH is the dangerous direction — it
|
||||||
|
# starts trusting entries no proxy of yours wrote. Too low just means callers share
|
||||||
|
# a rate-limit bucket.
|
||||||
|
#THOUGHTSYNC_TRUSTED_PROXY_HOPS=1
|
||||||
|
|
||||||
|
# How much the app says. Credential events (sign-ins, failures, throttles, new
|
||||||
|
# accounts, device tokens issued) are logged at INFO and read with
|
||||||
|
# `docker compose logs app`.
|
||||||
|
#THOUGHTSYNC_LOG_LEVEL=INFO
|
||||||
|
|
||||||
# Database identity. Changing these AFTER the first start does not rename anything
|
# Database identity. Changing these AFTER the first start does not rename anything
|
||||||
# that already exists — the volume keeps whatever the first run created.
|
# that already exists — the volume keeps whatever the first run created.
|
||||||
#POSTGRES_USER=thoughtsync
|
#POSTGRES_USER=thoughtsync
|
||||||
|
|||||||
+20
-4
@@ -7,7 +7,7 @@ one guessed password away from someone's whole note history.
|
|||||||
This is what the app does about that on its own, and the four things it cannot do for
|
This is what the app does about that on its own, and the four things it cannot do for
|
||||||
you.
|
you.
|
||||||
|
|
||||||
## Do these four things first
|
## Do these five things first
|
||||||
|
|
||||||
**1. Check registration is closed.** On a fresh instance this now takes care of
|
**1. Check registration is closed.** On a fresh instance this now takes care of
|
||||||
itself: the first account created becomes the admin *and* closes registration behind
|
itself: the first account created becomes the admin *and* closes registration behind
|
||||||
@@ -41,7 +41,19 @@ Once a browser has seen HSTS from your hostname it will refuse plain HTTP there
|
|||||||
year, even if the header stops. That is the point of it, but it is worth knowing
|
year, even if the header stops. That is the point of it, but it is worth knowing
|
||||||
before you put a hostname behind TLS temporarily.
|
before you put a hostname behind TLS temporarily.
|
||||||
|
|
||||||
**3. Stop publishing the app port.** The default compose binds `0.0.0.0:5000` so LAN
|
**3. Tell it how many proxies are in front of it.** `THOUGHTSYNC_TRUSTED_PROXY_HOPS`
|
||||||
|
defaults to `1` — one reverse proxy terminating TLS. Behind a CDN as well (Cloudflare
|
||||||
|
in front of your proxy) set it to `2`.
|
||||||
|
|
||||||
|
This decides which entry of `X-Forwarded-For` is believed, and it is a security
|
||||||
|
setting rather than a preference. The header grows left to right as a request
|
||||||
|
traverses, so the rightmost entries are the ones your own infrastructure wrote and
|
||||||
|
anything a caller forged sits to the left of them. Counting in from the right by the
|
||||||
|
number of proxies you actually run means a forged prefix can never be selected. Set it
|
||||||
|
too HIGH and it starts trusting entries no proxy of yours wrote; too low and several
|
||||||
|
callers share one rate-limit bucket, which is merely inconvenient.
|
||||||
|
|
||||||
|
**4. Stop publishing the app port.** The default compose binds `0.0.0.0:5000` so LAN
|
||||||
clients can reach it directly. Behind a proxy that is a second, unprotected front
|
clients can reach it directly. Behind a proxy that is a second, unprotected front
|
||||||
door. In `.env`:
|
door. In `.env`:
|
||||||
|
|
||||||
@@ -49,7 +61,7 @@ door. In `.env`:
|
|||||||
THOUGHTSYNC_BIND=127.0.0.1
|
THOUGHTSYNC_BIND=127.0.0.1
|
||||||
```
|
```
|
||||||
|
|
||||||
**4. Have a backup that includes the files.** Attachments are files on the
|
**5. Have a backup that includes the files.** Attachments are files on the
|
||||||
`thoughtsync-data` volume, not rows — a `pg_dump` restores notes whose images are all
|
`thoughtsync-data` volume, not rows — a `pg_dump` restores notes whose images are all
|
||||||
gone. Back up both:
|
gone. Back up both:
|
||||||
|
|
||||||
@@ -92,7 +104,11 @@ Know these before you decide who gets an account.
|
|||||||
- **No second factor.** A password is the whole of it.
|
- **No second factor.** A password is the whole of it.
|
||||||
- **No per-user storage quota.** Any account can upload attachments until the volume
|
- **No per-user storage quota.** Any account can upload attachments until the volume
|
||||||
is full. `max_attachment_mb` caps a single file, not a total.
|
is full. `max_attachment_mb` caps a single file, not a total.
|
||||||
- **No audit log.** Device tokens record `last_used_at`; sign-ins are not recorded.
|
- **No audit TABLE.** Credential events — sign-ins, failures, throttle trips, new
|
||||||
|
accounts, device tokens issued — are written to the application log and readable
|
||||||
|
with `docker compose logs app`, which is enough to see whether anyone is knocking.
|
||||||
|
They are not queryable, not retained beyond the container's log rotation, and not
|
||||||
|
attributable after the fact.
|
||||||
- **No invites.** Adding a second person means re-opening registration while they
|
- **No invites.** Adding a second person means re-opening registration while they
|
||||||
sign up, then closing it again. There is no per-person token, no expiry, and no
|
sign up, then closing it again. There is no per-person token, no expiry, and no
|
||||||
record of who invited whom.
|
record of who invited whom.
|
||||||
|
|||||||
+17
-16
@@ -1,13 +1,14 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import asyncio
|
import asyncio
|
||||||
|
import logging
|
||||||
import mimetypes
|
import mimetypes
|
||||||
import os
|
import os
|
||||||
import secrets
|
import secrets
|
||||||
from contextlib import suppress
|
from contextlib import suppress
|
||||||
from datetime import timedelta
|
from datetime import timedelta
|
||||||
|
|
||||||
from quart import Quart, has_request_context, jsonify, request, send_from_directory
|
from quart import Quart, jsonify, send_from_directory
|
||||||
from quart.sessions import SecureCookieSessionInterface
|
from quart.sessions import SecureCookieSessionInterface
|
||||||
|
|
||||||
from . import __version__
|
from . import __version__
|
||||||
@@ -17,12 +18,25 @@ from .config import Config
|
|||||||
from .db import session_scope
|
from .db import session_scope
|
||||||
from .labels import bp as labels_bp
|
from .labels import bp as labels_bp
|
||||||
from .notes import bp as notes_bp
|
from .notes import bp as notes_bp
|
||||||
|
from .proxy import is_https
|
||||||
from .retention import run_sweeper
|
from .retention import run_sweeper
|
||||||
from .saved_filters import bp as saved_filters_bp
|
from .saved_filters import bp as saved_filters_bp
|
||||||
from .settings import get_public_config, get_setting, load_or_create_secret_key
|
from .settings import get_public_config, get_setting, load_or_create_secret_key
|
||||||
from .settings_api import bp as settings_bp
|
from .settings_api import bp as settings_bp
|
||||||
from .sync import bp as sync_bp, protocol_advertisement
|
from .sync import bp as sync_bp, protocol_advertisement
|
||||||
|
|
||||||
|
# Without this, `logger.info` from this package goes nowhere: hypercorn configures its
|
||||||
|
# own access/error loggers and leaves the root logger at WARNING, so the credential
|
||||||
|
# events in auth.py would be invisible in `docker compose logs` — which is exactly
|
||||||
|
# where they are meant to be read until an audit table exists (task 2939).
|
||||||
|
#
|
||||||
|
# `force=False` (the default) so a host that has already configured logging keeps its
|
||||||
|
# own setup; LOG_LEVEL lets an operator turn it up without a code change.
|
||||||
|
logging.basicConfig(
|
||||||
|
level=os.environ.get("THOUGHTSYNC_LOG_LEVEL", "INFO").upper(),
|
||||||
|
format="%(asctime)s %(levelname)s %(name)s %(message)s",
|
||||||
|
)
|
||||||
|
|
||||||
STATIC_DIR = os.path.join(os.path.dirname(__file__), "static")
|
STATIC_DIR = os.path.join(os.path.dirname(__file__), "static")
|
||||||
|
|
||||||
# `.webmanifest` isn't in every base image's mime map; register it so the PWA
|
# `.webmanifest` isn't in every base image's mime map; register it so the PWA
|
||||||
@@ -30,19 +44,6 @@ STATIC_DIR = os.path.join(os.path.dirname(__file__), "static")
|
|||||||
mimetypes.add_type("application/manifest+json", ".webmanifest")
|
mimetypes.add_type("application/manifest+json", ".webmanifest")
|
||||||
|
|
||||||
|
|
||||||
def _is_https() -> bool:
|
|
||||||
"""Whether this request reached us over TLS — directly, or through a proxy that
|
|
||||||
terminated it and said so in X-Forwarded-Proto.
|
|
||||||
|
|
||||||
Shared by the session cookie's Secure flag and by HSTS, because they are the same
|
|
||||||
question and answering it twice is how the two drift apart.
|
|
||||||
"""
|
|
||||||
if not has_request_context():
|
|
||||||
return False
|
|
||||||
forwarded = request.headers.get("X-Forwarded-Proto", "").split(",")[0].strip().lower()
|
|
||||||
return forwarded == "https" or request.is_secure
|
|
||||||
|
|
||||||
|
|
||||||
class _AutoSecureSessionInterface(SecureCookieSessionInterface):
|
class _AutoSecureSessionInterface(SecureCookieSessionInterface):
|
||||||
"""Mark the session cookie `Secure` whenever the request arrived over HTTPS —
|
"""Mark the session cookie `Secure` whenever the request arrived over HTTPS —
|
||||||
directly, or via a TLS-terminating reverse proxy that sets X-Forwarded-Proto.
|
directly, or via a TLS-terminating reverse proxy that sets X-Forwarded-Proto.
|
||||||
@@ -54,7 +55,7 @@ class _AutoSecureSessionInterface(SecureCookieSessionInterface):
|
|||||||
"""
|
"""
|
||||||
|
|
||||||
def get_cookie_secure(self, app: Quart) -> bool:
|
def get_cookie_secure(self, app: Quart) -> bool:
|
||||||
return _is_https()
|
return is_https()
|
||||||
|
|
||||||
|
|
||||||
def create_app() -> Quart:
|
def create_app() -> Quart:
|
||||||
@@ -168,7 +169,7 @@ def create_app() -> Quart:
|
|||||||
# commit domains this app does not own. A browser still remembers the policy
|
# commit domains this app does not own. A browser still remembers the policy
|
||||||
# for up to a year after the header stops being sent, which is the point of
|
# for up to a year after the header stops being sent, which is the point of
|
||||||
# it — worth knowing before putting a hostname behind TLS temporarily.
|
# it — worth knowing before putting a hostname behind TLS temporarily.
|
||||||
if _is_https():
|
if is_https():
|
||||||
response.headers.setdefault("Strict-Transport-Security", "max-age=31536000")
|
response.headers.setdefault("Strict-Transport-Security", "max-age=31536000")
|
||||||
return response
|
return response
|
||||||
|
|
||||||
|
|||||||
+27
-1
@@ -1,6 +1,7 @@
|
|||||||
from __future__ import annotations
|
from __future__ import annotations
|
||||||
|
|
||||||
import functools
|
import functools
|
||||||
|
import logging
|
||||||
import uuid
|
import uuid
|
||||||
from datetime import datetime, timezone
|
from datetime import datetime, timezone
|
||||||
|
|
||||||
@@ -11,8 +12,8 @@ from .common import iso
|
|||||||
from .db import session_scope
|
from .db import session_scope
|
||||||
from .models.device_token import DeviceToken
|
from .models.device_token import DeviceToken
|
||||||
from .models.user import User
|
from .models.user import User
|
||||||
|
from .proxy import client_address
|
||||||
from .ratelimit import (
|
from .ratelimit import (
|
||||||
client_address,
|
|
||||||
register_by_address,
|
register_by_address,
|
||||||
sign_in_by_account,
|
sign_in_by_account,
|
||||||
sign_in_by_address,
|
sign_in_by_address,
|
||||||
@@ -22,6 +23,16 @@ from .settings import get_setting, set_settings
|
|||||||
|
|
||||||
bp = Blueprint("auth", __name__, url_prefix="/api/auth")
|
bp = Blueprint("auth", __name__, url_prefix="/api/auth")
|
||||||
|
|
||||||
|
# Every credential event goes to the app log — there is no audit TABLE yet (see task
|
||||||
|
# 2939), and until there is, `docker compose logs` is the only way to know whether
|
||||||
|
# anyone is knocking. That matters most in exactly the window this was written for: a
|
||||||
|
# freshly-exposed instance.
|
||||||
|
#
|
||||||
|
# The attempted email is included deliberately. It is the operator's own server, and
|
||||||
|
# "somebody failed a login" without saying against WHICH account tells you nothing you
|
||||||
|
# can act on. Passwords, obviously, never appear.
|
||||||
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
SESSION_KEY = "user_id"
|
SESSION_KEY = "user_id"
|
||||||
MIN_PASSWORD_LEN = 8
|
MIN_PASSWORD_LEN = 8
|
||||||
DEVICE_NAME_CAP = 100
|
DEVICE_NAME_CAP = 100
|
||||||
@@ -120,6 +131,7 @@ def _throttled(retry_after: int):
|
|||||||
`Retry-After` is standard and is the one thing a legitimate client (or person)
|
`Retry-After` is standard and is the one thing a legitimate client (or person)
|
||||||
genuinely needs.
|
genuinely needs.
|
||||||
"""
|
"""
|
||||||
|
logger.warning("throttled credential attempt from=%s retry_after=%ss", client_address(), retry_after)
|
||||||
return (
|
return (
|
||||||
jsonify({"error": "too many attempts — try again shortly"}),
|
jsonify({"error": "too many attempts — try again shortly"}),
|
||||||
429,
|
429,
|
||||||
@@ -186,6 +198,7 @@ async def register():
|
|||||||
# The first account bootstraps the admin and is always allowed, even when
|
# The first account bootstraps the admin and is always allowed, even when
|
||||||
# registration is otherwise closed.
|
# registration is otherwise closed.
|
||||||
if not is_first and not await get_setting(db, "allow_registration"):
|
if not is_first and not await get_setting(db, "allow_registration"):
|
||||||
|
logger.warning("registration refused (closed) email=%s from=%s", email, client_address())
|
||||||
return jsonify({"error": "registration is closed"}), 403
|
return jsonify({"error": "registration is closed"}), 403
|
||||||
existing = await db.scalar(select(User).where(User.email == email))
|
existing = await db.scalar(select(User).where(User.email == email))
|
||||||
if existing is not None:
|
if existing is not None:
|
||||||
@@ -215,6 +228,9 @@ async def register():
|
|||||||
await db.refresh(user)
|
await db.refresh(user)
|
||||||
session[SESSION_KEY] = str(user.id)
|
session[SESSION_KEY] = str(user.id)
|
||||||
session.permanent = True
|
session.permanent = True
|
||||||
|
logger.info(
|
||||||
|
"account created email=%s admin=%s from=%s", email, is_first, client_address()
|
||||||
|
)
|
||||||
return jsonify(_serialize_user(user)), 201
|
return jsonify(_serialize_user(user)), 201
|
||||||
|
|
||||||
|
|
||||||
@@ -236,13 +252,16 @@ async def login():
|
|||||||
# difference is a reliable oracle for which emails have accounts here.
|
# difference is a reliable oracle for which emails have accounts here.
|
||||||
dummy_verify(password)
|
dummy_verify(password)
|
||||||
_sign_in_failed(email)
|
_sign_in_failed(email)
|
||||||
|
logger.warning("sign-in failed (no such account) email=%s from=%s", email, client_address())
|
||||||
return jsonify({"error": "invalid email or password"}), 401
|
return jsonify({"error": "invalid email or password"}), 401
|
||||||
if not verify_password(password, user.password_hash):
|
if not verify_password(password, user.password_hash):
|
||||||
_sign_in_failed(email)
|
_sign_in_failed(email)
|
||||||
|
logger.warning("sign-in failed (bad password) email=%s from=%s", email, client_address())
|
||||||
return jsonify({"error": "invalid email or password"}), 401
|
return jsonify({"error": "invalid email or password"}), 401
|
||||||
_sign_in_succeeded(email)
|
_sign_in_succeeded(email)
|
||||||
session[SESSION_KEY] = str(user.id)
|
session[SESSION_KEY] = str(user.id)
|
||||||
session.permanent = True
|
session.permanent = True
|
||||||
|
logger.info("sign-in ok email=%s from=%s", email, client_address())
|
||||||
return jsonify(_serialize_user(user))
|
return jsonify(_serialize_user(user))
|
||||||
|
|
||||||
|
|
||||||
@@ -310,12 +329,19 @@ async def device_login():
|
|||||||
if user is None or not user.password_hash:
|
if user is None or not user.password_hash:
|
||||||
dummy_verify(password)
|
dummy_verify(password)
|
||||||
_sign_in_failed(email)
|
_sign_in_failed(email)
|
||||||
|
logger.warning("device-login failed (no such account) email=%s from=%s", email, client_address())
|
||||||
return jsonify({"error": "invalid email or password"}), 401
|
return jsonify({"error": "invalid email or password"}), 401
|
||||||
if not verify_password(password, user.password_hash):
|
if not verify_password(password, user.password_hash):
|
||||||
_sign_in_failed(email)
|
_sign_in_failed(email)
|
||||||
|
logger.warning("device-login failed (bad password) email=%s from=%s", email, client_address())
|
||||||
return jsonify({"error": "invalid email or password"}), 401
|
return jsonify({"error": "invalid email or password"}), 401
|
||||||
_sign_in_succeeded(email)
|
_sign_in_succeeded(email)
|
||||||
row, token = await _issue_device_token(db, user.id, data.get("name") or "")
|
row, token = await _issue_device_token(db, user.id, data.get("name") or "")
|
||||||
|
# A device token outlives the session that made it, so its creation is the
|
||||||
|
# most consequential thing on this blueprint.
|
||||||
|
logger.info(
|
||||||
|
"device token issued email=%s device=%s from=%s", email, row.name, client_address()
|
||||||
|
)
|
||||||
await db.commit()
|
await db.commit()
|
||||||
return jsonify({"token": token, "device": _serialize_device(row), "user": _serialize_user(user)}), 201
|
return jsonify({"token": token, "device": _serialize_device(row), "user": _serialize_user(user)}), 201
|
||||||
|
|
||||||
|
|||||||
@@ -15,6 +15,9 @@ class Config:
|
|||||||
If unset, a key is generated and persisted in the DB (see
|
If unset, a key is generated and persisted in the DB (see
|
||||||
``thoughtsync.settings.load_or_create_secret_key``), so sessions survive
|
``thoughtsync.settings.load_or_create_secret_key``), so sessions survive
|
||||||
restarts with no volume required.
|
restarts with no volume required.
|
||||||
|
- ``THOUGHTSYNC_TRUSTED_PROXY_HOPS`` — how many proxies in front of this app may
|
||||||
|
be believed when reading ``X-Forwarded-For`` / ``X-Forwarded-Proto``. Defaults
|
||||||
|
to 1 (one reverse proxy terminating TLS). See ``trusted_proxy_hops``.
|
||||||
|
|
||||||
Uploaded media lives under ``DATA_DIR`` — a fixed, authoritative path
|
Uploaded media lives under ``DATA_DIR`` — a fixed, authoritative path
|
||||||
(``/var/thoughtsync``), intentionally NOT configurable (a mutable data path only
|
(``/var/thoughtsync``), intentionally NOT configurable (a mutable data path only
|
||||||
@@ -48,3 +51,36 @@ class Config:
|
|||||||
def secret_key_env(cls) -> str | None:
|
def secret_key_env(cls) -> str | None:
|
||||||
"""Optional break-glass override for the cookie-signing secret."""
|
"""Optional break-glass override for the cookie-signing secret."""
|
||||||
return os.environ.get("THOUGHTSYNC_SECRET_KEY") or None
|
return os.environ.get("THOUGHTSYNC_SECRET_KEY") or None
|
||||||
|
|
||||||
|
@classmethod
|
||||||
|
def trusted_proxy_hops(cls) -> int:
|
||||||
|
"""How many proxies in front of this app may be believed.
|
||||||
|
|
||||||
|
`X-Forwarded-For` grows LEFT to RIGHT: each hop appends the address it saw.
|
||||||
|
So the RIGHTMOST entry was written by our own proxy and is the address that
|
||||||
|
actually connected to it, while anything a client sent arrives to the LEFT of
|
||||||
|
that — which is why the leftmost entry, the "original client", is precisely
|
||||||
|
the one a caller can forge.
|
||||||
|
|
||||||
|
With `n` trusted hops the real client is the nth entry from the right:
|
||||||
|
|
||||||
|
0 no proxy — ignore the header entirely, use the socket address
|
||||||
|
1 one reverse proxy terminating TLS (the default, and this deployment)
|
||||||
|
2 a CDN in front of that proxy — Cloudflare appended the client, our
|
||||||
|
proxy appended Cloudflare
|
||||||
|
|
||||||
|
Set it to the number of proxies you actually run. Too HIGH and a caller can
|
||||||
|
forge an address by padding the header; too low and everyone behind the CDN
|
||||||
|
shares one bucket. Too low is the safe direction, so it is the fallback when
|
||||||
|
the header is shorter than configured.
|
||||||
|
|
||||||
|
Env rather than the Settings UI (rule 25's "absolute bootstrap only" carve-
|
||||||
|
out): it is a property of the deployment topology, not a preference, and the
|
||||||
|
rate limiter consults it before opening a database connection — which is the
|
||||||
|
whole point of checking a throttle before doing expensive work.
|
||||||
|
"""
|
||||||
|
raw = os.environ.get("THOUGHTSYNC_TRUSTED_PROXY_HOPS", "1")
|
||||||
|
try:
|
||||||
|
return max(0, int(raw))
|
||||||
|
except (TypeError, ValueError):
|
||||||
|
return 1
|
||||||
|
|||||||
@@ -0,0 +1,74 @@
|
|||||||
|
"""Reading what the proxies in front of this app say about a request.
|
||||||
|
|
||||||
|
Two headers carry information the app cannot see for itself — who the client is
|
||||||
|
(`X-Forwarded-For`) and whether they arrived over TLS (`X-Forwarded-Proto`) — and both
|
||||||
|
are trusted by the same rule, so the rule lives in one place. Writing it twice is
|
||||||
|
precisely how issue 2183 happened: two places holding one decision, and only one of
|
||||||
|
them updated.
|
||||||
|
|
||||||
|
## The rule
|
||||||
|
|
||||||
|
A forwarding header grows LEFT to RIGHT. Each hop appends what IT saw, so the
|
||||||
|
rightmost entries are the ones our own infrastructure wrote, and anything a caller
|
||||||
|
sent arrives to the LEFT of those.
|
||||||
|
|
||||||
|
That inverts the intuitive reading. The leftmost entry is nominally "the original
|
||||||
|
client" — and is exactly the one a caller can forge, by sending the header themselves.
|
||||||
|
So we count in from the right by the number of proxies we actually run
|
||||||
|
(`THOUGHTSYNC_TRUSTED_PROXY_HOPS`, default 1), and a forged prefix can never be
|
||||||
|
selected no matter how much of it there is.
|
||||||
|
|
||||||
|
Too HIGH a hop count is the dangerous direction: it starts believing entries no proxy
|
||||||
|
of ours wrote. Too low just means several callers share a bucket. So when the header
|
||||||
|
is shorter than configured — fewer proxies than expected — we fall back to the socket
|
||||||
|
address rather than reaching further left.
|
||||||
|
"""
|
||||||
|
from __future__ import annotations
|
||||||
|
|
||||||
|
from quart import has_request_context, request
|
||||||
|
|
||||||
|
from .config import Config
|
||||||
|
|
||||||
|
|
||||||
|
def trusted_entry(header: str, hops: int) -> str | None:
|
||||||
|
"""The nth-from-the-right entry of a forwarding header, or None if there isn't one.
|
||||||
|
|
||||||
|
Pure, so the trust boundary is testable without a request context.
|
||||||
|
"""
|
||||||
|
if hops <= 0:
|
||||||
|
return None
|
||||||
|
entries = [part.strip() for part in header.split(",") if part.strip()]
|
||||||
|
if len(entries) < hops:
|
||||||
|
return None
|
||||||
|
return entries[-hops]
|
||||||
|
|
||||||
|
|
||||||
|
def forwarded_for(header: str, remote_addr: str | None, hops: int) -> str:
|
||||||
|
"""The client address a proxy chain vouches for, else this connection's peer."""
|
||||||
|
entry = trusted_entry(header, hops)
|
||||||
|
return (entry or remote_addr or "unknown")[:64] # bounded: becomes a dict key
|
||||||
|
|
||||||
|
|
||||||
|
def client_address() -> str:
|
||||||
|
"""The caller's address, as far as the deployment's own proxies vouch for it."""
|
||||||
|
return forwarded_for(
|
||||||
|
request.headers.get("X-Forwarded-For", ""),
|
||||||
|
request.remote_addr,
|
||||||
|
Config.trusted_proxy_hops(),
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def is_https() -> bool:
|
||||||
|
"""Whether this request reached us over TLS — directly, or via a trusted proxy.
|
||||||
|
|
||||||
|
Shared by the session cookie's `Secure` flag and by HSTS, because they are the same
|
||||||
|
question. Read with the same hop count as the address: a caller who sets
|
||||||
|
`X-Forwarded-Proto: https` on a plain-HTTP request puts it to the left of whatever
|
||||||
|
our proxy appended, so it is not what gets read.
|
||||||
|
"""
|
||||||
|
if not has_request_context():
|
||||||
|
return False
|
||||||
|
if request.is_secure:
|
||||||
|
return True
|
||||||
|
entry = trusted_entry(request.headers.get("X-Forwarded-Proto", ""), Config.trusted_proxy_hops())
|
||||||
|
return (entry or "").lower() == "https"
|
||||||
@@ -22,9 +22,11 @@ came from, and either one can refuse it:
|
|||||||
is what stops credential stuffing against one known email, no matter how many
|
is what stops credential stuffing against one known email, no matter how many
|
||||||
addresses the attempts arrive from.
|
addresses the attempts arrive from.
|
||||||
- **The address** bounds the damage from one source spraying many accounts. It is
|
- **The address** bounds the damage from one source spraying many accounts. It is
|
||||||
best-effort by nature — behind a reverse proxy the client address is read from
|
read from ``X-Forwarded-For``, counting in from the RIGHT by
|
||||||
``X-Forwarded-For``, which a caller can set to anything if the app is exposed
|
``THOUGHTSYNC_TRUSTED_PROXY_HOPS`` so that only entries our own proxies wrote are
|
||||||
directly. That is precisely why it is not the only key.
|
believed — a forged header lands to the left of those and is never selected. It is
|
||||||
|
still the weaker of the two keys, because it depends on that setting matching the
|
||||||
|
deployment; the account key depends on nothing.
|
||||||
|
|
||||||
Counting is by failure for the sign-in routes and by attempt for registration: a
|
Counting is by failure for the sign-in routes and by attempt for registration: a
|
||||||
correct password should never move someone closer to being locked out, but every
|
correct password should never move someone closer to being locked out, but every
|
||||||
@@ -35,7 +37,6 @@ from __future__ import annotations
|
|||||||
import time
|
import time
|
||||||
from collections import deque
|
from collections import deque
|
||||||
|
|
||||||
from quart import request
|
|
||||||
|
|
||||||
# Failed sign-ins tolerated per account before it stops answering, and for how long.
|
# Failed sign-ins tolerated per account before it stops answering, and for how long.
|
||||||
# Ten is comfortably above a person mistyping a password and far below anything that
|
# Ten is comfortably above a person mistyping a password and far below anything that
|
||||||
@@ -116,24 +117,6 @@ sign_in_by_address = SlidingWindow(ADDRESS_LIMIT, ADDRESS_WINDOW_S)
|
|||||||
register_by_address = SlidingWindow(REGISTER_LIMIT, REGISTER_WINDOW_S)
|
register_by_address = SlidingWindow(REGISTER_LIMIT, REGISTER_WINDOW_S)
|
||||||
|
|
||||||
|
|
||||||
def client_address() -> str:
|
|
||||||
"""The caller's address, as well as it can be known.
|
|
||||||
|
|
||||||
``X-Forwarded-For`` is a list appended to by each hop, so the leftmost entry is
|
|
||||||
the original client — and also the only entry a client can choose for itself.
|
|
||||||
It is trusted here anyway, because the alternative behind a reverse proxy is to
|
|
||||||
see the proxy's address for every request on earth and rate-limit the entire
|
|
||||||
internet as one caller. The account-keyed limit is the one that holds when this
|
|
||||||
one is lied to.
|
|
||||||
"""
|
|
||||||
forwarded = request.headers.get("X-Forwarded-For", "")
|
|
||||||
if forwarded:
|
|
||||||
first = forwarded.split(",")[0].strip()
|
|
||||||
if first:
|
|
||||||
return first[:64] # bounded: this becomes a dict key
|
|
||||||
return (request.remote_addr or "unknown")[:64]
|
|
||||||
|
|
||||||
|
|
||||||
def reset_all() -> None:
|
def reset_all() -> None:
|
||||||
"""Drop every counter. For tests — nothing in the app calls this."""
|
"""Drop every counter. For tests — nothing in the app calls this."""
|
||||||
for window in (sign_in_by_account, sign_in_by_address, register_by_address):
|
for window in (sign_in_by_account, sign_in_by_address, register_by_address):
|
||||||
|
|||||||
@@ -0,0 +1,68 @@
|
|||||||
|
"""The proxy trust boundary.
|
||||||
|
|
||||||
|
The whole security property is "a caller cannot forge their own address", and it rests
|
||||||
|
on counting in from the RIGHT of the header rather than the left. These are the cases
|
||||||
|
that tell the two apart — pure functions, no request context, no database.
|
||||||
|
"""
|
||||||
|
from thoughtsync.config import Config
|
||||||
|
from thoughtsync.proxy import forwarded_for, trusted_entry
|
||||||
|
|
||||||
|
PEER = "10.0.0.1" # the socket address: our own proxy, or the caller when unproxied
|
||||||
|
|
||||||
|
|
||||||
|
def test_default_is_one_hop():
|
||||||
|
# One reverse proxy terminating TLS — this deployment, and the only shape that is
|
||||||
|
# safe to assume. A wrong default here is a silent security bug, not a preference.
|
||||||
|
assert Config.trusted_proxy_hops() == 1
|
||||||
|
|
||||||
|
|
||||||
|
def test_no_proxy_ignores_the_header_entirely():
|
||||||
|
# hops=0 says nothing in front of us appends anything, so the header can only be
|
||||||
|
# something a caller invented.
|
||||||
|
assert forwarded_for("1.2.3.4", PEER, 0) == PEER
|
||||||
|
|
||||||
|
|
||||||
|
def test_one_hop_reads_what_our_proxy_wrote():
|
||||||
|
assert forwarded_for("203.0.113.7", PEER, 1) == "203.0.113.7"
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_forged_prefix_is_never_selected():
|
||||||
|
# THE test. A caller sends `X-Forwarded-For: 1.2.3.4`; our proxy appends the
|
||||||
|
# address it actually saw. Reading from the left would hand the caller a fresh
|
||||||
|
# rate-limit bucket for every value they invent.
|
||||||
|
assert forwarded_for("1.2.3.4, 203.0.113.7", PEER, 1) == "203.0.113.7"
|
||||||
|
# …and padding it doesn't help either.
|
||||||
|
assert forwarded_for("a, b, c, d, 203.0.113.7", PEER, 1) == "203.0.113.7"
|
||||||
|
|
||||||
|
|
||||||
|
def test_two_hops_sees_past_a_cdn():
|
||||||
|
# Cloudflare appended the real client; our proxy appended Cloudflare.
|
||||||
|
assert forwarded_for("203.0.113.7, 172.16.0.5", PEER, 2) == "203.0.113.7"
|
||||||
|
assert forwarded_for("1.2.3.4, 203.0.113.7, 172.16.0.5", PEER, 2) == "203.0.113.7"
|
||||||
|
|
||||||
|
|
||||||
|
def test_a_short_header_falls_back_rather_than_reaching_left():
|
||||||
|
# Fewer proxies than configured. Reaching further left would start believing
|
||||||
|
# entries no proxy of ours wrote, so the safe direction is the socket address —
|
||||||
|
# at worst several callers share one bucket.
|
||||||
|
assert forwarded_for("203.0.113.7", PEER, 2) == PEER
|
||||||
|
assert forwarded_for("", PEER, 1) == PEER
|
||||||
|
|
||||||
|
|
||||||
|
def test_malformed_headers_do_not_crash_or_leak_empties():
|
||||||
|
assert forwarded_for(",,,", PEER, 1) == PEER
|
||||||
|
assert forwarded_for(" , 203.0.113.7 , ", PEER, 1) == "203.0.113.7"
|
||||||
|
|
||||||
|
|
||||||
|
def test_the_key_is_length_bounded():
|
||||||
|
# It becomes a dict key in the limiter; an unbounded header must not become an
|
||||||
|
# unbounded allocation.
|
||||||
|
assert len(forwarded_for("x" * 5000, PEER, 1)) <= 64
|
||||||
|
|
||||||
|
|
||||||
|
def test_trusted_entry_reports_absence_rather_than_guessing():
|
||||||
|
# `is_https` needs to tell "no trusted entry" apart from "an entry saying http",
|
||||||
|
# which is why this returns None rather than a default.
|
||||||
|
assert trusted_entry("", 1) is None
|
||||||
|
assert trusted_entry("https", 0) is None
|
||||||
|
assert trusted_entry("http, https", 1) == "https"
|
||||||
+2
-13
@@ -128,21 +128,10 @@ async def test_register_is_throttled_by_address(app):
|
|||||||
resp = await client.post(
|
resp = await client.post(
|
||||||
"/api/auth/register",
|
"/api/auth/register",
|
||||||
json={"email": "new@example.com", "password": "a-long-enough-password"},
|
json={"email": "new@example.com", "password": "a-long-enough-password"},
|
||||||
|
# One entry, so with the default single trusted hop this IS the address the
|
||||||
|
# limiter keys on. The forged-prefix cases live in test_proxy.py.
|
||||||
headers={"X-Forwarded-For": "203.0.113.9"},
|
headers={"X-Forwarded-For": "203.0.113.9"},
|
||||||
)
|
)
|
||||||
assert resp.status_code == 429
|
assert resp.status_code == 429
|
||||||
|
|
||||||
|
|
||||||
async def test_client_address_prefers_the_forwarded_client(app):
|
|
||||||
# Behind a reverse proxy, remote_addr is the PROXY for every request on earth —
|
|
||||||
# keying on it would rate-limit the entire internet as one caller. The leftmost
|
|
||||||
# X-Forwarded-For entry is the original client.
|
|
||||||
async with app.test_request_context("/", headers={"X-Forwarded-For": "198.51.100.4, 10.0.0.1"}):
|
|
||||||
assert ratelimit.client_address() == "198.51.100.4"
|
|
||||||
|
|
||||||
|
|
||||||
async def test_client_address_falls_back_to_the_peer(app):
|
|
||||||
async with app.test_request_context("/"):
|
|
||||||
# No proxy header: whatever the peer address is, it must be a usable key
|
|
||||||
# rather than an empty string sharing one bucket with everyone.
|
|
||||||
assert ratelimit.client_address()
|
|
||||||
|
|||||||
Reference in New Issue
Block a user