Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR adds conditional auth router mounting based on ChangesAuth Flow Kill Switch & Router Conditional Mounting
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@surfsense_backend/app/app.py`:
- Around line 597-614: The registration_allowed() kill-switch currently doesn't
block refresh-token rotation because auth_router is mounted unconditionally and
the /auth/jwt/refresh handler in refresh_access_token can still mint sessions;
fix this by gating refresh-token rotation: either move the refresh route out of
auth_router so it is only mounted when registration_allowed() is True, or add an
explicit registration_allowed() check at the start of refresh_access_token (and
return the same “registration disabled” response) while leaving revoke/logout
handlers untouched; update references where auth_router is mounted to ensure
/auth/jwt/refresh is protected (also check the other mounting sites noted around
the auth_router usage).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f0ab8c64-f751-4b7e-a40d-6c3fba5f1184
📒 Files selected for processing (1)
surfsense_backend/app/app.py
| def registration_allowed(): | ||
| """Master auth kill switch keyed on the REGISTRATION_ENABLED env var. | ||
|
|
||
| Despite the name, this dependency does NOT only gate registration. When | ||
| REGISTRATION_ENABLED is FALSE it intentionally blocks every auth surface | ||
| that could mint or refresh a session for an attacker: | ||
|
|
||
| * email/password ``POST /auth/register`` | ||
| * email/password ``POST /auth/jwt/login`` | ||
| * the Google OAuth router (``/auth/google/authorize`` and the shared | ||
| ``/auth/google/callback`` handles both new signups and login for | ||
| existing users, so flipping this off locks both) | ||
| * the bespoke ``/auth/google/authorize-redirect`` helper used by the UI | ||
|
|
||
| Use it as a temporary "freeze all new sessions" lever during incident | ||
| response. It is not a way to disable signup while keeping login working; | ||
| for that, override ``UserManager.oauth_callback`` instead. | ||
| """ |
There was a problem hiding this comment.
The kill switch still leaves refresh-token session minting open.
registration_allowed() is now documented and wired as a “freeze all new sessions” control, but Lines 804-805 still mount auth_router unconditionally. In surfsense_backend/app/routes/auth_routes.py:23-56, /auth/jwt/refresh can still rotate a valid refresh token into a fresh access/refresh pair without this dependency. That means an attacker holding a stolen refresh token can keep minting sessions even after REGISTRATION_ENABLED is turned off.
Please gate the refresh route too, either by splitting it out from auth_router or by enforcing the check inside refresh_access_token while leaving revoke/logout available.
Also applies to: 759-794, 855-859
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@surfsense_backend/app/app.py` around lines 597 - 614, The
registration_allowed() kill-switch currently doesn't block refresh-token
rotation because auth_router is mounted unconditionally and the
/auth/jwt/refresh handler in refresh_access_token can still mint sessions; fix
this by gating refresh-token rotation: either move the refresh route out of
auth_router so it is only mounted when registration_allowed() is True, or add an
explicit registration_allowed() check at the start of refresh_access_token (and
return the same “registration disabled” response) while leaving revoke/logout
handlers untouched; update references where auth_router is mounted to ensure
/auth/jwt/refresh is protected (also check the other mounting sites noted around
the auth_router usage).
1 participant
Description
Motivation and Context
FIX #
Screenshots
API Changes
Change Type
Testing Performed
Checklist
High-level PR Summary
This PR fixes a security vulnerability where manual authentication endpoints (email/password registration and login) were exposed even when running in Google-OAuth-only mode, allowing bots to bypass the intended
AUTH_TYPE=GOOGLErestriction. The fix conditionally mounts email/password auth routers only when not in OAuth-only mode, extends theregistration_allowedkill switch to cover all session-minting endpoints (including login and OAuth flows), and adds comprehensive documentation explaining thatREGISTRATION_ENABLED=falseacts as an emergency "freeze all new sessions" lever for incident response.⏱️ Estimated Review Time: 30-90 minutes
💡 Review Order Suggestion
surfsense_backend/app/app.pySummary by CodeRabbit