Response body not consumed on HTTP error

When response.ok is false the function throws without draining or cancelling the body. Undici (Node's built-in fetch) keeps the socket checked-out until the body is GC-ed or the connection times out, and emits a "body was not consumed" diagnostic in some configurations. Because this path can be hit on every SharePoint 401/403/5xx, repeated failures would stall the keep-alive pool sooner than expected.

This is a comment left during a code review.
Path: extensions/msteams/src/attachments/remote-media.ts
Line: 33-36

Comment:
**Response body not consumed on HTTP error**

When `response.ok` is false the function throws without draining or cancelling the body. Undici (Node's built-in fetch) keeps the socket checked-out until the body is GC-ed or the connection times out, and emits a "body was not consumed" diagnostic in some configurations. Because this path can be hit on every SharePoint 401/403/5xx, repeated failures would stall the keep-alive pool sooner than expected.

```suggestion
  if (!response.ok) {
    await response.body?.cancel().catch(() => {});
    const statusText = response.statusText ? ` ${response.statusText}` : "";
    throw new Error(`HTTP ${response.status}${statusText}`);
  }
```

How can I resolve this? If you propose a fix, please make it concise.

Preserve SSRF IP checks in direct fetch mode

When useDirectFetch is enabled, this branch bypasses channel.media.fetchRemoteMedia(...ssrfPolicy) and therefore skips the strict fetchWithSsrFGuard path that enforces pinned-DNS/private-IP blocking. The current callers provide fetchImpl wrappers based on safeFetchWithPolicy without a resolveFn, so they only enforce hostname suffix allowlists; an allowlisted domain with attacker-controlled DNS (or rebinding) can now resolve to internal/private addresses that were blocked before this commit.

Useful? React with 👍 / 👎.

Keep strict SSRF IP validation on attachment downloads

This now forces all attachment URLs through useDirectFetch, which bypasses channel.media.fetchRemoteMedia(...ssrfPolicy) and its pinned-DNS/private-IP guard; the replacement path (fetchWithAuthFallback -> safeFetchWithPolicy) is called without resolveFn, so it only checks hostname suffixes. That means allowlisted domains can now resolve/redirect to private/internal IPs that were previously blocked, which is a security regression for generic attachment downloads (not just the SharePoint Node 24 workaround).

Useful? React with 👍 / 👎.

Redact URL-bearing fetch errors before warning logs

This logs raw err.message, but direct-fetch failures can bubble up messages from safeFetch that include full redirect URLs (for example blocked redirect targets). In Teams/SharePoint flows those URLs can carry signed query parameters, so failed downloads can leak sensitive link tokens into warn logs. Please redact URL content (or log host/status only) before attaching error metadata.

Useful? React with 👍 / 👎.

Reinstate IP-level SSRF checks for Bot Framework downloads

This change replaces fetchWithSsrFGuard(...ssrfPolicy) with safeFetchWithPolicy(...) for Bot Framework attachment fetches, but no resolveFn is supplied, so only hostname suffix allowlisting is enforced. In this path, an allowlisted hostname (or redirect target) that resolves to a private/loopback address will no longer be blocked, which is a security regression from the previous pinned-DNS/private-IP guard behavior used before this commit; the same pattern is applied to both attachment info and view fetches.

Useful? React with 👍 / 👎.

Cancel oversized response bodies before throwing

When Content-Length is above maxBytes, this branch throws immediately without canceling the response stream. In the new direct-fetch path, that leaves the body unread, so repeated oversized attachments can keep undici sockets busy and reduce connection reuse (a regression from the previous fetchRemoteMedia(...).release() cleanup path). Please cancel the body before throwing on this early-exit path.

Useful? React with 👍 / 👎.

Redact SharePoint fetch errors before warning logs

This warning logs raw err.message, but failures from safeFetchWithPolicy can include full redirect URLs. In SharePoint flows those URLs often carry signed query parameters, so a blocked/failed download can leak tokenized links into logs. Please redact URL content (or log host/status only) before attaching the error text.

Useful? React with 👍 / 👎.

Cancel Bot Framework response body before early return

After replacing fetchWithSsrFGuard(...).release() with safeFetchWithPolicy, this branch returns on non-OK responses without draining or canceling response.body. In undici, unread bodies can keep connections occupied until GC, so repeated failed attachment downloads (for example 401/404/500) can reduce connection reuse and hurt throughput. The previous implementation always released resources in a finally block, so this is a regression in the new path.

Useful? React with 👍 / 👎.

Redact Bot Framework fetch errors before warning logs

This logs raw err.message from safeFetchWithPolicy, whose thrown messages can include full redirect target URLs. Bot Framework attachment redirects can carry signed query parameters, so failed/blocked fetches can leak sensitive tokens into warning logs. Please redact URL/query content (or log host/status only) before attaching the error string.

Useful? React with 👍 / 👎.

Reinstate pinned DNS for direct attachment downloads

Setting useDirectFetch: true here forces all attachment fetches onto fetchRemoteMediaDirect, which calls fetchImpl without fetchWithSsrFGuard's pinned dispatcher. Fresh evidence versus earlier SSRF findings: even though resolveFn is now threaded through, safeFetchWithPolicy only performs a preflight DNS/IP check and then issues a separate fetch, so DNS can change between validation and connect. That is a security regression from the previous pinned-DNS path for environments that rely on strict SSRF guarantees (especially when allowHosts includes tenant-configured domains).

Useful? React with 👍 / 👎.

Cancel direct-fetch response before throwing on HTTP failure

When useDirectFetch is enabled, this branch throws on non-2xx status without draining or canceling response.body. In Node/undici, unread bodies can keep connections occupied until GC, so repeated failed attachment downloads (for example 401/403/500 responses) can reduce connection reuse and hurt throughput. The previous fetchRemoteMedia(...).release() path cleaned up transport resources even on failure.

Useful? React with 👍 / 👎.

Cancel Bot Framework view body on size precheck rejection

This early return exits when Content-Length exceeds maxBytes but leaves the response body unread. In the new safeFetchWithPolicy path there is no release() cleanup, so repeated oversized Bot Framework attachments can keep undici sockets busy and degrade download throughput. The prior guarded-fetch implementation always released resources in a finally block.

Useful? React with 👍 / 👎.