diff --git a/CHANGELOG.md b/CHANGELOG.md index ee51088..a7f0e1f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog +## 0.17.2 + +### 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/__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 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()