✅ Snyk checks have passed. No issues have been found so far.

Status	Scanner	Critical	High	Medium	Low	Total (0)
✅	Open Source Security	0	0	0	0	0 issues
✅	Licenses	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

✅ PR preview is ready!

📈 Frontend coverage change detected

The frontend unit test (vitest) coverage has increased by 0.0400%

• Current PR: 86.3000% (12497 lines, 1711 missed)
• Latest develop: 86.2600% (12497 lines, 1717 missed)

🎉 Great job on improving test coverage!

📊 View detailed coverage comparison

@cursor review

Summary

This PR fixes authentication-related CI issues caused by dependency updates, specifically addressing compatibility with authlib 1.6.6+. The new authlib version changed its behavior to always write OAuth state to session even when a cache is available, which broke Streamlit's Tornado integration since Tornado doesn't have persistent sessions across HTTP requests.

Main changes:

• Added get_state_data() and clear_state_data() method overrides in TornadoIntegration to retrieve/clear state directly from cache
• Changed session from None to an empty dict in oidc_mixin.py to satisfy authlib's new session requirements
• Added comprehensive unit tests for the new state management methods
• Updated existing test assertions to be compatible with authlib's new error message format

Code Quality

The implementation is clean, well-structured, and follows the existing patterns in the codebase:

Strengths:

• ✅ Clear class and method docstrings explaining the override rationale (lines 31-38, 45-50, 63-69 in authlib_tornado_integration.py)
• ✅ Appropriate use of # noqa: ARG002 for intentionally unused session parameters that are required by the parent class signature
• ✅ Type annotations are complete throughout the changes
• ✅ Key format f"_state_{self.name}_{state}" matches the parent class implementation, ensuring consistency
• ✅ The fix maintains compatibility with both older authlib versions and 1.6.6+

Minor observations:

• Line 54 in authlib_tornado_integration.py uses # type: ignore[attr-defined] for self._get_cache_data(key). This is acceptable since _get_cache_data is a protected method from the authlib parent class that isn't properly typed in the external library.

Test Coverage

The test coverage is excellent. The new tests thoroughly cover the critical state management functionality:

New tests in TornadoIntegrationStateDataTest:

1. test_get_state_data_from_cache_with_empty_session - Verifies cache retrieval works with empty session
2. test_get_state_data_returns_none_without_cache - Edge case: no cache configured
3. test_get_state_data_returns_none_for_missing_state - Edge case: state not in cache
4. test_clear_state_data_removes_from_cache - Verifies deletion functionality
5. test_full_state_flow_with_fresh_sessions - Critical: End-to-end flow test simulating real Tornado behavior with fresh sessions per request

Test quality assessment:

• ✅ Tests follow numpy docstring style with clear descriptions
• ✅ Tests are properly typed with explicit return type annotations
• ✅ Tests use real AuthCache instances rather than mocks where appropriate
• ✅ The full flow test (test_full_state_flow_with_fresh_sessions) is particularly valuable as it validates the entire OAuth state lifecycle with fresh sessions - exactly mimicking the real-world Tornado behavior

Test updates:

• The assertion change in oauth_authlib_routes_test.py (lines 120-121) from a single combined check to two separate assertions (b"Missing" and b"authorize_url") is appropriate - it accommodates potential variations in error message formatting from different authlib versions while still validating the essential error content.

Backwards Compatibility

Fully backwards compatible. The changes:

• ✅ Don't modify any public API
• ✅ Work with both older authlib versions (through cache) and authlib 1.6.6+ (with empty dict sessions)
• ✅ The overridden methods maintain the same signature as the parent class
• ✅ The empty dict {} satisfies authlib's new requirement to write state to session while the cache override ensures Tornado can still retrieve state across separate HTTP requests

Security & Risk

No security concerns identified:

• ✅ OAuth state continues to be properly managed through the cache mechanism
• ✅ The state data lifecycle (set → get → clear) is preserved correctly
• ✅ No changes to authentication flows, token handling, or cookie management
• ✅ The cache-based state storage is the appropriate pattern for Tornado's stateless request model

Regression risk: Low

• The changes are isolated to the authlib integration layer
• The core OAuth logic remains unchanged
• Comprehensive unit tests verify the fix works correctly

Recommendations

No blocking issues found. Minor optional suggestions:

1. Consider adding a comment about version compatibility (optional): In the class docstring for TornadoIntegration, you could reference the specific authlib version (1.6.6) that introduced the breaking change for future maintainability. The current docstring already explains this well, so this is purely optional.

2. Test assertion precision (optional): The split assertion in oauth_authlib_routes_test.py could be slightly more precise by checking for the presence of both terms in sequence, but the current approach is sufficient and handles version differences gracefully.

Verdict

APPROVED: This PR correctly fixes the authlib 1.6.6+ compatibility issue for Tornado's sessionless architecture. The implementation is clean, well-documented, thoroughly tested, and maintains backwards compatibility. The changes are isolated to the authentication integration layer with no impact on public APIs or security.

This is an automated AI review. Please verify the feedback and use your judgment.

Summary

This PR updates Streamlit’s Authlib↔Tornado integration to remain compatible with Authlib 1.6.6+ (which now always tracks OAuth state in “session”), by using a placeholder session dict and overriding state retrieval/cleanup to rely on Streamlit’s cache. It also tightens up a couple tests that became brittle after dependency updates.

Code Quality

• Overall: The intent is clear, the docstrings explain the Authlib behavior change well, and the approach (use cache + placeholder session) is reasonable.

• Potential robustness issue in _get_cache_data_safe fallback selection:

  • In lib/streamlit/web/server/authlib_tornado_integration.py, _get_cache_data_safe checks for the presence of a private Authlib helper _get_cache_data and calls it directly (L48-L54). If Authlib changes that private method’s signature (or it exists but behaves differently), this will raise (e.g., TypeError) and will not fall back to the direct-cache path.
  • Reference: authlib_tornado_integration.py L48-L54.
  • Suggestion: Wrap the call in a try/except (TypeError, Exception) (or at least TypeError) and then fall back to the JSON decode path.

• New invalid_state path is not handled by the callback handler:

  • In lib/streamlit/web/server/oidc_mixin.py, authorize_access_token now raises OAuthError(error="invalid_state", ...) when state_data is missing (L94-L99).
  • In lib/streamlit/web/server/oauth_authlib_routes.py, AuthCallbackHandler.get calls token = client.authorize_access_token(self) without any exception handling (L174-L176).
  • Result: if state_data is missing/expired/malformed, this will likely propagate and yield a 500 instead of a controlled redirect (and a log).
  • References:
    • oidc_mixin.py L94-L99
    • oauth_authlib_routes.py L174-L176
  • Suggestion: Catch OAuthError (and possibly broader exceptions from token exchange) in AuthCallbackHandler.get, log a sanitized message, and redirect to base (consistent with how error query params are handled at L147-L162).

• (Existing risk worth noting, now more relevant) AuthCallbackHandler._get_provider_by_state assumes all cache keys split into exactly 4 underscore-separated parts (oauth_authlib_routes.py L189-L193). If any other key enters auth_cache, this could raise ValueError and break callbacks.

Test Coverage

• Good additions:

  • Added focused unit tests for the overridden cache-only state flow in lib/tests/streamlit/web/server/authlib_tornado_integration_test.py (new file). The “fresh sessions per request” test (L96-L127) is especially aligned with the Tornado behavior this PR is targeting.
  • Added a unit test for the missing state-data path in lib/tests/streamlit/web/server/oidc_mixin_test.py (L105-L125).
  • Relaxed a brittle assertion in lib/tests/streamlit/web/server/oauth_authlib_routes_test.py to avoid pinning the exact error string (L115-L122).

• Gap:

  • There is no test asserting that AuthCallbackHandler gracefully handles exceptions from authorize_access_token (including the newly introduced invalid_state). Given the production-facing callback handler, this is an important behavior to lock down.

• Style note:

  • The new test file uses unittest.TestCase with bare asserts. This matches some existing patterns in the area, but the repo’s Python test guide prefers pytest-style tests where practical.

Backwards Compatibility

• Public API: No user-facing API changes.
• Runtime behavior:
  • The new placeholder-session approach should preserve compatibility across Authlib versions and avoid relying on persistent sessions in Tornado.
  • However, without handler-side exception handling, some error cases (expired/missing state) may manifest as 500s, which is a backwards regression in user experience and potentially in stability.

Security & Risk

• State validation: Adding an explicit invalid_state failure is good for correctness.
• Risk: Uncaught OAuthError in the callback handler can lead to 500s (and potentially error pages containing information depending on Tornado configuration). Handling and sanitizing errors in AuthCallbackHandler.get reduces this risk.

Recommendations

1. Handle OAuthError (and token-exchange exceptions) in AuthCallbackHandler.get and redirect to base with a sanitized log message. See lib/streamlit/web/server/oauth_authlib_routes.py:L174-L176.
2. Make _get_cache_data_safe truly “safe” by falling back to direct cache access when calling Authlib’s private _get_cache_data fails (e.g., signature mismatch). See lib/streamlit/web/server/authlib_tornado_integration.py:L48-L54.
3. Add a unit test in oauth_authlib_routes_test.py ensuring /oauth2callback does not 500 when authorize_access_token raises OAuthError (including invalid_state), and instead redirects to /.

Verdict

CHANGES REQUESTED: The core fix is sound and well-tested at the integration layer, but the new invalid_state behavior needs to be handled in AuthCallbackHandler to avoid uncaught exceptions/500s during real OAuth callbacks.

This is an automated AI review. Please verify the feedback and use your judgment.

📉 Python coverage change detected

The Python unit test coverage has decreased by 0.0055%

• Current PR: 92.8493% (20823 statements, 1489 missed)
• Latest develop: 92.8547% (20797 statements, 1486 missed)

✅ Coverage change is within normal range.

📊 View detailed coverage comparison

Handle OAuthError (and token-exchange exceptions) in AuthCallbackHandler.get

Make _get_cache_data_safe truly “safe”

These two aspects are somewhat valid, but already working that way in the previous state. They should be made more robust in a follow-up.

In lib/streamlit/web/server/authlib_tornado_integration.py, _get_cache_data_safe checks for the presence of a private Authlib helper _get_cache_data and calls it directly (L48-L54). If Authlib changes that private method’s signature (or it exists but behaves differently), this will raise (e.g., TypeError) and will not fall back to the direct-cache path.

See this had been fixed ✅