From c6015a11208bde375d62ae67ef2cd7773253465f Mon Sep 17 00:00:00 2001 From: Abreham Melese Date: Sat, 18 Jul 2026 16:10:50 -0700 Subject: [PATCH 1/2] fix(auth): gate cache deny on non-null user_id to prevent cross-user pollution When user_id is None (anonymous/unauthenticated requests), the outer exception handlers in auth_user call set_cache with a deny value using a generic cache key (empty user segment). Since all anonymous users share the same cache key, one user's auth failure can cause all subsequent anonymous requests to also fail auth for up to 5 minutes. This fix adds a user_id is not None guard before writing deny entries to the cache, so anonymous failures are not cached globally. Includes regression tests verifying that deny entries are only cached when user_id is known. Closes #5387 --- api/oss/src/middlewares/auth.py | 30 ++++--- .../unit/middlewares/test_cache_deny.py | 88 +++++++++++++++++++ 2 files changed, 104 insertions(+), 14 deletions(-) create mode 100644 api/oss/tests/pytest/unit/middlewares/test_cache_deny.py diff --git a/api/oss/src/middlewares/auth.py b/api/oss/src/middlewares/auth.py index 591fd85435..4db43fa907 100644 --- a/api/oss/src/middlewares/auth.py +++ b/api/oss/src/middlewares/auth.py @@ -767,13 +767,14 @@ def _deny(stage: str, reason: str, exc: Optional[Exception] = None): except UnauthorizedException as exc: _deny("catch_unauthorized", "UnauthorizedException propagated", exc) - await set_cache( - project_id=query_project_id, - user_id=user_id, - namespace="verify_bearer_token", - key=cache_key, - value={"deny": True}, - ) + if user_id is not None: + await set_cache( + project_id=query_project_id, + user_id=user_id, + namespace="verify_bearer_token", + key=cache_key, + value={"deny": True}, + ) raise exc @@ -784,13 +785,14 @@ def _deny(stage: str, reason: str, exc: Optional[Exception] = None): except Exception as exc: # pylint: disable=bare-except _deny("catch_all", "unexpected exception", exc) - await set_cache( - project_id=query_project_id, - user_id=user_id, - namespace="verify_bearer_token", - key=cache_key, - value={"deny": True}, - ) + if user_id is not None: + await set_cache( + project_id=query_project_id, + user_id=user_id, + namespace="verify_bearer_token", + key=cache_key, + value={"deny": True}, + ) raise UnauthorizedException() from exc diff --git a/api/oss/tests/pytest/unit/middlewares/test_cache_deny.py b/api/oss/tests/pytest/unit/middlewares/test_cache_deny.py new file mode 100644 index 0000000000..102f1c4417 --- /dev/null +++ b/api/oss/tests/pytest/unit/middlewares/test_cache_deny.py @@ -0,0 +1,88 @@ +import pytest +from unittest.mock import AsyncMock, patch, MagicMock + + +@pytest.mark.asyncio +async def test_cache_deny_not_written_when_user_id_is_none(): + """Verify that set_cache deny entries are not written when user_id is None. + + When user_id is None (anonymous requests), writing a deny entry to the cache + produces a generic key shared by all anonymous users. One user's auth failure + must not deny all other anonymous users for the cache TTL. + """ + from oss.src.middlewares.auth import auth_user + + mock_request = MagicMock() + mock_request.url.path = "/test" + mock_request.method = "GET" + mock_request.headers = {} + + mock_session = AsyncMock() + mock_session.get_user_id.return_value = None + + set_cache_calls = [] + + async def capture_set_cache(**kwargs): + set_cache_calls.append(kwargs) + return True + + with patch("oss.src.middlewares.auth.get_session", return_value=mock_session), \ + patch("oss.src.middlewares.auth.set_cache", side_effect=capture_set_cache) as mock_set_cache, \ + patch("oss.src.middlewares.auth.get_cache", return_value=None), \ + pytest.raises(Exception): + await auth_user( + request=mock_request, + bearer_token="", + query_project_id="test-project-id", + query_workspace_id="test-workspace-id", + ) + + # set_cache should NOT have been called with a deny value when user_id is None + deny_calls = [c for c in set_cache_calls if c.get("value") == {"deny": True}] + assert len(deny_calls) == 0, ( + f"set_cache was called with deny value {len(deny_calls)} time(s) " + f"when user_id is None. This would pollute the shared anonymous cache." + ) + + +@pytest.mark.asyncio +async def test_cache_deny_written_when_user_id_is_set(): + """Verify that set_cache deny entries ARE written when user_id is set. + + When user_id is known, caching the deny is correct — it prevents repeated + failing lookups for the same specific user. + """ + from oss.src.middlewares.auth import auth_user + + mock_request = MagicMock() + mock_request.url.path = "/test" + mock_request.method = "GET" + mock_request.headers = {} + + mock_session = AsyncMock() + mock_session.get_user_id.return_value = "user-123" + + set_cache_calls = [] + + async def capture_set_cache(**kwargs): + set_cache_calls.append(kwargs) + return True + + with patch("oss.src.middlewares.auth.get_session", return_value=mock_session), \ + patch("oss.src.middlewares.auth.set_cache", side_effect=capture_set_cache), \ + patch("oss.src.middlewares.auth.get_cache", return_value=None), \ + patch("oss.src.middlewares.auth.get_supertokens_user_by_id", side_effect=Exception("timeout")), \ + pytest.raises(Exception): + await auth_user( + request=mock_request, + bearer_token="", + query_project_id="test-project-id", + query_workspace_id="test-workspace-id", + ) + + # set_cache SHOULD have been called with a deny value when user_id is set + deny_calls = [c for c in set_cache_calls if c.get("value") == {"deny": True}] + assert len(deny_calls) >= 1, ( + "set_cache was not called with deny value when user_id was set. " + "Auth failures for known users should be cached." + ) \ No newline at end of file From 383764636909e78e3ed37be8be8f4aa099053bb7 Mon Sep 17 00:00:00 2001 From: Abreham Melese Date: Sat, 18 Jul 2026 16:49:39 -0700 Subject: [PATCH 2/2] fix(test): correct test imports and logic per CodeRabbit review - Import verify_bearer_token instead of nonexistent auth_user - Fix test_cache_deny_written_when_user_id_is_set: mock get_supertokens_user_by_id to return None (user not found) instead of raising Exception (which triggers GatewayTimeoutException and bypasses the catch-all handler) - Remove unused mock_set_cache variable assignment --- .../pytest/unit/middlewares/test_cache_deny.py | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/api/oss/tests/pytest/unit/middlewares/test_cache_deny.py b/api/oss/tests/pytest/unit/middlewares/test_cache_deny.py index 102f1c4417..54762cff41 100644 --- a/api/oss/tests/pytest/unit/middlewares/test_cache_deny.py +++ b/api/oss/tests/pytest/unit/middlewares/test_cache_deny.py @@ -10,7 +10,7 @@ async def test_cache_deny_not_written_when_user_id_is_none(): produces a generic key shared by all anonymous users. One user's auth failure must not deny all other anonymous users for the cache TTL. """ - from oss.src.middlewares.auth import auth_user + from oss.src.middlewares.auth import verify_bearer_token mock_request = MagicMock() mock_request.url.path = "/test" @@ -27,10 +27,9 @@ async def capture_set_cache(**kwargs): return True with patch("oss.src.middlewares.auth.get_session", return_value=mock_session), \ - patch("oss.src.middlewares.auth.set_cache", side_effect=capture_set_cache) as mock_set_cache, \ - patch("oss.src.middlewares.auth.get_cache", return_value=None), \ + patch("oss.src.middlewares.auth.set_cache", side_effect=capture_set_cache), \ pytest.raises(Exception): - await auth_user( + await verify_bearer_token( request=mock_request, bearer_token="", query_project_id="test-project-id", @@ -52,7 +51,7 @@ async def test_cache_deny_written_when_user_id_is_set(): When user_id is known, caching the deny is correct — it prevents repeated failing lookups for the same specific user. """ - from oss.src.middlewares.auth import auth_user + from oss.src.middlewares.auth import verify_bearer_token mock_request = MagicMock() mock_request.url.path = "/test" @@ -68,12 +67,15 @@ async def capture_set_cache(**kwargs): set_cache_calls.append(kwargs) return True + # get_cache returns None (cache miss), get_supertokens_user_by_id returns + # None (user not found), which raises UnauthorizedException. The outer + # handler catches it and caches the deny with the now-populated user_id. with patch("oss.src.middlewares.auth.get_session", return_value=mock_session), \ patch("oss.src.middlewares.auth.set_cache", side_effect=capture_set_cache), \ patch("oss.src.middlewares.auth.get_cache", return_value=None), \ - patch("oss.src.middlewares.auth.get_supertokens_user_by_id", side_effect=Exception("timeout")), \ + patch("oss.src.middlewares.auth.get_supertokens_user_by_id", return_value=None), \ pytest.raises(Exception): - await auth_user( + await verify_bearer_token( request=mock_request, bearer_token="", query_project_id="test-project-id", @@ -85,4 +87,4 @@ async def capture_set_cache(**kwargs): assert len(deny_calls) >= 1, ( "set_cache was not called with deny value when user_id was set. " "Auth failures for known users should be cached." - ) \ No newline at end of file + )