From 53bf08149b54e5a228a8b1b0fa0746bfce13ce18 Mon Sep 17 00:00:00 2001 From: meetp06 <65815199+meetp06@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:28:10 -0700 Subject: [PATCH 1/5] fix(nodes): validate Gmail token_uri against Google's OAuth endpoints (#1560) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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 --- nodes/src/nodes/tool_gmail/gmail_client.py | 43 +++++++++++++++++++++- nodes/test/tool_gmail/test_gmail.py | 32 ++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/nodes/src/nodes/tool_gmail/gmail_client.py b/nodes/src/nodes/tool_gmail/gmail_client.py index c2254c4a1..4bc5b6f43 100644 --- a/nodes/src/nodes/tool_gmail/gmail_client.py +++ b/nodes/src/nodes/tool_gmail/gmail_client.py @@ -87,6 +87,45 @@ def resolve_refresh_url(token_url: object) -> str | None: return token_url +# Google's canonical OAuth token endpoint, used when a token omits token_uri. +_GOOGLE_TOKEN_URI = 'https://oauth2.googleapis.com/token' + +# Hosts the google-auth library may POST the refresh_token to when minting a +# new access token. Like the broker URL, token_uri rides in the untrusted token +# payload, so a token-supplied value is honored only if it is https and its host +# is one of Google's official token endpoints — otherwise a tampered token could +# redirect the refresh_token to an attacker host. +_GOOGLE_TOKEN_HOSTS = frozenset({'oauth2.googleapis.com', 'accounts.google.com'}) + + +def resolve_token_uri(token_uri: object) -> str: + """Validate a token-supplied OAuth token endpoint against Google's hosts. + + ``token_uri`` is where the google-auth library POSTs the ``refresh_token`` + to obtain a fresh access token. The token payload is untrusted (it rides in + saved pipes and broker responses), so a tampered token could point this at + an attacker host and exfiltrate the refresh_token. + + Returns the URL when it is https and its host is one of Google's official + token endpoints. Falls back to the canonical endpoint when the token omits + the field. Raises ValueError for any other value so a tampered token fails + loud instead of silently posting credentials elsewhere. + """ + if not token_uri: + return _GOOGLE_TOKEN_URI + if not isinstance(token_uri, str): + raise ValueError('Gmail token token_uri must be a string') + + parsed = urlparse(token_uri) + if parsed.scheme != 'https' or not parsed.hostname or parsed.hostname.lower() not in _GOOGLE_TOKEN_HOSTS: + raise ValueError( + f'Gmail token token_uri {token_uri!r} is not a trusted Google OAuth token endpoint ' + '(expected https and one of: ' + ', '.join(sorted(_GOOGLE_TOKEN_HOSTS)) + '). ' + 'Reconnect your Google account.' + ) + return token_uri + + # HTTP status codes worth retrying (rate-limit + transient server errors). _RETRY_STATUSES = {429, 500, 502, 503, 504} @@ -136,7 +175,9 @@ def build_service(auth_type: str, cfg: dict, scopes: list[str]) -> Any: refresh_token = info.get('refresh_token') client_id = info.get('client_id') client_secret = info.get('client_secret') - token_uri = info.get('token_uri', 'https://oauth2.googleapis.com/token') + # Untrusted input: reject any token endpoint not owned by Google so a + # tampered token can never redirect the refresh_token POST. + token_uri = resolve_token_uri(info.get('token_uri')) # Untrusted input: reject any refresh URL not pointing at a known broker. oauth_server_url = resolve_refresh_url(info.get('oauth_server_url')) expiry_date_ms = info.get('expiry_date') diff --git a/nodes/test/tool_gmail/test_gmail.py b/nodes/test/tool_gmail/test_gmail.py index 814add131..16b64d178 100644 --- a/nodes/test/tool_gmail/test_gmail.py +++ b/nodes/test/tool_gmail/test_gmail.py @@ -213,6 +213,38 @@ def test_refresh_url_env_override_adds_host(monkeypatch): assert gmail_client.resolve_refresh_url('https://oauth2.rocketride.ai/refresh') +def test_token_uri_accepts_google_endpoints(): + for url in ( + 'https://oauth2.googleapis.com/token', + 'https://accounts.google.com/o/oauth2/token', + ): + assert gmail_client.resolve_token_uri(url) == url + + +def test_token_uri_absent_falls_back_to_canonical(): + assert gmail_client.resolve_token_uri(None) == 'https://oauth2.googleapis.com/token' + assert gmail_client.resolve_token_uri('') == 'https://oauth2.googleapis.com/token' + + +@pytest.mark.parametrize( + 'bad', + [ + 'http://oauth2.googleapis.com/token', # scheme downgrade + 'https://evil.example.com/token', # attacker host — would exfiltrate refresh_token + 'https://oauth2.googleapis.com.evil.com/token', # suffix confusion + 'ftp://oauth2.googleapis.com/token', + ], +) +def test_token_uri_rejects_untrusted(bad): + with pytest.raises(ValueError): + gmail_client.resolve_token_uri(bad) + + +def test_token_uri_rejects_non_string(): + with pytest.raises(ValueError): + gmail_client.resolve_token_uri({'url': 'https://oauth2.googleapis.com/token'}) + + def test_message_get_cleans_headers(): raw = { 'id': 'm1', From 1df8e7b73fc52589fc33a57616193e7a55a92ce6 Mon Sep 17 00:00:00 2001 From: meetp06 <65815199+meetp06@users.noreply.github.com> Date: Mon, 13 Jul 2026 16:45:00 -0700 Subject: [PATCH 2/5] fix(nodes): harden Gmail broker token refresh (#1560) The broker refresh path in gmail_client.py had several reliability bugs called out in #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 --- nodes/src/nodes/tool_gmail/gmail_client.py | 87 +++++++++++++++----- nodes/test/tool_gmail/test_gmail.py | 93 ++++++++++++++++++++++ 2 files changed, 158 insertions(+), 22 deletions(-) diff --git a/nodes/src/nodes/tool_gmail/gmail_client.py b/nodes/src/nodes/tool_gmail/gmail_client.py index 4bc5b6f43..ffca5092f 100644 --- a/nodes/src/nodes/tool_gmail/gmail_client.py +++ b/nodes/src/nodes/tool_gmail/gmail_client.py @@ -129,6 +129,11 @@ def resolve_token_uri(token_uri: object) -> str: # HTTP status codes worth retrying (rate-limit + transient server errors). _RETRY_STATUSES = {429, 500, 502, 503, 504} +# Fallback access-token lifetime when the broker response omits expiry_date. +# Matches Google's standard access-token lifetime; without it the credential +# keeps a stale past expiry and google-auth refreshes on every API call. +_DEFAULT_ACCESS_TOKEN_LIFETIME_S = 3600 + # Google's full-access Gmail scope — a superset of all granular Gmail scopes. _GMAIL_FULL_SCOPE = 'https://mail.google.com/' @@ -157,6 +162,65 @@ def _decode_blob(value: str) -> str: return value +def _refresh_access_token_via_broker(broker_url: str, refresh_token: str): + """POST the refresh_token to the OAuth broker and return (access_token, expiry). + + Every failure is surfaced as a google-auth ``RefreshError`` with a clear + reconnect message rather than leaking a raw transport error or silently + keeping a stale token: + + - An HTTP error status means the broker was reached but rejected the + request — it must not be misreported as "broker unreachable" (an + ``HTTPError`` is a subclass of ``URLError``, so it is matched first). + - A connection failure or a socket timeout during ``read()`` (an + ``OSError``, not a ``URLError``) is reported as unreachable. + - A non-JSON or malformed body is reported as an invalid response. + - A 200 without ``access_token`` raises instead of keeping the stale token. + - A 200 without ``expiry_date`` falls back to a default lifetime instead of + leaving a stale past expiry that forces a refresh on every call. + + Returns: + tuple[str, datetime.datetime]: the new access token and its expiry. + """ + import datetime as _dt + import urllib.error as _uerr + import urllib.request as _req + + import google.auth.exceptions as _gae + + body = json.dumps({'refresh_token': refresh_token}).encode() + req = _req.Request(broker_url, data=body, headers={'Content-Type': 'application/json'}, method='POST') + try: + with _req.urlopen(req, timeout=10) as resp: + raw = resp.read() + data = json.loads(raw.decode()) + except _uerr.HTTPError as exc: + raise _gae.RefreshError( + f'Gmail token refresh was rejected by the broker (HTTP {exc.code}). Please reconnect your Google account.' + ) from exc + except (_uerr.URLError, OSError) as exc: + raise _gae.RefreshError( + f'Gmail token refresh failed (broker unreachable: {exc}). Please reconnect your Google account.' + ) from exc + except ValueError as exc: + raise _gae.RefreshError( + 'Gmail token refresh returned an invalid response from the broker. Please reconnect your Google account.' + ) from exc + + access_token = data.get('access_token') + if 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: + expiry = _dt.datetime.utcfromtimestamp(ms / 1000) + else: + expiry = _dt.datetime.utcnow() + _dt.timedelta(seconds=_DEFAULT_ACCESS_TOKEN_LIFETIME_S) + return access_token, expiry + + def build_service(auth_type: str, cfg: dict, scopes: list[str]) -> Any: """Build a Gmail v1 service from node config, scoped to ``scopes``. @@ -223,34 +287,13 @@ def build_service(auth_type: str, cfg: dict, scopes: list[str]) -> Any: class _BrokerCredentials(Credentials): def refresh(self, request: object) -> None: # type: ignore[override] - import urllib.error as _uerr - import urllib.request as _req - import google.auth.exceptions as _gae if not _broker_url or not self.refresh_token: raise _gae.RefreshError( 'Gmail access token has expired. Please reconnect your Google account in the node settings.' ) - body = json.dumps({'refresh_token': self.refresh_token}).encode() - req = _req.Request( - _broker_url, - data=body, - headers={'Content-Type': 'application/json'}, - method='POST', - ) - try: - with _req.urlopen(req, timeout=10) as resp: - data = json.loads(resp.read().decode()) - except _uerr.URLError as exc: - raise _gae.RefreshError( - f'Gmail token refresh failed (broker unreachable: {exc}). ' - 'Please reconnect your Google account.' - ) from exc - self.token = data.get('access_token') or self.token - ms = data.get('expiry_date') - if ms: - self.expiry = _dt.datetime.utcfromtimestamp(ms / 1000) + self.token, self.expiry = _refresh_access_token_via_broker(_broker_url, self.refresh_token) creds = _BrokerCredentials( token=access_token, diff --git a/nodes/test/tool_gmail/test_gmail.py b/nodes/test/tool_gmail/test_gmail.py index 16b64d178..ffeb77f5f 100644 --- a/nodes/test/tool_gmail/test_gmail.py +++ b/nodes/test/tool_gmail/test_gmail.py @@ -245,6 +245,99 @@ def test_token_uri_rejects_non_string(): gmail_client.resolve_token_uri({'url': 'https://oauth2.googleapis.com/token'}) +# --------------------------------------------------------------------------- +# _refresh_access_token_via_broker — broker refresh reliability (#1560) +# --------------------------------------------------------------------------- + +import datetime as _dt # noqa: E402 +import urllib.error as _uerr # noqa: E402 + +import google.auth.exceptions as _gae # noqa: E402 + + +class _FakeResp: + """Minimal urlopen() response context manager for tests.""" + + def __init__(self, *, body=b'', read_raises=None): + self._body = body + self._read_raises = read_raises + + def read(self): + if self._read_raises is not None: + raise self._read_raises + return self._body + + def __enter__(self): + return self + + def __exit__(self, *exc): + return False + + +def _make_urlopen(*, raises=None, body=b'', read_raises=None): + def _open(req, timeout=None): + if raises is not None: + raise raises + return _FakeResp(body=body, read_raises=read_raises) + + return _open + + +def test_broker_refresh_success_uses_broker_expiry(monkeypatch): + payload = json.dumps({'access_token': 'AT', 'expiry_date': 1_000_000}).encode() + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=payload)) + tok, exp = gmail_client._refresh_access_token_via_broker('https://oauth2.rocketride.ai/refresh', 'RT') + assert tok == 'AT' + assert exp == _dt.datetime.utcfromtimestamp(1000) + + +def test_broker_refresh_defaults_expiry_when_missing(monkeypatch): + # A 200 without expiry_date must not leave a stale past expiry (which would + # force a refresh on every API call); fall back to a sane future lifetime. + payload = json.dumps({'access_token': 'AT'}).encode() + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=payload)) + before = _dt.datetime.utcnow() + tok, exp = gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') + assert tok == 'AT' + assert exp > before + + +def test_broker_refresh_missing_access_token_raises(monkeypatch): + # A 200 without access_token must fail loud, not silently keep the stale token. + payload = json.dumps({'expiry_date': 1_000_000}).encode() + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=payload)) + with pytest.raises(_gae.RefreshError, match='no access token'): + gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') + + +def test_broker_refresh_http_error_reports_rejected(monkeypatch): + # HTTPError is a subclass of URLError: the broker was reached but rejected + # the request, so it must not be misreported as "unreachable". + err = _uerr.HTTPError('https://broker/refresh', 500, 'boom', {}, None) + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(raises=err)) + with pytest.raises(_gae.RefreshError, match='rejected by the broker'): + gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') + + +def test_broker_refresh_urlerror_reports_unreachable(monkeypatch): + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(raises=_uerr.URLError('down'))) + with pytest.raises(_gae.RefreshError, match='unreachable'): + gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') + + +def test_broker_refresh_read_timeout_reports_unreachable(monkeypatch): + # A timeout during resp.read() raises OSError (TimeoutError), not URLError. + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(read_raises=TimeoutError('read timed out'))) + with pytest.raises(_gae.RefreshError, match='unreachable'): + gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') + + +def test_broker_refresh_invalid_json_reports_invalid(monkeypatch): + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=b'not json')) + with pytest.raises(_gae.RefreshError, match='invalid response'): + gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') + + def test_message_get_cleans_headers(): raw = { 'id': 'm1', From 19932ba191f1a1d6f8cad37ee8d22dff27918207 Mon Sep 17 00:00:00 2001 From: meetp06 <65815199+meetp06@users.noreply.github.com> Date: Mon, 13 Jul 2026 18:57:03 -0700 Subject: [PATCH 3/5] fix(nodes): accept schemeless RR_OAUTH_BROKER_URL in Gmail allowlist (#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 --- nodes/src/nodes/tool_gmail/gmail_client.py | 6 +++++- nodes/test/tool_gmail/test_gmail.py | 8 ++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/nodes/src/nodes/tool_gmail/gmail_client.py b/nodes/src/nodes/tool_gmail/gmail_client.py index ffca5092f..4422d4b05 100644 --- a/nodes/src/nodes/tool_gmail/gmail_client.py +++ b/nodes/src/nodes/tool_gmail/gmail_client.py @@ -73,7 +73,11 @@ def resolve_refresh_url(token_url: object) -> str | None: allowed = set(_BROKER_HOSTS) env_broker = os.environ.get('RR_OAUTH_BROKER_URL', '') if env_broker: - env_host = urlparse(env_broker).hostname + # A schemeless value (e.g. "broker.example.com") parses with + # hostname=None and would be silently dropped from the allowlist, so + # add a scheme before parsing when one is missing. + to_parse = env_broker if '://' in env_broker else f'https://{env_broker}' + env_host = urlparse(to_parse).hostname if env_host: allowed.add(env_host.lower()) diff --git a/nodes/test/tool_gmail/test_gmail.py b/nodes/test/tool_gmail/test_gmail.py index ffeb77f5f..4685c3578 100644 --- a/nodes/test/tool_gmail/test_gmail.py +++ b/nodes/test/tool_gmail/test_gmail.py @@ -213,6 +213,14 @@ def test_refresh_url_env_override_adds_host(monkeypatch): assert gmail_client.resolve_refresh_url('https://oauth2.rocketride.ai/refresh') +def test_refresh_url_env_override_accepts_schemeless_host(monkeypatch): + # A schemeless RR_OAUTH_BROKER_URL must still register its host (urlparse + # otherwise yields hostname=None and the host is silently dropped). + monkeypatch.setenv('RR_OAUTH_BROKER_URL', 'broker.internal.example') + url = 'https://broker.internal.example/refresh' + assert gmail_client.resolve_refresh_url(url) == url + + def test_token_uri_accepts_google_endpoints(): for url in ( 'https://oauth2.googleapis.com/token', From 1247dc8fb848e57e31edf6a118e75daedd060018 Mon Sep 17 00:00:00 2001 From: meetp06 <65815199+meetp06@users.noreply.github.com> Date: Mon, 13 Jul 2026 19:44:56 -0700 Subject: [PATCH 4/5] test(nodes): stub google.auth.exceptions so gmail tests run without the SDK MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- nodes/test/mocks/google/auth/__init__.py | 6 ++++ nodes/test/mocks/google/auth/exceptions.py | 19 +++++++++++ nodes/test/tool_gmail/test_gmail.py | 39 ++++++++++++++-------- 3 files changed, 50 insertions(+), 14 deletions(-) create mode 100644 nodes/test/mocks/google/auth/__init__.py create mode 100644 nodes/test/mocks/google/auth/exceptions.py diff --git a/nodes/test/mocks/google/auth/__init__.py b/nodes/test/mocks/google/auth/__init__.py new file mode 100644 index 000000000..543e36ff0 --- /dev/null +++ b/nodes/test/mocks/google/auth/__init__.py @@ -0,0 +1,6 @@ +# ============================================================================= +# MIT License +# Copyright (c) 2026 Aparavi Software AG +# ============================================================================= + +"""Mock google.auth package (exceptions) for tool_gmail broker-refresh tests.""" diff --git a/nodes/test/mocks/google/auth/exceptions.py b/nodes/test/mocks/google/auth/exceptions.py new file mode 100644 index 000000000..77f3d7592 --- /dev/null +++ b/nodes/test/mocks/google/auth/exceptions.py @@ -0,0 +1,19 @@ +# ============================================================================= +# MIT License +# Copyright (c) 2026 Aparavi Software AG +# ============================================================================= + +"""Minimal stand-in for google.auth.exceptions used in tests. + +Only ``RefreshError`` is needed: the Gmail broker-refresh path raises it to +signal a failed token refresh, and the real ``google-auth`` package is not +installed in the node test environment. +""" + + +class GoogleAuthError(Exception): + """Base class mirroring google.auth.exceptions.GoogleAuthError.""" + + +class RefreshError(GoogleAuthError): + """Raised when a credential refresh fails (mirrors the real RefreshError).""" diff --git a/nodes/test/tool_gmail/test_gmail.py b/nodes/test/tool_gmail/test_gmail.py index 4685c3578..78223e963 100644 --- a/nodes/test/tool_gmail/test_gmail.py +++ b/nodes/test/tool_gmail/test_gmail.py @@ -260,8 +260,6 @@ def test_token_uri_rejects_non_string(): import datetime as _dt # noqa: E402 import urllib.error as _uerr # noqa: E402 -import google.auth.exceptions as _gae # noqa: E402 - class _FakeResp: """Minimal urlopen() response context manager for tests.""" @@ -291,7 +289,20 @@ def _open(req, timeout=None): return _open -def test_broker_refresh_success_uses_broker_expiry(monkeypatch): +@pytest.fixture +def refresh_error(monkeypatch): + """Prepend the mock Google SDK so the broker helper's ``import + google.auth.exceptions`` resolves (the real SDK is absent in the node test + env), and hand the test the RefreshError type it raises. + """ + mocks = Path(__file__).resolve().parents[1] / 'mocks' + monkeypatch.syspath_prepend(str(mocks)) + import google.auth.exceptions as _gae + + return _gae.RefreshError + + +def test_broker_refresh_success_uses_broker_expiry(monkeypatch, refresh_error): payload = json.dumps({'access_token': 'AT', 'expiry_date': 1_000_000}).encode() monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=payload)) tok, exp = gmail_client._refresh_access_token_via_broker('https://oauth2.rocketride.ai/refresh', 'RT') @@ -299,7 +310,7 @@ def test_broker_refresh_success_uses_broker_expiry(monkeypatch): assert exp == _dt.datetime.utcfromtimestamp(1000) -def test_broker_refresh_defaults_expiry_when_missing(monkeypatch): +def test_broker_refresh_defaults_expiry_when_missing(monkeypatch, refresh_error): # A 200 without expiry_date must not leave a stale past expiry (which would # force a refresh on every API call); fall back to a sane future lifetime. payload = json.dumps({'access_token': 'AT'}).encode() @@ -310,39 +321,39 @@ def test_broker_refresh_defaults_expiry_when_missing(monkeypatch): assert exp > before -def test_broker_refresh_missing_access_token_raises(monkeypatch): +def test_broker_refresh_missing_access_token_raises(monkeypatch, refresh_error): # A 200 without access_token must fail loud, not silently keep the stale token. payload = json.dumps({'expiry_date': 1_000_000}).encode() monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=payload)) - with pytest.raises(_gae.RefreshError, match='no access token'): + with pytest.raises(refresh_error, match='no access token'): gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') -def test_broker_refresh_http_error_reports_rejected(monkeypatch): +def test_broker_refresh_http_error_reports_rejected(monkeypatch, refresh_error): # HTTPError is a subclass of URLError: the broker was reached but rejected # the request, so it must not be misreported as "unreachable". err = _uerr.HTTPError('https://broker/refresh', 500, 'boom', {}, None) monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(raises=err)) - with pytest.raises(_gae.RefreshError, match='rejected by the broker'): + with pytest.raises(refresh_error, match='rejected by the broker'): gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') -def test_broker_refresh_urlerror_reports_unreachable(monkeypatch): +def test_broker_refresh_urlerror_reports_unreachable(monkeypatch, refresh_error): monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(raises=_uerr.URLError('down'))) - with pytest.raises(_gae.RefreshError, match='unreachable'): + with pytest.raises(refresh_error, match='unreachable'): gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') -def test_broker_refresh_read_timeout_reports_unreachable(monkeypatch): +def test_broker_refresh_read_timeout_reports_unreachable(monkeypatch, refresh_error): # A timeout during resp.read() raises OSError (TimeoutError), not URLError. monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(read_raises=TimeoutError('read timed out'))) - with pytest.raises(_gae.RefreshError, match='unreachable'): + with pytest.raises(refresh_error, match='unreachable'): gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') -def test_broker_refresh_invalid_json_reports_invalid(monkeypatch): +def test_broker_refresh_invalid_json_reports_invalid(monkeypatch, refresh_error): monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=b'not json')) - with pytest.raises(_gae.RefreshError, match='invalid response'): + with pytest.raises(refresh_error, match='invalid response'): gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') From dc2d83be4a0b970920acbad5e0bb40b50f994fe4 Mon Sep 17 00:00:00 2001 From: meetp06 <65815199+meetp06@users.noreply.github.com> Date: Mon, 13 Jul 2026 19:56:31 -0700 Subject: [PATCH 5/5] fix(nodes): validate malformed broker/token_uri payloads (review) Address CodeRabbit review on #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 --- nodes/src/nodes/tool_gmail/gmail_client.py | 25 ++++++++++++++---- nodes/test/tool_gmail/test_gmail.py | 30 ++++++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/nodes/src/nodes/tool_gmail/gmail_client.py b/nodes/src/nodes/tool_gmail/gmail_client.py index 4422d4b05..81dca5c58 100644 --- a/nodes/src/nodes/tool_gmail/gmail_client.py +++ b/nodes/src/nodes/tool_gmail/gmail_client.py @@ -115,7 +115,9 @@ def resolve_token_uri(token_uri: object) -> str: the field. Raises ValueError for any other value so a tampered token fails loud instead of silently posting credentials elsewhere. """ - if not token_uri: + # Only an absent value (None or empty string) falls back; other falsy + # non-strings (0, False, [], {}) are tampering and must fail loud. + if token_uri is None or token_uri == '': return _GOOGLE_TOKEN_URI if not isinstance(token_uri, str): raise ValueError('Gmail token token_uri must be a string') @@ -211,17 +213,30 @@ def _refresh_access_token_via_broker(broker_url: str, refresh_token: str): 'Gmail token refresh returned an invalid response from the broker. Please reconnect your Google account.' ) from exc + # Valid JSON that is not an object (null, a number, a list) would crash + # data.get(...) with AttributeError — treat it as an invalid response. + if not isinstance(data, dict): + raise _gae.RefreshError( + 'Gmail token refresh returned an invalid response from the broker. Please reconnect your Google account.' + ) + 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: - expiry = _dt.datetime.utcfromtimestamp(ms / 1000) - else: + if ms is None: expiry = _dt.datetime.utcnow() + _dt.timedelta(seconds=_DEFAULT_ACCESS_TOKEN_LIFETIME_S) + elif isinstance(ms, bool) or not isinstance(ms, (int, float)): + # A non-numeric expiry_date would raise a raw TypeError during + # timestamp conversion — surface it as an invalid response instead. + raise _gae.RefreshError( + 'Gmail token refresh returned an invalid response from the broker. Please reconnect your Google account.' + ) + else: + expiry = _dt.datetime.utcfromtimestamp(ms / 1000) return access_token, expiry diff --git a/nodes/test/tool_gmail/test_gmail.py b/nodes/test/tool_gmail/test_gmail.py index 78223e963..0bfb8f5ce 100644 --- a/nodes/test/tool_gmail/test_gmail.py +++ b/nodes/test/tool_gmail/test_gmail.py @@ -253,6 +253,14 @@ def test_token_uri_rejects_non_string(): gmail_client.resolve_token_uri({'url': 'https://oauth2.googleapis.com/token'}) +@pytest.mark.parametrize('bad', [0, False, [], {}]) +def test_token_uri_rejects_falsy_non_string(bad): + # Only None/'' fall back to the canonical endpoint; other falsy non-strings + # are tampering and must raise, not silently return the default. + with pytest.raises(ValueError): + gmail_client.resolve_token_uri(bad) + + # --------------------------------------------------------------------------- # _refresh_access_token_via_broker — broker refresh reliability (#1560) # --------------------------------------------------------------------------- @@ -357,6 +365,28 @@ def test_broker_refresh_invalid_json_reports_invalid(monkeypatch, refresh_error) gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') +def test_broker_refresh_non_dict_body_reports_invalid(monkeypatch, refresh_error): + # Valid JSON that is not an object (list/number/null) must not crash with + # AttributeError — treat it as an invalid response. + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=b'[1, 2, 3]')) + with pytest.raises(refresh_error, match='invalid response'): + gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') + + +def test_broker_refresh_non_string_access_token_raises(monkeypatch, refresh_error): + payload = json.dumps({'access_token': 123, 'expiry_date': 1_000_000}).encode() + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=payload)) + with pytest.raises(refresh_error, match='no access token'): + gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') + + +def test_broker_refresh_non_numeric_expiry_reports_invalid(monkeypatch, refresh_error): + payload = json.dumps({'access_token': 'AT', 'expiry_date': 'soon'}).encode() + monkeypatch.setattr('urllib.request.urlopen', _make_urlopen(body=payload)) + with pytest.raises(refresh_error, match='invalid response'): + gmail_client._refresh_access_token_via_broker('https://broker/refresh', 'RT') + + def test_message_get_cleans_headers(): raw = { 'id': 'm1',