From df9dfde55601340ba7a26000d3983029469c4d8a Mon Sep 17 00:00:00 2001 From: Mohammed Farhan K C Date: Tue, 29 Sep 2026 20:26:09 +0530 Subject: [PATCH 1/2] Give each auth route its own rate-limit bucket Every Redis-backed limiter writes into one keyspace, and the key was the client IP alone, so all six auth routes counted into a single sorted set and the smallest cap won for all of them. With register at 3, any three auth requests, a token refresh or a failed login included, returned 429 on the next registration for the rest of the window. It also undid the separate bucket reauth was given in 0.17.0. The key now carries the route name. Existing counters reset once on deploy. Namespacing at the caller rather than inside a backend keeps custom backends working unchanged, and keeps these buckets clear of any limiter an application passes to RateLimitMiddleware. Auditing the two backends against each other for the same class of problem turned up two more: The Redis challenge store raised on a non-positive TTL where the in-memory one simply wrote an already-expired entry, so PASSKEY_CHALLENGE_TTL=0 answered 500 from a route that needs no account. Both now store nothing and return None. No numeric setting had bounds. MAX_LOGIN_ATTEMPTS=0 locked every account on its first failed login; ACCESS_TOKEN_EXPIRE_MINUTES=0 issued dead tokens. These are rejected at construction now. The three settings that document a meaningful zero keep it. The regression tests run against both backends, since the memory one cannot show any of this. --- CHANGELOG.md | 8 ++++ docs/configuration.md | 5 ++ fastapi_fullauth/config.py | 35 ++++++++------ fastapi_fullauth/protection/challenges.py | 7 +++ fastapi_fullauth/protection/ratelimit.py | 18 +++++++- tests/test_config.py | 48 +++++++++++++++++++ tests/test_passkey.py | 22 +++++++++ tests/test_security.py | 56 +++++++++++++++++++++++ 8 files changed, 184 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ee51088..96dd8a0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## Unreleased + +### Fixed + +- **The Redis challenge store raised on a non-positive TTL** where the in-memory one wrote an entry that was simply already expired. With `PASSKEY_CHALLENGE_TTL` set to `0`, beginning a passkey sign-in answered `500` on Redis, on a route that needs no account. Both backends now agree: nothing is stored and the next `pop()` returns `None`. +- **Numeric settings reject values that cannot work.** Durations, expiries, attempt counts and rate-limit caps are validated at construction, so a mistake fails at startup instead of somewhere unrelated later: `MAX_LOGIN_ATTEMPTS=0` locked every account on its first failed login, and `PASSKEY_CHALLENGE_TTL=0` raised from Redis mid-request. The three settings that document a meaningful zero keep it: `JWT_LEEWAY_SECONDS`, `PASSWORD_MAX_LENGTH` and `REAUTH_MAX_AGE_SECONDS`. **If your configuration holds one of these impossible values, the application now refuses to start and says which setting.** +- **Auth rate limits shared one bucket per IP on Redis.** Each route gets its own limiter with its own cap, but the key was the client IP alone, and every Redis-backed limiter writes into the same keyspace, so all six auth routes counted into a single sorted set. The smallest cap won for all of them: with `register` at 3, any three auth requests, a token refresh or a failed login included, returned `429` on the next registration attempt for the rest of the window. It also broke the separation `reauth` was given in 0.17.0, where failed password checks were meant not to touch the sign-in budget. The key now carries the route name. Only Redis was affected; the in-memory backend keeps its counts per limiter instance, which is why the test suite never saw it, and why the new regression test runs against both backends. **Note:** the key format changed, so existing counters reset once on deploy. + ## 0.17.1 ### Changed diff --git a/docs/configuration.md b/docs/configuration.md index de91d81..2a5b424 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -124,6 +124,11 @@ If you want to be defensively explicit that no file is ever read, pass `FullAuth ## Reference +Durations, expiries, attempt counts and rate-limit caps must be at least 1, and a +value that cannot work is rejected when the config is constructed rather than +failing somewhere unrelated later. `JWT_LEEWAY_SECONDS`, `PASSWORD_MAX_LENGTH` +and `REAUTH_MAX_AGE_SECONDS` accept `0`, which means something for each of them. + ### Core | Option | Type | Default | Description | diff --git a/fastapi_fullauth/config.py b/fastapi_fullauth/config.py index 7fa75bc..0719ec2 100644 --- a/fastapi_fullauth/config.py +++ b/fastapi_fullauth/config.py @@ -40,6 +40,14 @@ class AuthRateLimits: # bucket, so failed attempts cannot exhaust an IP's sign-in budget. reauth: int = 5 + def __post_init__(self) -> None: + for name, value in vars(self).items(): + if value < 1: + raise ValueError( + f"AUTH_RATE_LIMITS.{name} must be at least 1; " + f"{value} would reject every request to that route." + ) + class FullAuthConfig(BaseSettings): model_config = SettingsConfigDict( @@ -61,19 +69,20 @@ class FullAuthConfig(BaseSettings): SECRET_KEY: str | None = None ALGORITHM: Literal["HS256", "HS384", "HS512"] = "HS256" - ACCESS_TOKEN_EXPIRE_MINUTES: int = 30 - REFRESH_TOKEN_EXPIRE_DAYS: int = 30 + ACCESS_TOKEN_EXPIRE_MINUTES: int = Field(default=30, ge=1) + REFRESH_TOKEN_EXPIRE_DAYS: int = Field(default=30, ge=1) REFRESH_TOKEN_ROTATION: bool = True - JWT_LEEWAY_SECONDS: int = 30 + JWT_LEEWAY_SECONDS: int = Field(default=30, ge=0) - PASSWORD_RESET_EXPIRE_MINUTES: int = 15 - EMAIL_VERIFY_EXPIRE_MINUTES: int = 1440 + PASSWORD_RESET_EXPIRE_MINUTES: int = Field(default=15, ge=1) + EMAIL_VERIFY_EXPIRE_MINUTES: int = Field(default=1440, ge=1) PASSWORD_HASH_ALGORITHM: Literal["argon2id", "bcrypt"] = "argon2id" - PASSWORD_MIN_LENGTH: int = 8 + PASSWORD_MIN_LENGTH: int = Field(default=8, ge=1) # Upper bound so an attacker can't spend the server's CPU hashing a huge # body. bcrypt caps at 72 bytes anyway; argon2 will hash whatever it is given. - PASSWORD_MAX_LENGTH: int = 4096 + # 0 disables the cap, hence ge=0 rather than ge=1. + PASSWORD_MAX_LENGTH: int = Field(default=4096, ge=0) LOGIN_FIELD: str = "email" # When True, login runs a dummy password verify on unknown-user / no-password @@ -86,12 +95,12 @@ class FullAuthConfig(BaseSettings): # destructive action (deleting the account, setting a first password). # Supplying the current password proves it too. 0 accepts only the password, # which locks passwordless accounts out of those actions. - REAUTH_MAX_AGE_SECONDS: int = 300 + REAUTH_MAX_AGE_SECONDS: int = Field(default=300, ge=0) LOCKOUT_ENABLED: bool = True LOCKOUT_BACKEND: Literal["memory", "redis"] = "memory" - MAX_LOGIN_ATTEMPTS: int = 5 - LOCKOUT_DURATION_MINUTES: int = 15 + MAX_LOGIN_ATTEMPTS: int = Field(default=5, ge=1) + LOCKOUT_DURATION_MINUTES: int = Field(default=15, ge=1) RATE_LIMIT_BACKEND: Literal["memory", "redis"] = "memory" TRUSTED_PROXY_HEADERS: Annotated[list[str], NoDecode] = [] @@ -104,14 +113,14 @@ class FullAuthConfig(BaseSettings): AUTH_RATE_LIMIT_ENABLED: bool = True AUTH_RATE_LIMITS: AuthRateLimits = Field(default_factory=AuthRateLimits) - AUTH_RATE_LIMIT_WINDOW_SECONDS: int = 60 + AUTH_RATE_LIMIT_WINDOW_SECONDS: int = Field(default=60, ge=1) REDIS_URL: str | None = None BLACKLIST_ENABLED: bool = True BLACKLIST_BACKEND: Literal["memory", "redis"] = "memory" - OAUTH_STATE_EXPIRE_SECONDS: int = 300 + OAUTH_STATE_EXPIRE_SECONDS: int = Field(default=300, ge=1) OAUTH_AUTO_LINK_BY_EMAIL: bool = True OAUTH_PKCE_ENABLED: bool = True @@ -124,7 +133,7 @@ class FullAuthConfig(BaseSettings): PASSKEY_RP_NAME: str | None = None PASSKEY_ORIGINS: Annotated[list[str], NoDecode] = [] PASSKEY_CHALLENGE_BACKEND: Literal["memory", "redis"] = "memory" - PASSKEY_CHALLENGE_TTL: int = 60 + PASSKEY_CHALLENGE_TTL: int = Field(default=60, ge=1) PASSKEY_REQUIRE_USER_VERIFICATION: bool = True API_PREFIX: str = "/api/v1" diff --git a/fastapi_fullauth/protection/challenges.py b/fastapi_fullauth/protection/challenges.py index 3019303..9e85daf 100644 --- a/fastapi_fullauth/protection/challenges.py +++ b/fastapi_fullauth/protection/challenges.py @@ -82,6 +82,13 @@ def __init__(self, redis_url: str) -> None: self._prefix = "fullauth:challenge:" async def store(self, key: str, challenge: str, ttl: int = 60) -> None: + # Redis rejects a non-positive expiry outright, where the in-memory store + # just writes an entry that is already expired. Skip the write instead, + # so both backends answer None to the next pop() rather than one of them + # raising into a 500 on a route that needs no account. + if ttl <= 0: + logger.warning("Challenge not stored; ttl=%s is not positive", ttl) + return await self._redis.set(f"{self._prefix}{key}", challenge, ex=ttl) async def pop(self, key: str) -> str | None: diff --git a/fastapi_fullauth/protection/ratelimit.py b/fastapi_fullauth/protection/ratelimit.py index 41b5a39..d632110 100644 --- a/fastapi_fullauth/protection/ratelimit.py +++ b/fastapi_fullauth/protection/ratelimit.py @@ -257,10 +257,24 @@ async def aclose(self) -> None: async def check(self, route_name: str, client_ip: str) -> None: limiter = self._limiters.get(route_name) - if limiter and not await limiter.is_allowed(client_ip): + if limiter is None: + return + + # The route belongs in the key, not just in the limiter lookup. Every + # Redis-backed limiter shares one keyspace, so keying on the IP alone + # put all six routes in a single bucket and the smallest cap, register + # at 3, became the effective limit for all of them: three refreshes + # locked out registration for a minute. The in-memory backend keeps its + # counts per instance, which is why only Redis showed it. Namespacing + # here rather than inside a backend keeps custom backends working + # unchanged, and keeps these buckets clear of any limiter the + # application passes to RateLimitMiddleware. + key = f"{route_name}:{client_ip}" + + if not await limiter.is_allowed(key): from fastapi import HTTPException - reset_in = await limiter.reset_time(client_ip) + reset_in = await limiter.reset_time(key) logger.warning("Auth rate limit exceeded: route=%s, ip=%s", route_name, client_ip) raise HTTPException( status_code=429, diff --git a/tests/test_config.py b/tests/test_config.py index 31085db..eec0349 100644 --- a/tests/test_config.py +++ b/tests/test_config.py @@ -7,6 +7,7 @@ import pytest from fastapi import FastAPI, Request from httpx import ASGITransport, AsyncClient +from pydantic import ValidationError from sqlalchemy.ext.asyncio import async_sessionmaker, create_async_engine from sqlmodel import SQLModel @@ -15,6 +16,8 @@ from fastapi_fullauth.types import CreateUserSchema, UserSchema from tests.conftest import make_test_adapter +SECRET = "test-secret-key-that-is-long-enough-32b" + async def _make_db(): engine = create_async_engine("sqlite+aiosqlite://", echo=False) @@ -974,3 +977,48 @@ async def test_cookie_backend_with_csrf_no_warning(config, adapter): with warnings.catch_warnings(): warnings.simplefilter("error") fa.init_app(app) + + +# ── Numeric settings reject values that cannot work ────────────────── + + +@pytest.mark.parametrize( + "setting", + [ + "ACCESS_TOKEN_EXPIRE_MINUTES", + "REFRESH_TOKEN_EXPIRE_DAYS", + "PASSWORD_RESET_EXPIRE_MINUTES", + "EMAIL_VERIFY_EXPIRE_MINUTES", + "MAX_LOGIN_ATTEMPTS", + "LOCKOUT_DURATION_MINUTES", + "AUTH_RATE_LIMIT_WINDOW_SECONDS", + "OAUTH_STATE_EXPIRE_SECONDS", + "PASSKEY_CHALLENGE_TTL", + ], +) +def test_non_positive_durations_and_counts_are_rejected(setting): + """Zero or negative here is never meaningful, and the failure it causes is + remote from the cause: PASSKEY_CHALLENGE_TTL=0 raised from Redis on a public + route, and MAX_LOGIN_ATTEMPTS=0 locked every account on its first failure. + Fail at construction instead.""" + with pytest.raises(ValidationError): + FullAuthConfig(SECRET_KEY=SECRET, **{setting: 0}) + + +def test_the_meaningful_zeros_are_still_accepted(): + """Three settings document zero as a real choice, so they must keep it.""" + config = FullAuthConfig( + SECRET_KEY=SECRET, + JWT_LEEWAY_SECONDS=0, # clock-sensitive tests rely on this + PASSWORD_MAX_LENGTH=0, # disables the cap + REAUTH_MAX_AGE_SECONDS=0, # accept only the current password + ) + assert config.JWT_LEEWAY_SECONDS == 0 + assert config.PASSWORD_MAX_LENGTH == 0 + assert config.REAUTH_MAX_AGE_SECONDS == 0 + + +def test_a_rate_limit_cap_below_one_is_rejected(): + """A cap of 0 rejects every request to that route, which nobody means.""" + with pytest.raises(ValueError, match="at least 1"): + AuthRateLimits(login=0) diff --git a/tests/test_passkey.py b/tests/test_passkey.py index 6eae916..7ca0626 100644 --- a/tests/test_passkey.py +++ b/tests/test_passkey.py @@ -278,3 +278,25 @@ async def test_challenge_store_evicts_challenges_that_were_never_used(): await store.store("fresh", "c", ttl=60) assert list(store._store) == ["fresh"] + + +@pytest.mark.asyncio +async def test_challenge_stores_agree_on_a_non_positive_ttl(): + """Redis rejects a non-positive expiry where the in-memory store writes an + entry that is already expired. Production picks the Redis backend whenever + REDIS_URL is set, so a divergence here is a 500 on a route that needs no + account, reachable by setting PASSKEY_CHALLENGE_TTL badly.""" + import fakeredis.aioredis + + from fastapi_fullauth.protection.challenges import RedisChallengeStore + + memory = InMemoryChallengeStore() + redis = RedisChallengeStore.__new__(RedisChallengeStore) + redis._redis = fakeredis.aioredis.FakeRedis(decode_responses=True) + redis._prefix = "fullauth:challenge:" + + for ttl in (0, -5): + await memory.store("k", "challenge", ttl=ttl) + await redis.store("k", "challenge", ttl=ttl) + assert await memory.pop("k") is None + assert await redis.pop("k") is None diff --git a/tests/test_security.py b/tests/test_security.py index bac2855..50d128a 100644 --- a/tests/test_security.py +++ b/tests/test_security.py @@ -790,3 +790,59 @@ async def test_in_memory_sweep_never_drops_live_entries(): await blacklist.add("trigger", ttl_seconds=10) assert await blacklist.is_blacklisted("long-lived") assert "short" not in blacklist._blacklisted + + +# ── Auth rate limits are per route ─────────────────────────────────── + + +def _auth_limiter(backend: str): + """An AuthRateLimiter on the given backend, with Redis faked in place. + + The Redis limiters are built by the real code path, then pointed at one + shared fake server, which is what makes a cross-route collision visible. + """ + from fastapi_fullauth import FullAuthConfig + from fastapi_fullauth.protection.ratelimit import AuthRateLimiter + + kwargs = {"SECRET_KEY": "test-secret-key-that-is-long-enough-32b", "BACKEND": backend} + if backend == "redis": + kwargs["REDIS_URL"] = "redis://rate-limit-test:6379/0" + limiter = AuthRateLimiter(FullAuthConfig(**kwargs)) + + if backend == "redis": + import fakeredis.aioredis + + server = fakeredis.aioredis.FakeServer() + for each in limiter._limiters.values(): + each._redis = fakeredis.aioredis.FakeRedis(server=server, decode_responses=True) + return limiter + + +@pytest.mark.parametrize("backend", ["memory", "redis"]) +@pytest.mark.asyncio +async def test_auth_routes_do_not_share_a_rate_limit_bucket(backend): + """Every Redis limiter shares one keyspace, so the key has to carry the + route. Keyed on the IP alone, all six routes counted into one bucket and the + smallest cap won: three refreshes locked out registration. The in-memory + backend counts per instance and never showed it, hence both backends here.""" + from fastapi import HTTPException + + limiter = _auth_limiter(backend) + ip = "203.0.113.9" + + # refresh allows 30; spend enough to exhaust register's cap of 3. + for _ in range(5): + await limiter.check("refresh", ip) + + # register has not been called at all, so it must still have its full cap. + for _ in range(3): + await limiter.check("register", ip) + + with pytest.raises(HTTPException) as exc: + await limiter.check("register", ip) + assert exc.value.status_code == 429 + + # And exhausting register must leave the documented separate bucket alone. + await limiter.check("reauth", ip) + + await limiter.aclose() From 986ae70e5c52bdc5fc444453361793d3c3ffd5ef Mon Sep 17 00:00:00 2001 From: Mohammed Farhan K C Date: Tue, 29 Sep 2026 20:28:23 +0530 Subject: [PATCH 2/2] Release 0.17.2 Bumps the version and moves the Unreleased changelog entries under 0.17.2. --- CHANGELOG.md | 2 +- fastapi_fullauth/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96dd8a0..a7f0e1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,6 @@ # Changelog -## Unreleased +## 0.17.2 ### Fixed diff --git a/fastapi_fullauth/__init__.py b/fastapi_fullauth/__init__.py index 10d6fed..a37dd6c 100644 --- a/fastapi_fullauth/__init__.py +++ b/fastapi_fullauth/__init__.py @@ -1,4 +1,4 @@ -__version__ = "0.17.1" +__version__ = "0.17.2" from fastapi_fullauth.config import AuthRateLimits, FullAuthConfig from fastapi_fullauth.fullauth import FullAuth