web: honor ui/download-file with confirmation and http(s) allowlist

[cliffhall]

Closes #1569

Honors ui/download-file requests from MCP App views (re-implementation of the relevant slice of PR #1510).

Changes

createAppBridgeFactory advertises the downloadFile host capability and attaches an ondownloadfile handler:

• Confirmation required — a host-mediated window.confirm lists the requested file(s); a declined prompt or empty payload returns isError without acting.
• Inline resources (text or base64 blob) are written to disk via the shared downloadBlob object-URL anchor; base64 is decoded via base64ToBytes.
• Resource links are opened in a new tab, restricted to http(s) via the shared isHttpUrl allowlist (javascript:/data:/file:/malformed rejected).
• Label sanitization — control + format characters stripped and length clamped, so a server-supplied filename/URI can't inject newlines or reflow the prompt.
• Partial-batch handling — isError only when nothing downloaded; skipped items warned.

Consumes the existing lib/downloadFile.ts (Wave 1). onopenlink is left as-is (unchanged behavior).

Tests

Factory tests for the full download surface: text/blob/link, decline, empty, throw, non-http rejection, label sanitization (newlines, bidi/zero-width) + clamping, partial batch. Per-file coverage ≥90 on all four dimensions; full npm run ci passes locally.

Stacking

Stacked on #1568 (v2/1568-size-displaymode). Merge order: #1565 → #1566 → #1567 → #1568 → this.

🤖 Generated with Claude Code

https://claude.ai/code/session_01BrihGWcrM9JGRyu41nzZYw

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 40s —— View job

---

Review: ui/download-file handler

• Read the diff and changed files
• Review the download handler implementation
• Review the tests
• Post review feedback

Overall this is a clean, well-scoped PR. The handler is defensive in the right places (empty payload short-circuit, per-item try/catch, http(s) allowlist reuse, label sanitization against prompt-spoofing), and the test suite is genuinely thorough — decline, empty, throw, partial batch, base64 decode, bidi/zero-width stripping, and clamping are all exercised. Nice work leaning on the shared lib/downloadFile.ts helpers.

A few observations, all minor — nothing blocking.

1. Prompt says "download" but resource links navigate (semantic mismatch)

clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts — the confirmation reads:

This MCP App wants to download N file(s):

But downloadResourceItem opens resource_links in a new tab rather than saving them (correctly documented in the JSDoc). A user who approves a "download" and instead gets a tab navigation may be surprised. Consider either wording the prompt more neutrally (e.g. "download or open") or distinguishing the two item kinds in the summary (e.g. prefix links with "↗"). Purely a UX nicety.

2. No cap on batch size

After a single window.confirm, the loop fires every item — a malicious/buggy app could request, say, 500 items and spawn 500 tabs / downloads from one approval. The count is surfaced in the prompt (good), so the user isn't blind, but a sanity cap (reject or truncate very large batches) would harden it. Low priority.

3. Popup-blocked window.open counts as success

window.open(parsed.href, "_blank", "noopener,noreferrer");
return true;

window.open returns null when the browser blocks the popup, but the item is still counted as succeeded. Checking the return value would let a fully-blocked batch report isError: true instead of a silent no-op. Edge case — most hosts allow programmatic opens inside a user-gesture-triggered confirm, so likely fine in practice.

4. Nits

• describeDownloadItem(item) is called twice for skipped items (once for the summary, once for the skipped push). Negligible, but you could compute labels once up front.
• The truncation keeping the start of the URL (slice(0, 77)) is a good call for links — the host/scheme stays visible for the user's trust decision. Worth a one-line comment noting that's intentional, since it reads as arbitrary otherwise.

Security & correctness — looks good

• isHttpUrl re-parses the raw URI (not the sanitized label), so the display-vs-action gap can't be exploited. ✅
• atob throwing on malformed base64 is caught by the per-item try/catch → item skipped, not a crash. ✅
• Label sanitization strips \p{Cc}/\p{Cf} before the prompt, blocking newline/bidi injection. ✅
• Deferred revokeObjectURL is preserved via the shared downloadBlob. ✅
  · v2/1569-download-file

[cliffhall]

Thanks — addressed in a3a5bfa:

1. Prompt says "download" but links navigate — applied. The confirmation now reads "This MCP App wants to download or open N item(s)", and describeDownloadItem(item, forPrompt=true) prefixes each resource_link with "↗" in the summary, so a link that opens in a tab is visually distinct from an embedded file that saves to disk. Added a test asserting the ↗ prefix.

2. No cap on batch size — applied. Added MAX_DOWNLOAD_ITEMS = 20; a batch larger than that is rejected (isError: true) before the prompt, with a console.warn. One approval can no longer fan out into hundreds of tabs/downloads. Added a test (21 items → rejected, no confirm, no window.open, warn logged).

3. Popup-blocked window.open counts as success — declined, with reason. We open with noopener,noreferrer, and per the HTML spec window.open returns null when noopener is set even on a successful open. So the return value can't distinguish a blocked popup from a successful noopener navigation — checking it would misreport every successful link open as a failure. Left as-is.

4a. describeDownloadItem called twice for skipped items — not changed. Negligible (only on the error/skip path), and computing labels up front would complicate the summary-vs-skip distinction (the summary now uses forPrompt=true, the skip log uses the raw label). Left as-is.

4b. Start-of-URL truncation — applied (comment). Added a comment noting the slice(0, 77) keeps the label's start intentionally, so a link's scheme+host stays visible for the user's trust decision.

Security confirmations (isHttpUrl re-parses the raw URI, atob throw caught per-item, Cc/Cf stripping, deferred revoke) — all still hold. npm run ci is green.

[cliffhall]

@claude review

[claude]

Claude finished @cliffhall's task in 1m 12s —— View job

---

Re-review: ui/download-file handler

• Read the current diff (incl. a3a5bfa follow-up)
• Verify the previous review items are addressed
• Review implementation + tests for new issues
• Post review feedback

All four points from the prior review are handled well, and the two you declined are declined for the right reasons:

Prior point	Status
1. "download" vs. navigate wording	✅ Prompt now "download or open"; resource_links prefixed with ↗ (describeDownloadItem(item, true)), tested.
2. No batch cap	✅ MAX_DOWNLOAD_ITEMS = 20, checked before the prompt, console.warn on reject, tested (21 → rejected, no confirm/open).
3. Popup-blocked window.open = success	✅ Correctly declined — with noopener the HTML spec returns null even on a successful open, so the return value can't distinguish blocked from opened. Checking it would misreport every success.
4a. Double describeDownloadItem	✅ Left (negligible, skip-path only).
4b. slice(0, 77) intent	✅ Comment added at downloadFile.ts / sanitizeDownloadLabel.

What holds up under a fresh pass

• Label sanitization order is right. The prompt is built from sanitizeDownloadLabel(describeDownloadItem(item, true)) while downloadResourceItem / isHttpUrl act on the raw item.uri — display and action are decoupled, so a spoofed label can't change what actually opens. The ↗ (U+2197, category So) survives the \p{Cc}\p{Cf} strip, so links stay marked. ✅
• Cap ordering — empty → oversized → confirm → act — means a hostile 500-item batch never reaches window.confirm. ✅
• Per-item try/catch still contains atob/blob throws → item skipped, isError only when nothing succeeded. ✅
• Tests are comprehensive: capability advertisement, text/blob/link, decline, empty, throw, non-http rejection (incl. data:/file:/malformed), newline + bidi/zero-width stripping, clamping, ↗ prefix, oversized batch, deferred revokeObjectURL.

One minor observation (non-blocking)

createAppBridgeFactory.ts:161-168 — for an inline EmbeddedResource whose resource object carries neither text nor blob (types forbid it, but the payload comes from an untrusted app), the "blob" in resource check is false and the code falls to resource.text, which is undefined. new Blob([undefined], …) stringifies that to the literal text "undefined", so the item counts as succeeded and the user gets a file containing the word "undefined" rather than being told the item was unusable. Cosmetic — no security impact — but you could treat a resource with neither field as a skip (return false) for symmetry with the link-rejection path.

const resource = item.resource;
if (!("blob" in resource) && typeof resource.text !== "string") return false;

Overall: clean, well-scoped, thoroughly tested — looks good to merge. Nice follow-through on the review.
· v2/1569-download-file

[cliffhall]

Applied the round-2 observation in a follow-up: an embedded resource with neither text nor blob is now skipped (return false) instead of writing a new Blob([undefined]) file containing the literal "undefined". Added a test asserting no object URL is created and the batch reports isError. npm run ci green. Thanks for the thorough reviews!

[cliffhall]

Superseded by the wave rollup PR #1660 (retargeted to v2/main), which contains this change as part of the full cumulative diff. Closing this per-issue PR in favor of the rollup so the wave merges as one PR. Branch retained until #1660 merges; the linked issue will be closed and its board card moved to Done when the rollup lands.