bvandeusen
09b5f874b6
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) Failing after 9s
CI & Build / integration (push) Failing after 12s
CI & Build / Build & push image (push) Successful in 32s
Desktop (Tauri) / Windows installer (cross-compiled) (push) Successful in 2m17s
Desktop (Tauri) / Tauri desktop (Linux) (push) Successful in 4m14s
Desktop (Tauri) / Update manifest (push) Successful in 5s
Operator: *"proxy hops defaults to 1 and should be in the settings UI not in the envs, we need the security values to be in the UI."* Overrules the call I made yesterday, and rule 25 is on your side — I argued deployment-topology, but the operator has to be able to SEE what protects them, and reading a container's environment is not seeing. Six new settings in a **Security** group: trusted proxy hops (default 1), the per-account and per-address sign-in limits with their shared window, and the sign-up limit with its own. `THOUGHTSYNC_TRUSTED_PROXY_HOPS` is gone; the rate limits are no longer hardcoded constants. **The hard part was keeping the throttle cheap.** It consults these BEFORE opening a database connection — deliberately, because a refused attempt is meant to cost nothing, and the hop count is needed to know who is even asking. A query per attempt would undo both. So there is a small cache seeded from the registry defaults (the app works with no database at all, which is what the DB-free unit lane relies on), loaded at boot, and refreshed on every settings save — the same live-update contract `session_ttl_days` already had. `SlidingWindow` now takes its limit and window as SUPPLIERS rather than values, so a saved number applies to the next attempt instead of the next deploy. **Bounds are rejected, not clamped.** A hop count of 99 would trust anything a caller sent; a sign-in limit of 0 would lock every account out permanently. Both now fail validation with a message naming the range, and the number input carries min/max so the browser objects first. Silently storing a different number than the one typed is how somebody ends up believing a protection is set to something it is not. `MAX_BUCKETS` stays a constant on purpose: it protects the limiter from itself rather than the app from a caller, and there is no operator judgment to apply. Two integration tests, because the whole point is the round trip: a dangerous value refused, a legitimate one reaching the cache the throttle reads and persisting; and every Security row reaching the admin payload with bounds and a description that explains itself.
75 lines
3.0 KiB
Python
75 lines
3.0 KiB
Python
"""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
|
|
(the **Trusted proxy hops** setting, 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 .settings import live
|
|
|
|
|
|
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,
|
|
live("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", ""), live("trusted_proxy_hops"))
|
|
return (entry or "").lower() == "https"
|