fix: microsoft and google connector OAuth JWT validation#1907
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (2)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds ChangesJWT Signature Verification for OAuth Tokens
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/connectors/google_drive_acl.py (1)
8-8:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove unused
jwtimport.The
jwtmodule is no longer used directly after refactoring to useverify_google_id_token.🔧 Proposed fix
-import jwt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connectors/google_drive_acl.py` at line 8, Remove the unused `import jwt` statement from the imports section of the file. The jwt module is no longer needed since the code has been refactored to use verify_google_id_token instead of jwt operations.Source: Pipeline failures
src/connectors/microsoft_graph_acl.py (1)
9-9:⚠️ Potential issue | 🟡 MinorRemove the unused
jwtimport.The
jwtmodule is imported on line 9 but never used directly in this file. Token verification is handled throughverify_microsoft_access_tokenfromutils.jwt_verification, making the top-leveljwtimport unnecessary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/connectors/microsoft_graph_acl.py` at line 9, The jwt module is imported at the top of the file but is never used directly since token verification is handled through the verify_microsoft_access_token function imported from utils.jwt_verification. Remove the unused import statement `import jwt` from the file to clean up the imports and eliminate the unnecessary dependency.
🧹 Nitpick comments (1)
src/utils/jwt_verification.py (1)
246-252: 💤 Low value
verify_iss=Truehas no effect without anissuerargument.PyJWT requires the
issuerparameter to actually validate the issuer claim. Settingverify_iss=Truewithout passingissuerresults in no validation. The manual check at lines 255-262 does validate correctly, but the option here is misleading.Either remove
verify_iss: Truefrom options (since validation is manual), or pass a validator. For Microsoft tokens where the issuer varies by tenant, the manual check is appropriate.🔧 Suggested clarification
options={ "verify_signature": True, "verify_exp": True, "verify_aud": True, - "verify_iss": True, + "verify_iss": False, # Validated manually below due to tenant-specific issuers },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/jwt_verification.py` around lines 246 - 252, The `verify_iss: True` option in the options dictionary passed to jwt.decode() has no effect without an accompanying `issuer` parameter, making it misleading. Since the code performs manual issuer validation in the subsequent check (lines 255-262), which is appropriate for Microsoft tokens with tenant-specific issuers, remove the `verify_iss: True` entry from the options dictionary to accurately reflect that issuer validation is being handled manually rather than by PyJWT's built-in mechanism.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/jwt_verification.py`:
- Around line 84-86: The exception raised in the except block at lines 84-86 and
also at lines 120-121 is missing exception chaining. Modify both raise
statements to use the `from e` syntax (e.g., `raise
JWTVerificationError(f"Failed to fetch JWKS: {e}") from e`) to preserve the
original exception traceback and maintain the exception chain for better
debugging context.
- Around line 267-283: Add `from e` to all exception re-raise statements in the
jwt_verification.py exception handling block to preserve exception chains for
proper traceback debugging. Specifically, modify the raise statements for
InvalidSignatureError, ExpiredTokenError, InvalidAudienceError,
InvalidIssuerError, and JWTVerificationError to use the syntax `raise
CustomError(...) from e` instead of `raise CustomError(...)`. This ensures the
original exception context is maintained throughout the pipeline for better
error investigation.
- Around line 176-192: The exception re-raises in the JWT verification error
handling block are not preserving the exception chain, which is required by the
pipeline linting standards. Add `from e` to each of the raise statements for
InvalidSignatureError, ExpiredTokenError, InvalidAudienceError,
InvalidIssuerError, and JWTVerificationError to maintain the full traceback
chain. This allows the original exception context to be preserved for better
debugging.
---
Outside diff comments:
In `@src/connectors/google_drive_acl.py`:
- Line 8: Remove the unused `import jwt` statement from the imports section of
the file. The jwt module is no longer needed since the code has been refactored
to use verify_google_id_token instead of jwt operations.
In `@src/connectors/microsoft_graph_acl.py`:
- Line 9: The jwt module is imported at the top of the file but is never used
directly since token verification is handled through the
verify_microsoft_access_token function imported from utils.jwt_verification.
Remove the unused import statement `import jwt` from the file to clean up the
imports and eliminate the unnecessary dependency.
---
Nitpick comments:
In `@src/utils/jwt_verification.py`:
- Around line 246-252: The `verify_iss: True` option in the options dictionary
passed to jwt.decode() has no effect without an accompanying `issuer` parameter,
making it misleading. Since the code performs manual issuer validation in the
subsequent check (lines 255-262), which is appropriate for Microsoft tokens with
tenant-specific issuers, remove the `verify_iss: True` entry from the options
dictionary to accurately reflect that issuer validation is being handled
manually rather than by PyJWT's built-in mechanism.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 51730932-d615-4451-83cf-40e3164e6f8f
📒 Files selected for processing (4)
src/config/settings.pysrc/connectors/google_drive_acl.pysrc/connectors/microsoft_graph_acl.pysrc/utils/jwt_verification.py
Security: Add JWT Signature Verification for OAuth Tokens
Summary
Implements full JWT signature verification for Google and Microsoft OAuth tokens to address three critical SonarQube security findings (python:S5659). All JWT tokens are now cryptographically validated.
Security Issues Fixed
google_drive_acl.py:66- Google ID token decoded without signature verificationmicrosoft_graph_acl.py:32- Microsoft access token decoded without signature verificationmicrosoft_graph_acl.py:211- Microsoft access token decoded without signature verificationChanges
New Files
src/utils/jwt_verification.py- JWT verification utility with JWKS cachingModified Files
src/config/settings.py- AddedMICROSOFT_GRAPH_OAUTH_CLIENT_IDconfigurationsrc/connectors/google_drive_acl.py- Updated_email_from_id_token()with signature verificationsrc/connectors/microsoft_graph_acl.py- Updatedtenant_id_from_access_token()and_decode_microsoft_user_identifiers()with signature verificationSecurity Improvements
All JWT tokens now undergo full validation:
Prevents:
Summary by CodeRabbit
New Features
Security & Authentication