fix(auth): gate cache deny on non-null user_id to prevent cross-user pollution#5389
fix(auth): gate cache deny on non-null user_id to prevent cross-user pollution#5389truecallerabreham wants to merge 2 commits into
Conversation
…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 Agenta-AI#5387
dosubot
Bot
added
the
size:M
|
@truecallerabreham is attempting to deploy a commit to the agenta projects Team on Vercel. A member of the Team first needs to authorize it. |
|
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe auth middleware now skips deny-cache writes when no user ID is available. Unit tests cover anonymous sessions and confirm identified sessions still receive deny entries. ChangesAuth cache scoping
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
api/oss/tests/pytest/unit/middlewares/test_cache_deny.py (1)
29-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove unused variable assignment.
Static analysis indicates that
mock_set_cacheis assigned but never used (Flake8 F841).♻️ Proposed refactor
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.set_cache", side_effect=capture_set_cache), \ patch("oss.src.middlewares.auth.get_cache", return_value=None), \ pytest.raises(Exception):Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e5aa8be8-d861-4c42-982c-ca92fcd02538
📒 Files selected for processing (2)
api/oss/src/middlewares/auth.pyapi/oss/tests/pytest/unit/middlewares/test_cache_deny.py
| 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.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix test logic: GatewayTimeoutException bypasses the catch-all handler.
The test patches get_supertokens_user_by_id to raise an Exception. However, inside the middleware, this exception is caught and re-raised as a GatewayTimeoutException (line 451). This specific exception is explicitly handled earlier in the exception block (line 765) and re-raised directly, completely bypassing the catch-all Exception handler where the new user_id is not None check and caching logic reside.
As a result, set_cache will never be called, and assert len(deny_calls) >= 1 will fail.
To correctly test the catch-all exception handler caching when user_id is populated, you can mock get_cache to return a user payload (which sets user_id) and then raise an exception on a downstream cache read to trigger the correct handler.
💚 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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( | |
| 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", side_effect=mock_get_cache), \ | |
| pytest.raises(Exception): | |
| await auth_user( |
- 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
|
Auth.py diff: See files changed for the full diff of the cache deny fix. The fix adds if user_id is not None: guards around set_cache calls in both exception handlers. |
truecallerabreham
left a comment
truecallerabreham
left a comment
There was a problem hiding this comment.
Demo Screenshot\n\n
\n\nThe diff shows the if user_id is not None: guard added around both set_cache deny calls.
2 participants
Summary
When user_id is None (anonymous/unauthenticated requests), the outer exception handlers in �erify_bearer_token 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.
Changes
Demo
Before fix - set_cache called unconditionally with user_id=None:
python except UnauthorizedException as exc: _deny("catch_unauthorized", "UnauthorizedException propagated", exc) await set_cache( project_id=query_project_id, user_id=user_id, # None for anonymous users! namespace="verify_bearer_token", key=cache_key, value={"deny": True}, ) raise excAfter fix - set_cache gated on user_id is not None:
python except UnauthorizedException as exc: _deny("catch_unauthorized", "UnauthorizedException propagated", exc) if user_id is not None: # Guard added await set_cache( project_id=query_project_id, user_id=user_id, namespace="verify_bearer_token", key=cache_key, value={"deny": True}, ) raise excImpact
Testing
Related