Skip to content

[4.41/G2] Google Chat: explicit bot identity, media-API downloads, same-space message ids, native pagination #223

Description

@patrick-chinchill

Summary

Remaining Google Chat hardening from 4.38–4.41: (1) the bot identity comes only from explicit config (bot_user_id / GOOGLE_CHAT_BOT_USER_ID); today it is learned from the first BOT mention and persisted 30 days, so another bot can become "self". Self-detection fails closed and only our own mentions are normalized. (2) Attachment bytes come only from the media API by resourceName; the token-bearing downloadUri fallback is removed. (3) Edit/delete/react reject message ids from another space, and fetch_message is added. (4) Forward history uses bounded native pageToken pagination.

Upstream changes

  • 2f40a322 fix(gchat): send alt=media when downloading attachments by resourceName (#801) — chat@4.38.0 — adds alt=media to media downloads. Already present in Python (google_chat/adapter.py:2805); only its tests apply.
  • 32687038 fix(gchat): use media api for attachment downloads (#830) — chat@4.38.1 — fetchData exists only when attachmentDataRef.resourceName is set; rehydrate is a no-op without it; downloadUri is display-only; media errors go through handleGoogleChatError.
  • f485255b fix(adapters): harden webhook tenant isolation (#877) — chat@4.40.0 — Google Chat parts only: botUserId from config/env only (no learning, no state restore; initialize() warns when unset); isMessageFromSelf returns true for BOT senders when the id is unknown; only mentions with user.name === botUserId are normalized; forward history is one messages.list call (pageSize = min(max(limit,1),1000), pageToken = cursor, orderBy: "createTime asc", nextCursor = nextPageToken).
  • d6343460 fix(gchat): reject message ids from another space (#938) — chat@4.41.0 — parseMessageName must fully match ^spaces/([A-Za-z0-9_-]+)/messages/([A-Za-z0-9_-]+(?:\.[A-Za-z0-9_-]+)*)$; assertMessageInSpace runs in edit, delete, addReaction, removeReaction; new fetchMessage returns null on 404.

Current Python behavior

All refs in src/chat_sdk/adapters/google_chat/adapter.py.

  • Identity: :142 self._bot_user_id = None, no config/env source (grep -rn GOOGLE_CHAT_BOT_USER_ID src/ is empty); :484-492 restores gchat:botUserId from state; :2663-2682 learns the id from the first BOT USER_MENTION and persists it 30 days; :2653-2698 rewrites every BOT mention; :2706-2726 returns False for BOT senders when the id is unknown.
  • Downloads: :2728-2761 builds fetch_data from resourceName or downloadUri; :2793-2842 falls back to a GET of downloadUri carrying the service-account bearer token, gated only by the host allowlist _is_trusted_gchat_download_url (:2763-2791: .googleapis.com, .googleusercontent.com, .google.com); :2844-2872 rehydrates from fetch_metadata["url"]; resource_name is interpolated unvalidated at :2805.
  • Message ids: edit_message (:1644), delete_message (:1718), add_reaction (:1752), remove_reaction (:1780) put message_id straight into request paths. No parse_message_name / fetch_message exists; thread_utils.py has only encode, decode and is_dm_thread.
  • Pagination: :2031-2104 _fetch_messages_forward loops over every page (pageSize 1000), then slices by a message-name cursor.

Scope

  • types.py: bot_user_id: str | None = field(default=None, kw_only=True) ([4.41/G1] Google Chat: bind webhook JWT verification to configured identity #222 convention); constructor resolves config, then env, with is not None.
  • initialize(): warn once when the id is unset; remove the state restore and never read/write gchat:botUserId again (a best-effort delete of the stale key is optional).
  • _normalize_bot_mentions: drop the learning code and its pinned task; rewrite only annotations whose userMention.user.name == self._bot_user_id (none when unset).
  • _is_message_from_self: known id → exact match; unknown id and sender.type == "BOT" → True with a debug log.
  • Downloads: fetch_data only when resource_name is set (url stays for display); media API only, errors via _handle_google_chat_error(error, "fetchAttachmentData") so a 429 becomes AdapterRateLimitError; rehydrate without resourceName returns the attachment unchanged; delete _is_trusted_gchat_download_url if unused.
  • thread_utils.py: parse_message_name(message_id) -> (space_name, message_id) via re.fullmatch, raising ValidationError("gchat", ...).
  • _assert_message_in_space(thread_id, message_id) runs first in edit_message, delete_message, add_reaction, remove_reaction.
  • fetch_message(thread_id, message_id) -> Message | None: assert the space, GET {message_id}; 404 → None, other errors via _handle_google_chat_error; parse with the thread Google reports (msg["thread"]["name"]).
  • _fetch_messages_forward: one request (pageSize=str(min(max(limit, 1), 1000)), orderBy="createTime asc", pageToken=cursor when not None, plus the filter); next_cursor = nextPageToken.

Out of scope

Porting notes

  • Fail closed. With no id configured every BOT sender counts as self: no reply loops, but other bots' messages are ignored. Document prominently.
  • Mention rewrite order. Keep the existing reverse-startIndex sort (:2653) when filtering to our own id.
  • parse_message_name: copy upstream's pattern verbatim and use re.fullmatch (re.match with $ accepts a trailing \n).
  • Media resource_name (Python-only hardening): validate its shape before interpolating (reject ?, #, %, whitespace, ./.. segments) with ValidationError, since upstream relies on the googleapis client's encoding.
  • Cursors change from a message name to an opaque page token, so persisted forward cursors become invalid (expect an API error). Document this.
  • Removing the learned-id path also removes its _pin_task state write; leave no un-awaited coroutine behind.

Tests

Adapter tests are not fidelity-mapped. Port from packages/adapter-gchat/src/index.test.ts:

  • › "constructor / initialization": "should use the configured bot user ID instead of persisted state", "should not restore an untrusted learned bot ID from state"

  • › "constructor env var resolution": "should resolve botUserId from GOOGLE_CHAT_BOT_USER_ID"

  • › "parseMessage": "should provide fetchData when only attachmentDataRef is present (no downloadUri)", "should not fetch downloadUri when media.download fails", "should throw AdapterRateLimitError when media.download returns 429 and no downloadUri exists", "should not provide fetchData when only downloadUri is present", "should not provide fetchData when neither resourceName nor downloadUri exist", "should not rehydrate fetchData from a download URL"

  • › "normalizeBotMentions (via parseMessage)": "should not learn bot user ID from inbound annotations", "should ignore mentions of other bots", "should normalize only this app when another bot is mentioned first"

  • › "isMessageFromSelf (via parseMessage)": "should fail closed for bot senders when botUserId is unknown"

  • › "message space check": "editMessage rejects a message from another space", "deleteMessage rejects a message from another space", "addReaction rejects a message from another space", "removeReaction rejects a message from another space", it.each "rejects a malformed message id (%s)", "accepts a message in the thread's space"

  • › "fetchMessage": "returns the message with the thread Google reports for it", "returns null when the message does not exist", "rejects a message from another space without calling the API"

  • › "fetchMessages (forward direction)": "should fetch one bounded forward page", "should support cursor-based forward pagination"

  • thread-utils.test.ts › "parseMessageName": "parses a server-assigned message name", "parses a client-assigned message name", it.each "rejects %j".

  • Update rather than duplicate existing tests: flip the learned/restored-id tests (test_gchat_comprehensive.py::test_restore_bot_user_id_from_state :394, ::test_does_not_overwrite_existing_bot_user_id :405, test_gchat_api.py::test_learns_bot_id_from_annotations :957, ::test_persists_bot_id_after_learning :1027, test_gchat_webhook.py::test_learn_bot_user_id_from_annotations :325); move test_gchat_api.py::test_fetch_forward / test_fetch_forward_with_cursor to single-page semantics; switch tests passing bare message ids to full spaces/.../messages/... names.

Acceptance criteria

  • Full validation command from CLAUDE.md passes.
  • docs/UPSTREAM_SYNC.md: remove or narrow the Google Chat part of the :666 allowlist row (downloadUri is no longer fetched); add a row for the resource-name validation.
  • CHANGELOG entry under "Unreleased (4.41 wave)" with Breaking (Google Chat) notes: set bot_user_id / GOOGLE_CHAT_BOT_USER_ID or other bots' messages are ignored; persisted forward cursors are invalid; attachments without resourceName have no fetch_data.
  • Consumer-visible changes called out.

Dependencies

Blocked by #222.

Metadata

  • Effort: M
  • Consumer impact: high for Google Chat (new required bot_user_id config for correct bot handling; persisted cursor format change); none for Slack/Teams.
  • Suggested branch: sync/4.41-g2

Part of #184.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions