fix(teams): fall back to content.downloadUrl for SharePoint/OneDrive attachments (supersedes #136)

docs/UPSTREAM_SYNC.md

@@ -663,6 +663,7 @@ stay explicit instead of being rediscovered in code review.
 | 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) | 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 | No validation — `fetchData` blindly GETs `fetchMetadata.url` (Twilio: `fetchMetadata.twilioMediaUrl`) and forwards the workspace/bot token (Twilio: the account SID + auth token as HTTP Basic) | 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. The Twilio adapter (vercel/chat#558) shares the exact pattern — `fetchTwilioMedia` GETs the rehydrated URL with the adapter's Basic auth and no host check. 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}`. Regression coverage: `tests/test_twilio_adapter.py::TestRehydrateAttachment::test_media_downloader_refuses_untrusted_hosts`. |
+| 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. |
 | Slack Socket Mode reconnect loop | Outer reconnect loop on top of `slack_sdk.socket_mode.aiohttp.SocketModeClient` (which itself has `auto_reconnect_enabled=True`). Exponential backoff (1s → 30s) with explicit shutdown signaling and a tracked `asyncio.Task` so `disconnect()` can cancel cleanly | Single `SocketModeClient` instance from `@slack/socket-mode`; relies entirely on the package's internal reconnect | Hazard #5 (async task lifecycle): a long-lived WebSocket needs an explicit shutdown path so `disconnect()` doesn't leak the loop, and a guarded outer reconnect path so the adapter survives `connect()` itself raising (which the inner client doesn't retry). Inner auto-reconnect still runs; the outer loop is belt-and-suspenders, not a divergence in observable behavior. |
 | Slack Socket Mode listener serverless variant | Not ported | `startSocketModeListener()` / `runSocketModeListener()` open a transient socket for `durationMs` and forward events via HTTP POST | Vercel-specific pattern (cron-triggered ephemeral listener with `waitUntil`). The forwarded-event receiver (`x-slack-socket-token` handling in `handle_webhook`) is ported so a separate Python process can run the long-lived listener; the deployment glue itself isn't part of the SDK. |

src/chat_sdk/adapters/teams/adapter.py

@@ -1235,7 +1235,22 @@ def _create_attachment(self, att: dict[str, Any]) -> Attachment:
         elif content_type.startswith("audio/"):
             att_type = "audio"
 
-        url = att.get("contentUrl")
+        # Python-first divergence (upstream reads ``contentUrl`` only; see
+        # adapter-teams/src/index.ts:833 and docs/UPSTREAM_SYNC.md Known
+        # Non-Parity). A SharePoint/OneDrive file shared in a personal or group
+        # chat arrives as a ``application/vnd.microsoft.teams.file.download.info``
+        # attachment that carries BOTH a top-level ``contentUrl`` (pointing at the
+        # SharePoint/OneDrive item, which 403s on an anonymous GET) and a nested
+        # ``content.downloadUrl`` — a short-lived pre-signed link that fetches with
+        # no auth header. For that attachment type the top-level URL is unusable,
+        # so we prefer the pre-signed ``content.downloadUrl``. Every other
+        # attachment (inline images, etc.) keeps the upstream ``contentUrl`` path.
+        content = att.get("content")
+        download_url = content.get("downloadUrl") if isinstance(content, dict) else None
+        if content_type == "application/vnd.microsoft.teams.file.download.info" and download_url:
+            url = download_url
+        else:
+            url = att.get("contentUrl") or download_url
         return Attachment(
             type=att_type,
             url=url,

tests/test_teams_adapter.py

@@ -511,6 +511,166 @@ def test_is_trusted_teams_download_url_allowlist(self):
         assert not TeamsAdapter._is_trusted_teams_download_url("https://graph.microsoft.com.attacker.tld/x")
 
 
+# ---------------------------------------------------------------------------
+# file.download.info attachments (content.downloadUrl fallback)
+#
+# Python-first divergence (supersedes stale PR #136). A SharePoint/OneDrive
+# file shared in a personal/group chat arrives as a
+# ``application/vnd.microsoft.teams.file.download.info`` attachment whose
+# top-level ``contentUrl`` (the SharePoint item) 403s on an anonymous GET,
+# while the nested ``content.downloadUrl`` is a pre-signed link that works.
+# Upstream reads ``contentUrl`` only (adapter-teams/src/index.ts:833), so the
+# attachment is undownloadable. We prefer ``content.downloadUrl`` for this
+# attachment type. See docs/UPSTREAM_SYNC.md Known Non-Parity.
+# ---------------------------------------------------------------------------
+
+_FILE_DOWNLOAD_INFO = "application/vnd.microsoft.teams.file.download.info"
+
+
+def _file_download_activity(attachment: dict) -> dict:
+    return {
+        "type": "message",
+        "id": "msg-file-dl",
+        "text": "here is a file",
+        "from": {"id": "user-1", "name": "Alice"},
+        "conversation": {"id": "19:abc@thread.tacv2"},
+        "serviceUrl": "https://smba.trafficmanager.net/teams/",
+        "attachments": [attachment],
+    }
+
+
+class TestFileDownloadInfoAttachment:
+    def test_prefers_download_url_over_403_content_url(self):
+        """A file.download.info attachment carries BOTH a (403-ing) contentUrl
+        and a pre-signed content.downloadUrl; the Attachment must use the
+        downloadUrl, not the SharePoint contentUrl.
+
+        Pins the exact URL (not just "non-None") so a mutation that reverts to
+        ``contentUrl`` is caught: the SharePoint item URL would surface instead.
+        """
+        adapter = _make_adapter(app_id="test-app")
+        content_url = "https://contoso.sharepoint.com/personal/jadams/Documents/report.pdf"
+        download_url = "https://contoso.sharepoint.com/_layouts/download.aspx?presigned=abc123"
+        activity = _file_download_activity(
+            {
+                "contentType": _FILE_DOWNLOAD_INFO,
+                "contentUrl": content_url,
+                "name": "report.pdf",
+                "content": {
+                    "downloadUrl": download_url,
+                    "uniqueId": "1150D938-8870-4044-9F2C-5BBDEBA70C9D",
+                    "fileType": "pdf",
+                },
+            }
+        )
+        msg = adapter.parse_message(activity)
+        assert len(msg.attachments) == 1
+        att = msg.attachments[0]
+        assert att.url == download_url
+        assert att.url != content_url
+        # fetch_metadata must carry the SAME (working) URL so rehydrate rebuilds
+        # the closure around the pre-signed link, not the 403-ing one.
+        assert att.fetch_metadata == {"url": download_url}
+        assert att.name == "report.pdf"
+        assert att.type == "file"
+
+    def test_uses_download_url_when_content_url_absent(self):
+        """A file.download.info attachment with no top-level contentUrl still
+        yields a downloadable Attachment via content.downloadUrl."""
+        adapter = _make_adapter(app_id="test-app")
+        download_url = "https://contoso.sharepoint.com/_layouts/download.aspx?presigned=xyz"
+        activity = _file_download_activity(
+            {
+                "contentType": _FILE_DOWNLOAD_INFO,
+                "name": "notes.txt",
+                "content": {"downloadUrl": download_url, "fileType": "txt"},
+            }
+        )
+        msg = adapter.parse_message(activity)
+        assert len(msg.attachments) == 1
+        att = msg.attachments[0]
+        assert att.url == download_url
+        assert att.fetch_metadata == {"url": download_url}
+        assert att.fetch_data is not None
+
+    def test_regular_attachment_uses_content_url_unchanged(self):
+        """An ordinary (inline image) attachment keeps the upstream contentUrl
+        path — the downloadUrl fallback must not perturb it.
+
+        The attachment deliberately ALSO carries a ``content.downloadUrl`` so an
+        over-eager mutation that prefers ``content.downloadUrl`` for *every*
+        attachment type (rather than only ``file.download.info``) is caught: it
+        would surface the downloadUrl instead of the inline-image contentUrl.
+        """
+        adapter = _make_adapter(app_id="test-app")
+        content_url = "https://smba.trafficmanager.net/teams/v3/attachments/img/photo.png"
+        activity = _file_download_activity(
+            {
+                "contentType": "image/png",
+                "contentUrl": content_url,
+                "name": "photo.png",
+                # A bogus competing downloadUrl that must be IGNORED for a
+                # non-file.download.info attachment.
+                "content": {"downloadUrl": "https://attacker.example.com/wrong.png"},
+            }
+        )
+        msg = adapter.parse_message(activity)
+        assert len(msg.attachments) == 1
+        att = msg.attachments[0]
+        assert att.url == content_url
+        assert att.type == "image"
+        assert att.fetch_metadata == {"url": content_url}
+
+    @pytest.mark.asyncio
+    async def test_download_url_flows_through_trusted_fetch(self):
+        """The pre-signed downloadUrl becomes the URL the SSRF-gated fetch
+        closure GETs — proving the fallback URL is what actually gets fetched.
+
+        A SharePoint host is in the Teams allowlist, so the GET proceeds and
+        returns the stubbed bytes.
+        """
+        adapter = _make_adapter(app_id="test-app")
+        download_url = "https://contoso.sharepoint.com/_layouts/download.aspx?presigned=ok"
+        session = _MockAiohttpSession(payload=b"file-contents")
+        adapter._get_http_session = AsyncMock(return_value=session)  # type: ignore[method-assign]
+
+        activity = _file_download_activity(
+            {
+                "contentType": _FILE_DOWNLOAD_INFO,
+                "contentUrl": "https://contoso.sharepoint.com/personal/jadams/Documents/x.pdf",
+                "name": "x.pdf",
+                "content": {"downloadUrl": download_url, "fileType": "pdf"},
+            }
+        )
+        att = adapter.parse_message(activity).attachments[0]
+        assert att.fetch_data is not None
+        assert await att.fetch_data() == b"file-contents"
+        # The pre-signed URL — not the SharePoint item — is what gets fetched.
+        assert session.get_calls == [download_url]
+
+    @pytest.mark.asyncio
+    async def test_download_url_still_gated_by_ssrf_allowlist(self):
+        """Even via the downloadUrl path, an untrusted host fails closed at
+        fetch time — the fallback does NOT bypass the SSRF allowlist."""
+        adapter = _make_adapter(app_id="test-app")
+        # If the gate were bypassed, the session would be awaited — it must not.
+        adapter._get_http_session = AsyncMock()  # type: ignore[method-assign]
+
+        activity = _file_download_activity(
+            {
+                "contentType": _FILE_DOWNLOAD_INFO,
+                "name": "evil.bin",
+                "content": {"downloadUrl": "https://attacker.example.com/exfil.bin"},
+            }
+        )
+        att = adapter.parse_message(activity).attachments[0]
+        assert att.url == "https://attacker.example.com/exfil.bin"
+        assert att.fetch_data is not None
+        with pytest.raises(ValidationError):
+            await att.fetch_data()
+        adapter._get_http_session.assert_not_awaited()
+
+
 # ---------------------------------------------------------------------------
 # normalizeMentions (via parseMessage)
 # ---------------------------------------------------------------------------