From 3baceb524c155cb748fdab34906ca5e16e2e5c57 Mon Sep 17 00:00:00 2001 From: nazarli-shabnam Date: Fri, 10 Jul 2026 13:25:48 +0400 Subject: [PATCH] fix(auth): surface refresh-token revocation failure instead of silently reporting success revoke_all_user_refresh_tokens() caught any Redis exception and returned 0, indistinguishable from "this user genuinely had no active refresh tokens." Its only caller, password-reset confirm, relies on it to guarantee that any refresh token an attacker might hold becomes invalid the moment a user resets their password. On a Redis hiccup, the endpoint still returned 200 "ok" while silently leaving those tokens valid -- undermining the entire point of revoking on reset. - token_store.py: return type is now int | None. None means "could not complete the revocation," which callers must not treat as "0 tokens existed." Logs now include the user_id for easier correlation. - router.py: password_reset_confirm_endpoint raises 503 (not 200) when revocation returns None. The password change itself is not rolled back (it already succeeded and reverting it would be worse UX), but the response now honestly reflects that the "revoke other sessions" guarantee couldn't be confirmed. Added tests/test_token_store_revocation.py with a fake Redis, covering all three cases directly: 0 tokens existed, N tokens revoked, and a Redis failure returning None (explicitly asserted != 0, the exact distinction that was previously lost). Verified the unaffected happy path end-to-end against a live docker-compose stack: issued a real refresh token for a test user, requested+confirmed a password reset through the actual HTTP endpoints, and confirmed both that the password changed (bcrypt verify) and that the refresh token's Redis key was actually deleted. Fixes #27 --- api/app/app/core/token_store.py | 16 ++++- api/app/app/modules/auth/router.py | 16 ++++- api/app/tests/test_token_store_revocation.py | 68 ++++++++++++++++++++ 3 files changed, 96 insertions(+), 4 deletions(-) create mode 100644 api/app/tests/test_token_store_revocation.py diff --git a/api/app/app/core/token_store.py b/api/app/app/core/token_store.py index db7a1d6..737baad 100644 --- a/api/app/app/core/token_store.py +++ b/api/app/app/core/token_store.py @@ -59,7 +59,17 @@ def revoke_refresh_jti(jti: str) -> None: logger.error("Failed to revoke refresh jti: %s", exc) -def revoke_all_user_refresh_tokens(user_id: str) -> int: +def revoke_all_user_refresh_tokens(user_id: str) -> int | None: + """Revoke every refresh token issued to a user. + + Returns the number of tokens revoked (0 if none existed), or None if + the operation could not be completed (e.g. Redis unreachable) -- the + caller MUST treat None as "revocation not guaranteed", not as + "nothing to revoke". This matters most for password-reset, which + relies on this call to actually invalidate any session an attacker + may hold; silently reporting success on a Redis hiccup would leave + those sessions valid with no indication anything went wrong. + """ try: redis = get_redis() index_key = f"{USER_INDEX_PREFIX}{user_id}" @@ -74,5 +84,5 @@ def revoke_all_user_refresh_tokens(user_id: str) -> int: pipe.execute() return len(jtis) except Exception as exc: - logger.error("Failed to bulk-revoke refresh tokens: %s", exc) - return 0 + logger.error("Failed to bulk-revoke refresh tokens for %s: %s", user_id, exc) + return None diff --git a/api/app/app/modules/auth/router.py b/api/app/app/modules/auth/router.py index 8d8b386..4b8e888 100644 --- a/api/app/app/modules/auth/router.py +++ b/api/app/app/modules/auth/router.py @@ -126,7 +126,21 @@ def password_reset_confirm_endpoint( detail="Invalid or expired reset token.", ) if result.user_id: - revoke_all_user_refresh_tokens(result.user_id) + revoked = revoke_all_user_refresh_tokens(result.user_id) + if revoked is None: + # The password itself was already changed -- don't pretend + # that didn't happen -- but we can't guarantee any refresh + # token an attacker holds was actually invalidated, which is + # the whole point of revoking on reset. Surface that instead + # of silently reporting full success. + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail=( + "Your password was changed, but we couldn't confirm " + "your other sessions were signed out. Please sign " + "out of other devices manually, or try again shortly." + ), + ) return SimpleStatusResponse() diff --git a/api/app/tests/test_token_store_revocation.py b/api/app/tests/test_token_store_revocation.py new file mode 100644 index 0000000..9ba7645 --- /dev/null +++ b/api/app/tests/test_token_store_revocation.py @@ -0,0 +1,68 @@ +"""revoke_all_user_refresh_tokens must distinguish "nothing to revoke" +(0) from "couldn't complete the revocation" (None) -- callers like +password-reset confirm rely on this to know whether it's safe to report +success.""" + +from typing import Any + + +from app.core import token_store + + +class FakePipeline: + def __init__(self, redis: "FakeRedis") -> None: + self._redis = redis + self._ops: list[tuple[str, tuple[Any, ...]]] = [] + + def delete(self, key: str) -> "FakePipeline": + self._ops.append(("delete", (key,))) + return self + + def execute(self) -> list[Any]: + for op, args in self._ops: + getattr(self._redis, f"_do_{op}")(*args) + return [None] * len(self._ops) + + +class FakeRedis: + def __init__( + self, members: set[bytes] | None = None, fail: bool = False + ) -> None: + self._members = members or set() + self._fail = fail + self._store: dict[str, bytes] = {} + + def smembers(self, _key: str) -> set[bytes]: + if self._fail: + raise ConnectionError("redis unreachable") + return self._members + + def pipeline(self) -> FakePipeline: + return FakePipeline(self) + + def _do_delete(self, key: str) -> None: + self._store.pop(key, None) + + +def test_returns_zero_when_user_has_no_tokens(monkeypatch: Any) -> None: + fake = FakeRedis(members=set()) + monkeypatch.setattr(token_store, "get_redis", lambda: fake) + assert token_store.revoke_all_user_refresh_tokens("user-1") == 0 + + +def test_returns_count_when_tokens_revoked(monkeypatch: Any) -> None: + fake = FakeRedis(members={b"jti-a", b"jti-b", b"jti-c"}) + monkeypatch.setattr(token_store, "get_redis", lambda: fake) + assert token_store.revoke_all_user_refresh_tokens("user-1") == 3 + + +def test_returns_none_not_zero_when_redis_fails(monkeypatch: Any) -> None: + fake = FakeRedis(fail=True) + monkeypatch.setattr(token_store, "get_redis", lambda: fake) + result = token_store.revoke_all_user_refresh_tokens("user-1") + # The critical assertion: a Redis failure must be distinguishable + # from "there was nothing to revoke". Returning 0 here would let a + # caller (e.g. password-reset confirm) believe revocation succeeded + # when it didn't run at all. + assert result is None + assert result != 0