-
Notifications
You must be signed in to change notification settings - Fork 429
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
fix: Catch errors when failing to verify JWTs #5153
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 3 Skipped Deployments
|
Docker builds report
|
Uffizzi Preview |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5153 +/- ##
==========================================
+ Coverage 97.47% 97.50% +0.02%
==========================================
Files 1224 1224
Lines 42590 42623 +33
==========================================
+ Hits 41515 41558 +43
+ Misses 1075 1065 -10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I am surprised that the new code path is covered by tests and no existing tests are modified, which suggests the backend behaviour is not changed by this PR? |
@khvn26 I don't see any tests for this code, but in any case you can test this by trying to fetch a non-existent SAML configuration. Even if you are using an invalid JWT cookie, this should return 404 when using this fix:
Before this change, the request fails and prevents users with expired or invalid tokens from logging in via SAML:
|
api/tests/unit/custom_auth/jwt_cookie/test_unit_jwt_cookie_authentication.py
Outdated
Show resolved
Hide resolved
api/tests/unit/custom_auth/jwt_cookie/test_unit_jwt_cookie_authentication.py
Outdated
Show resolved
Hide resolved
api/tests/unit/custom_auth/jwt_cookie/test_unit_jwt_cookie_authentication.py
Outdated
Show resolved
Hide resolved
5b357b4
to
c7525cb
Compare
b892781
to
2d9271f
Compare
Fixes #4982.
The issue as described is incorrect, as the SAML routes already use
AllowAny
. The problem is actually caused byget_validated_token
returning an exception when called with an invalid JWT. We now catch any of these expected errors and gracefully fail the authentication instead.This is completely untested.