fix(nodes): harden Gmail credential handling — token_uri, broker refresh, schemeless broker URL (#1560)#1569
Open
meetp06 wants to merge 5 commits into
Conversation
…rocketride-org#1560) tool_gmail trusted `token_uri` straight from the saved token payload (`gmail_client.py`, `info.get('token_uri', ...)`). That value is where the google-auth library POSTs the refresh_token to mint a new access token, so a tampered token JSON could redirect the POST — carrying the refresh_token — to an attacker-controlled host and exfiltrate the long-lived credential. Add `resolve_token_uri()`, mirroring the existing `resolve_refresh_url()` broker-host validation: accept a token-supplied endpoint only when it is https and its host is one of Google's official token endpoints (oauth2.googleapis.com, accounts.google.com), fall back to the canonical endpoint when the field is absent, and raise ValueError otherwise so a tampered token fails loud instead of silently posting credentials elsewhere. Apply it at the point where token_uri is read. Tests: cover the accepted Google endpoints, the absent-field fallback, and rejection of scheme downgrade, attacker host, suffix-confusion lookalikes, and non-string input. Full tool_gmail suite passes (91 tests). Scope: this is the token_uri validation item from rocketride-org#1560. The broker-refresh reliability fixes, 403 rate-limit retry, protobuf floor bump, and the hoist of the shared client into nodes/core are separate items left as follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
meetp06
requested review from
Rod-Christensen,
jmaionchi and
stepmikhaylov
as code owners
July 13, 2026 23:28
Contributor
🤖 Internal: Discord sync markerAuto-managed by the Discord notification workflow. Stores the linked Discord message ID and forum thread ID. Do not edit or delete. |
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGmail OAuth token endpoints now require trusted HTTPS Google hosts, while broker refresh handling centralizes token exchange, expiry calculation, response validation, and ChangesGmail OAuth credential handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant BrokerCredentials
participant _refresh_access_token_via_broker
participant OAuthBroker
BrokerCredentials->>_refresh_access_token_via_broker: pass refresh token
_refresh_access_token_via_broker->>OAuthBroker: POST refresh request
OAuthBroker-->>_refresh_access_token_via_broker: JSON access_token and expiry_date
_refresh_access_token_via_broker-->>BrokerCredentials: return token and expiry
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: 1
🤖 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 `@nodes/src/nodes/tool_gmail/gmail_client.py`:
- Around line 114-117: Update the token_uri validation logic in the Gmail token
URI helper so only None or the documented empty-string value falls back to
_GOOGLE_TOKEN_URI; validate the type before applying any falsy fallback,
ensuring falsy non-string values such as 0, False, lists, and dictionaries raise
ValueError.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 96a4bb51-d4d9-4671-ab26-ffd181d67b40
📒 Files selected for processing (2)
nodes/src/nodes/tool_gmail/gmail_client.pynodes/test/tool_gmail/test_gmail.py
The broker refresh path in gmail_client.py had several reliability bugs called out in rocketride-org#1560: - HTTPError is a subclass of URLError, so a broker that was reached but returned an error status was misreported as "broker unreachable". - A 200 response without access_token silently kept the stale token. - A 200 without expiry_date left a stale past expiry, so google-auth issued a refresh POST on every subsequent API call (broker quota churn). - A socket timeout during resp.read() raises OSError, not URLError, so it escaped the handler as a raw error. Extract the network/parse logic into a module-level `_refresh_access_token_via_broker()` helper (also makes it unit-testable) that: catches HTTPError separately with an accurate message, treats URLError/OSError (incl. read timeouts) as unreachable, reports malformed bodies as invalid, raises when access_token is missing, and falls back to a default access-token lifetime when the broker omits expiry_date. Tests mock urlopen to cover success (broker + default expiry), missing access_token, HTTP error, connection error, read timeout, and invalid JSON. Full tool_gmail suite passes (98 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
meetp06
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@nodes/src/nodes/tool_gmail/gmail_client.py`:
- Around line 191-215: Validate that the parsed response in the Gmail token
refresh flow is a dictionary before calling data.get('access_token'). Treat any
other valid JSON shape, including null, numbers, and lists, as an invalid broker
response and raise the existing _gae.RefreshError with the same invalid-response
message used for parsing failures.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 86829721-d54b-4071-bb6f-a07cc1819593
📒 Files selected for processing (2)
nodes/src/nodes/tool_gmail/gmail_client.pynodes/test/tool_gmail/test_gmail.py
…ocketride-org#1560) A schemeless RR_OAUTH_BROKER_URL (e.g. "broker.example.com") parsed with urlparse().hostname == None, so a self-hosted broker host was silently dropped from the refresh allowlist and its own refresh URLs were rejected. Prepend a scheme before parsing when one is missing so the host is registered. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
meetp06
changed the title
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nodes/src/nodes/tool_gmail/gmail_client.py (1)
214-224: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winReject malformed
access_tokenandexpiry_datevalues asRefreshError.The current truthiness check accepts non-string access tokens, while non-numeric
expiry_datevalues can raise rawTypeError/ValueErrorduring timestamp conversion. Validate both fields and map malformed payloads to the existing invalid-response error; add regression tests for these shapes.🐛 Proposed validation
+ import math + access_token = data.get('access_token') - if not access_token: + if not isinstance(access_token, str) or not access_token: raise _gae.RefreshError( 'Gmail token refresh returned no access token from the broker. Please reconnect your Google account.' ) ms = data.get('expiry_date') - if ms: + if ms is not None: + if isinstance(ms, bool) or not isinstance(ms, (int, float)) or not math.isfinite(ms): + raise _gae.RefreshError( + 'Gmail token refresh returned an invalid response from the broker. Please reconnect your Google account.' + ) expiry = _dt.datetime.utcfromtimestamp(ms / 1000)🤖 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 `@nodes/src/nodes/tool_gmail/gmail_client.py` around lines 214 - 224, Update the broker response handling around access_token and expiry_date to validate that access_token is a non-empty string and expiry_date, when present, is numeric and convertible to a timestamp. Raise the existing _gae.RefreshError with the invalid-response message for malformed values instead of allowing TypeError or ValueError to escape, while preserving the default lifetime when expiry_date is absent. Add regression tests covering non-string/empty access tokens and invalid expiry_date shapes.
🤖 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.
Outside diff comments:
In `@nodes/src/nodes/tool_gmail/gmail_client.py`:
- Around line 214-224: Update the broker response handling around access_token
and expiry_date to validate that access_token is a non-empty string and
expiry_date, when present, is numeric and convertible to a timestamp. Raise the
existing _gae.RefreshError with the invalid-response message for malformed
values instead of allowing TypeError or ValueError to escape, while preserving
the default lifetime when expiry_date is absent. Add regression tests covering
non-string/empty access tokens and invalid expiry_date shapes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b381b0b7-e06f-4fb5-9433-7ffb5c7a88b3
📒 Files selected for processing (2)
nodes/src/nodes/tool_gmail/gmail_client.pynodes/test/tool_gmail/test_gmail.py
…he SDK The broker-refresh tests imported google.auth.exceptions at module top level, which fails at collection in the node test environment where the Google SDK is not installed (ModuleNotFoundError: No module named 'google.auth') — breaking the whole test_gmail module in CI. Add a mock google.auth.exceptions (mirroring the existing google.oauth2 mocks) and load it per-test via monkeypatch.syspath_prepend, matching the pattern the build_service tests already use. The RefreshError type is obtained through a fixture instead of a top-level import, so collection no longer depends on the real SDK. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address CodeRabbit review on rocketride-org#1569: - resolve_token_uri: only None/'' fall back to the canonical endpoint; other falsy non-strings (0, False, [], {}) now raise instead of silently returning the default. - broker refresh: reject a non-dict JSON body (list/number/null) that would otherwise crash data.get() with AttributeError; require access_token to be a non-empty string; and reject a non-numeric expiry_date instead of letting a raw TypeError escape. All map to the existing invalid-response RefreshError. Added tests for each shape. Full tool_gmail suite passes (106 tests). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Author
|
Thanks @coderabbitai — addressed all three in the latest commit:
Also fixed the CI break: the broker tests imported |
Contributor
|
Tip For best results, initiate chat on the files or code changes.
106 passing across the suite is a solid signal. Nice work tightening this up. (=^･ω･^=) ﾞ ✏️ Learnings added
|
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
1 participant
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.
Addresses #1560 — the credential-hardening items (see Scope).
1. Security: validate
token_uri(refresh_token exfiltration)tool_gmailtrustedtoken_uristraight from the untrusted saved token. That URL is where google-auth POSTs therefresh_token; a tampered token could redirect it to an attacker host and exfiltrate the long-lived credential.resolve_token_uri()(mirrors the existingresolve_refresh_url) now accepts only https URLs on Google's official token endpoints (oauth2.googleapis.com,accounts.google.com), falls back to the canonical endpoint when absent, and raises otherwise.2. Reliability: broker token refresh
Fixed several bugs in the broker refresh path, extracted into a testable
_refresh_access_token_via_broker()helper:HTTPErrorsubclassesURLError, so a broker reached-but-erroring was reported as "unreachable". Now caught separately with an accurate message.access_tokensilently kept the stale token → now raises.expiry_dateleft a stale past expiry → refresh POST on every API call (quota churn). Now falls back to a default access-token lifetime.resp.read()raisesOSError, notURLError→ now treated as unreachable.3. Config: schemeless
RR_OAUTH_BROKER_URLA schemeless value (
broker.example.com) parsed withhostname=Noneand was silently dropped from the refresh allowlist. Now a scheme is prepended before parsing so the self-hosted broker host registers.Tests
resolve_token_uri: Google endpoints accepted, absent → canonical, rejects scheme downgrade / attacker host / suffix-confusion lookalike / non-string.urlopen): success (broker + default expiry), missingaccess_token, HTTP error, connection error, read timeout, invalid JSON.Verification (run locally)
Scope
Covers the credential-hardening items of #1560 (token_uri, broker refresh, schemeless broker URL). The remaining items — 403 rate-limit retry, protobuf floor bump, and hoisting the shared client into
nodes/core/— are left as separate follow-up.🤖 Generated with Claude Code
Summary by CodeRabbit