fix(msteams): fetch DM media via Bot Framework path for a: conversation IDs (#62219)#63951
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa85fc01cc
ℹ️ 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".
| const baseUrl = `${normalizeServiceUrl(params.serviceUrl)}/v3/attachments/${encodeURIComponent(params.attachmentId)}`; | ||
| if (!isUrlAllowed(baseUrl, policy.allowHosts)) { | ||
| return undefined; |
There was a problem hiding this comment.
Enforce auth allowlist before sending Bot Framework token
downloadMSTeamsBotFrameworkAttachment only validates serviceUrl against policy.allowHosts, but both Bot Framework requests always include Authorization: Bearer .... That bypasses the repo’s existing authAllowHosts credential-bleed guard used in other attachment paths, so deployments that intentionally keep mediaAuthAllowHosts narrower than mediaAllowHosts can leak bot tokens to hosts that are download-allowed but not auth-allowed. Token-bearing BF requests should be gated by policy.authAllowHosts (or auth header stripped when outside that list).
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes personal DM media fetches in MS Teams by routing attachments with Bot Framework conversation IDs ( Confidence Score: 5/5Safe to merge; all remaining findings are P2 observability suggestions that do not affect correctness. The routing logic, SSRF guards, token handling, size limits, and Graph-skip semantics are all correct. Tests cover the key paths including regression for 19: IDs. The only open item is a missing debug log for the serviceUrl-absent edge case, which is a P2 improvement. extensions/msteams/src/monitor-handler/inbound-media.ts — see P2 comment about silent skip when serviceUrl is absent for a BF DM Prompt To Fix All With AIThis is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/inbound-media.ts
Line: 68-94
Comment:
**Missing diagnostic log when `serviceUrl` is absent for a Bot Framework DM**
When `hasHtmlAttachment` is true and `isBotFrameworkPersonalChatId(conversationId)` is true but `serviceUrl` is falsy, neither the Bot Framework nor the Graph path executes — and no log is emitted. The Graph path is also skipped (because `!isBotFrameworkPersonalChatId(conversationId)` is false), so media is silently dropped with no diagnostic. Adding a debug log here makes the skip observable when `activity.serviceUrl` is unexpectedly absent.
```suggestion
if (hasHtmlAttachment && isBotFrameworkPersonalChatId(conversationId)) {
if (!serviceUrl) {
log.debug?.("bot framework attachment skipped (missing serviceUrl)", {
conversationType,
conversationId,
});
} else {
const attachmentIds = extractMSTeamsHtmlAttachmentIds(attachments);
if (attachmentIds.length === 0) {
log.debug?.("bot framework attachment ids unavailable", {
conversationType,
conversationId,
});
} else {
const bfMedia = await downloadMSTeamsBotFrameworkAttachments({
serviceUrl,
attachmentIds,
tokenProvider,
maxBytes,
allowHosts,
authAllowHosts: params.authAllowHosts,
preserveFilenames,
});
if (bfMedia.media.length > 0) {
mediaList = bfMedia.media;
} else {
log.debug?.("bot framework attachments fetch empty", {
conversationType,
attachmentCount: bfMedia.attachmentCount ?? attachmentIds.length,
});
}
}
}
}
```
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(msteams): fetch DM media via Bot Fra..." | Re-trigger Greptile |
| if (hasHtmlAttachment && serviceUrl && isBotFrameworkPersonalChatId(conversationId)) { | ||
| const attachmentIds = extractMSTeamsHtmlAttachmentIds(attachments); | ||
| if (attachmentIds.length === 0) { | ||
| log.debug?.("bot framework attachment ids unavailable", { | ||
| conversationType, | ||
| conversationId, | ||
| }); | ||
| } else { | ||
| const bfMedia = await downloadMSTeamsBotFrameworkAttachments({ | ||
| serviceUrl, | ||
| attachmentIds, | ||
| tokenProvider, | ||
| maxBytes, | ||
| allowHosts, | ||
| authAllowHosts: params.authAllowHosts, | ||
| preserveFilenames, | ||
| }); | ||
| if (bfMedia.media.length > 0) { | ||
| mediaList = bfMedia.media; | ||
| } else { | ||
| log.debug?.("bot framework attachments fetch empty", { | ||
| conversationType, | ||
| attachmentCount: bfMedia.attachmentCount ?? attachmentIds.length, | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
serviceUrl is absent for a Bot Framework DM
When hasHtmlAttachment is true and isBotFrameworkPersonalChatId(conversationId) is true but serviceUrl is falsy, neither the Bot Framework nor the Graph path executes — and no log is emitted. The Graph path is also skipped (because !isBotFrameworkPersonalChatId(conversationId) is false), so media is silently dropped with no diagnostic. Adding a debug log here makes the skip observable when activity.serviceUrl is unexpectedly absent.
| if (hasHtmlAttachment && serviceUrl && isBotFrameworkPersonalChatId(conversationId)) { | |
| const attachmentIds = extractMSTeamsHtmlAttachmentIds(attachments); | |
| if (attachmentIds.length === 0) { | |
| log.debug?.("bot framework attachment ids unavailable", { | |
| conversationType, | |
| conversationId, | |
| }); | |
| } else { | |
| const bfMedia = await downloadMSTeamsBotFrameworkAttachments({ | |
| serviceUrl, | |
| attachmentIds, | |
| tokenProvider, | |
| maxBytes, | |
| allowHosts, | |
| authAllowHosts: params.authAllowHosts, | |
| preserveFilenames, | |
| }); | |
| if (bfMedia.media.length > 0) { | |
| mediaList = bfMedia.media; | |
| } else { | |
| log.debug?.("bot framework attachments fetch empty", { | |
| conversationType, | |
| attachmentCount: bfMedia.attachmentCount ?? attachmentIds.length, | |
| }); | |
| } | |
| } | |
| } | |
| if (hasHtmlAttachment && isBotFrameworkPersonalChatId(conversationId)) { | |
| if (!serviceUrl) { | |
| log.debug?.("bot framework attachment skipped (missing serviceUrl)", { | |
| conversationType, | |
| conversationId, | |
| }); | |
| } else { | |
| const attachmentIds = extractMSTeamsHtmlAttachmentIds(attachments); | |
| if (attachmentIds.length === 0) { | |
| log.debug?.("bot framework attachment ids unavailable", { | |
| conversationType, | |
| conversationId, | |
| }); | |
| } else { | |
| const bfMedia = await downloadMSTeamsBotFrameworkAttachments({ | |
| serviceUrl, | |
| attachmentIds, | |
| tokenProvider, | |
| maxBytes, | |
| allowHosts, | |
| authAllowHosts: params.authAllowHosts, | |
| preserveFilenames, | |
| }); | |
| if (bfMedia.media.length > 0) { | |
| mediaList = bfMedia.media; | |
| } else { | |
| log.debug?.("bot framework attachments fetch empty", { | |
| conversationType, | |
| attachmentCount: bfMedia.attachmentCount ?? attachmentIds.length, | |
| }); | |
| } | |
| } | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: extensions/msteams/src/monitor-handler/inbound-media.ts
Line: 68-94
Comment:
**Missing diagnostic log when `serviceUrl` is absent for a Bot Framework DM**
When `hasHtmlAttachment` is true and `isBotFrameworkPersonalChatId(conversationId)` is true but `serviceUrl` is falsy, neither the Bot Framework nor the Graph path executes — and no log is emitted. The Graph path is also skipped (because `!isBotFrameworkPersonalChatId(conversationId)` is false), so media is silently dropped with no diagnostic. Adding a debug log here makes the skip observable when `activity.serviceUrl` is unexpectedly absent.
```suggestion
if (hasHtmlAttachment && isBotFrameworkPersonalChatId(conversationId)) {
if (!serviceUrl) {
log.debug?.("bot framework attachment skipped (missing serviceUrl)", {
conversationType,
conversationId,
});
} else {
const attachmentIds = extractMSTeamsHtmlAttachmentIds(attachments);
if (attachmentIds.length === 0) {
log.debug?.("bot framework attachment ids unavailable", {
conversationType,
conversationId,
});
} else {
const bfMedia = await downloadMSTeamsBotFrameworkAttachments({
serviceUrl,
attachmentIds,
tokenProvider,
maxBytes,
allowHosts,
authAllowHosts: params.authAllowHosts,
preserveFilenames,
});
if (bfMedia.media.length > 0) {
mediaList = bfMedia.media;
} else {
log.debug?.("bot framework attachments fetch empty", {
conversationType,
attachmentCount: bfMedia.attachmentCount ?? attachmentIds.length,
});
}
}
}
}
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62f4a995b1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // or `8:orgid:...`) which Graph's `/chats/{id}` endpoint rejects with | ||
| // "Invalid ThreadId". Fetch media via the Bot Framework v3 attachments | ||
| // endpoint instead, which speaks the same identifier space. | ||
| if (hasHtmlAttachment && isBotFrameworkPersonalChatId(conversationId)) { |
There was a problem hiding this comment.
Route BF attachment flow using the raw conversation ID
resolveMSTeamsInboundMedia now gates the Bot Framework branch on isBotFrameworkPersonalChatId(conversationId), but this function is called from message-handler with conversationId already normalized via translateMSTeamsDmConversationIdForGraph for a: DMs when aadObjectId and appId are present. In that path, the ID becomes 19:...@unq.gbl.spaces, this condition is false, and the new Bot Framework media fetch never runs; the code falls back to Graph only. That leaves the new DM fix inactive for translated a: conversations instead of consistently using the BF attachment endpoint.
Useful? React with 👍 / 👎.
…on IDs (openclaw#62219) (openclaw#63951) * fix(msteams): fetch DM media via Bot Framework path for a: conversation IDs (openclaw#62219) * fix(msteams): log skipped BF DM media fetches --------- Co-authored-by: Brad Groux <bradgroux@users.noreply.github.com>
…on IDs (openclaw#62219) (openclaw#63951) * fix(msteams): fetch DM media via Bot Framework path for a: conversation IDs (openclaw#62219) * fix(msteams): log skipped BF DM media fetches --------- Co-authored-by: Brad Groux <bradgroux@users.noreply.github.com>
…on IDs (openclaw#62219) (openclaw#63951) * fix(msteams): fetch DM media via Bot Framework path for a: conversation IDs (openclaw#62219) * fix(msteams): log skipped BF DM media fetches --------- Co-authored-by: Brad Groux <bradgroux@users.noreply.github.com>
Summary
Personal DM media fetches were failing because the plugin tried to use Graph
/chats/{a:xxx}/messages/...which Graph rejects with "Invalid ThreadId" (only19:xxx@thread.*/19:xxx@unq.gbl.spacesIDs are accepted on/chats). File attachments (PDFs, documents) sent in Teams personal DMs were silently dropped with only a<media:document>placeholder reaching the agent.Fix
a:and8:orgid:prefixes) via a newisBotFrameworkPersonalChatIdhelper.GET {serviceUrl}/v3/attachments/{attachmentId}+.../views/{viewId}) using the bot's Bot Framework token scope.extractMSTeamsHtmlAttachmentIdshelper (un-sliced counterpart to the existing diagnostic summary).19:xxx@thread.*) and group chats.activity.serviceUrlthroughresolveMSTeamsInboundMediaso the new code path has the Bot Framework service URL it needs.Fixes #62219
Test plan
a:and8:orgid:IDs route to Bot Framework, Graph URLs never built19:IDs continue to route to Graph (regression)serviceUrlleaves both paths inert (safe degradation)extensions/msteamssuite passes (595 tests)