fix: issue #70873 jwt token signature validation #2076
Merged
Merged
Conversation
github-actions
Bot
added
backend
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds Google ID-token and Microsoft access-token JWT verification with JWKS caching, tenant allow-list configuration, OAuth connector enforcement, issuer-verification diagnostics, and comprehensive unit tests. ChangesOAuth JWT security
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthCallback
participant AuthService
participant GoogleVerification
participant GoogleJWKS
participant TokenStore
OAuthCallback->>AuthService: exchange authorization code
AuthService->>GoogleVerification: verify id_token
GoogleVerification->>GoogleJWKS: fetch signing keys
GoogleJWKS-->>GoogleVerification: JWKS document
GoogleVerification-->>AuthService: verified claims
AuthService->>TokenStore: persist tokens and id_token
sequenceDiagram
participant OneDriveOrSharePoint
participant MicrosoftVerification
participant MicrosoftJWKS
OneDriveOrSharePoint->>MicrosoftVerification: acquired access token and tenant settings
MicrosoftVerification->>MicrosoftJWKS: fetch versioned tenant JWKS
MicrosoftJWKS-->>MicrosoftVerification: signing keys
MicrosoftVerification-->>OneDriveOrSharePoint: verified claims or verification error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
github-actions
Bot
added
bug
rodageve
changed the title
github-actions
Bot
added
bug
rodageve
force-pushed
the
fix/jwt-validation-main
branch
from
July 13, 2026 17:36
d114cdf to
5ccb5ec
Compare
github-actions
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/connectors/google_drive/oauth.py (1)
147-188: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMove ID-token verification after refresh
_verify_id_tokenruns on the persisted token before the refresh path, so a still-refreshable session can be rejected once the stored ID token expires. Reorder it to validate the post-refresh credential state.🤖 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/oauth.py` around lines 147 - 188, Move the _verify_id_token call in load_credentials to after the credential refresh block, so verification uses the post-refresh self.creds.id_token. Keep the existing conditional check and refresh behavior unchanged, and ensure verification still occurs before returning self.creds.
🧹 Nitpick comments (2)
tests/unit/utils/test_jwt_verification.py (1)
302-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisleading test name/docstring — this asserts pass-through, not a raise.
test_wrong_audience_raisesand its docstring describe anInvalidAudienceError, but the body actually verifies the pass-through path (returns unverified claims whenclient_id != aud). Rename to reflect the behavior (e.g.test_non_matching_audience_is_passed_through) and trim the stale multi-paragraph docstring to avoid confusion.🤖 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 `@tests/unit/utils/test_jwt_verification.py` around lines 302 - 324, Rename test_wrong_audience_raises to reflect that mismatched audiences are passed through, such as test_non_matching_audience_is_passed_through. Replace the stale multi-paragraph docstring with a concise description of the pass-through behavior, keeping the existing assertions and verification flow unchanged.src/services/auth_service.py (1)
286-293: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReaching into a private connector helper across module boundaries.
_verify_id_tokenis prefixed as private ingoogle_drive/oauth.py, yet it's imported and called here. Consider exposing it as a public function (drop the underscore) or exporting the verification logic through a shared/public API so this cross-module dependency is intentional and stable.🤖 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/services/auth_service.py` around lines 286 - 293, Replace the cross-module import and call of the private _verify_id_token helper in the Google OAuth callback with a stable public API: rename it to a public symbol such as verify_id_token in connectors.google_drive.oauth and update all references, including auth_service.py. Preserve the existing validation timing and error behavior.
🤖 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/connectors/google_drive/oauth.py`:
- Around line 36-39: Update the logging in the Google ID token verification flow
around verify_google_id_token to stop emitting claims.get("email"); omit the
value or log only a non-PII identifier such as claims.get("sub"), while
preserving the verification and return behavior.
In `@src/connectors/onedrive/oauth.py`:
- Around line 11-46: The duplicated _verify_access_token helper must be moved
into a shared Microsoft OAuth utility and imported by both connectors. Update
src/connectors/onedrive/oauth.py lines 11-46 and
src/connectors/sharepoint/oauth.py lines 11-46 to remove the local copies,
import the shared helper, and replace each stdlib logger with
get_logger(__name__) from utils.logging_config; preserve the existing
verification behavior and logging calls.
---
Outside diff comments:
In `@src/connectors/google_drive/oauth.py`:
- Around line 147-188: Move the _verify_id_token call in load_credentials to
after the credential refresh block, so verification uses the post-refresh
self.creds.id_token. Keep the existing conditional check and refresh behavior
unchanged, and ensure verification still occurs before returning self.creds.
---
Nitpick comments:
In `@src/services/auth_service.py`:
- Around line 286-293: Replace the cross-module import and call of the private
_verify_id_token helper in the Google OAuth callback with a stable public API:
rename it to a public symbol such as verify_id_token in
connectors.google_drive.oauth and update all references, including
auth_service.py. Preserve the existing validation timing and error behavior.
In `@tests/unit/utils/test_jwt_verification.py`:
- Around line 302-324: Rename test_wrong_audience_raises to reflect that
mismatched audiences are passed through, such as
test_non_matching_audience_is_passed_through. Replace the stale multi-paragraph
docstring with a concise description of the pass-through behavior, keeping the
existing assertions and verification flow unchanged.
🪄 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: 7a6f193a-3738-4c61-b909-ea54de508741
📒 Files selected for processing (11)
src/config/settings.pysrc/config/utils.pysrc/connectors/google_drive/oauth.pysrc/connectors/onedrive/oauth.pysrc/connectors/sharepoint/oauth.pysrc/services/auth_service.pysrc/tui/config_fields.pysrc/tui/managers/env_manager.pysrc/utils/env_utils.pysrc/utils/jwt_verification.pytests/unit/utils/test_jwt_verification.py
github-actions
Bot
added
bug
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/utils/jwt_verification.py (1)
94-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate signing-key lookup logic.
_get_signing_key(Lines 94-127) and the inline block inverify_microsoft_access_token(Lines 349-361) both extractkid, loop overjwks["keys"], raise the same "not found" errors, and callRSAAlgorithm.from_jwk. The Microsoft path needs the raw JWKS entry too (for theissuerproperty), so it can't reuse_get_signing_keyas-is, but this duplication risks the two implementations drifting (e.g., differing error messages/exception handling) on security-critical code.Consider extending
_get_signing_keyto optionally return the raw JWKS entry alongside the key, and reuse it in both call sites.Also applies to: 349-361
🤖 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 94 - 127, Consolidate signing-key lookup between _get_signing_key and verify_microsoft_access_token by extending _get_signing_key to optionally return both the converted RSA key and its raw JWKS entry. Update the Microsoft verification path to use this helper and retain the raw entry for issuer access, while preserving shared kid validation, not-found errors, and JWT decode handling.
🤖 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 200-209: Update _resolve_ms_jwks_url to validate tenant_id against
the expected tenant-ID/alias format before formatting either JWKS URL. Reject
missing or invalid values before _fetch_jwks can run, while preserving the
existing token-version URL selection for valid tenants.
- Around line 212-246: Update the Microsoft JWKS validation flow around
_validate_ms_issuer and signing_key_entry.get("issuer", "") to handle an absent
issuer explicitly instead of passing an empty string for comparison. When the
JWKS key omits issuer, derive the expected issuer from the available discovery
metadata or use the appropriate Microsoft issuer validation path, while
preserving strict matching when a signing-key issuer is present.
---
Nitpick comments:
In `@src/utils/jwt_verification.py`:
- Around line 94-127: Consolidate signing-key lookup between _get_signing_key
and verify_microsoft_access_token by extending _get_signing_key to optionally
return both the converted RSA key and its raw JWKS entry. Update the Microsoft
verification path to use this helper and retain the raw entry for issuer access,
while preserving shared kid validation, not-found errors, and JWT decode
handling.
🪄 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: 357f6d6f-5233-4038-adde-c59c12f7ece2
📒 Files selected for processing (11)
src/config/settings.pysrc/config/utils.pysrc/connectors/google_drive/oauth.pysrc/connectors/onedrive/oauth.pysrc/connectors/sharepoint/oauth.pysrc/services/auth_service.pysrc/tui/config_fields.pysrc/tui/managers/env_manager.pysrc/utils/env_utils.pysrc/utils/jwt_verification.pytests/unit/utils/test_jwt_verification.py
🚧 Files skipped from review as they are similar to previous changes (10)
- src/tui/config_fields.py
- src/tui/managers/env_manager.py
- tests/unit/utils/test_jwt_verification.py
- src/utils/env_utils.py
- src/services/auth_service.py
- src/connectors/google_drive/oauth.py
- src/config/settings.py
- src/connectors/onedrive/oauth.py
- src/connectors/sharepoint/oauth.py
- src/config/utils.py
Comment on lines
+200
to
+209
| def _resolve_ms_jwks_url(tenant_id: str, token_version: str) -> str: | ||
| """ | ||
| Return the correct JWKS URL for the given tenant and token version. | ||
|
|
||
| Per Microsoft docs: v1.0 tokens must be validated against the v1 metadata | ||
| endpoint (no /v2.0/), v2.0 tokens against the v2 endpoint. | ||
| """ | ||
| if token_version == "1.0": | ||
| return MICROSOFT_JWKS_URL_V1_TEMPLATE.format(tenant=tenant_id) | ||
| return MICROSOFT_JWKS_URL_V2_TEMPLATE.format(tenant=tenant_id) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the file structure and inspect the relevant sections.
git ls-files 'src/utils/jwt_verification.py'
wc -l src/utils/jwt_verification.py
sed -n '1,140p' src/utils/jwt_verification.py
printf '\n---\n'
sed -n '180,360p' src/utils/jwt_verification.pyRepository: langflow-ai/openrag
Length of output: 12606
🏁 Script executed:
python3 - <<'PY'
from urllib.parse import urlsplit
template_v1 = "https://login.microsoftonline.com/{tenant}/discovery/keys"
template_v2 = "https://login.microsoftonline.com/{tenant}/discovery/v2.0/keys"
samples = [
"00000000-0000-0000-0000-000000000000",
"common",
"foo/bar",
"../bar",
"foo?x=1",
"foo#frag",
"foo%2fbar",
"foo\nbar",
"https://evil.com",
]
for s in samples:
for name, tmpl in [("v1", template_v1), ("v2", template_v2)]:
try:
url = tmpl.format(tenant=s)
parts = urlsplit(url)
print(name, repr(s), "=>", url)
print(" scheme:", parts.scheme, "netloc:", parts.netloc, "path:", parts.path, "query:", parts.query, "fragment:", parts.fragment)
except Exception as e:
print(name, repr(s), "=> ERROR", type(e).__name__, e)
print()
PYRepository: langflow-ai/openrag
Length of output: 3600
Validate tenant_id before building the JWKS URL. The host is fixed, but untrusted tid values still shape the request path, cache key, and logs when tenant_id is omitted. Reject anything outside the expected tenant-ID/alias format before _fetch_jwks.
🤖 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 200 - 209, Update
_resolve_ms_jwks_url to validate tenant_id against the expected tenant-ID/alias
format before formatting either JWKS URL. Reject missing or invalid values
before _fetch_jwks can run, while preserving the existing token-version URL
selection for valid tenants.
Source: Linters/SAST tools
lucaseduoli
requested changes
Jul 13, 2026
lucaseduoli
left a comment
lucaseduoli
left a comment
Collaborator
There was a problem hiding this comment.
Got some failures with the OneDrive and Sharepoint verifications when running them locally:
Unable to get Microsoft Graph token for user ACL aliases
- error: Verification failed: Not enough segments
And I got this error with Google:
Google Directory group ACL lookup denied; check Admin SDK API and admin.directory.group.readonly consent
Collaborator
Author
The OneDrive error is legit, fixing. This error seems unrelated - |
github-actions
Bot
added
bug
github-actions
Bot
added
bug
rodageve
force-pushed
the
fix/jwt-validation-main
branch
from
July 13, 2026 20:27
be74698 to
000d6a0
Compare
- Replace invalid error= parameter with proper string formatting - Fixes mypy call-arg errors in sharepoint and onedrive oauth.py
- Add type guards for _current_account.get() calls - Fixes 'None has no attribute get' errors on lines 149 and 212
…ture, fail oauth connection on jwt validation
…udience, fix v1/v2 JWKS endpoint, fix issuer validation algorithm
- Add DEBUG/WARNING logs to get_public_key_from_issuer() and verify_jwt_from_issuer() in src/config/utils.py: - Cache hit/fetch/store at DEBUG - Per-exception-type WARNING (expired, invalid sig, issuer, audience) - Broad catch-all WARNING for other failures - SUCCESS debug log with issuer and sub - src/services/auth_service.py: call _verify_id_token() immediately after Google OAuth code exchange (before persisting credentials) and persist id_token to token file so load_credentials() can re-verify it
MSAL can return opaque (non-JWT) access tokens for Graph and SharePoint
resources. PyJWT raises DecodeError('Not enough segments') for these,
which propagated as JWTVerificationError and surfaced as:
'Unable to get Microsoft Graph token for user ACL aliases'
Fix: probe the token with get_unverified_header() before invoking
verify_microsoft_access_token(). On DecodeError, log at DEBUG and return
None — the token was obtained via the confidential-client OAuth flow so
it is trusted. Real JWT failures (bad sig, expired, etc.) still raise.
…WKS issuer
- google_drive/oauth.py: replace PII email with sub in Google ID token
verified debug log
- microsoft_oauth_utils.py (new): extract _verify_access_token into a
shared module using get_logger; remove verbatim duplicate from both
sharepoint/oauth.py and onedrive/oauth.py — both now import via alias
_verify_access_token = verify_ms_access_token
- sharepoint/oauth.py, onedrive/oauth.py: switch module logger from
stdlib logging.getLogger to get_logger(__name__) as required for src/
- jwt_verification.py: fix _validate_ms_issuer() rejecting valid tokens
when JWKS key entry omits 'issuer' (common for tenant-specific JWKS).
signing_key_entry.get('issuer', '') returned '' causing every match to
fail. Now treats missing issuer as None and falls back to validating
iss against known MS URL prefixes (login.microsoftonline.com /
sts.windows.net) + tid-in-iss check. Adds 4 unit tests.
rodageve
force-pushed
the
fix/jwt-validation-main
branch
from
July 14, 2026 13:52
5e09465 to
0348051
Compare
github-actions
Bot
added
bug
github-actions
Bot
added
bug
…ion failures The Google ID token (~1h TTL) is only meaningful at OAuth login time. Verifying it on every subsequent load_credentials call means any cache miss (process restart, connector eviction) more than ~1h after the user's original OAuth flow raises ExpiredTokenError and fails the entire background ingestion run — even though the OAuth access token is still refreshable. _verify_id_token() is already called in handle_authorization_callback immediately after the code exchange, which is the correct and only necessary place for this check. Remove the redundant call from load_credentials so credential refresh and ingestion continue to work for the lifetime of the refresh token.
github-actions
Bot
added
bug
github-actions
Bot
added
bug
lucaseduoli
approved these changes
Jul 14, 2026
lucaseduoli
left a comment
lucaseduoli
left a comment
Collaborator
There was a problem hiding this comment.
Working flawlessly, tested ingestion for google drive, onedrive and sharepoint, and tested google oauth login
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
JWT token validation logs were absent at DEBUG level when running on release-cpd-0.1. Additionally, two security gaps were identified: verify_jwt_from_issuer() silently swallowed all validation failures with no log output, and the Google OAuth callback never verified the ID token signature before persisting credentials.
Root Cause
src/config/utils.py — introduced by PR #1956 — was missing all logging instrumentation. Both get_public_key_from_issuer() and verify_jwt_from_issuer() returned None on any failure path without emitting a single log line. Separately, AuthService.handle_oauth_callback() performs its own raw token exchange for Google Drive and never called _verify_id_token(), leaving the Google ID token signature unverified; id_token was also not persisted, blocking re-verification on credential reload.
Solution
Cherry-picked PR fix: Add JWT signature validation for OAuth connectors #1956 (jwt-validation-cpd) onto main — brings src/utils/jwt_verification.py, src/config/utils.py, Microsoft/Google connector token verification, src/auth/request_identity.py [AUTH] call-site logging, and the full unit test suite
Added per-exception-type WARNING logs to verify_jwt_from_issuer() (expired, invalid signature, invalid issuer, invalid audience, catch-all) and DEBUG logs for cache hit/fetch/store in get_public_key_from_issuer()
Wired _verify_id_token() into AuthService.handle_oauth_callback() for google_drive — raises JWTVerificationError on failure before credentials are saved; persists id_token to token file so load_credentials() can re-verify on reload
NOTE - We should only validate the Google ID token signature once upon completing the OAuth flow, the token will expire over time once stored / cached on disk - there is no way to refresh the token, .. ingestion of files happens with refreshable Google Auth Token.
Changes
Summary by CodeRabbit
MICROSOFT_ALLOWED_TENANT_IDS(comma-separated) to the setup UI and environment output.