fix(teams): surface pre-signed content.downloadUrl for chat file attachments#136
Conversation
📝 WalkthroughWalkthroughThe pull request implements end-to-end propagation of Slack file upload confirmations through the messaging pipeline, captures them in raw payloads, adds privacy safeguards for message history, and fixes Teams file attachment URL selection. Version is bumped to 0.4.27.2 and changes are documented in CHANGELOG.md. ChangesUpload confirmation and raw propagation
Teams attachment fix
Release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 several fixes and enhancements across adapters: it surfaces Slack-confirmed file IDs on RawMessage.raw, ensures _MessageHistoryCache nulls out raw before persisting to prevent storage inflation, and updates the Teams adapter to prefer pre-signed download URLs over SharePoint URLs to avoid 403 errors. While the version in pyproject.toml was bumped to 0.4.27.2 to include the Teams fix, there is no corresponding entry in CHANGELOG.md. Please apply the suggested changelog update to document this new version.
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.
| @@ -1,5 +1,18 @@ | |||
| # Changelog | |||
|
|
|||
| ## 0.4.27.1 (2026-05-29) | |||
There was a problem hiding this comment.
The version in pyproject.toml has been bumped to 0.4.27.2 to include the Teams pre-signed download URL fix, but there is no corresponding entry in the changelog for this new version. Please add a changelog entry for 0.4.27.2 to document this fix.
| ## 0.4.27.1 (2026-05-29) | |
| ## 0.4.27.2 (2026-05-30) | |
| Python-only patch on the 0.4.27 line. No upstream version change. | |
| ### Fixes | |
| - **Teams now prefers pre-signed content.downloadUrl for chat file attachments** — Files uploaded in personal/group chats point to auth-required SharePoint/OneDrive items and return 403 to anonymous GET requests. Surfacing the pre-signed content.downloadUrl ensures the file is readable without requiring Files.Read.All permissions. | |
| ## 0.4.27.1 (2026-05-29) |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5bce385f7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| files = extract_files(message) | ||
| if files: | ||
| await self._upload_files(files, channel, thread_ts or None) | ||
| uploaded_file_ids = await self._upload_files(files, channel, thread_ts or None) |
There was a problem hiding this comment.
When posting files through the real Slack SDK, this newly surfaced value will usually be [] even after successful uploads because _upload_files() only extracts IDs from a nested files[].files[].id shape, while files_upload_v2 returns completion data as files: [{id: ...}] (and may also add file for single uploads). In the text+files and file-only paths introduced here, consumers gating on SentMessage.raw["uploaded_file_ids"] will see an empty confirmation despite Slack having accepted the files; the new tests mock the nested shape, so they don't catch the production response shape.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/test_thread_faithful.py (1)
263-285:PostableMarkdown(files=...)is valid; adjust only for clarity
PostableMarkdownis a dataclass with bothattachmentsandfilesfields, soPostableMarkdown(markdown="report", files=[])won’t raise a constructorTypeError. Since this test’s mockedadapter.post_messageignores thePostableMarkdownpayload anyway,filesvsattachmentsdoesn’t affect the assertion (you can simplify/align for readability).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_thread_faithful.py` around lines 263 - 285, The test test_should_propagate_adapter_raw_onto_sent_message uses PostableMarkdown(markdown="report", files=[]) which is valid but confusing; change the PostableMarkdown instantiation to use the dataclass's attachments field (e.g., PostableMarkdown(markdown="report", attachments=[])) for clarity and consistency with other tests and the PostableMarkdown definition, keeping everything else (custom_post, adapter.post_message, RawMessage returned and assertions against result.raw) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 3-14: The changelog header currently reads "## 0.4.27.1
(2026-05-29)" but the package was published as 0.4.27.2; update the CHANGELOG.md
top section to match the shipped version by renaming the header to "## 0.4.27.2
(2026-05-29)" or add a new "## 0.4.27.2" entry that contains the same release
notes, ensuring the header string "## 0.4.27.1 (2026-05-29)" is replaced or
superseded so the changelog aligns with the pyproject.toml version bump.
In `@src/chat_sdk/adapters/teams/adapter.py`:
- Line 933: The current assignment url = download_url or att.get("contentUrl")
uses truthiness and will incorrectly fall back when download_url is an empty
string; change it to use an explicit None check so url is download_url if
download_url is not None, otherwise att.get("contentUrl") — update the
expression around the url variable where download_url and att.get("contentUrl")
are used (in the Teams adapter code) to use the form download_url if
download_url is not None else att.get("contentUrl").
---
Nitpick comments:
In `@tests/test_thread_faithful.py`:
- Around line 263-285: The test
test_should_propagate_adapter_raw_onto_sent_message uses
PostableMarkdown(markdown="report", files=[]) which is valid but confusing;
change the PostableMarkdown instantiation to use the dataclass's attachments
field (e.g., PostableMarkdown(markdown="report", attachments=[])) for clarity
and consistency with other tests and the PostableMarkdown definition, keeping
everything else (custom_post, adapter.post_message, RawMessage returned and
assertions against result.raw) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a9d7f9b7-fe2b-4c76-90e1-88456049b96d
📒 Files selected for processing (9)
CHANGELOG.mdpyproject.tomlsrc/chat_sdk/adapters/slack/adapter.pysrc/chat_sdk/adapters/teams/adapter.pysrc/chat_sdk/chat.pysrc/chat_sdk/thread.pytests/test_slack_api.pytests/test_teams_adapter.pytests/test_thread_faithful.py
| # item and returns 403 to an anonymous GET, so prefer the pre-signed URL. | ||
| content = att.get("content") | ||
| download_url = content.get("downloadUrl") if isinstance(content, dict) else None | ||
| url = download_url or att.get("contentUrl") |
There was a problem hiding this comment.
Use explicit None check instead of truthiness operator.
Line 933 uses download_url or att.get("contentUrl") which can incorrectly fall back to contentUrl if download_url is an empty string (falsy but not None). As per coding guidelines, use x if x is not None else default to avoid truthiness traps.
🔧 Suggested fix
- url = download_url or att.get("contentUrl")
+ url = download_url if download_url is not None else att.get("contentUrl")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| url = download_url or att.get("contentUrl") | |
| url = download_url if download_url is not None else att.get("contentUrl") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/chat_sdk/adapters/teams/adapter.py` at line 933, The current assignment
url = download_url or att.get("contentUrl") uses truthiness and will incorrectly
fall back when download_url is an empty string; change it to use an explicit
None check so url is download_url if download_url is not None, otherwise
att.get("contentUrl") — update the expression around the url variable where
download_url and att.get("contentUrl") are used (in the Teams adapter code) to
use the form download_url if download_url is not None else
att.get("contentUrl").
Source: Coding guidelines
tony-chinchill-ai
force-pushed
the
fix/teams-attachment-downloadurl
branch
from
d5bce38 to
073fd2b
Compare
|
@coderabbitai review Rebased onto |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the current PR diff
gh pr diff 136 --repo Chinchill-AI/chat-sdk-python
echo "=== PR FILES CHANGED ==="
gh pr view 136 --repo Chinchill-AI/chat-sdk-python --json files --jq '.files[].path'Length of output: 5000 🏁 Script executed: #!/bin/bash
# Read the teams adapter and its test
cat src/chat_sdk/adapters/teams/adapter.py
echo "=== TEST FILE ==="
cat tests/test_teams_adapter.pyLength of output: 147084 🏁 Script executed: #!/bin/bash
# Check CHANGELOG
cat CHANGELOG.md | head -60Length of output: 6368
content = att.get("content")
download_url = content.get("downloadUrl") if isinstance(content, dict) else None
url = download_url or att.get("contentUrl")
One non-blocking note (pre-existing, not introduced here):
Everything looks clean. The prior [approve] |
…chments TeamsAdapter._create_attachment read only the top-level contentUrl, which for a file uploaded in a personal/group chat points at the auth-required SharePoint/OneDrive item and returns 403 to an anonymous GET. The activity also carries content.downloadUrl, a short-lived pre-signed link that fetches with no auth header. Prefer it when present so the file is readable without granting Files.Read.All. Rebased onto main (0.4.29a3 line); CHANGELOG entry added, pyproject version bump left to the coordinating release PR per the unreleased-section convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
tony-chinchill-ai
force-pushed
the
fix/teams-attachment-downloadurl
branch
from
073fd2b to
7e51b5d
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@patrick-chinchill this is ready to merge when you have a minute. What it is: a single commit ( Why it shows Asks:
Context: this unblocks inbound Teams file reads. The adapter was dropping the pre-signed |
…attachments (#167) 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).
|
Incorporated and superseded by #167 (merged) — rebuilt to current standards: content-type-scoped |
2 participants
Problem
TeamsAdapter._create_attachmentreads only the top-levelcontentUrl. For a file uploaded in a personal/group chat that points at the auth-required SharePoint/OneDrive item and returns 403 to an anonymous GET. The activity also carriescontent.downloadUrl— a short-lived pre-signed link that fetches with no auth header — which was being dropped.Fix
Prefer
content.downloadUrlwhen present so the file is readable without grantingFiles.Read.All.Rebased onto
main; single commit touching only the Teams adapter + its test, with a CHANGELOG entry under the0.4.29a3 (unreleased)section (pyproject version bump left to the coordinating release PR per the unreleased-section convention). 46 Teams adapter tests pass, including a new regression test asserting the pre-signed URL is surfaced over the SharePointcontentUrl.