diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a842132..7414a001 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -77,6 +77,31 @@ Ports upstream `270b1c25` (#518, chat@4.35.0), `7a192235` (#787, chat@4.37.0) an - Verification uses PyJWT with a fixed 1-hour async key cache, not google-auth-library's `verifyIdToken` / `verifySignedJwtWithCertsAsync`. The time checks are ported by hand to match google-auth-library: `exp` and `iat` are required, with 300 s of clock skew, and a token whose `exp` is 24 hours or more in the future is rejected (PyJWT has no such limit). The issuer is compared exactly by hand on both paths, not through PyJWT's `issuer=`, because PyJWT 2.10.0 matched it as a substring (CVE-2024-53861). - The constructor warning when `endpoint_url` and `disable_signature_verification` are both set is Python-only. Upstream has the same precedence but does not log it. +### Google Chat: explicit bot identity, media-API downloads, same-space message ids, native forward pagination (#223, security) + +Ports upstream `32687038` (vercel/chat#830, chat@4.38.1), the Google Chat part of `f485255b` (vercel/chat#877, chat@4.40.0) and `d6343460` (vercel/chat#938, chat@4.41.0). `2f40a322` (vercel/chat#801, `alt=media`) was already in place; only its test is ported. + +- **Bot identity comes only from config.** New `GoogleChatAdapterConfig.bot_user_id` (keyword-only; env `GOOGLE_CHAT_BOT_USER_ID`, config wins) is the app's canonical `users/...` resource name. The adapter no longer learns its id from the first BOT mention, and never reads or writes the `gchat:botUserId` state key. Before, any bot mentioned first became "self" for 30 days, so its messages were silently dropped as the app's own. `initialize()` logs a warning once when the id is unset. +- **Self-detection fails closed.** With `bot_user_id` set, `is_me` is an exact sender match. Without it, every `BOT` sender counts as self, so the app never replies to itself in a loop. +- **Only this app's mentions are normalized.** `_normalize_bot_mentions` rewrites an annotation to `@{user_name}` only when its `userMention.user.name` equals `bot_user_id`; mentions of other bots stay as written. +- **Attachment bytes come only from the media API.** `fetch_data` exists only when the attachment has `attachmentDataRef.resourceName` and downloads `/v1/media/{resourceName}?alt=media`. The service-account token is no longer sent to `downloadUri` (as a fallback after a media error, or for URL-only attachments). `rehydrate_attachment` without a `resourceName` returns the attachment unchanged. Media errors go through the adapter's error handler, so a 429 raises `AdapterRateLimitError`; other failures raise the adapter's Google API error with the HTTP status as `code` (was `NetworkError`). The `_is_trusted_gchat_download_url` host allowlist is removed, since no URL is fetched any more. +- **Message ids must belong to the thread's space.** `edit_message`, `delete_message`, `add_reaction` and `remove_reaction` require a full `spaces/{space}/messages/{message}` name in the thread's space and raise `ValidationError` before any API call otherwise ("Invalid Google Chat message id" for a malformed name, "does not belong to space" for another space). New `thread_utils.parse_message_name()` returns a `GoogleChatMessageName(space_name, message_id)`. +- **New `GoogleChatAdapter.fetch_message(thread_id, message_id)`** returns the message with the thread id Google reports for it (`message.thread.name`), or `None` on 404, after the same space check. +- **Forward history is one bounded page.** `fetch_messages(direction="forward")` makes one `messages.list` call (`pageSize = min(max(limit, 1), 1000)`, `orderBy = "createTime asc"`, `pageToken = cursor`) and returns `nextPageToken` as `next_cursor`. Before, it downloaded the whole thread on every call and sliced it by a message-name cursor. + +#### Breaking (Google Chat) + +- **Set `bot_user_id` (or `GOOGLE_CHAT_BOT_USER_ID`)** to the `sender.name` of a message your app posted, such as `users/123456789`. Without it, other bots' messages are ignored (every BOT sender is treated as self) and `@`-mentions of your app are no longer rewritten to `@{user_name}`, so default mention detection may stop matching. The id learned by earlier releases (`gchat:botUserId` in state) is not reused; the stale key expires on its own. +- **Persisted forward cursors are invalid.** Forward `next_cursor` is now an opaque Google page token, not a message name. Passing a cursor saved by an earlier release fails with an API error; restart iteration without a cursor. Backward cursors are unchanged. +- **Attachments without `attachmentDataRef.resourceName` have no `fetch_data`** (`url` still carries `downloadUri` for display). Rehydrated attachments whose `fetch_metadata` has only a `url` also get none. +- **Bare message ids are rejected.** Callers passing `msg1`-style ids, or ids from another space, to edit/delete/reaction calls now get `ValidationError`. Ids returned by the adapter (`SentMessage.id`, `Message.id`) are already full names. + +#### Python-specific (divergence from upstream) + +- A media `resourceName` is validated before it is put into the request path: an empty value, `?`, `#`, `%`, backslash, anything outside printable ASCII (whitespace, control or non-ASCII characters), or a `.` / `..` segment raise `ValidationError` before a token is minted. Upstream hands the value to the googleapis client, which encodes it. +- An empty `bot_user_id` counts as unset, no mention is rewritten when the id is unset (upstream would still rewrite a BOT annotation with no `user.name`), and the "not configured" warning is logged once per adapter rather than on every `initialize()`. +- Forward `fetch_messages` with `limit=0` sends `pageSize=1` and returns at most one message. The limit resolves with `is not None`, so `0` is not replaced by the default; upstream's `options.limit || 100` requests 100. + ### Security - **Linear: comment-thread and agent-session history are now bound to the thread's issue** (#231, security; ports vercel/chat#965 `d7aa75b1` and #974 `2d2b933a`, chat@4.41.1). Linear thread ids carry an issue id plus a comment id or agent session id, and `fetch_messages` trusted the second segment without checking it against the first, so a caller naming issue A could read comment or session history from issue B. Anything that authorizes by issue id was affected, for example conversation-scoped AI tools. diff --git a/docs/SECURITY.md b/docs/SECURITY.md index aee8f6d1..fbfff520 100644 --- a/docs/SECURITY.md +++ b/docs/SECURITY.md @@ -72,6 +72,9 @@ The Teams adapter implements its own JWT validation rather than using the Micros - **Transports verify independently**: a request shape whose verifier is not configured is rejected with 401 unless `disable_signature_verification` is set. The constructor refuses to start when no verifier is configured and the opt-out is not set. - **A configured verifier beats the opt-out**: `disable_signature_verification` only covers a transport that has no verifier. Setting `endpoint_url` (even only for button routing) makes it a direct-webhook verifier, so direct webhooks are verified and the opt-out no longer applies to them. The constructor logs a warning when both are set. - **Endpoint inference**: when `endpoint_url` is unset, the button-click routing URL is inferred from `request.url`, but only after a direct webhook passes verification. It is never used as a verification audience. +- **Bot identity** (#223): `bot_user_id` / `GOOGLE_CHAT_BOT_USER_ID` (the app's canonical `users/...` name) is the only source of "self". It is never learned from inbound mentions or read back from state, so another bot cannot become "self". When it is unset, every `BOT` sender is treated as self (no reply loops, but other bots' messages are ignored) and no bot mention is normalized. +- **Attachment downloads** (#223): bytes come only from the Chat media API (`/v1/media/{resourceName}?alt=media`). The event's `downloadUri` is display-only and never fetched, so the service-account token is never sent to a URL taken from a payload or from persisted `fetch_metadata`. A `resourceName` with `?`, `#`, `%`, backslash, anything outside printable ASCII, or a `.` / `..` segment is rejected before a token is minted. +- **Message ids bound to the thread's space** (#223): `edit_message`, `delete_message`, `add_reaction`, `remove_reaction` and `fetch_message` require a full `spaces/{space}/messages/{message}` name in the thread's own space and raise `ValidationError` otherwise, before any API call. ### GitHub diff --git a/docs/UPSTREAM_SYNC.md b/docs/UPSTREAM_SYNC.md index 3443b4d6..bf3a4a72 100644 --- a/docs/UPSTREAM_SYNC.md +++ b/docs/UPSTREAM_SYNC.md @@ -934,7 +934,7 @@ stay explicit instead of being rediscovered in code review. | `ConcurrencyConfig.max_concurrent` slot scope | **Single global `asyncio.Semaphore`** — caps total in-flight handlers across all threads to `max_concurrent` | **Per-thread slot map** — `acquireConcurrentSlot(threadId, maxConcurrent)` keys the in-flight counter by `threadId`, so each thread has its own N-way bound | When upstream caught up (vercel/chat#419) it implemented per-thread slots; the Python port shipped earlier with a global semaphore and the slot-scope distinction wasn't visible in the original divergence row. Result: a deployment with `max_concurrent=2` and 100 active threads serializes everything globally on Python (peak in-flight = 2 across all threads) but allows 200 concurrent handlers on TS (2 per thread × 100). The `chat.test.ts > should track slots per thread independently` fidelity entry is `pytest.mark.skip`-ped in `tests/test_chat_faithful.py` until the implementation is restructured to a `dict[thread_id, asyncio.Semaphore]` (with cleanup-on-empty to avoid unbounded growth). Tracked as a follow-up. | | Redis lock token format | `{token_prefix}_{ms}_{secrets.token_hex(16)}` — always 32 hex chars, CSPRNG-sourced | `ioredis_${Date.now()}_${Math.random().toString(36).substring(2, 15)}` — base36, ≤13 chars, **not** CSPRNG | Interop via `IoRedisStateAdapter(token_prefix="ioredis")` still works for lock-release (release/extend compare by full-string equality, and each runtime only releases what it issued), but the token byte-shape diverges. Intentional — CSPRNG should not be regressed to `Math.random()` for cosmetic byte-for-byte compatibility. | | `StreamingPlan.is_supported()` / `get_fallback_text()` | Raise `RuntimeError` to fail loudly if a generic posting path (e.g. `ChannelImpl.post`, `post_postable_object`) tries to consume a `StreamingPlan` as a normal `PostableObject` | Silently return `True` / `""` — `ChannelImpl.post` would route through `postPostableObject` and post an empty-string fallback | Prevents `StreamingPlan` being silently routed through non-stream-aware posting paths where upstream would post a blank message or attempt a wrong-shape `adapter.post_object("stream", ...)` call. Internal dispatch is guarded by the `kind == "stream"` short-circuit in `post_postable_object` / `Thread.post`; this also protects third-party code that duck-types PostableObjects. | -| `rehydrate_attachment` URL allowlist (Slack / Teams / Google Chat / Twilio / Messenger) | Validates the downloaded URL's scheme (https) + host against a per-adapter allowlist inside the fetch closure; raises `ValidationError` on untrusted hosts before forwarding bearer/Basic credentials. **Twilio is layered:** the suffix allowlist runs first in the adapter closure, then the `fetch_twilio_media` primitive applies upstream's exact-origin check (below), so both must pass. **Messenger** raises `NetworkError` instead (see the Messenger download-guard row below) | Slack / Teams / Google Chat: no validation — `fetchData` blindly GETs `fetchMetadata.url` and forwards the workspace/bot token. **Twilio (since chat@4.38.1, vercel/chat#831 / `d8103a10`):** `fetchTwilioMedia` requires `new URL(url).origin === new URL(apiUrl ?? apiBaseUrl ?? DEFAULT_API_URL).origin` before resolving credentials, else `TwilioApiError("Twilio media URL must match the configured Twilio API origin", {status: 0})`; there is no separate host allowlist. **Messenger (since chat@4.39.0, vercel/chat 153bd964):** validates via the shared `downloadAttachment` host allowlist (see the Messenger download-guard row below) | SSRF + token-exfil risk upstream: after the 4.26 `rehydrateAttachment` hook lands, a crafted `fetchMetadata` in persisted state can redirect auth'd downloads to an arbitrary host. Python port enforces `CLAUDE.md`'s "Validate external URLs before requests (SSRF)" rule. The check runs inside the download closure (not at build time) so an attachment trusted at parse time still fails closed if the allowlist tightens later. Allowlist: Slack = `{files.slack.com, slack.com, *.slack.com, *.slack-edge.com}`; Teams = `{smba.trafficmanager.net, graph.microsoft.com, attachments.office.net, *.botframework.com, *.graph.microsoft.com, *.sharepoint.com, *.officeapps.live.com, *.office.com, *.office365.com, *.onedrive.com, *.microsoft.com}`; Google Chat = `{chat.googleapis.com, googleapis.com, *.googleapis.com, *.googleusercontent.com, *.google.com}`; Twilio = `{twilio.com, api.twilio.com, *.twilio.com, *.twiliocdn.com}`; Messenger = `{fbsbx.com, *.fbsbx.com, fbcdn.net, *.fbcdn.net}` (no credentials are forwarded, but `fallback`/link-share payload URLs are user-controlled, so the guard is an SSRF fix; it raises `NetworkError("messenger", "Refusing to fetch an untrusted attachment URL")`, upstream's message for a non-allowlisted host (IP-literal / unparseable-URL messages differ, see divergence (3) of the Messenger row below) — **upstream Messenger now validates too** since vercel/chat 153bd964 / chat@4.39.0, so Messenger is parity here, see the Messenger download-guard row below). **Twilio origin check is parity** (4.41 wave, #235): `fetch_twilio_media(url, *, api_url=None, api_base_url=None, ...)` compares `(scheme, lowercased host, effective port)` from `urlsplit` against `api_url` → `api_base_url` → `DEFAULT_API_URL` (`is not None` fallbacks), and the adapter passes its `api_url`, so a regional `api_url` constrains media downloads. Python-only strictness inside that check: URLs with whitespace, ASCII control characters or a backslash are refused outright (WHATWG strips/rewrites those, `urlsplit` does not match it exactly, so failing closed avoids a parser differential). The origin check covers every Twilio attachment download (freshly received webhook `MediaUrlN` as well as rehydrated attachments), as upstream's does, so any non-default `api_url` refuses inbound media on `api.twilio.com`. The Twilio suffix allowlist is **kept** as defence in depth: with the origin check it only adds a refusal when `api_url` points at a non-Twilio or non-`https` host (e.g. a proxy or local mock). Combined, that config can download **no** media at all: `api.twilio.com` URLs fail the origin check and URLs on the `api_url` origin fail the allowlist (pre-PR, such configs still downloaded `api.twilio.com` media; upstream, which has no allowlist, would download proxy-origin media); `*.twiliocdn.com` is now effectively unreachable because it is never the API origin. Redirects: `api.twilio.com` media answers with a redirect to a pre-signed CDN URL; the adapter's aiohttp transport follows it and aiohttp (every version in our `>=3.9` floor; verified in 3.9.0 and the locked 3.14.3 `client.py`) drops the `Authorization` header when the redirect changes origin, so Basic auth is not forwarded to the CDN. Redirect/byte-cap policy itself is tracked in #204. Regression coverage: `tests/test_twilio_adapter.py::TestRehydrateAttachment::test_rejects_rehydrated_media_from_an_untrusted_origin` (both layers) and `::test_rehydrated_media_is_constrained_to_the_configured_api_url` and `::test_non_twilio_api_url_refuses_media_on_every_origin` (proxy dead end); `tests/test_twilio_api.py::TestFetchTwilioMedia` (origin cases); `tests/test_messenger_webhook.py::TestRehydrateAttachment::test_rehydrated_closure_refuses_untrusted_url_without_io`. | +| `rehydrate_attachment` URL allowlist (Slack / Teams / Twilio / Messenger) | Validates the downloaded URL's scheme (https) + host against a per-adapter allowlist inside the fetch closure; raises `ValidationError` on untrusted hosts before forwarding bearer/Basic credentials. **Twilio is layered:** the suffix allowlist runs first in the adapter closure, then the `fetch_twilio_media` primitive applies upstream's exact-origin check (below), so both must pass. **Messenger** raises `NetworkError` instead (see the Messenger download-guard row below) | Slack / Teams: no validation — `fetchData` blindly GETs `fetchMetadata.url` and forwards the workspace/bot token. **Twilio (since chat@4.38.1, vercel/chat#831 / `d8103a10`):** `fetchTwilioMedia` requires `new URL(url).origin === new URL(apiUrl ?? apiBaseUrl ?? DEFAULT_API_URL).origin` before resolving credentials, else `TwilioApiError("Twilio media URL must match the configured Twilio API origin", {status: 0})`; there is no separate host allowlist. **Messenger (since chat@4.39.0, vercel/chat 153bd964):** validates via the shared `downloadAttachment` host allowlist (see the Messenger download-guard row below) | SSRF + token-exfil risk upstream: after the 4.26 `rehydrateAttachment` hook lands, a crafted `fetchMetadata` in persisted state can redirect auth'd downloads to an arbitrary host. Python port enforces `CLAUDE.md`'s "Validate external URLs before requests (SSRF)" rule. The check runs inside the download closure (not at build time) so an attachment trusted at parse time still fails closed if the allowlist tightens later. Allowlist: Slack = `{files.slack.com, slack.com, *.slack.com, *.slack-edge.com}`; Teams = `{smba.trafficmanager.net, graph.microsoft.com, attachments.office.net, *.botframework.com, *.graph.microsoft.com, *.sharepoint.com, *.officeapps.live.com, *.office.com, *.office365.com, *.onedrive.com, *.microsoft.com}`; Twilio = `{twilio.com, api.twilio.com, *.twilio.com, *.twiliocdn.com}`; Messenger = `{fbsbx.com, *.fbsbx.com, fbcdn.net, *.fbcdn.net}` (no credentials are forwarded, but `fallback`/link-share payload URLs are user-controlled, so the guard is an SSRF fix; it raises `NetworkError("messenger", "Refusing to fetch an untrusted attachment URL")`, upstream's message for a non-allowlisted host (IP-literal / unparseable-URL messages differ, see divergence (3) of the Messenger row below) — **upstream Messenger now validates too** since vercel/chat 153bd964 / chat@4.39.0, so Messenger is parity here, see the Messenger download-guard row below). **Twilio origin check is parity** (4.41 wave, #235): `fetch_twilio_media(url, *, api_url=None, api_base_url=None, ...)` compares `(scheme, lowercased host, effective port)` from `urlsplit` against `api_url` → `api_base_url` → `DEFAULT_API_URL` (`is not None` fallbacks), and the adapter passes its `api_url`, so a regional `api_url` constrains media downloads. Python-only strictness inside that check: URLs with whitespace, ASCII control characters or a backslash are refused outright (WHATWG strips/rewrites those, `urlsplit` does not match it exactly, so failing closed avoids a parser differential). The origin check covers every Twilio attachment download (freshly received webhook `MediaUrlN` as well as rehydrated attachments), as upstream's does, so any non-default `api_url` refuses inbound media on `api.twilio.com`. The Twilio suffix allowlist is **kept** as defence in depth: with the origin check it only adds a refusal when `api_url` points at a non-Twilio or non-`https` host (e.g. a proxy or local mock). Combined, that config can download **no** media at all: `api.twilio.com` URLs fail the origin check and URLs on the `api_url` origin fail the allowlist (pre-PR, such configs still downloaded `api.twilio.com` media; upstream, which has no allowlist, would download proxy-origin media); `*.twiliocdn.com` is now effectively unreachable because it is never the API origin. Redirects: `api.twilio.com` media answers with a redirect to a pre-signed CDN URL; the adapter's aiohttp transport follows it and aiohttp (every version in our `>=3.9` floor; verified in 3.9.0 and the locked 3.14.3 `client.py`) drops the `Authorization` header when the redirect changes origin, so Basic auth is not forwarded to the CDN. Redirect/byte-cap policy itself is tracked in #204. **Google Chat left this row in #223:** upstream `32687038` (vercel/chat#830, chat@4.38.1) removed the `downloadUri` fetch, so Google Chat attachment bytes come only from the media API by `resourceName` and no URL is ever fetched (parity); the `_is_trusted_gchat_download_url` allowlist is deleted. The remaining Python-only check is the media `resourceName` row below. Regression coverage: `tests/test_twilio_adapter.py::TestRehydrateAttachment::test_rejects_rehydrated_media_from_an_untrusted_origin` (both layers) and `::test_rehydrated_media_is_constrained_to_the_configured_api_url` and `::test_non_twilio_api_url_refuses_media_on_every_origin` (proxy dead end); `tests/test_twilio_api.py::TestFetchTwilioMedia` (origin cases); `tests/test_messenger_webhook.py::TestRehydrateAttachment::test_rehydrated_closure_refuses_untrusted_url_without_io`. | | Messenger attachment download guard (vercel/chat 153bd964, chat@4.39.0; #234) | `MessengerAdapter._download_attachment` checks the URL with `_is_trusted_messenger_media_url` **before any I/O** and inside the download closure (so fresh and `rehydrate_attachment`-rebuilt closures are both covered; `attachment.url` and the `fetch_metadata={"url": ...}` shape are unchanged). Follows redirects manually (`allow_redirects=False`, at most 5, `Location` resolved against the current hop and re-validated; as upstream, the hop limit is checked before the `Location` is validated, so a 6th redirect always raises `"Too many attachment redirects"`), 25 MB cap (declared `Content-Length` rejected up front, streamed body aborted past the cap), 30 s deadline over every hop + body read (`asyncio.timeout`, plus a per-request `aiohttp.ClientTimeout(total=30)`), non-2xx → `NetworkError("messenger", "Failed to fetch file: {status} {reason}")`, any other failure → `NetworkError("messenger", "Failed to download Messenger attachment")`. **Three divergences:** (1) **weaker — no DNS / private-IP resolution check.** The host allowlist alone rejects IP literals (including decimal-integer hosts like `2130706433`) and non-Meta names, but a Meta-CDN hostname is not re-checked against private ranges after resolution. Connection-bound DNS checks arrive with the shared guarded downloader (SH1, #204); remove this divergence when Messenger migrates onto it (#239). (2) **stricter URL parsing** — also rejects userinfo (`user@host`), an explicit port other than 443, and any host that is not plain lowercase DNS labels (percent-encoded, non-ASCII — the raw authority must be ASCII, so e.g. U+212A KELVIN SIGN cannot fold to `k` via `str.lower()` — bracketed IPv6, empty label, trailing dot), plus whitespace/control characters and backslashes anywhere in the URL, so `urlsplit` and aiohttp/yarl cannot disagree about which host is contacted. (3) **error-message text for IP literals and unparseable URLs.** Every URL/`Location` the check rejects raises the single `"Refusing to fetch an untrusted attachment URL"`. Upstream instead raises `"Refusing to fetch an internal attachment URL"` for a private/internal IP literal (e.g. `https://127.0.0.1/file`, `https://169.254.169.254/...`) and, when WHATWG `new URL(...)` throws on an unparseable URL or `Location` (e.g. `not a url`, `https://[::1/file`), the generic `"Failed to download Messenger attachment"` wrapper from `fetch.ts`. Both sides raise `NetworkError("messenger", ...)` with no network I/O, so only the message differs (upstream's `fetch.test.ts` asserts only `toThrow(NetworkError)` for these). Disappears when Messenger moves onto the shared downloader (#239). | `packages/adapter-messenger/src/fetch.ts` `download()` → `@chat-adapter/shared` `downloadAttachment(url, {adapter: "messenger", hosts: ["fbsbx.com", "fbcdn.net"]})`: https-only, exact host or subdomain (case-insensitive), redirects revalidated (max 5), 25 MB decoded-body cap, 30 s deadline, private/internal IPs refused both as literals and after DNS resolution through a pinned resolver. | **Consumer-visible:** Messenger `fetch_data()` for `fallback`/link-share (or any other) attachments whose URL is not on a Meta CDN host now raises `NetworkError` instead of downloading; `attachment.url` is still populated for display. Regression coverage: `tests/test_messenger_fetch.py` (ports `fetch.test.ts` `describe("Messenger attachment fetch")` + Python-specific redirect/cap/deadline/parser-differential cases) and `tests/test_messenger_webhook.py` (`test_downloads_attachment_successfully`, `test_rejects_external_fallback_downloads_before_the_network`). | | Teams file attachment URL source (`_create_attachment`) | For a `application/vnd.microsoft.teams.file.download.info` attachment, prefers the nested `content.downloadUrl` (a short-lived pre-signed link) over the top-level `contentUrl`. Every other attachment type keeps the upstream `contentUrl`-first path (with `content.downloadUrl` only as a fallback when `contentUrl` is missing/falsy). | Reads `att.contentUrl` only — `createAttachment(att)`'s param type doesn't even include `content` (`adapter-teams/src/index.ts:833`, `const url = att.contentUrl`) | **Hard-UX-failure divergence.** A SharePoint/OneDrive file shared in a personal or group chat arrives as a `file.download.info` attachment that carries BOTH a top-level `contentUrl` (the SharePoint/OneDrive item, e.g. `https://contoso.sharepoint.com/.../file.txt`, which returns **403** to an anonymous GET because it needs a SharePoint auth context) AND a nested `content.downloadUrl` — a pre-authenticated link the [Bot Framework docs](https://learn.microsoft.com/en-us/microsoftteams/platform/bots/how-to/bots-filesv4#message-activity-with-file-attachment-example) explicitly say to issue an `HTTP GET` against. Upstream (and our pre-fix adapter) read `contentUrl` only, so the attachment is **undownloadable** (`fetch_data` 403s). We prefer `content.downloadUrl` for this attachment type so the download actually works. The resulting URL still flows through the unchanged `_build_teams_fetch_data` / `rehydrate_attachment` SSRF allowlist (download hosts are SharePoint/OneDrive for Business → already covered by `*.sharepoint.com` / `*.onedrive.com` in the row above — no host added). To be filed as an upstream issue against vercel/chat. Supersedes stale PR #136. Regression coverage: `tests/test_teams_adapter.py::TestFileDownloadInfoAttachment`. | | `_rehydrate_message` with `Message` input | Falls through to the `rehydrate_attachment` pass even when the dequeued entry is already a `Message` instance | Early-returns on `raw instanceof Message` before rehydration | The Python port's Redis + Postgres `dequeue()` upgrade raw JSON to `Message.from_json(...)` before returning (upstream's dequeue returns the raw JSON.parse'd dict). Upstream's `instanceof Message` shortcut therefore only fires for in-memory state, but ours would fire for persistent backends too, leaving `fetch_data` stripped forever. The rehydrate pass still skips any attachment that already has `fetch_data`, so in-memory callers pay no cost. | @@ -971,6 +971,9 @@ stay explicit instead of being rediscovered in code review. | `chat/adapters` static adapter catalog (chat@4.31.0, new `./adapters` package subpath) | Not ported | A new SDK-free metadata module (`packages/chat/src/adapters/index.ts`, 19 `test()` cases): types `EnvVar`/`EnvGroup`/`AdapterEnvSpec`/`CatalogAdapter`, an `ADAPTERS` registry of ~25 official + vendor-official adapters, and exports `ADAPTER_NAMES`/`AdapterSlug`/`getAdapter`/`isAdapterSlug`/`listPlatformAdapters`/`listStateAdapters`/`listEnvVars`/`getSecretEnvVars`. Imports no provider SDK, so it's safe for build scripts, onboarding/setup screens, and config-discovery UIs that need package + env-var metadata (incl. secret-masking flags) without loading an adapter. | **Not meaningfully portable verbatim, and not yet needed.** The catalog's spine is TypeScript-ecosystem-specific: every entry is addressed by an **npm `packageName`** (`@chat-adapter/slack`, `@kapso/chat-adapter`, …) and the registry includes ~13 **vendor-official adapters this Python SDK does not ship** (AgentPhone, Kapso, Lark/Feishu, Liveblocks, Beeper Matrix, Resend, Sendblue, ioredis, …). A 1:1 port would ship actively-misleading data to a `pip`-installed consumer (`@chat-adapter/slack` instead of `chat-sdk[slack]`; `get_adapter("kapso")` returning metadata for something uninstallable here). The only Python-applicable slice — per-platform env-var requirements + secret flags — is the same across the 9 platform + 3 state adapters we do ship, but exposing it would be a **new Python-native feature** (a divergent rewrite around `pip` extras, not a port), and no current consumer (chinchill-api configures adapters in code, not via env-var discovery) needs it. Nothing in chat-sdk's core depends on the catalog. Deferred demand-driven: a Python-native `adapters` catalog (our adapters only, `pip`-extra install names, `get_secret_env_vars` masking) can be designed against real requirements if/when a consumer needs config discovery. The unmapped test file is additionally covered by the issue #78 fidelity-scope note. | | `GoogleChatAdapterConfig` identity fields (#222; upstream `7a192235` / `c3b5a08e`) | `workspace_add_on_service_account_email` and `pubsub_service_account_email` are declared `field(default=None, kw_only=True)` after `disable_signature_verification`, which stays the last *positional* field. Passing an 11th positional argument raises `TypeError` | Optional properties on an options object (`workspaceAddOnServiceAccountEmail`, `pubsubServiceAccountEmail`), which have no positional order | `GoogleChatAdapterConfig` is a positional dataclass. Adding the fields positionally would shift existing callers' arguments, and a shifted argument could land in an identity field the verifier trusts. Pinned by `TestDisableSignatureVerificationFieldOrder` in `tests/test_gchat_verification.py`. #223 adds `bot_user_id` the same way | | Google Chat webhook JWT verification mechanics (#222; upstream `270b1c25` / `7a192235` / `c3b5a08e`) | OIDC tokens (endpoint-URL and Pub/Sub) are decoded with PyJWT against Google's JWKS (`oauth2/v3/certs`): RS256, strict `aud` equality, `exp`/`iat`/`iss` required, 300 s leeway. `iss` is then checked by hand against {`accounts.google.com`, `https://accounts.google.com`}. Project-number tokens are decoded against the Chat issuer's X.509 certs with the same PyJWT options, and `iss` is then compared by hand with `chat@system.gserviceaccount.com` (not PyJWT's `issuer=`, which PyJWT 2.10.0 matched as a substring, CVE-2024-53861). On both paths, a token whose `exp` is 24 h or more ahead of the wall clock is rejected by hand, because PyJWT has no maximum-lifetime check. Both key sets are fetched with the shared aiohttp session and cached as `(fetched_at, {kid: key})` for a fixed 1 hour on an injectable monotonic clock. A failed or empty fetch is never cached, and an unknown `kid` is rejected without a refetch | `OAuth2Client.verifyIdToken` (federated certs, cached per `Cache-Control`) and `verifySignedJwtWithCertsAsync` with the Chat certs, cached for 1 hour | No maintained async google-auth equivalent exists, and the old `PyJWKClient` did blocking I/O on the event loop. The claim checks are ported to match google-auth-library, including its `exp >= now + 86400` rejection (`DEFAULT_MAX_TOKEN_LIFETIME_SECS_`), and `email_verified` checked by identity (`is True`) and add-on emails compared with `==` and shape-tested with `re.fullmatch`. Google's keys are published hours before use (`max-age` is about 6 h), so a 1-hour TTL does not miss rotations. Pinned by `TestEndpointUrlVerification`, `TestProjectNumberVerification`, `TestDirectTokenTimeClaims` and `TestVerificationKeyCache` | +| Google Chat media `resourceName` validation (#223; upstream `32687038`) | `fetch_data` (fresh and `rehydrate_attachment`) runs `_validate_media_resource_name` before minting a token: a non-string or empty value, `?`, `#`, `%`, backslash, anything outside printable ASCII (whitespace, control and non-ASCII characters), or a `.` / `..` path segment raise `ValidationError("gchat", "Invalid Google Chat attachment resource name: ...")`. Anything else is accepted, since `resourceName` is opaque. The value is then interpolated into `https://chat.googleapis.com/v1/media/{resourceName}?alt=media` | Passes `resourceName` to the googleapis client's `media.download`, which encodes it; no shape check | The Python port builds the URL itself, and `resourceName` can come from serialized `fetch_metadata`. Without the check a tampered value could add query parameters, or `..` segments (which the URL layer normalizes) could point the service-account token at another `/v1/` endpoint. Media errors go through `_handle_google_chat_error(error, "fetchAttachmentData")` as upstream (429 → `AdapterRateLimitError`, otherwise the `_GoogleApiError` carrying the HTTP status is re-raised). Pinned by `tests/test_google_chat_adapter.py::TestRehydrateAttachment::test_rehydrated_fetch_data_rejects_unsafe_resource_names` | +| Google Chat bot identity edge cases (#223; upstream `f485255b`) | `bot_user_id` resolves config → `GOOGLE_CHAT_BOT_USER_ID` with `is not None`, and an empty string then counts as unset. `_normalize_bot_mentions` returns early when the id is unset, so no annotation is rewritten. The "not configured" warning is logged once per adapter, not on every `initialize()`. `parse_message_name` returns a `GoogleChatMessageName` `NamedTuple` (`space_name`, `message_id`) and raises `ValidationError` for a non-`str` id | `botUserId = config.botUserId ?? env ?? undefined`, so `""` is kept (every check treats it as falsy, except that an annotation whose `user.name` is also `""` would be rewritten). With no id, `botUser.name !== this.botUserId` still rewrites a BOT annotation that has no `user.name`. Warns on every `initialize()` | Upstream intends "only annotations matching the configured bot user ID are rewritten"; the early return makes "none when unset" hold for malformed annotations too. Warning once avoids repeat noise when a serverless host re-initializes. Pinned by `tests/test_gchat_comprehensive.py::TestConstructorEnvVarResolution::test_empty_bot_user_id_env_var_is_treated_as_unset`, `::TestConstructorEnvVarResolution::test_empty_configured_bot_user_id_does_not_fall_back_to_env_var` (config `""` wins over the env var, so `is not None` rather than `or`), `::TestNormalizeBotMentionsComprehensive::test_nameless_bot_annotation_not_rewritten_when_bot_user_id_unset` and `::TestInitializeBotUserId::test_warns_once_when_bot_user_id_is_unset` | +| Google Chat forward `fetch_messages` with `limit=0` (#223; upstream `d6343460`) | `limit` resolves with `is not None` (repo-wide port rule), so `0` stays `0`; the upstream clamp `min(max(limit, 1), 1000)` then sends `pageSize=1`, and the call returns at most one message. Negative limits also send `pageSize=1`, as upstream | `const limit = options.limit \|\| 100` turns `0` into `100` before the same clamp, so `limit: 0` fetches a page of 100 | The truthiness fallback is the trap the port rules remove in every adapter (`limit=0` is never silently replaced by a default). Keeping the clamp means `0` still makes one bounded API call rather than an unbounded or default-sized one. Backward `limit=0` is unchanged by #223. Pinned by `tests/test_gchat_api.py::TestFetchMessages::test_forward_page_size_is_clamped` (`(0, "1")`) | | WhatsApp webhook raw-body logging (#187) | `handle_webhook` logs no body: the pre-verification `"WhatsApp webhook raw body"` debug log is removed, and `"WhatsApp webhook invalid JSON"` logs `{bodyBytes, contentType}` | Upstream `adapter-whatsapp/src/index.ts` (still at chat@4.41.1) logs `body.substring(0, 500)` **before** signature verification and `bodyPreview: body.substring(0, 200)` on invalid JSON | Log hygiene. The body arrives before authentication and carries message content and phone numbers, and DEBUG is commonly on in dev/staging. It is the same weakness class upstream fixed for GitHub (`fc7df9c4`) and GChat/Slack/Teams (`f485255b`). Regression tests: `tests/test_webhook_log_hygiene.py::TestWhatsAppLogHygiene`. Delete this row once upstream drops the logs. | | Message-content debug logs (#187) | GChat `"message event"` logs `{space, textLength}` (no `sender` display name, no `text` prefix). GChat `"Pub/Sub parsed message"` drops `text` and `author`. The slash-command debug logs (`Chat` `"Incoming slash command"`, Slack `"Processing Slack slash command"`, Discord `"Processing Discord slash command"`) log `textLength` instead of `text`. `textLength` counts characters (code points). | Upstream (chat@4.41.1) still logs the GChat sender display name + `text.slice(0, 50)`, the full Pub/Sub message `text` + author `fullName`, and slash-command `text` | Upstream's `f485255b` removed message text from `chat.ts` "Incoming message", and these are the same class of log. User-authored message text is kept out of DEBUG sinks. Action/reaction logs that still carry `user`/`user_name` stay at parity. Regression tests: `tests/test_webhook_log_hygiene.py` (`TestGoogleChatLogHygiene`, `TestSlackLogHygiene::test_slash_command_log_has_text_length_not_text`, `TestDiscordLogHygiene`, `TestChatLogHygiene::test_slash_command_log_has_text_length_not_text`). | diff --git a/src/chat_sdk/adapters/google_chat/adapter.py b/src/chat_sdk/adapters/google_chat/adapter.py index 3c9e54e2..dc73f3bf 100644 --- a/src/chat_sdk/adapters/google_chat/adapter.py +++ b/src/chat_sdk/adapters/google_chat/adapter.py @@ -28,6 +28,7 @@ decode_thread_id, encode_thread_id, is_dm_thread, + parse_message_name, ) from chat_sdk.adapters.google_chat.types import ( GoogleChatAdapterConfig, @@ -166,7 +167,17 @@ def __init__(self, config: GoogleChatAdapterConfig | None = None) -> None: self._persist_message_history: bool | None = None self._logger: Logger = config.logger or ConsoleLogger("info").child("gchat") self._user_name = config.user_name or "bot" - self._bot_user_id: str | None = None + # Explicit identity only (upstream f485255b): config wins over the env + # var (``??`` semantics). Never learned from mentions or restored from + # state -- a learned id let another bot become "self". An empty string + # is treated as unset, and the unset warning is logged once. + # Divergence from upstream -- see docs/UPSTREAM_SYNC.md (bot identity + # edge cases). + configured_bot_user_id = ( + config.bot_user_id if config.bot_user_id is not None else os.environ.get("GOOGLE_CHAT_BOT_USER_ID") + ) + self._bot_user_id: str | None = configured_bot_user_id or None + self._warned_no_bot_user_id = False self._chat: ChatInstance | None = None self._state: StateAdapter | None = None self._format_converter = GoogleChatFormatConverter() @@ -555,15 +566,16 @@ async def initialize(self, chat: ChatInstance) -> None: # Update user info cache to use the state adapter for persistence self._user_info_cache = UserInfoCache(self._state, self._logger) - # Restore persisted bot user ID from state (for serverless environments) - if not self._bot_user_id: - saved_bot_user_id = await self._state.get("gchat:botUserId") - if saved_bot_user_id: - self._bot_user_id = saved_bot_user_id - self._logger.debug( - "Restored bot user ID from state", - {"botUserId": self._bot_user_id}, - ) + # The bot identity comes only from config / GOOGLE_CHAT_BOT_USER_ID + # (upstream f485255b). The legacy learned ``gchat:botUserId`` state key + # is deliberately never read: it may hold another bot's id. + if not self._bot_user_id and not self._warned_no_bot_user_id: + self._warned_no_bot_user_id = True + self._logger.warn( + "Google Chat botUserId is not configured. BOT senders will be " + "treated as self to prevent reply loops, and bot mentions cannot " + "be normalized. Set bot_user_id or GOOGLE_CHAT_BOT_USER_ID." + ) self._logger.info("Google Chat adapter initialized") @@ -1928,6 +1940,7 @@ async def edit_message( message: AdapterPostableMessage, ) -> RawMessage: """Edit an existing message.""" + self._assert_message_in_space(thread_id, message_id) try: card = extract_card(message) @@ -1997,6 +2010,7 @@ async def edit_message( async def delete_message(self, thread_id: str, message_id: str) -> None: """Delete a message.""" + self._assert_message_in_space(thread_id, message_id) try: self._logger.debug("GChat API: spaces.messages.delete", {"messageId": message_id}) @@ -2036,6 +2050,7 @@ async def add_reaction( emoji: EmojiValue | str, ) -> None: """Add a reaction to a message.""" + self._assert_message_in_space(thread_id, message_id) gchat_emoji = emoji_to_gchat(emoji) try: @@ -2064,6 +2079,7 @@ async def remove_reaction( emoji: EmojiValue | str, ) -> None: """Remove a reaction from a message.""" + self._assert_message_in_space(thread_id, message_id) gchat_emoji = emoji_to_gchat(emoji) try: @@ -2317,70 +2333,57 @@ async def _fetch_messages_forward( cursor: str | None, use_impersonation: bool = False, ) -> FetchResult: - """Fetch messages in forward direction (oldest first).""" + """Fetch messages in forward direction (oldest first). + + Google Chat supports ascending creation time with native page tokens + (upstream f485255b). The API token is the opaque Chat SDK cursor, so + each call fetches one bounded page and a deleted message cannot reset + iteration to page one. Cursors from earlier releases (message names) + are not page tokens and are rejected by the API. + """ + page_size = min(max(limit, 1), 1000) self._logger.debug( "GChat API: spaces.messages.list (forward)", { "spaceName": space_name, "filter": filter_str, - "limit": limit, + "pageSize": page_size, "cursor": cursor, }, ) - # Fetch all messages (GChat defaults to createTime ASC = oldest first) - all_raw_messages: list[dict[str, Any]] = [] - page_token: str | None = None - - while True: - params: dict[str, str] = {"pageSize": "1000"} - if page_token: - params["pageToken"] = page_token - if filter_str: - params["filter"] = filter_str + params: dict[str, str] = { + "pageSize": str(page_size), + "orderBy": "createTime asc", + } + if cursor is not None: + params["pageToken"] = cursor + if filter_str: + params["filter"] = filter_str - response = await self._gchat_api_request( - "GET", - f"{space_name}/messages", - params=params, - use_impersonation=use_impersonation, - ) + response = await self._gchat_api_request( + "GET", + f"{space_name}/messages", + params=params, + use_impersonation=use_impersonation, + ) - page_messages = response.get("messages") or [] - all_raw_messages.extend(page_messages) - page_token = response.get("nextPageToken") - if not page_token: - break + raw_messages = response.get("messages") or [] self._logger.debug( - "GChat API: fetched all messages for forward pagination", - {"totalCount": len(all_raw_messages)}, + "GChat API: spaces.messages.list response (forward)", + { + "messageCount": len(raw_messages), + "hasNextPageToken": bool(response.get("nextPageToken")), + }, ) - # Find starting position based on cursor - start_index = 0 - if cursor: - for i, msg in enumerate(all_raw_messages): - if msg.get("name") == cursor: - start_index = i + 1 - break - - # Get the requested slice - selected_messages = all_raw_messages[start_index : start_index + limit] - messages = [] - for msg in selected_messages: + for msg in raw_messages: parsed = await self._parse_gchat_list_message(msg, space_name, thread_id) messages.append(parsed) - # Determine nextCursor - next_cursor: str | None = None - if start_index + limit < len(all_raw_messages) and selected_messages: - last_msg = selected_messages[-1] - if last_msg.get("name"): - next_cursor = last_msg["name"] - - return FetchResult(messages=messages, next_cursor=next_cursor) + return FetchResult(messages=messages, next_cursor=response.get("nextPageToken") or None) async def _parse_gchat_list_message( self, @@ -2888,6 +2891,51 @@ def is_dm(self, thread_id: str) -> bool: """Check if a thread is a direct message conversation.""" return is_dm_thread(thread_id) + def _assert_message_in_space(self, thread_id: str, message_id: str) -> None: + """Reject a message id that does not live in the thread's space. + + Google Chat message ids are full resource names + (``spaces/{space}/messages/{message}``) that identify a message on + their own, so the edit, delete, and reaction calls never consult the + thread. Check that the message lives in the thread's space, so a caller + cannot pair a permitted thread id with a message from another space + (upstream d6343460). Raises ``ValidationError``. + """ + space_name = self.decode_thread_id(thread_id).space_name + message = parse_message_name(message_id) + if message.space_name != space_name: + raise ValidationError( + "gchat", + f'Message "{message_id}" does not belong to space "{space_name}" of thread "{thread_id}"', + ) + + async def fetch_message(self, thread_id: str, message_id: str) -> Message | None: + """Fetch a single message by resource name. + + The returned message carries the thread id Google reports for it + (``message["thread"]["name"]``), not the one supplied, so callers can + confirm which thread a message really belongs to before acting on it. + Returns ``None`` when the message does not exist (404). + + Reads with the app's own credentials even when ``impersonate_user`` is + configured, as upstream does (``this.chatApi``, not + ``impersonatedChatApi``). Unlike history listing, a single-message + read is used to confirm a message before acting on it as the app, so + it must only see what the app itself can see; a message visible only to + the delegated user reads as a 403/404 rather than via the user's + broader access. + """ + self._assert_message_in_space(thread_id, message_id) + space_name = self.decode_thread_id(thread_id).space_name + try: + self._logger.debug("GChat API: spaces.messages.get", {"messageId": message_id}) + response = await self._gchat_api_request("GET", message_id) + return await self._parse_gchat_list_message(response, space_name, thread_id) + except Exception as error: + if getattr(error, "code", None) == 404: + return None + self._handle_google_chat_error(error, "fetchMessage") + # ========================================================================= # Message parsing / rendering # ========================================================================= @@ -2918,14 +2966,22 @@ def render_formatted(self, content: FormattedContent) -> str: # ========================================================================= def _normalize_bot_mentions(self, message: dict[str, Any]) -> str: - """Normalize bot mentions in message text. + """Normalize this app's mentions in message text. Google Chat uses the bot's display name (e.g., "@Chat SDK Demo") but the - Chat SDK expects "@{userName}" format. This replaces bot mentions with - the adapter's userName so mention detection works properly. - Also learns the bot's user ID from annotations for isMe detection. + Chat SDK expects "@{userName}" format. This replaces mentions of THIS + app with the adapter's userName so mention detection works properly. + Only annotations whose ``userMention.user.name`` equals the configured + ``bot_user_id`` are rewritten -- none when it is unset. The id is never + learned from annotations (upstream f485255b). """ text = message.get("text", "") + bot_user_id = self._bot_user_id + # Divergence from upstream -- see docs/UPSTREAM_SYNC.md (bot identity + # edge cases): with no id, nothing is rewritten, even an annotation + # that lacks ``user.name``. + if not bot_user_id: + return text annotations = message.get("annotations") or [] # Process in reverse order (highest startIndex first) to avoid index drift @@ -2937,31 +2993,10 @@ def _normalize_bot_mentions(self, message: dict[str, Any]) -> str: and (annotation.get("userMention") or {}).get("user", {}).get("type") == "BOT" ): bot_user = annotation["userMention"]["user"] + if bot_user.get("name") != bot_user_id: + continue bot_display_name = bot_user.get("displayName") - # Learn our bot's user ID from mentions and persist to state - if bot_user.get("name") and not self._bot_user_id: - self._bot_user_id = bot_user["name"] - self._logger.info( - "Learned bot user ID from mention", - {"botUserId": self._bot_user_id}, - ) - # Persist to state for serverless environments - if self._state: - import asyncio - - try: - loop = asyncio.get_running_loop() - _pin_task( - loop.create_task( - self._state.set( - "gchat:botUserId", self._bot_user_id, ttl_ms=30 * 24 * 60 * 60 * 1000 - ) - ) - ) - except RuntimeError: - pass - # Replace the bot mention with @{userName} start_index = annotation.get("startIndex") length = annotation.get("length") @@ -2986,22 +3021,25 @@ def _normalize_bot_mentions(self, message: dict[str, Any]) -> str: def _is_message_from_self(self, message: dict[str, Any]) -> bool: """Check if a message is from this bot. - Bot user ID is learned dynamically from message annotations when the bot - is @mentioned. Until we learn the ID, we cannot reliably determine isMe. + A configured ``bot_user_id`` permits exact matching in multi-bot + spaces. Without it, fail closed for ``BOT`` senders to prevent + self-reply loops (upstream f485255b). """ - sender_id = (message.get("sender") or {}).get("name") + sender = message.get("sender") or {} + sender_id = sender.get("name") # Use exact match when we know our bot ID if self._bot_user_id and sender_id: return sender_id == self._bot_user_id - # If we don't know our bot ID yet, we can't reliably determine isMe - if not self._bot_user_id and (message.get("sender") or {}).get("type") == "BOT": + # Fail closed when identity is unavailable: processing an unknown BOT + # sender could reprocess this app's own replies indefinitely. + if not self._bot_user_id and sender.get("type") == "BOT": self._logger.debug( - "Cannot determine isMe - bot user ID not yet learned. " - "Bot ID is learned from @mentions. Assuming message is not from self.", + "Cannot distinguish BOT sender without botUserId; treating it as self.", {"senderId": sender_id}, ) + return True return False @@ -3026,9 +3064,12 @@ def _create_attachment(self, att: dict[str, Any]) -> Attachment: if url: fetch_meta["url"] = url + # ``downloadUri`` is display-only (upstream 32687038): attachment bytes + # come solely from the media API by ``resourceName``, so no request + # ever carries the service-account token to a URL taken from the event. fetch_data: Callable[[], Awaitable[bytes]] | None = None - if resource_name or url: - fetch_data = self._build_gchat_fetch_data(resource_name, url) + if resource_name: + fetch_data = self._build_gchat_fetch_data(resource_name) return Attachment( type=att_type, # type: ignore[arg-type] @@ -3039,111 +3080,60 @@ def _create_attachment(self, att: dict[str, Any]) -> Attachment: fetch_metadata=fetch_meta or None, ) - @staticmethod - def _is_trusted_gchat_download_url(url: str) -> bool: - """Gate Google Chat attachment downloads to Google-owned hosts. - - After ``rehydrate_attachment`` reconstructs the fetch closure - from serialized ``fetch_metadata``, the URL may have been - tampered with in the state store. We refuse to forward the - OAuth access token unless the host is a known Google-owned host. + def _build_gchat_fetch_data(self, resource_name: str) -> Callable[[], Awaitable[bytes]]: + """Build a lazy ``fetch_data`` closure for a Google Chat attachment. - This is a Python-first divergence: upstream Google Chat adapter - does not validate the URL. See ``docs/UPSTREAM_SYNC.md`` Known - Non-Parity. + Downloads via the Chat media API (``media.download`` with + ``alt=media``) only. API errors go through + ``_handle_google_chat_error`` so a 429 surfaces as + ``AdapterRateLimitError``. """ - try: - parsed = urlparse(url) - except (ValueError, TypeError): - return False - if parsed.scheme != "https": - return False - host = (parsed.hostname or "").lower() - if not host: - return False - allowed_suffixes = ( - ".googleapis.com", - ".googleusercontent.com", - ".google.com", - ) - if host.endswith(allowed_suffixes): - return True - return host in {"chat.googleapis.com", "googleapis.com"} - - def _build_gchat_fetch_data( - self, - resource_name: str | None, - url: str | None, - ) -> Callable[[], Awaitable[bytes]]: - """Build a lazy ``fetch_data`` closure for a Google Chat attachment.""" adapter = self async def _fetch_data() -> bytes: - # Prefer media.download API - if resource_name: + # Divergence from upstream -- see docs/UPSTREAM_SYNC.md (media + # resourceName validation). Checked before a token is minted. + _validate_media_resource_name(resource_name) + try: token = await adapter._get_access_token() - download_url = f"https://chat.googleapis.com/v1/media/{resource_name}?alt=media" + download_url = f"{GCHAT_API_BASE}/media/{resource_name}?alt=media" session = await adapter._get_http_session() async with session.get( download_url, headers={"Authorization": f"Bearer {token}"}, ) as response: if response.status >= 400: - raise NetworkError( - "gchat", - f"Failed to download media: {response.status}", + raise _GoogleApiError( + message=f"Failed to download media: {response.status}", + code=response.status, ) return await response.read() - - # Fallback to direct URL fetch (downloadUri). Validate the - # host before forwarding the OAuth token — the URL may have - # been rebuilt from serialized metadata and tampered with. - if url: - if not adapter._is_trusted_gchat_download_url(url): - raise ValidationError( - "gchat", - f"Refusing to fetch Google Chat file from untrusted URL: {url}", - ) - token = await adapter._get_access_token() - session = await adapter._get_http_session() - async with session.get( - url, - headers={"Authorization": f"Bearer {token}"}, - ) as response: - if response.status >= 400: - raise NetworkError( - "gchat", - f"Failed to fetch file: {response.status}", - ) - return await response.read() - - raise AuthenticationError("gchat", "Cannot fetch file: no URL or resource name") + except Exception as error: + adapter._handle_google_chat_error(error, "fetchAttachmentData") return _fetch_data def rehydrate_attachment(self, attachment: Attachment) -> Attachment: """Reconstruct ``fetch_data`` on a deserialized Google Chat attachment. - Pulls ``resourceName`` (preferred, used with media.download API) and - ``url`` (fallback) from ``fetch_metadata``. Returns the attachment - unchanged when neither identifier is present. + Uses only ``fetch_metadata["resourceName"]`` (media API). Returns the + attachment unchanged when it is absent -- a serialized download URL is + never turned back into a fetch (upstream 32687038). """ meta = attachment.fetch_metadata if attachment.fetch_metadata is not None else {} resource_name = meta.get("resourceName") - meta_url = meta.get("url") - url = meta_url if meta_url is not None else attachment.url - if resource_name is None and url is None: + if not resource_name: return attachment return Attachment( type=attachment.type, - url=url, + url=attachment.url, name=attachment.name, mime_type=attachment.mime_type, size=attachment.size, width=attachment.width, height=attachment.height, data=attachment.data, - fetch_data=self._build_gchat_fetch_data(resource_name, url), + fetch_data=self._build_gchat_fetch_data(resource_name), fetch_metadata=attachment.fetch_metadata, ) @@ -3216,6 +3206,39 @@ def __init__( self.errors = errors +# Characters that must never appear in a media ``resourceName``: they would +# change what the ``/v1/media/{resourceName}`` URL addresses (query, fragment, +# percent-decoding) or how it is split. Anything outside printable ASCII +# (whitespace, control and non-ASCII characters) is rejected separately. +_MEDIA_RESOURCE_NAME_FORBIDDEN_CHARS = frozenset("?#%\\") + + +def _validate_media_resource_name(resource_name: Any) -> None: + """Reject a media ``resourceName`` that could escape ``/v1/media/``. + + Divergence from upstream -- see docs/UPSTREAM_SYNC.md. Upstream passes the + value to the googleapis client, which encodes it; here it is interpolated + into the request path, and it can come from serialized ``fetch_metadata`` + (``rehydrate_attachment``). ``resourceName`` is otherwise opaque, so this is + a denylist: a non-string or empty value, ``?``, ``#``, ``%``, backslash, + anything outside printable ASCII (``!``..``~``, so whitespace, control and + non-ASCII characters), and ``.`` / ``..`` path segments (which the URL + layer would normalize away) raise ``ValidationError`` before any token is + minted. + """ + valid = ( + isinstance(resource_name, str) + and resource_name != "" + and all("!" <= ch <= "~" and ch not in _MEDIA_RESOURCE_NAME_FORBIDDEN_CHARS for ch in resource_name) + and not any(segment in {".", ".."} for segment in resource_name.split("/")) + ) + if not valid: + raise ValidationError( + "gchat", + f"Invalid Google Chat attachment resource name: {json.dumps(resource_name, default=repr)}", + ) + + def _exceeds_max_token_lifetime(payload: dict[str, Any]) -> bool: """Whether ``exp`` is at least 24 hours ahead of the wall clock. diff --git a/src/chat_sdk/adapters/google_chat/thread_utils.py b/src/chat_sdk/adapters/google_chat/thread_utils.py index b6b6b103..9740bbc0 100644 --- a/src/chat_sdk/adapters/google_chat/thread_utils.py +++ b/src/chat_sdk/adapters/google_chat/thread_utils.py @@ -6,7 +6,10 @@ from __future__ import annotations import base64 +import json +import re from dataclasses import dataclass +from typing import Any, NamedTuple from chat_sdk.shared import ValidationError @@ -68,6 +71,43 @@ def decode_thread_id(thread_id: str) -> GoogleChatThreadId: ) +class GoogleChatMessageName(NamedTuple): + """Google Chat message resource name data (upstream ``GoogleChatMessageName``).""" + + space_name: str + """Space resource name, e.g. ``spaces/AAQAJ9CXYcg``.""" + message_id: str + """Message segment, e.g. ``FGEOaAwNIcs.FGEOaAwNIcs`` or ``client-my-id``.""" + + +# Exactly ``spaces/{space}/messages/{message}`` (upstream d6343460 pattern; its +# ``^``/``$`` anchors are replaced by ``fullmatch``). +# Segments are the characters Google uses in resource ids; a dot may only join +# two non-empty groups, so ``.`` and ``..`` segments, empty segments, ``?``, +# ``#``, ``%``, ``/``, and whitespace are all rejected before the name reaches +# the API. ASCII ranges are spelled out, and the pattern is applied with +# ``fullmatch`` (``re.match`` with ``$`` would accept a trailing ``\n``). +_MESSAGE_NAME_PATTERN = re.compile(r"spaces/([A-Za-z0-9_-]+)/messages/([A-Za-z0-9_-]+(?:\.[A-Za-z0-9_-]+)*)") + + +def parse_message_name(message_id: Any) -> GoogleChatMessageName: + """Parse a Google Chat message resource name. + + Message ids identify a message on their own, so anything that lets a name + resolve somewhere other than it appears to (path traversal, query strings, + percent-encoding) is rejected with ``ValidationError`` rather than + normalized downstream. + """ + match = _MESSAGE_NAME_PATTERN.fullmatch(message_id) if isinstance(message_id, str) else None + if match is None: + raise ValidationError( + "gchat", + f"Invalid Google Chat message id: {json.dumps(message_id, ensure_ascii=False, default=repr)} " + "(expected spaces/{space}/messages/{message})", + ) + return GoogleChatMessageName(space_name=f"spaces/{match.group(1)}", message_id=match.group(2)) + + def is_dm_thread(thread_id: str) -> bool: """Check if a thread is a direct message conversation. diff --git a/src/chat_sdk/adapters/google_chat/types.py b/src/chat_sdk/adapters/google_chat/types.py index 129f86fa..23950a9b 100644 --- a/src/chat_sdk/adapters/google_chat/types.py +++ b/src/chat_sdk/adapters/google_chat/types.py @@ -365,3 +365,13 @@ class GoogleChatAdapterConfig: # Falls back to the GOOGLE_CHAT_PUBSUB_SERVICE_ACCOUNT_EMAIL env var when # left unset (None). pubsub_service_account_email: str | None = field(default=None, kw_only=True) + + # Canonical ``users/...`` resource name of this Chat app (the + # ``sender.name`` of a verified message the app authored), e.g. + # ``users/123456789``. Used for exact self-message detection and to decide + # which bot mention to normalize to ``@{user_name}``. The adapter never + # learns this from inbound mentions (upstream f485255b). When unset, every + # ``BOT`` sender is treated as self (fail closed, no reply loops -- but + # other bots' messages are ignored) and no bot mention is normalized. + # Falls back to the GOOGLE_CHAT_BOT_USER_ID env var when left unset (None). + bot_user_id: str | None = field(default=None, kw_only=True) diff --git a/tests/conftest.py b/tests/conftest.py index 38d3ab92..d452f49f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -22,6 +22,20 @@ def anyio_backend(): return "asyncio" +# Env vars that change adapter behaviour when merely present in the shell. +# Cleared for every test so a developer or CI environment configured for a +# real deployment (for example GOOGLE_CHAT_BOT_USER_ID, which the Google Chat +# docs tell users to export) cannot flip "identity not configured" tests. +# Tests that need a value set it explicitly with ``monkeypatch.setenv``. +_AMBIENT_IDENTITY_ENV_KEYS = ("GOOGLE_CHAT_BOT_USER_ID",) + + +@pytest.fixture(autouse=True) +def _clear_ambient_identity_env(monkeypatch: pytest.MonkeyPatch) -> None: + for key in _AMBIENT_IDENTITY_ENV_KEYS: + monkeypatch.delenv(key, raising=False) + + @pytest.fixture(autouse=True) def _no_github_bot_user_id_env(monkeypatch: pytest.MonkeyPatch) -> None: """Keep ``GITHUB_BOT_USER_ID`` out of every test unless it sets it. diff --git a/tests/test_gchat_api.py b/tests/test_gchat_api.py index 01d1dba3..33ff58bd 100644 --- a/tests/test_gchat_api.py +++ b/tests/test_gchat_api.py @@ -7,7 +7,8 @@ Covers: postMessage, editMessage, deleteMessage, fetchMessages, fetchChannelMessages, listThreads, openDM, addReaction, removeReaction, startTyping, stream, user info caching, workspace events subscription -lifecycle, bot user ID learning from annotations. +lifecycle, explicit bot user ID (never learned from annotations), message +space check, fetchMessage. """ from __future__ import annotations @@ -395,7 +396,9 @@ async def test_fetch_backward(self): assert result.next_cursor is None @pytest.mark.asyncio - async def test_fetch_forward(self): + async def test_fetch_one_bounded_forward_page(self): + # Upstream f485255b: forward history is ONE native messages.list call + # (createTime asc, pageSize = limit) and nextPageToken is the cursor. adapter, api, _ = await _init_adapter() tid = _encode_tid("spaces/ABC123", "spaces/ABC123/threads/T1") api.set_response_prefix( @@ -418,12 +421,35 @@ async def test_fetch_forward(self): "thread": {"name": "spaces/ABC123/threads/T1"}, }, ], + "nextPageToken": "page-2", }, ) - result = await adapter.fetch_messages(tid, FetchOptions(direction="forward")) + result = await adapter.fetch_messages(tid, FetchOptions(direction="forward", limit=2)) - assert len(result.messages) == 2 + assert [m.text for m in result.messages] == ["First", "Second"] + assert result.next_cursor == "page-2" + calls = api.get_calls("GET", "spaces/ABC123/messages") + assert len(calls) == 1 + assert calls[0]["params"] == { + "pageSize": "2", + "orderBy": "createTime asc", + "filter": 'thread.name = "spaces/ABC123/threads/T1"', + } + + @pytest.mark.asyncio + @pytest.mark.parametrize(("limit", "page_size"), [(0, "1"), (-5, "1"), (1000, "1000"), (5000, "1000")]) + async def test_forward_page_size_is_clamped(self, limit: int, page_size: str): + adapter, api, _ = await _init_adapter() + tid = _encode_tid("spaces/ABC123") + api.set_response_prefix("GET", "spaces/ABC123/messages", {"messages": []}) + + result = await adapter.fetch_messages(tid, FetchOptions(direction="forward", limit=limit)) + + assert result.next_cursor is None + calls = api.get_calls("GET", "spaces/ABC123/messages") + assert len(calls) == 1 + assert calls[0]["params"]["pageSize"] == page_size @pytest.mark.asyncio async def test_fetch_backward_with_pagination(self): @@ -451,7 +477,9 @@ async def test_fetch_backward_with_pagination(self): assert result.next_cursor == "page2_token" @pytest.mark.asyncio - async def test_fetch_forward_with_cursor(self): + async def test_supports_cursor_based_forward_pagination(self): + # The forward cursor is the opaque API page token, passed through as + # ``pageToken`` -- never matched against message names. adapter, api, _ = await _init_adapter() tid = _encode_tid("spaces/ABC123", "spaces/ABC123/threads/T1") api.set_response_prefix( @@ -459,15 +487,9 @@ async def test_fetch_forward_with_cursor(self): "spaces/ABC123/messages", { "messages": [ - { - "name": "spaces/ABC123/messages/msg1", - "text": "First", - "sender": {"name": "users/100", "displayName": "User", "type": "HUMAN"}, - "createTime": "2024-01-01T00:00:00Z", - }, { "name": "spaces/ABC123/messages/msg2", - "text": "Second (cursor start)", + "text": "Second", "sender": {"name": "users/101", "displayName": "User2", "type": "HUMAN"}, "createTime": "2024-01-01T00:01:00Z", }, @@ -483,12 +505,16 @@ async def test_fetch_forward_with_cursor(self): result = await adapter.fetch_messages( tid, - FetchOptions(direction="forward", cursor="spaces/ABC123/messages/msg1", limit=1), + FetchOptions(direction="forward", cursor="page-2", limit=10), ) - # Should start after cursor (msg1), so should get msg2 - assert len(result.messages) == 1 - assert result.messages[0].id == "spaces/ABC123/messages/msg2" + assert [m.text for m in result.messages] == ["Second", "Third"] + assert result.next_cursor is None + calls = api.get_calls("GET", "spaces/ABC123/messages") + assert len(calls) == 1 + assert calls[0]["params"]["pageToken"] == "page-2" + assert calls[0]["params"]["pageSize"] == "10" + assert calls[0]["params"]["orderBy"] == "createTime asc" # ============================================================================= @@ -654,6 +680,173 @@ async def test_creates_new_dm_when_not_found(self): assert body["memberships"][0]["member"]["name"] == "users/67890" +# ============================================================================= +# Message space check (upstream d6343460) +# ============================================================================= + +_SPACE_THREAD = "gchat:spaces/ABC123" +_FOREIGN_MESSAGE = "spaces/OTHER/messages/msg9" +_FOREIGN_SPACE = "does not belong to space" +_INVALID_MESSAGE_ID = "Invalid Google Chat message id" + + +class TestMessageSpaceCheck: + @pytest.mark.asyncio + async def test_edit_message_rejects_a_message_from_another_space(self): + adapter, api, _ = await _init_adapter() + with pytest.raises(ValidationError, match=_FOREIGN_SPACE): + await adapter.edit_message(_SPACE_THREAD, _FOREIGN_MESSAGE, "edit") + assert api.calls == [] + + @pytest.mark.asyncio + async def test_delete_message_rejects_a_message_from_another_space(self): + adapter, api, _ = await _init_adapter() + with pytest.raises(ValidationError, match=_FOREIGN_SPACE): + await adapter.delete_message(_SPACE_THREAD, _FOREIGN_MESSAGE) + assert api.calls == [] + + @pytest.mark.asyncio + async def test_add_reaction_rejects_a_message_from_another_space(self): + adapter, api, _ = await _init_adapter() + with pytest.raises(ValidationError, match=_FOREIGN_SPACE): + await adapter.add_reaction(_SPACE_THREAD, _FOREIGN_MESSAGE, "\U0001f44d") + assert api.calls == [] + + @pytest.mark.asyncio + async def test_remove_reaction_rejects_a_message_from_another_space(self): + adapter, api, _ = await _init_adapter() + with pytest.raises(ValidationError, match=_FOREIGN_SPACE): + await adapter.remove_reaction(_SPACE_THREAD, _FOREIGN_MESSAGE, "\U0001f44d") + assert api.calls == [] + + @pytest.mark.asyncio + @pytest.mark.parametrize( + "message_id", + [ + pytest.param("spaces/ABC123/messages/../../OTHER/messages/msg9", id="path traversal"), + pytest.param("spaces/ABC123/messages/./msg1", id="dot segment"), + pytest.param("spaces/ABC123/messages/msg1/reactions/r1", id="extra segment"), + pytest.param("spaces/ABC123/messages/msg1?alt=json", id="query string"), + pytest.param("spaces/ABC123/messages/msg1#x", id="fragment"), + pytest.param("spaces/ABC123/messages/%2e%2e/OTHER", id="percent encoding"), + pytest.param("spaces/ABC123/messages/msg1 ", id="whitespace"), + pytest.param("msg1", id="bare id"), + pytest.param("", id="empty"), + ], + ) + async def test_rejects_a_malformed_message_id(self, message_id: str): + adapter, api, _ = await _init_adapter() + with pytest.raises(ValidationError, match=_INVALID_MESSAGE_ID): + await adapter.delete_message(_SPACE_THREAD, message_id) + assert api.calls == [] + + @pytest.mark.asyncio + async def test_accepts_a_message_in_the_threads_space(self): + adapter, api, _ = await _init_adapter() + await adapter.delete_message(_SPACE_THREAD, "spaces/ABC123/messages/msg1") + assert [(c["method"], c["path"]) for c in api.calls] == [("DELETE", "spaces/ABC123/messages/msg1")] + + @pytest.mark.asyncio + async def test_rejects_a_space_that_only_shares_the_threads_prefix(self): + # Exact space comparison, not a prefix check: spaces/ABC1234 is not + # spaces/ABC123. + adapter, api, _ = await _init_adapter() + with pytest.raises(ValidationError, match=_FOREIGN_SPACE): + await adapter.delete_message(_SPACE_THREAD, "spaces/ABC1234/messages/msg1") + assert api.calls == [] + + @pytest.mark.asyncio + async def test_threaded_and_dm_thread_ids_use_their_space(self): + adapter, api, _ = await _init_adapter() + threaded = _encode_tid("spaces/ABC123", "spaces/ABC123/threads/T1") + dm = _encode_tid("spaces/DM1", is_dm=True) + + await adapter.delete_message(threaded, "spaces/ABC123/messages/msg1") + await adapter.delete_message(dm, "spaces/DM1/messages/msg2") + with pytest.raises(ValidationError, match=_FOREIGN_SPACE): + await adapter.delete_message(dm, "spaces/ABC123/messages/msg1") + + assert [c["path"] for c in api.calls] == ["spaces/ABC123/messages/msg1", "spaces/DM1/messages/msg2"] + + +# ============================================================================= +# fetchMessage (upstream d6343460) +# ============================================================================= + + +class TestFetchMessage: + @pytest.mark.asyncio + async def test_returns_the_message_with_the_thread_google_reports_for_it(self): + adapter, api, _ = await _init_adapter() + api.set_response( + "GET", + "spaces/ABC123/messages/msg1", + { + "name": "spaces/ABC123/messages/msg1", + "text": "hello", + "thread": {"name": "spaces/ABC123/threads/other"}, + "sender": {"name": "users/1", "displayName": "Alice", "type": "HUMAN"}, + "createTime": "2024-01-01T00:00:00Z", + }, + ) + + message = await adapter.fetch_message(_SPACE_THREAD, "spaces/ABC123/messages/msg1") + + assert [(c["method"], c["path"]) for c in api.calls] == [("GET", "spaces/ABC123/messages/msg1")] + assert message is not None + assert message.id == "spaces/ABC123/messages/msg1" + assert message.text == "hello" + assert message.thread_id == adapter.encode_thread_id( + GoogleChatThreadId(space_name="spaces/ABC123", thread_name="spaces/ABC123/threads/other") + ) + + @pytest.mark.asyncio + async def test_returns_none_when_the_message_does_not_exist(self): + adapter, api, _ = await _init_adapter() + api.set_response("GET", "spaces/ABC123/messages/gone", _FakeApiError(404, "Not found")) + + assert await adapter.fetch_message(_SPACE_THREAD, "spaces/ABC123/messages/gone") is None + + @pytest.mark.asyncio + async def test_reads_with_app_credentials_even_when_impersonation_is_configured(self): + # Parity with upstream fetchMessage, which calls this.chatApi (app + # auth) rather than impersonatedChatApi, unlike fetchMessages. + adapter, api, _ = await _init_adapter(impersonate_user="admin@example.com") + api.set_response( + "GET", + "spaces/ABC123/messages/msg1", + { + "name": "spaces/ABC123/messages/msg1", + "text": "hello", + "sender": {"name": "users/1", "displayName": "Alice", "type": "HUMAN"}, + "createTime": "2024-01-01T00:00:00Z", + }, + ) + + await adapter.fetch_message(_SPACE_THREAD, "spaces/ABC123/messages/msg1") + + calls = api.get_calls("GET", "spaces/ABC123/messages/msg1") + assert [c["use_impersonation"] for c in calls] == [False] + + @pytest.mark.asyncio + async def test_rejects_a_message_from_another_space_without_calling_the_api(self): + adapter, api, _ = await _init_adapter() + with pytest.raises(ValidationError, match=_FOREIGN_SPACE): + await adapter.fetch_message(_SPACE_THREAD, _FOREIGN_MESSAGE) + assert api.calls == [] + + @pytest.mark.asyncio + async def test_other_api_errors_go_through_error_handling(self): + adapter, api, _ = await _init_adapter() + api.set_response("GET", "spaces/ABC123/messages/msg1", _FakeApiError(429, "Rate limited")) + with pytest.raises(AdapterRateLimitError): + await adapter.fetch_message(_SPACE_THREAD, "spaces/ABC123/messages/msg1") + + api.set_response("GET", "spaces/ABC123/messages/msg1", _FakeApiError(403, "Forbidden")) + with pytest.raises(_FakeApiError, match="Forbidden"): + await adapter.fetch_message(_SPACE_THREAD, "spaces/ABC123/messages/msg1") + + # ============================================================================= # addReaction / removeReaction Tests # ============================================================================= @@ -949,132 +1142,76 @@ async def wait_and_set(): # ============================================================================= -# Bot user ID learning from annotations Tests +# Bot user ID: explicit configuration only (upstream f485255b) # ============================================================================= -class TestBotUserIdLearning: - def test_learns_bot_id_from_annotations(self): - adapter = _make_adapter() - assert adapter.bot_user_id is None - - event = { - "chat": { - "messagePayload": { - "space": {"name": "spaces/ABC123", "type": "ROOM"}, - "message": { - "name": "spaces/ABC123/messages/msg1", - "sender": {"name": "users/100", "displayName": "User", "type": "HUMAN"}, - "text": "@BotName hi", - "createTime": "2024-01-01T00:00:00Z", - "annotations": [ - { - "type": "USER_MENTION", - "startIndex": 0, - "length": 8, - "userMention": { - "user": { - "name": "users/LEARNED_BOT_ID", - "displayName": "BotName", - "type": "BOT", - }, - "type": "MENTION", - }, - } - ], - }, +def _mention_event(bot_name: str, display_name: str, text: str) -> dict[str, Any]: + return { + "chat": { + "messagePayload": { + "space": {"name": "spaces/ABC123", "type": "ROOM"}, + "message": { + "name": "spaces/ABC123/messages/msg1", + "sender": {"name": "users/100", "displayName": "User", "type": "HUMAN"}, + "text": text, + "createTime": "2024-01-01T00:00:00Z", + "annotations": [ + { + "type": "USER_MENTION", + "startIndex": 0, + "length": len(display_name) + 1, + "userMention": { + "user": {"name": bot_name, "displayName": display_name, "type": "BOT"}, + "type": "MENTION", + }, + } + ], }, }, - } - adapter.parse_message(event) - assert adapter.bot_user_id == "users/LEARNED_BOT_ID" + }, + } - def test_does_not_overwrite_existing_bot_id(self): - adapter = _make_adapter() - adapter._bot_user_id = "users/FIRST_BOT" - event = { - "chat": { - "messagePayload": { - "space": {"name": "spaces/ABC123", "type": "ROOM"}, - "message": { - "name": "spaces/ABC123/messages/msg1", - "sender": {"name": "users/100", "displayName": "User", "type": "HUMAN"}, - "text": "@AnotherBot hi", - "createTime": "2024-01-01T00:00:00Z", - "annotations": [ - { - "type": "USER_MENTION", - "startIndex": 0, - "length": 11, - "userMention": { - "user": { - "name": "users/SECOND_BOT", - "displayName": "AnotherBot", - "type": "BOT", - }, - "type": "MENTION", - }, - } - ], - }, - }, - }, - } - adapter.parse_message(event) - assert adapter.bot_user_id == "users/FIRST_BOT" +class TestBotUserIdIsNotLearned: + @pytest.mark.asyncio + async def test_mention_does_not_write_bot_id_to_state(self): + # Earlier releases persisted the first mentioned BOT as + # ``gchat:botUserId`` via a fire-and-forget task. Nothing may be + # written now, even after pending tasks get a chance to run. + adapter, _, state = await _init_adapter() - def test_persists_bot_id_after_learning(self): - """The bot user ID should be available for self-detection after learning.""" - adapter = _make_adapter() + adapter.parse_message(_mention_event("users/LEARNED_BOT_ID", "BotName", "@BotName hi")) + await asyncio.sleep(0) - # First message: learn bot ID - event = { - "chat": { - "messagePayload": { - "space": {"name": "spaces/ABC123", "type": "ROOM"}, - "message": { - "name": "spaces/ABC123/messages/msg1", - "sender": {"name": "users/100", "displayName": "User", "type": "HUMAN"}, - "text": "@MyBot hello", - "createTime": "2024-01-01T00:00:00Z", - "annotations": [ - { - "type": "USER_MENTION", - "startIndex": 0, - "length": 6, - "userMention": { - "user": { - "name": "users/MY_BOT_ID", - "displayName": "MyBot", - "type": "BOT", - }, - "type": "MENTION", - }, - } - ], - }, - }, - }, - } - adapter.parse_message(event) + assert adapter.bot_user_id is None + assert "gchat:botUserId" not in state._storage + # Only the sender's display name is cached; no identity key is set. + assert [c.args[0] for c in state.set.await_args_list] == ["gchat:user:users/100"] + + def test_mentioned_bot_does_not_become_self(self): + # The takeover this closes: another bot mentioned first used to become + # "self", so its messages were dropped as the app's own. + adapter = _make_adapter(bot_user_id="users/OUR_BOT") + + adapter.parse_message(_mention_event("users/OTHER_BOT", "OtherBot", "@OtherBot hello")) - # Second message: detect self - self_event = { + other_bot_event = { "chat": { "messagePayload": { "space": {"name": "spaces/ABC123", "type": "ROOM"}, "message": { "name": "spaces/ABC123/messages/msg2", - "sender": {"name": "users/MY_BOT_ID", "displayName": "MyBot", "type": "BOT"}, + "sender": {"name": "users/OTHER_BOT", "displayName": "OtherBot", "type": "BOT"}, "text": "Hello back", "createTime": "2024-01-01T00:00:01Z", }, }, }, } - msg = adapter.parse_message(self_event) - assert msg.author.is_me is True + msg = adapter.parse_message(other_bot_event) + assert adapter.bot_user_id == "users/OUR_BOT" + assert msg.author.is_me is False # ============================================================================= diff --git a/tests/test_gchat_comprehensive.py b/tests/test_gchat_comprehensive.py index 7d18d8ac..85913507 100644 --- a/tests/test_gchat_comprehensive.py +++ b/tests/test_gchat_comprehensive.py @@ -350,6 +350,29 @@ def test_resolves_impersonate_user_from_env_var(self): self._clear_gchat_env() os.environ.update(saved) + def test_resolves_bot_user_id_from_env_var(self, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setenv("GOOGLE_CHAT_BOT_USER_ID", "users/BOT_ENV") + adapter = _make_adapter() + assert adapter.bot_user_id == "users/BOT_ENV" + + def test_configured_bot_user_id_takes_priority_over_env_var(self, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setenv("GOOGLE_CHAT_BOT_USER_ID", "users/BOT_ENV") + adapter = _make_adapter(bot_user_id="users/BOT_CONFIG") + assert adapter.bot_user_id == "users/BOT_CONFIG" + + def test_empty_configured_bot_user_id_does_not_fall_back_to_env_var(self, monkeypatch: pytest.MonkeyPatch): + # `is not None` precedence (upstream `??`): an explicit "" is a + # configured value, so the env var is not consulted and the id + # stays unset. `config.bot_user_id or env` would pick up the env. + monkeypatch.setenv("GOOGLE_CHAT_BOT_USER_ID", "users/BOT_ENV") + adapter = _make_adapter(bot_user_id="") + assert adapter.bot_user_id is None + + def test_empty_bot_user_id_env_var_is_treated_as_unset(self, monkeypatch: pytest.MonkeyPatch): + monkeypatch.setenv("GOOGLE_CHAT_BOT_USER_ID", "") + adapter = _make_adapter() + assert adapter.bot_user_id is None + def test_config_credentials_take_priority_over_env_vars(self): saved = {k: v for k, v in os.environ.items() if k.startswith("GOOGLE_CHAT_")} try: @@ -389,22 +412,23 @@ def test_default_user_name_is_bot(self): # =========================================================================== -class TestInitializeRestoreBotUserId: +class TestInitializeBotUserId: @pytest.mark.asyncio - async def test_restore_bot_user_id_from_state(self): - adapter = _make_adapter() + async def test_uses_configured_bot_user_id_instead_of_persisted_state(self): + adapter = _make_adapter(bot_user_id="users/BOT_CONFIGURED") state = _make_mock_state() state._storage["gchat:botUserId"] = "users/BOT999" chat = _make_mock_chat(state) await adapter.initialize(chat) - assert adapter.bot_user_id == "users/BOT999" + assert adapter.bot_user_id == "users/BOT_CONFIGURED" @pytest.mark.asyncio - async def test_does_not_overwrite_existing_bot_user_id(self): + async def test_does_not_restore_untrusted_learned_bot_id_from_state(self): + # Upstream f485255b: the learned id persisted by earlier releases may + # belong to another bot, so the legacy state key is never read. adapter = _make_adapter() - adapter._bot_user_id = "users/EXISTING" state = _make_mock_state() state._storage["gchat:botUserId"] = "users/OTHERFROMSTATE" @@ -412,7 +436,30 @@ async def test_does_not_overwrite_existing_bot_user_id(self): await adapter.initialize(chat) - assert adapter.bot_user_id == "users/EXISTING" + assert adapter.bot_user_id is None + assert all(c.args[0] != "gchat:botUserId" for c in state.get.await_args_list) + + @pytest.mark.asyncio + async def test_warns_once_when_bot_user_id_is_unset(self): + logger = MagicMock() + adapter = _make_adapter(logger=logger) + chat = _make_mock_chat(_make_mock_state()) + + await adapter.initialize(chat) + await adapter.initialize(chat) + + warnings = [c.args[0] for c in logger.warn.call_args_list if "botUserId is not configured" in c.args[0]] + assert len(warnings) == 1 + assert "GOOGLE_CHAT_BOT_USER_ID" in warnings[0] + + @pytest.mark.asyncio + async def test_no_warning_when_bot_user_id_is_configured(self): + logger = MagicMock() + adapter = _make_adapter(logger=logger, bot_user_id="users/BOT123") + + await adapter.initialize(_make_mock_chat(_make_mock_state())) + + assert not any("botUserId is not configured" in c.args[0] for c in logger.warn.call_args_list) # =========================================================================== @@ -433,10 +480,10 @@ def test_no_fetch_data_when_neither_download_uri_nor_resource_name(self): ], ) msg = adapter.parse_message(event) - assert len(msg.attachments) >= 1 - # URL should be None or empty if no downloadUri + assert len(msg.attachments) == 1 att = msg.attachments[0] - assert att.url is None or att.url == "" + assert att.fetch_data is None + assert att.url is None def test_file_type_attachment_classified(self): adapter = _make_adapter() @@ -546,7 +593,7 @@ def test_message_with_non_bot_mention_annotation(self): assert "Hello" in msg.text or "hello" in msg.text def test_multiple_bot_mentions_replaced(self): - adapter = _make_adapter(user_name="mybot") + adapter = _make_adapter(bot_user_id="users/BOT1", user_name="mybot") event = _make_message_event( message_text="@Bot hi @Bot bye", annotations=[ @@ -579,8 +626,26 @@ def test_multiple_bot_mentions_replaced(self): ], ) msg = adapter.parse_message(event) - assert "@mybot" in msg.text - assert "@Bot" not in msg.text + assert msg.text == "@mybot hi @mybot bye" + + def test_nameless_bot_annotation_not_rewritten_when_bot_user_id_unset(self): + # Divergence from upstream -- see docs/UPSTREAM_SYNC.md (bot identity + # edge cases). Upstream compares ``undefined !== undefined`` and would + # rewrite this annotation; with no configured id nothing is rewritten. + adapter = _make_adapter(user_name="mybot") + event = _make_message_event( + message_text="@Bot hi", + annotations=[ + { + "type": "USER_MENTION", + "startIndex": 0, + "length": 4, + "userMention": {"user": {"displayName": "Bot", "type": "BOT"}, "type": "MENTION"}, + }, + ], + ) + msg = adapter.parse_message(event) + assert msg.text == "@Bot hi" # =========================================================================== @@ -590,8 +655,7 @@ def test_multiple_bot_mentions_replaced(self): class TestIsMessageFromSelfByDisplayName: def test_detects_self_by_user_id_match(self): - adapter = _make_adapter() - adapter._bot_user_id = "users/BOT123" + adapter = _make_adapter(bot_user_id="users/BOT123") event = _make_message_event( sender_name="users/BOT123", sender_type="BOT", @@ -600,23 +664,19 @@ def test_detects_self_by_user_id_match(self): msg = adapter.parse_message(event) assert msg.author.is_me is True - def test_detects_self_by_user_name_fallback(self): - """When botUserId is not set, use displayName matching with user_name.""" + def test_unknown_bot_user_id_does_not_mark_human_senders_as_self(self): + """The fail-closed rule for an unset bot_user_id covers BOT senders only.""" adapter = _make_adapter(user_name="MyBot") - # No bot_user_id set event = _make_message_event( - sender_name="users/UNKNOWN_BOT", - sender_type="BOT", + sender_name="users/HUMAN1", + sender_type="HUMAN", sender_display_name="MyBot", ) msg = adapter.parse_message(event) - # Depending on implementation, may or may not match by display name - # Just assert it doesn't crash - assert isinstance(msg.author.is_me, bool) + assert msg.author.is_me is False def test_other_bot_not_detected_as_self(self): - adapter = _make_adapter() - adapter._bot_user_id = "users/BOT123" + adapter = _make_adapter(bot_user_id="users/BOT123") event = _make_message_event( sender_name="users/OTHER_BOT", sender_type="BOT", diff --git a/tests/test_gchat_webhook.py b/tests/test_gchat_webhook.py index a03e8dc7..08db5ec4 100644 --- a/tests/test_gchat_webhook.py +++ b/tests/test_gchat_webhook.py @@ -299,7 +299,7 @@ def test_video_and_audio_attachment_types(self): class TestBotMentions: def test_replace_bot_mention_with_user_name(self): - adapter = _make_adapter(user_name="mybot") + adapter = _make_adapter(bot_user_id="users/BOT123", user_name="mybot") event = _make_message_event( message_text="@Chat SDK Demo hello", annotations=[ @@ -322,7 +322,10 @@ def test_replace_bot_mention_with_user_name(self): assert "@mybot" in msg.text assert "@Chat SDK Demo" not in msg.text - def test_learn_bot_user_id_from_annotations(self): + def test_does_not_learn_bot_user_id_from_inbound_annotations(self): + # Upstream f485255b: the identity is never learned from a mention (a + # learned id let another bot become "self"), and with no configured + # id no mention is normalized. adapter = _make_adapter() assert adapter.bot_user_id is None @@ -344,12 +347,12 @@ def test_learn_bot_user_id_from_annotations(self): } ], ) - adapter.parse_message(event) - assert adapter.bot_user_id == "users/LEARNED_BOT_ID" + msg = adapter.parse_message(event) + assert adapter.bot_user_id is None + assert msg.text == "@BotName hi" - def test_does_not_overwrite_bot_user_id(self): - adapter = _make_adapter() - adapter._bot_user_id = "users/FIRST_BOT" + def test_ignores_mentions_of_other_bots(self): + adapter = _make_adapter(bot_user_id="users/FIRST_BOT") event = _make_message_event( message_text="@AnotherBot hi", @@ -369,8 +372,39 @@ def test_does_not_overwrite_bot_user_id(self): } ], ) - adapter.parse_message(event) + msg = adapter.parse_message(event) assert adapter.bot_user_id == "users/FIRST_BOT" + assert msg.text == "@AnotherBot hi" + + def test_normalizes_only_this_app_when_another_bot_is_mentioned_first(self): + adapter = _make_adapter(bot_user_id="users/OUR_BOT", user_name="ourbot") + + event = _make_message_event( + message_text="@OtherBot @OurBot hi", + annotations=[ + { + "type": "USER_MENTION", + "startIndex": 0, + "length": 9, + "userMention": { + "user": {"name": "users/OTHER_BOT", "displayName": "OtherBot", "type": "BOT"}, + "type": "MENTION", + }, + }, + { + "type": "USER_MENTION", + "startIndex": 10, + "length": 7, + "userMention": { + "user": {"name": "users/OUR_BOT", "displayName": "OurBot", "type": "BOT"}, + "type": "MENTION", + }, + }, + ], + ) + msg = adapter.parse_message(event) + assert msg.text == "@OtherBot @ourbot hi" + assert adapter.bot_user_id == "users/OUR_BOT" # --------------------------------------------------------------------------- @@ -380,8 +414,7 @@ def test_does_not_overwrite_bot_user_id(self): class TestIsMessageFromSelf: def test_detects_self_message(self): - adapter = _make_adapter() - adapter._bot_user_id = "users/BOT123" + adapter = _make_adapter(bot_user_id="users/BOT123") event = _make_message_event( sender_name="users/BOT123", sender_type="BOT", @@ -391,8 +424,7 @@ def test_detects_self_message(self): assert msg.author.is_me is True def test_other_bot_not_self(self): - adapter = _make_adapter() - adapter._bot_user_id = "users/BOT123" + adapter = _make_adapter(bot_user_id="users/BOT123") event = _make_message_event( sender_name="users/OTHER_BOT", sender_type="BOT", @@ -401,14 +433,16 @@ def test_other_bot_not_self(self): msg = adapter.parse_message(event) assert msg.author.is_me is False - def test_unknown_bot_id_returns_false(self): + def test_fails_closed_for_bot_senders_when_bot_user_id_is_unknown(self): + # Upstream f485255b: without a configured id every BOT sender counts + # as self, so the app can never reply to its own messages in a loop. adapter = _make_adapter() event = _make_message_event( sender_type="BOT", sender_display_name="SomeBot", ) msg = adapter.parse_message(event) - assert msg.author.is_me is False + assert msg.author.is_me is True # --------------------------------------------------------------------------- diff --git a/tests/test_google_chat_adapter.py b/tests/test_google_chat_adapter.py index cc862ccb..84290e23 100644 --- a/tests/test_google_chat_adapter.py +++ b/tests/test_google_chat_adapter.py @@ -8,16 +8,19 @@ from chat_sdk.adapters.google_chat.adapter import GoogleChatAdapter from chat_sdk.adapters.google_chat.thread_utils import ( + GoogleChatMessageName, GoogleChatThreadId, decode_thread_id, encode_thread_id, is_dm_thread, + parse_message_name, ) from chat_sdk.adapters.google_chat.types import ( GoogleChatAdapterConfig, ServiceAccountCredentials, ) -from chat_sdk.shared.errors import ValidationError +from chat_sdk.shared.errors import AdapterRateLimitError, ValidationError +from chat_sdk.types import Attachment # --------------------------------------------------------------------------- # Helpers @@ -168,6 +171,69 @@ def test_thread_with_encoded_data(self): assert is_dm_thread(tid) is False +# --------------------------------------------------------------------------- +# parse_message_name (upstream thread-utils.test.ts, d6343460) +# --------------------------------------------------------------------------- + + +class TestParseMessageName: + def test_parses_a_server_assigned_message_name(self): + assert parse_message_name("spaces/AAQAJ9CXYcg/messages/FGEOaAwNIcs.FGEOaAwNIcs") == GoogleChatMessageName( + space_name="spaces/AAQAJ9CXYcg", + message_id="FGEOaAwNIcs.FGEOaAwNIcs", + ) + + def test_parses_a_client_assigned_message_name(self): + space_name, message_id = parse_message_name("spaces/ABC_1-2/messages/client-my_id-3") + assert space_name == "spaces/ABC_1-2" + assert message_id == "client-my_id-3" + + @pytest.mark.parametrize( + "name", + [ + "spaces/ABC/messages/../../OTHER/messages/x", + "spaces/ABC/messages/./x", + "spaces/ABC/messages/x/", + "spaces/ABC/messages/", + "spaces/ABC/messages/x?y=1", + "spaces/ABC/messages/x#y", + "spaces/ABC/messages/%2e%2e", + "spaces/ABC/messages/a..b", + "spaces//messages/x", + "spaces/ABC/threads/x", + "/spaces/ABC/messages/x", + "https://chat.googleapis.com/v1/spaces/ABC/messages/x", + "x", + "", + ], + ) + def test_rejects(self, name: str): + with pytest.raises(ValidationError, match="Invalid Google Chat message id"): + parse_message_name(name) + + # Python-only sweep: ``re.match`` + ``$`` would accept a trailing newline, + # and ``\w`` / ``\d`` would accept non-ASCII letters and digits. + @pytest.mark.parametrize( + "name", + [ + "spaces/ABC/messages/x\n", + "spaces/ABC/messages/x\r\n", + "spaces/ABC/messages/\uff58", + "spaces/ABC/messages/x\u0661", + "spaces/ABC\n/messages/x", + "spaces/ABC/messages/.x", + "spaces/ABC/messages/x.", + ], + ) + def test_rejects_non_ascii_and_trailing_newlines(self, name: str): + with pytest.raises(ValidationError, match="Invalid Google Chat message id"): + parse_message_name(name) + + def test_rejects_a_non_string_id(self): + with pytest.raises(ValidationError, match="Invalid Google Chat message id"): + parse_message_name(None) + + # --------------------------------------------------------------------------- # create_google_chat_adapter factory # --------------------------------------------------------------------------- @@ -216,6 +282,179 @@ def test_custom_user_name(self): assert adapter.user_name == "mybot" +# --------------------------------------------------------------------------- +# Media download (upstream 32687038) +# --------------------------------------------------------------------------- + + +class _FakeResponse: + def __init__(self, status: int, body: bytes) -> None: + self.status = status + self._body = body + + async def read(self) -> bytes: + return self._body + + async def __aenter__(self) -> _FakeResponse: + return self + + async def __aexit__(self, *exc: object) -> None: + return None + + +class _FakeSession: + """Records ``session.get`` calls made by the media download closure.""" + + def __init__(self, status: int = 200, body: bytes = b"\x89PNG") -> None: + self.status = status + self.body = body + self.calls: list[tuple[str, dict[str, str]]] = [] + self.closed = False + + def get(self, url: str, *, headers: dict[str, str]) -> _FakeResponse: + self.calls.append((url, headers)) + return _FakeResponse(self.status, self.body) + + +def _media_adapter(status: int = 200, body: bytes = b"\x89PNG") -> tuple[GoogleChatAdapter, _FakeSession]: + from unittest.mock import AsyncMock + + adapter = _make_adapter() + session = _FakeSession(status, body) + adapter._get_access_token = AsyncMock(return_value="test-token") # type: ignore[method-assign] + adapter._get_http_session = AsyncMock(return_value=session) # type: ignore[method-assign] + return adapter, session + + +def _attachment_event(attachment: dict) -> dict: + return { + "chat": { + "messagePayload": { + "space": {"name": "spaces/ABC123", "type": "ROOM"}, + "message": { + "name": "spaces/ABC123/messages/msg1", + "sender": {"name": "users/100", "displayName": "User", "type": "HUMAN"}, + "text": "file", + "createTime": "2024-01-01T00:00:00Z", + "attachment": [attachment], + }, + }, + }, + } + + +class TestMediaDownload: + """Attachment bytes come only from the media API (upstream 32687038).""" + + @pytest.mark.asyncio + async def test_uses_media_download_api_when_attachment_data_ref_is_present(self): + adapter, session = _media_adapter() + msg = adapter.parse_message( + _attachment_event( + { + "name": "att1", + "contentName": "photo.png", + "contentType": "image/png", + "downloadUri": "https://example.com/photo.png", + "attachmentDataRef": {"resourceName": "spaces/ABC123/attachments/att1"}, + } + ) + ) + att = msg.attachments[0] + assert att.fetch_data is not None + assert att.url == "https://example.com/photo.png" + + assert await att.fetch_data() == b"\x89PNG" + assert session.calls == [ + ( + "https://chat.googleapis.com/v1/media/spaces/ABC123/attachments/att1?alt=media", + {"Authorization": "Bearer test-token"}, + ) + ] + + @pytest.mark.asyncio + async def test_provides_fetch_data_when_only_attachment_data_ref_is_present(self): + adapter, session = _media_adapter() + msg = adapter.parse_message( + _attachment_event( + { + "name": "att1", + "contentName": "photo.png", + "contentType": "image/png", + "attachmentDataRef": {"resourceName": "spaces/ABC123/attachments/att1"}, + } + ) + ) + att = msg.attachments[0] + assert att.fetch_data is not None + assert att.url is None + assert await att.fetch_data() == b"\x89PNG" + assert [url for url, _ in session.calls] == [ + "https://chat.googleapis.com/v1/media/spaces/ABC123/attachments/att1?alt=media" + ] + + @pytest.mark.asyncio + async def test_does_not_fetch_download_uri_when_media_download_fails(self): + from chat_sdk.adapters.google_chat.adapter import _GoogleApiError + + adapter, session = _media_adapter(status=403) + msg = adapter.parse_message( + _attachment_event( + { + "name": "att1", + "contentName": "photo.png", + "contentType": "image/png", + "downloadUri": "https://example.com/photo.png", + "attachmentDataRef": {"resourceName": "spaces/ABC123/attachments/att1"}, + } + ) + ) + att = msg.attachments[0] + assert att.fetch_data is not None + with pytest.raises(_GoogleApiError) as exc_info: + await att.fetch_data() + assert exc_info.value.code == 403 + # Exactly one request, to the media API -- never the downloadUri. + assert [url for url, _ in session.calls] == [ + "https://chat.googleapis.com/v1/media/spaces/ABC123/attachments/att1?alt=media" + ] + + @pytest.mark.asyncio + async def test_raises_adapter_rate_limit_error_when_media_download_returns_429(self): + adapter, _ = _media_adapter(status=429) + msg = adapter.parse_message( + _attachment_event( + { + "name": "att1", + "contentName": "photo.png", + "contentType": "image/png", + "attachmentDataRef": {"resourceName": "spaces/ABC123/attachments/att1"}, + } + ) + ) + att = msg.attachments[0] + assert att.fetch_data is not None + with pytest.raises(AdapterRateLimitError): + await att.fetch_data() + + def test_does_not_provide_fetch_data_when_only_download_uri_is_present(self): + adapter = _make_adapter() + msg = adapter.parse_message( + _attachment_event( + { + "name": "att1", + "contentName": "photo.png", + "contentType": "image/png", + "downloadUri": "https://example.com/photo.png", + } + ) + ) + att = msg.attachments[0] + assert att.fetch_data is None + assert att.url == "https://example.com/photo.png" + assert att.fetch_metadata == {"url": "https://example.com/photo.png"} + + # --------------------------------------------------------------------------- # rehydrate_attachment # --------------------------------------------------------------------------- @@ -224,75 +463,100 @@ def test_custom_user_name(self): class TestRehydrateAttachment: """Cover ``GoogleChatAdapter.rehydrate_attachment``.""" - def test_rehydrates_from_resource_name(self): - from chat_sdk.types import Attachment - - adapter = _make_adapter() + @pytest.mark.asyncio + async def test_rehydrates_from_resource_name(self): + adapter, session = _media_adapter() attachment = Attachment( type="image", + url="https://example.com/display.png", fetch_metadata={"resourceName": "spaces/ABC/messages/X/attachments/Y"}, ) rehydrated = adapter.rehydrate_attachment(attachment) assert rehydrated.fetch_data is not None + assert rehydrated.url == "https://example.com/display.png" assert rehydrated.fetch_metadata == { "resourceName": "spaces/ABC/messages/X/attachments/Y", } + assert await rehydrated.fetch_data() == b"\x89PNG" + assert [url for url, _ in session.calls] == [ + "https://chat.googleapis.com/v1/media/spaces/ABC/messages/X/attachments/Y?alt=media" + ] - def test_rehydrates_from_url_when_no_resource_name(self): - from chat_sdk.types import Attachment - + def test_does_not_rehydrate_fetch_data_from_a_download_url(self): adapter = _make_adapter() attachment = Attachment( type="file", - url="https://chat.googleapis.com/v1/media/X?alt=media", - fetch_metadata={"url": "https://chat.googleapis.com/v1/media/X?alt=media"}, + url="https://example.com/document.pdf", + fetch_metadata={"url": "https://example.com/document.pdf"}, ) rehydrated = adapter.rehydrate_attachment(attachment) - assert rehydrated.fetch_data is not None + assert rehydrated is attachment + assert rehydrated.fetch_data is None def test_returns_unchanged_when_no_metadata(self): - from chat_sdk.types import Attachment - adapter = _make_adapter() attachment = Attachment(type="file", name="local.bin") rehydrated = adapter.rehydrate_attachment(attachment) assert rehydrated is attachment - # Python-first divergence: SSRF guard on the downloadUri fallback path. - # The resource_name branch stays trusted (the URL is constructed by - # `_build_gchat_fetch_data` from a validated ``spaces/.../messages/...`` - # identifier, not from an attacker-controllable string). The `url` - # branch is the one that accepts serialized fetch_metadata. + # Divergence from upstream -- see docs/UPSTREAM_SYNC.md (media + # resourceName validation). ``resourceName`` is interpolated into the + # request path and can come from serialized ``fetch_metadata``, so shapes + # that would change what ``/v1/media/{resourceName}`` addresses are + # rejected before a token is minted or a request is made. @pytest.mark.asyncio - async def test_rehydrated_fetch_data_rejects_untrusted_url(self): - from unittest.mock import AsyncMock - - from chat_sdk.types import Attachment + @pytest.mark.parametrize( + "resource_name", + [ + "../../spaces/X/messages/Y", + "spaces/ABC/attachments/../../../spaces/X", + "spaces/ABC/./attachments/Y", + "..", + ".", + "spaces/ABC/attachments/Y?alt=json", + "spaces/ABC/attachments/Y#frag", + "spaces/ABC/attachments/%2e%2e", + "spaces/ABC/attachments/Y Z", + "spaces/ABC/attachments/Y\n", + "spaces/ABC/attachments/Y\t", + "spaces/ABC/attachments/Y\x00", + "spaces/ABC/attachments/Y\x85", + "spaces\\..\\x", + "spaces/ABC/attachments/\uff0e\uff0e", + ], + ) + async def test_rehydrated_fetch_data_rejects_unsafe_resource_names(self, resource_name: str): + adapter, session = _media_adapter() + attachment = Attachment(type="image", fetch_metadata={"resourceName": resource_name}) + rehydrated = adapter.rehydrate_attachment(attachment) + assert rehydrated.fetch_data is not None + with pytest.raises(ValidationError, match="Invalid Google Chat attachment resource name"): + await rehydrated.fetch_data() + adapter._get_access_token.assert_not_awaited() # type: ignore[attr-defined] + adapter._get_http_session.assert_not_awaited() # type: ignore[attr-defined] + assert session.calls == [] - adapter = _make_adapter() - # These must never be awaited — validation rejects first. - adapter._get_access_token = AsyncMock() # type: ignore[method-assign] - adapter._get_http_session = AsyncMock() # type: ignore[method-assign] + @pytest.mark.asyncio + async def test_rehydrated_fetch_data_rejects_a_non_string_resource_name(self): + adapter, session = _media_adapter() + rehydrated = adapter.rehydrate_attachment(Attachment(type="file", fetch_metadata={"resourceName": 42})) + assert rehydrated.fetch_data is not None + with pytest.raises(ValidationError, match="Invalid Google Chat attachment resource name"): + await rehydrated.fetch_data() + assert session.calls == [] + @pytest.mark.asyncio + async def test_accepts_opaque_resource_names(self): + # resourceName is opaque (it may be a base64-style token), so only the + # denylisted shapes are rejected. + adapter, session = _media_adapter() attachment = Attachment( type="image", - url="https://attacker.example.com/pwn", - fetch_metadata={"url": "https://attacker.example.com/pwn"}, + fetch_metadata={"resourceName": "ClxjaGF0LmNvbS9+abc_DEF-123=/x.y"}, ) rehydrated = adapter.rehydrate_attachment(attachment) assert rehydrated.fetch_data is not None - with pytest.raises(ValidationError): - await rehydrated.fetch_data() - adapter._get_access_token.assert_not_awaited() - adapter._get_http_session.assert_not_awaited() - - def test_is_trusted_gchat_download_url_allowlist(self): - assert GoogleChatAdapter._is_trusted_gchat_download_url("https://chat.googleapis.com/v1/media/X?alt=media") - assert GoogleChatAdapter._is_trusted_gchat_download_url("https://lh3.googleusercontent.com/photo.jpg") - assert GoogleChatAdapter._is_trusted_gchat_download_url("https://foo.google.com/x") - # Rejects non-HTTPS - assert not GoogleChatAdapter._is_trusted_gchat_download_url("http://chat.googleapis.com/x") - # Rejects arbitrary hosts - assert not GoogleChatAdapter._is_trusted_gchat_download_url("https://attacker.example/x") - # Rejects look-alikes - assert not GoogleChatAdapter._is_trusted_gchat_download_url("https://chat.googleapis.com.attacker.tld/x") + assert await rehydrated.fetch_data() == b"\x89PNG" + assert [url for url, _ in session.calls] == [ + "https://chat.googleapis.com/v1/media/ClxjaGF0LmNvbS9+abc_DEF-123=/x.y?alt=media" + ]