fix(teams): fall back to content.downloadUrl for SharePoint/OneDrive attachments (supersedes #136)#167
Conversation
…attachments 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` (the SharePoint/OneDrive item, which returns 403 to an anonymous GET) and a nested `content.downloadUrl` — a short-lived pre-signed link the Bot Framework docs say to issue an HTTP GET against. Upstream Teams (`adapter-teams/src/index.ts:833`, `const url = att.contentUrl`) and our pre-fix adapter read `contentUrl` only, so the attachment is undownloadable (`fetch_data` 403s). `_create_attachment` now prefers `content.downloadUrl` for that attachment type; every other attachment (inline images, etc.) keeps the upstream `contentUrl`-first path. The resulting URL flows through the unchanged `_build_teams_fetch_data` / `rehydrate_attachment` SSRF allowlist — SharePoint/OneDrive download hosts are already covered by `*.sharepoint.com` / `*.onedrive.com`, so no host was added. Justified Python-only divergence (hard-UX-failure: attachment cannot be downloaded). Documented in docs/UPSTREAM_SYNC.md Known Non-Parity; to be filed upstream. Supersedes stale PR #136. Tests: TestFileDownloadInfoAttachment — downloadUrl preferred over a 403-ing contentUrl, downloadUrl used when contentUrl absent, regular attachment with a competing downloadUrl unchanged, downloadUrl flows through the trusted fetch, and the downloadUrl path is still gated by the SSRF allowlist. Each fails on a plausible mutation (verified against contentUrl-only revert, over-eager downloadUrl-for-all, and dropped-SSRF-gate mutations).
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthrough
ChangesTeams file.download.info URL selection
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Python-first divergence in the Teams adapter to prefer the pre-signed content.downloadUrl over the top-level contentUrl for application/vnd.microsoft.teams.file.download.info attachments, resolving a 403 error issue when downloading SharePoint/OneDrive files shared in chats. It also updates the upstream synchronization documentation and adds comprehensive unit tests. The reviewer suggests defensively validating that download_url and content_url are strings to prevent potential AttributeError exceptions when parsing malformed payloads, and using explicit is not None checks.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| 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 |
There was a problem hiding this comment.
To ensure robustness and prevent potential unhandled AttributeError exceptions, we should defensively verify that both download_url and content_url are strings. If a malformed payload contains a non-string value (such as a dictionary or list) for these fields, passing them to urlparse inside _is_trusted_teams_download_url will raise an AttributeError (e.g., 'dict' object has no attribute 'find'), which is not caught by the except (ValueError, TypeError): block. Additionally, when checking for these optional string values, use is not None instead of a truthiness check to avoid silently ignoring falsy but valid values (like empty strings).
content = att.get("content")
download_url = content.get("downloadUrl") if isinstance(content, dict) else None
if not isinstance(download_url, str):
download_url = None
content_url = att.get("contentUrl")
if not isinstance(content_url, str):
content_url = None
if content_type == "application/vnd.microsoft.teams.file.download.info" and download_url is not None:
url = download_url
else:
url = content_url if content_url is not None else download_urlReferences
- When checking for optional values that can be falsy but valid (e.g., 0, empty string, empty list), use
is not Noneinstead of a truthiness check to avoid silently ignoring them.
1 participant
Problem
A SharePoint/OneDrive file shared in a personal or group chat arrives as a
application/vnd.microsoft.teams.file.download.infoattachment that carries both:contentUrl— the SharePoint/OneDrive item (e.g.https://contoso.sharepoint.com/.../report.pdf), which returns 403 to an anonymous GET because it needs a SharePoint auth context, andcontent.downloadUrl— a short-lived pre-authenticated link the Bot Framework docs explicitly say to issue anHTTP GETagainst.Upstream Teams (
adapter-teams/src/index.ts:833,const url = att.contentUrl— itscreateAttachmentparam type doesn't even includecontent) and our pre-fix adapter readcontentUrlonly, so the attachment'sfetch_data403s and the file is undownloadable.Fix
_create_attachmentnow preferscontent.downloadUrlfor thefile.download.infoattachment type. Every other attachment (inline images, etc.) keeps the upstreamcontentUrl-first path (content.downloadUrlis only used as a fallback there whencontentUrlis missing/falsy):The resulting URL flows through the unchanged
_build_teams_fetch_data/rehydrate_attachmentSSRF allowlist. SharePoint/OneDrive for Business download hosts are already covered by*.sharepoint.com/*.onedrive.comin that allowlist, so no host was added.Divergence
Justified Python-only divergence — hard-UX-failure (attachment cannot be downloaded), allowed by the divergence policy. Documented in
docs/UPSTREAM_SYNC.mdKnown Non-Parity; to be filed as an upstream issue against vercel/chat. Supersedes stale/changes-requested PR #136 (rebuilt cleanly on currentmain, content-type-scoped rather than a blanketdownloadUrl or contentUrl).Tests
TestFileDownloadInfoAttachment(tests/test_teams_adapter.py):contentUrlcontentUrlis absentcontent.downloadUrlis left unchangedEach was verified to fail on a plausible mutation: contentUrl-only revert, over-eager
downloadUrl-for-all-types, and a dropped SSRF gate.Gauntlet
ruff check— cleanruff format --check— cleanaudit_test_quality.py— 0 hard failurespyrefly check— 0 errorspytest— 5085 passed, 4 skipped (pre-existing)verify_test_fidelity.py— 100% matched, 0 missingSummary by CodeRabbit
Bug Fixes
Documentation