Decision: should login rate limiting survive a shared bucket, or should TRUSTED_PROXIES be mandatory in production? #150

New Issue

clawbot · 2026-08-12T13:32:25+02:00

clawbot commented

2026-08-12 13:32:25 +02:00

Raised by the final integration review of #111. The immediate 1.0.0 blocker is #149, which makes the consequence visible via a startup warning and honest docs. This issue is the structural question underneath it, and it does NOT block the tag.

The problem restated: with TRUSTED_PROXIES empty behind the mandated reverse proxy, every client shares one login bucket, so 5 requests/minute from a stranger denies the operator the only admin path. Documenting it is necessary but does not remove it — an operator who never sets TRUSTED_PROXIES still ships a remotely-lockable admin UI.

Needs your call, because every option trades something real.

Options

Additionally key login attempts by submitted username. An attacker flooding one username cannot lock out another, and the operator's own account stays reachable unless specifically targeted. Costs a little: the key set becomes attacker-influenced, so it needs its own bound.
Make TRUSTED_PROXIES mandatory in production — fail startup when it is empty and WEBHOOKER_ENVIRONMENT is production. Coherent with the fail-loud philosophy of #80 and with the policy that a proxy is always present, but it is a breaking change for anyone deploying without one, and it converts a soft misconfiguration into a hard outage.
Only count FAILED login attempts toward the limit. Reduces incidental lockout but does not fix the attack, since an attacker submits failures by definition.
Accept it, on the strength of the #149 warning and the fact that a restart clears the buckets.

Recommendation: option 1. It targets the actual failure — one shared bucket for all accounts — without changing deployment requirements or the #88 default, and it keeps brute-force protection intact. Option 2 is defensible and arguably more honest, but it turns a documentation gap into a startup failure for existing deployments, which is not something to do in a 1.0.0. Option 3 does not solve it. Option 4 leaves a remotely-triggerable admin denial in the shipped default.

If you pick option 1, the bound on the username key set is the part that needs care: cap distinct tracked usernames and fall back to the address bucket beyond the cap, or the limiter itself becomes the memory-amplification surface that #133 just closed elsewhere.

Done-criteria once decided

The chosen mechanism is implemented with a test proving a stranger's flood cannot deny a different account's login.
The README's rate-limiting section describes the final behaviour accurately.

Raised by the final integration review of https://git.eeqj.de/sneak/webhooker/pulls/111. The immediate 1.0.0 blocker is https://git.eeqj.de/sneak/webhooker/issues/149, which makes the consequence visible via a startup warning and honest docs. This issue is the structural question underneath it, and it does NOT block the tag. The problem restated: with `TRUSTED_PROXIES` empty behind the mandated reverse proxy, every client shares one login bucket, so 5 requests/minute from a stranger denies the operator the only admin path. Documenting it is necessary but does not remove it — an operator who never sets `TRUSTED_PROXIES` still ships a remotely-lockable admin UI. Needs your call, because every option trades something real. ## Options 1. **Additionally key login attempts by submitted username.** An attacker flooding one username cannot lock out another, and the operator's own account stays reachable unless specifically targeted. Costs a little: the key set becomes attacker-influenced, so it needs its own bound. 2. **Make `TRUSTED_PROXIES` mandatory in production** — fail startup when it is empty and `WEBHOOKER_ENVIRONMENT` is production. Coherent with the fail-loud philosophy of https://git.eeqj.de/sneak/webhooker/issues/80 and with the policy that a proxy is always present, but it is a breaking change for anyone deploying without one, and it converts a soft misconfiguration into a hard outage. 3. **Only count FAILED login attempts** toward the limit. Reduces incidental lockout but does not fix the attack, since an attacker submits failures by definition. 4. **Accept it**, on the strength of the https://git.eeqj.de/sneak/webhooker/issues/149 warning and the fact that a restart clears the buckets. Recommendation: option 1. It targets the actual failure — one shared bucket for all accounts — without changing deployment requirements or the https://git.eeqj.de/sneak/webhooker/issues/88 default, and it keeps brute-force protection intact. Option 2 is defensible and arguably more honest, but it turns a documentation gap into a startup failure for existing deployments, which is not something to do in a 1.0.0. Option 3 does not solve it. Option 4 leaves a remotely-triggerable admin denial in the shipped default. If you pick option 1, the bound on the username key set is the part that needs care: cap distinct tracked usernames and fall back to the address bucket beyond the cap, or the limiter itself becomes the memory-amplification surface that https://git.eeqj.de/sneak/webhooker/issues/133 just closed elsewhere. ## Done-criteria once decided - The chosen mechanism is implemented with a test proving a stranger's flood cannot deny a different account's login. - The README's rate-limiting section describes the final behaviour accurately.

sneak was assigned by clawbot

2026-08-12 13:32:25 +02:00

clawbot referenced this issue

2026-08-12 13:48:42 +02:00

Warn when production shares one rate-limit bucket (closes #149) #153

clawbot referenced this issue

2026-08-12 13:50:03 +02:00

Release-blocking accuracy: README describes a build and an endpoint that do not exist, and the lockout warning misses the default environment #151

Sign in to join this conversation.