fix(connect): catch authlib OAuthError in background token refresh thread - #2111
fix(connect): catch authlib OAuthError in background token refresh thread#2111Solaris-star wants to merge 1 commit into
Conversation
…read The periodic_refresh_token daemon thread only caught httpx.HTTPError. When the identity provider rejects a refresh at the OAuth2 protocol level (e.g. Keycloak returns 400 invalid_grant due to token rotation, reuse, revocation, or session expiry), authlib raises OAuthError which inherits from AuthlibBaseError — NOT from httpx.HTTPError. The uncaught exception killed the daemon thread silently, permanently breaking the connection's ability to refresh tokens (weaviate#2110). Add AuthlibBaseError to the except clause so both transport-level (HTTPError) and protocol-level (OAuthError) failures are caught, warned, and retried after 1 second. Adds test/test_token_refresh.py with 4 tests verifying: - AuthlibBaseError is NOT a subclass of HTTPError (confirms the bug) - The fixed except clause catches AuthlibBaseError - HTTPError is still caught (no regression) - Concrete OAuthError subclass is also caught Fixes weaviate#2110
![orca-security-eu[bot]](https://avatars.githubusercontent.com/in/276244?s=60&v=4){kind=small}
There was a problem hiding this comment.
Orca Security Scan Summary
| Status | Check | Issues by priority | |
|---|---|---|---|
 Passed |
Infrastructure as Code |  0  0  0  0 |
View in Orca |
| Passed |
SAST | 0 0 0 0 |
View in Orca |
| Passed |
Secrets | 0 0 0 0 |
View in Orca |
| Passed |
Vulnerabilities | 0 0 0 0 |
View in Orca |
|
To avoid any confusion in the future about your contribution to Weaviate, we work with a Contributor License Agreement. If you agree, you can simply add a comment to this PR that you agree with the CLA so that we can merge. |
|
I have read and agree to the Contributor License Agreement. |
| refresh_session() | ||
| refresh_time = update_refresh_time() | ||
| except HTTPError as exc: | ||
| except (HTTPError, AuthlibBaseError) as exc: |
There was a problem hiding this comment.
I think in general this makes sense, but I'm worried a bit about continued retrying for non-recoverable errors. Could you add a check for which kind of error it is to determine if we should retry every second?
| except (HTTPError, AuthlibBaseError): | ||
| pass | ||
| else: | ||
| pytest.fail("OAuthError was not caught by (HTTPError, AuthlibBaseError)") |
There was a problem hiding this comment.
This tests general python behaviour, I dont think we should have them in the repo
3 participants
Summary
Fixes #2110
The
periodic_refresh_tokendaemon thread only caughthttpx.HTTPError. When the identity provider rejects a refresh at the OAuth2 protocol level (e.g. Keycloak returns400 invalid_grantdue to token rotation, reuse, revocation, or session expiry), authlib raisesOAuthErrorwhich inherits fromAuthlibBaseError— NOT fromhttpx.HTTPError. The uncaught exception killed the daemon thread silently, permanently breaking the connection's ability to refresh tokens.Fix
Add
AuthlibBaseErrorto the except clause so both transport-level (HTTPError) and protocol-level (OAuthError) failures are caught, warned, and retried after 1 second:Tests
Added
test/test_token_refresh.pywith 4 tests:test_authlib_base_error_is_not_http_error— confirmsAuthlibBaseErroris NOT a subclass ofHTTPError(proves the bug exists)test_authlib_base_error_caught_by_fixed_except_clause— the fixed clause catches ittest_http_error_still_caught_by_fixed_except_clause— no regression on existing behaviortest_oauth_error_subclass_caught— concreteOAuthErrorsubclass is also caught