Harden coder attachment fetching and pin the AI SDK packages - #261
Merged
Conversation
Attachment urls are remote input relayed from a chat channel and were fetched with no scheme or host restriction, so a malicious channel message could point the coder's attachment download at an internal service or a cloud metadata endpoint and have the response bytes land in the workspace for the coder to read. Add assertSafeAttachmentUrl, which requires https, rejects urls carrying credentials, rejects IPv4/IPv6 literals in non-public ranges (loopback, private, link-local including the metadata address, CGNAT, 0.0.0.0/8, IPv4-mapped IPv6), and rejects localhost/.internal hostnames. An optional DEVX_ATTACHMENT_HOST_ALLOWLIST env var narrows this to an explicit host list for locked-down deployments. The check is documented as hostname-based only: it does not stop DNS rebinding or a public hostname resolving to a private address at fetch time. Wired into materializeAttachments inside the existing per-file try/catch, before the fetch, so a rejected url is skipped and logged exactly like any other per-file failure and never fails the turn.
materializeAttachments buffered the whole response with `await res.arrayBuffer()` before ever comparing its length to ATTACHMENT_MAX_BYTES, so an oversized or endless response could exhaust worker memory even though it was ultimately rejected. Add readCappedBody(res, maxBytes): it rejects up front when an honest Content-Length header already exceeds the cap, and otherwise reads the body incrementally via res.body.getReader(), cancelling the reader and throwing as soon as the running total exceeds maxBytes. A missing or lying Content-Length can no longer defeat the cap. The 20MB limit, error wording, and per-file skip-and-log behaviour are unchanged.
The chat stream handler applied the remote-channel autonomy rule twice on the providerConfig path: once inline in that branch's settings object literal, and again in the blanket re-application run after the if/else for the legacy branch. The two calls were idempotent so behaviour never diverged, but it left two call sites to keep in sync by hand. Restore auto_approve in the providerConfig branch to the plain user preference and let the single post-if/else block be the one place the channel rule is applied, to both branches alike.
deno.json resolved npm:ai and the four npm:@ai-sdk/* provider packages against @latest, so a silent major bump on any of them could change or remove APIs this code depends on with no change on our side — for example the coder calls openai.chat(model), which lives on the provider object today but is not guaranteed across majors. Pin all five to a caret range of the version deno.lock currently resolves (ai@^7.0.68, @ai-sdk/anthropic@^4.0.39, @ai-sdk/openai@^4.0.43, @ai-sdk/google@^4.0.45, @ai-sdk/amazon-bedrock@^5.0.58) so patches and minors still flow but a major cannot land silently. The @modelcontextprotocol and edn-data entries were already bounded and are untouched.
p-hoffmann
marked this pull request as draft
August 24, 2026 09:05
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #261 +/- ##
========================================
Coverage 52.64% 52.64%
========================================
Files 169 169
Lines 72798 72798
========================================
Hits 38323 38323
Misses 34475 34475
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
assertSafeAttachmentUrl only ever validated the first url; the actual fetch in materializeAttachments then ran with Deno's default `redirect: "follow"`. A public, allowlist-passing host returning a 3xx to a private/link-local target (e.g. the cloud metadata address) was followed with no further validation, nullifying the guard end to end — including the allowlist path. Add fetchAttachment(), which fetches with `redirect: "manual"` and walks redirects itself: each Location header is resolved against the current hop (it may be relative) and re-validated through assertSafeAttachmentUrl before being followed, capped at 5 hops. Exceeding the cap, a 3xx with no Location, or a hop that fails validation throws, which the existing per-file try/catch in materializeAttachments already logs and skips without failing the turn. fetchImpl is injectable so tests can exercise the redirect walk against a fake fetch. The fetch also now carries an AbortSignal.timeout so a stalled hop cannot hang the turn. materializeAttachments now calls fetchAttachment instead of fetching the pre-validated url directly.
Four hostname/IP checks in assertSafeAttachmentUrl were incomplete:
- WHATWG preserves a trailing dot on a domain (it only strips it from
IPv4 literals), so "localhost." and "metadata.google.internal."
passed every check by not literally matching. Strip one trailing
dot from url.hostname up front so deny rules and the allowlist
compare against the same normalized name.
- The IPv6 loopback check required the last group to equal 1, so the
all-zero address "::" fell through everything. "::" reaches
localhost on Linux, exactly like 0.0.0.0 on the v4 side, which was
already blocked — now rejected the same way.
- Three more IPv6 forms that embed an IPv4 address in their low 32
bits were not evaluated against the IPv4 deny rules: the deprecated
IPv4-compatible ::/96, IPv4-translated ::ffff:0:0/96, and the NAT64
well-known prefix 64:ff9b::/96. Each now has its embedded IPv4 tail
run through the same non-public test as the existing IPv4-mapped
check.
- The IPv6 expander used filter(Boolean), which silently dropped a
malformed extra colon (e.g. "1:::2") instead of rejecting it, and
octets.map(Number) accepted non-decimal ("0x7f") or empty octet
strings. Both are now validated strictly. More importantly, the
expander failed OPEN: an unparseable literal was treated as an
ordinary DNS name and fell through the deny rules entirely. It now
fails CLOSED — a colon-bearing hostname that cannot be parsed with
confidence is rejected rather than allowed.
Also widen the IPv4 deny rules to cover 192.0.0.0/24 (IETF protocol
assignments), 198.18.0.0/15 (benchmarking), 224.0.0.0/4 (multicast),
and 240.0.0.0/4 (reserved, which covers 255.255.255.255).
Separately, the SSRF regression test used a plain http:// metadata
url, which the scheme check alone already rejects — the test would
still pass with every IP deny rule deleted. Switched it to https so
it actually exercises the IP rules it's meant to guard.
Two leak paths in readCappedBody, the streaming size-cap reader: - The Content-Length-declared-too-large path threw without ever touching res.body, leaving the connection undrained. It now cancels the body before throwing. - If reader.read() itself rejected mid-stream (a network failure), the reader was left locked with no cancel and no release. The read loop is now wrapped in try/finally so the reader is always canceled and released regardless of how the loop exits. Also correct the existing "rejected without reading" test, which only proved res.body.locked stayed false — true only because THIS code never called getReader(), not proof the stream was never pulled. It now makes pulling the stream itself fail with a distinguishable error, so if the Content-Length short-circuit ever regressed, the observed rejection would flip to that error instead of "too large". Added direct tests for the body-cancel and reader-release fixes above.
…ate IP ranges The SSRF guard blocked the link-local IP 169.254.169.254 but not the public hostnames that resolve to it (metadata.goog etc reach the same cloud instance-metadata service). Add an explicit, label-anchored deny list for those names. Also close the remaining private-range gaps: 6to4 (2002::/16, checked against its embedded IPv4), deprecated IPv6 site-local (fec0::/10), and the 6to4 relay anycast IPv4 block (192.88.99.0/24). Lowercase and trim DEVX_ATTACHMENT_HOST_ALLOWLIST entries at parse time so a mixed-case entry does not silently block every attachment.
…he whole chain fetchAttachment's redirect loop left the 3xx response's body undrained before looping to the next hop or throwing on max-redirects / missing Location, and materializeAttachments left a non-ok response's body undrained before throwing. Both now cancel the body first. Also replace the per-hop AbortSignal.timeout with a single signal created once before the loop and reused for every hop, so the 60s budget bounds the entire redirect chain instead of resetting on each hop. Add proof tests for three already-correct behaviors: a protocol-relative redirect to the metadata address is rejected without ever fetching it, an allowlist rejects a redirect to a non-allowlisted host at hop 2, and a non-allowlisted host with a trailing dot is rejected.
p-hoffmann
marked this pull request as ready for review
August 24, 2026 09:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.