-
Notifications
You must be signed in to change notification settings - Fork 593
fix(auth): gate cache deny on non-null user_id to prevent cross-user pollution #5389
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -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( | ||||||||||||||||||||||||||||||||||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Fix test logic: The test patches As a result, To correctly test the catch-all exception handler caching when 💚 Proposed fix to trigger the catch-all handler+ async def mock_get_cache(*args, **kwargs):
+ if kwargs.get("namespace") == "get_supertokens_user_by_id":
+ # Return a valid user so `user_id` gets populated
+ return {"user_id": "user-123", "user_email": "test@test.com"}
+ # Raise an exception on downstream read to trigger the catch-all handler
+ raise Exception("mocked downstream error")
+
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_cache", side_effect=mock_get_cache), \
pytest.raises(Exception):
await auth_user(📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||
| 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." | ||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.