Summary
RateLimiter::checkWithKey() in api/lib/RateLimiter.php returns true (request allowed) if the underlying Redis lookup returns null. Since Redis is an optional dependency for self-hosted deployments of this project (per config.inc.default.php), any deployment where Redis is unavailable — down, misconfigured, network partition, etc. — silently loses rate limiting on every endpoint that relies on it: Login, CreateAccount, ResetPassword, ConfirmMFADevice, and others.
This is a "fail open" design: the safe default for a security control is to fail closed (deny/restrict) when its backing store is unreachable, not fail open (allow unrestricted). As written, an operational issue with Redis (which has nothing to do with an attacker) turns into a silent removal of brute-force protection across the whole instance, with no logging or alerting to indicate this has happened.
Suggested fix
A few options, roughly in order of preference:
- Fail closed with a safety valve: if Redis is unreachable, apply a conservative fallback (e.g., a stricter default limit, or reject the request with a 503-style "try again later") rather than allowing unrestricted access. This needs to be balanced against availability — a full fail-closed approach means Redis becomes a hard dependency for login/signup/etc., so this should probably be configurable.
- Log/alert on fallback: at minimum, emit a clear error-level log line (and ideally a metric, since this project already has Prometheus integration) whenever
checkWithKey() can't reach Redis, so operators notice degraded protection instead of it failing silently.
- Document the tradeoff: if fail-open is intentionally kept as the default for availability reasons, this should be called out explicitly in the docs/config comments for
config.inc.default.php, since it's not obvious from reading the endpoint code that rate limiting has this dependency.
Happy to put together a PR for whichever approach seems right — just wanted to raise it for discussion first since the right tradeoff here depends on how much you want to prioritize availability vs. fail-safe behavior for self-hosted instances.
Summary
RateLimiter::checkWithKey()inapi/lib/RateLimiter.phpreturnstrue(request allowed) if the underlying Redis lookup returnsnull. Since Redis is an optional dependency for self-hosted deployments of this project (perconfig.inc.default.php), any deployment where Redis is unavailable — down, misconfigured, network partition, etc. — silently loses rate limiting on every endpoint that relies on it:Login,CreateAccount,ResetPassword,ConfirmMFADevice, and others.This is a "fail open" design: the safe default for a security control is to fail closed (deny/restrict) when its backing store is unreachable, not fail open (allow unrestricted). As written, an operational issue with Redis (which has nothing to do with an attacker) turns into a silent removal of brute-force protection across the whole instance, with no logging or alerting to indicate this has happened.
Suggested fix
A few options, roughly in order of preference:
checkWithKey()can't reach Redis, so operators notice degraded protection instead of it failing silently.config.inc.default.php, since it's not obvious from reading the endpoint code that rate limiting has this dependency.Happy to put together a PR for whichever approach seems right — just wanted to raise it for discussion first since the right tradeoff here depends on how much you want to prioritize availability vs. fail-safe behavior for self-hosted instances.