Skip to content

fix(server): allow downloads from HTML previews - #14363

Closed
stickerdaniel wants to merge 1 commit into
pingdotgg:mainfrom
stickerdaniel:fix/html-preview-downloads
Closed

stickerdaniel wants to merge 1 commit into
pingdotgg:mainfrom
stickerdaniel:fix/html-preview-downloads

Conversation

@stickerdaniel

@stickerdaniel stickerdaniel commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Closes #14362.

What Changed

HTML previews may start downloads. allow-downloads joins the sandbox in the CSP served with HTML assets and in the file preview iframe. Both need it, because the iframe sandbox and the document's CSP sandbox combine.

Why

A page's own download button did nothing, without any error. The opaque origin stays intact.

Trade-off: the token also lets a page start a download on load without a click. Chromium has no sandbox token limited to user-initiated downloads. Desktop shows Electron's save dialog; a browser client follows its download settings.

Verified with vp test run apps/server/src/http.test.ts (23/23), fmt and lint. In Chromium, a page under the old policies downloaded nothing; under the new ones the file downloaded. Not run in the desktop or mobile apps.

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • No visual UI change
  • This change does not involve animation

Claude Opus 5.5 via Claude Code in T3 Code.

Copilot AI balanced review requested due to automatic review settings September 30, 2026 04:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Sep 30, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This changes the default security policy and iframe sandbox for every HTML preview, enabling downloads and broadening an existing browser permission. Despite the small, focused diff, the always-on security-boundary change warrants human review.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: pingdotgg/t3code/.coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 3ca194a1-7feb-4800-a8ca-bbe9c3b52598

📥 Commits

Reviewing files that changed from the base of the PR and between 0fcd5f9 and c7fc9e6.

📒 Files selected for processing (3)
  • apps/server/src/http.test.ts
  • apps/server/src/http.ts
  • apps/web/src/components/files/BrowserDocumentFrame.tsx

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

The server Content-Security-Policy and browser HTML iframe sandbox now include allow-downloads. Server policy tests expect this permission.

Changes

HTML preview downloads

Layer / File(s) Summary
Add download permission
apps/server/src/http.ts, apps/web/src/components/files/BrowserDocumentFrame.tsx, apps/server/src/http.test.ts
The server policy and HTML iframe sandbox add allow-downloads. Server tests expect the permission in the inline attachment and HTML asset policies.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~8 minutes

Change: Bug fix

Suggested reviewers: t3dotgg

Fixed issue severity: <fixed_issue_severity>Low</fixed_issue_severity>

Merge Risk: ⚪ Minimal · up to c7fc9

The change enables the requested preview downloads, and no concrete merge-blocking risk is established by the supplied context.

Security Architecture Review

Security architecture risk: 🔵 Low · up to c7fc9

HTML previews can now initiate downloads while retaining their isolated origin. No additional app or native privileges were demonstrated, but desktop save confirmation and repeated or interrupted download behavior remain unverified.

Retained concerns

  • Low · security · inferred: Untrusted HTML gains access to the user's download save flow, but its consent and lifecycle guarantees have not been established for the desktop runtime. Unwanted or repeated download attempts are newly possible; automatic persistence, destination bypass and unsafe interruption recovery are not verified findings.
Security review details

Security Blast Radius

  • inferred — The identified expansion reaches users rendering affected inline HTML, including agent-written previews and standalone HTML asset documents. Its sensitive outcome is local persistence through the user's download runtime; broader server, tenant or filesystem-read authority was not established.

Security Findings and Attack Paths

  • inferred — Adversarial HTML rendered in a preview can now attempt downloads that the previous combined sandbox blocked. Whether unsolicited or repeated attempts produce files, prompts or throttling depends on unverified runtime mediation; this is not evidence of arbitrary destination writes or code execution.

Trust Boundaries and Controls

  • observed — The desktop app window uses contextIsolation: true, nodeIntegration: false and sandbox: true. These are counterevidence to direct native API access from HTML; they do not establish the download destination or confirmation contract.
  • observed — Existing asset access includes RPC read-scope authorization and signed, expiring URL resolution. The PR changes sandbox permission rather than these authorization checks; full fine-grained authorization at URL issuance remains partially covered.

Resilience and Maintainability Implications

  • inferred — The inspected preview path forwards a URL and the server streams content; neither owns a download completion or cleanup transaction. Cancellation, partial-file handling, repeated requests, concurrency and recovery therefore depend on external runtime behavior whose security guarantees remain unverified.

Hardening Proposals

  • proposed — Define and validate the download consent and containment contract on supported browsers and the packaged desktop runtime, including script initiation, repeated requests, destination selection, cancellation and interrupted saves. Add host-side mediation only if runtime behavior fails that contract.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Issue #14362 requires allow-downloads in the HTML asset CSP and the BrowserDocumentFrame iframe sandbox. apps/server/src/http.ts adds the directive to HTML_CONTENT_SECURITY_POLICY. `apps/web/s…
Out of Scope Changes check ✅ Passed The changes are limited to the two sandbox directives and the matching server header tests. Each change directly supports issue #14362. No unrelated product behavior or files are changed.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 3 files.
Title check ✅ Passed The title clearly and concisely describes the primary change: enabling downloads from HTML previews.
Description check ✅ Passed The description includes the required What Changed, Why, and Checklist sections. It explains the sandbox changes, the reason for the fix, the trade-off, validation results, and the absence of visual U…
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Member

Note

This comment is posted by Julius' dot

The download-button failure in #14362 is clear, and both sandbox changes address it. The unresolved direction is whether HTML previews should also be able to start downloads without a user click: the PR explicitly identifies that consequence, and neither the linked issue nor this discussion contains a maintainer decision on it.

Can a maintainer confirm the intended download behavior and scope, including page-initiated downloads and the desktop save flow? The prior-approval rule calls for agreeing intentional behavior changes first. Leaving this open for that decision; the Chromium result and 23 passing header tests are useful evidence, while desktop behavior remains unverified.

@juliusmarminge

Copy link
Copy Markdown
Member

Note

Grok responding on behalf of Julius.

Closing as superseded — #16950 already landed the allow-downloads sandbox fix and closed #14362. Thanks for the original PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Download buttons in HTML previews do nothing

3 participants