Fix browser download trigger parity - #6258
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughCentralizes download-forcing logic in ChangesDownload Policy Centralization and Scripted Download Interception
Sequence DiagramsequenceDiagram
participant Page as Web Page JS
participant WKHandler as ScriptedDownloadMessageHandler
participant CmuxWebView
participant Resolver as BrowserDownloadFilenameResolver
participant Session as downloadURLViaSession
Note over Page,CmuxWebView: JS bootstrap installed at document-start
Page->>WKHandler: postMessage(kind, url/dataURL, suggestedFilename)
WKHandler->>CmuxWebView: handleScriptedDownloadMessage(_:) on MainActor
CmuxWebView->>CmuxWebView: startScriptedDownload(_:suggestedFilename:)
CmuxWebView->>Session: downloadURLViaSession(url, traceID, .scriptedDownload)
Note over CmuxWebView,Resolver: navigationResponse path
CmuxWebView->>Resolver: navigationResponseDownloadReason(mimeType, canShowMIMEType, contentDisposition)
Resolver-->>CmuxWebView: "content-disposition" or "forceDownloadMIME" or "cannotShowMIME" or nil
CmuxWebView->>Session: decisionHandler(.download) if reason != nil
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/CmuxWebView.swift (1)
1608-1615:⚠️ Potential issue | 🟠 MajorAdd
@MainActorannotation todownloadURLViaSession.This method is callable from other same-module files (e.g.,
CmuxWebView+ScriptedDownloads.swift) and accesses WebKit'shttpCookieStoreand AppKit'sNSSavePanel. While the current scripted-download path usesMainActor.assumeIsolated, the method itself lacks an explicit@MainActorcontract, allowing future callers to invoke it from background threads. Mark it@MainActorto enforce main-thread-only access, consistent with how other cookie-store access in the codebase (BrowserPanel, TerminalController) wraps these operations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/CmuxWebView.swift` around lines 1608 - 1615, The downloadURLViaSession method in CmuxWebView.swift accesses WebKit's httpCookieStore and AppKit's NSSavePanel, which are main-thread-only APIs, but lacks an explicit `@MainActor` contract. Add the `@MainActor` annotation to the downloadURLViaSession method signature to enforce main-thread-only access. This will prevent future callers from accidentally invoking it from background threads and will make the main-thread requirement explicit in the method's contract, consistent with how other similar cookie-store access is handled elsewhere in the codebase such as in BrowserPanel and TerminalController.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift:
- Around line 48-58: The readBlobForDownload function uses readAsDataURL to
convert the entire blob to a base64-encoded string, which causes memory issues
with large files. Implement a size cap check before calling reader.readAsDataURL
on the blob parameter; if the blob size exceeds a reasonable threshold, handle
it with explicit error handling (e.g., reject the download or show an error
message to the user). Alternatively, replace the base64 approach with chunked or
streamed transfer of the blob data to a native temporary file to avoid
materializing the entire file in WebContent memory at once.
- Around line 194-232: The handleScriptedDownloadMessage method is a
page-accessible entry point that currently lacks security validation before
passing URLs to the credentialed downloader. Add security gates to this function
to: verify an unforgeable injected token is present in the message body,
validate that user activation and correct origin conditions are met, explicitly
reject file: URLs from scripted messages, and ensure that when passing the
validated URL to startScriptedDownload, HTTP credentials are either scoped
appropriately to the target URL or the request is routed through WebKit's
built-in download machinery instead of the credentialed session. These
validations must occur in handleScriptedDownloadMessage before the guard
statement that validates the URL itself.
---
Outside diff comments:
In `@Sources/Panels/CmuxWebView.swift`:
- Around line 1608-1615: The downloadURLViaSession method in CmuxWebView.swift
accesses WebKit's httpCookieStore and AppKit's NSSavePanel, which are
main-thread-only APIs, but lacks an explicit `@MainActor` contract. Add the
`@MainActor` annotation to the downloadURLViaSession method signature to enforce
main-thread-only access. This will prevent future callers from accidentally
invoking it from background threads and will make the main-thread requirement
explicit in the method's contract, consistent with how other similar
cookie-store access is handled elsewhere in the codebase such as in BrowserPanel
and TerminalController.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5854124-2bb6-47d9-a3d1-202bc71a20b0
📒 Files selected for processing (7)
Sources/Panels/BrowserDownloadFilenameResolver.swiftSources/Panels/BrowserPanel.swiftSources/Panels/BrowserPopupWindowController.swiftSources/Panels/CmuxWebView+ScriptedDownloads.swiftSources/Panels/CmuxWebView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/BrowserDownloadFilenameResolverTests.swift
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const readBlobForDownload = (blob, suggestedFilename) => { | ||
| try { | ||
| if (!blob) return; | ||
| if (blobDownloadInFlight) return; | ||
| if (typeof blob.size === "number" && blob.size > maxPayloadBytes) return; | ||
| blobDownloadInFlight = true; | ||
| const filename = String(suggestedFilename || blob.name || ""); | ||
| const reader = new FileReader(); | ||
| const finish = () => { | ||
| blobDownloadInFlight = false; | ||
| }; | ||
| reader.onload = () => { | ||
| if (typeof reader.result === "string" && reader.result.length > 0) { | ||
| postDataURLDownload(reader.result, filename); | ||
| } | ||
| finish(); | ||
| }; | ||
| reader.onerror = finish; | ||
| reader.onabort = finish; | ||
| reader.readAsDataURL(blob); | ||
| } catch (_) { | ||
| blobDownloadInFlight = false; | ||
| } | ||
| }; |
There was a problem hiding this comment.
blobDownloadInFlight is a per-page serial gate: while a FileReader.readAsDataURL call is in progress (potentially seconds for large files), any second blob-download trigger returns silently with no user feedback, no retry, and no error. The user clicks "Download" a second time and nothing happens. The flag is reset only when the first read resolves/errors, so for a 100 MB blob (≈133 MB after base64 encoding), any overlapping download is permanently lost.
Consider queuing the second request or showing a brief in-app error when the gate is occupied, rather than silently discarding it.
There was a problem hiding this comment.
Adjusted in 14b16e7: an occupied blob read now returns false to avoid consuming a second trigger through the native bridge.
— Claude Code
There was a problem hiding this comment.
Addressed in 14b16e7: a concurrent blob read now returns false from the interceptor instead of consuming the click, so the bridge no longer silently swallows the user's second action while one read is in flight.
— Claude Code
| private static let forceDownloadMIMETypes: Set<String> = [ | ||
| "application/gzip", | ||
| "application/json", | ||
| "application/octet-stream", | ||
| "application/x-gzip", | ||
| "application/x-zip-compressed", | ||
| "application/zip", | ||
| "text/csv", | ||
| ] |
There was a problem hiding this comment.
application/json force-download breaks inline JSON viewing
Adding application/json to forceDownloadMIMETypes means every navigation to a JSON endpoint — REST API debug views, admin consoles, or any direct URL that returns JSON — now triggers a save dialog instead of rendering inline. WKWebView can show application/json in its raw text renderer, and many pages deliberately serve JSON for in-browser inspection. This is a broader behavior change than the CSV/PDF fix described in the PR and will affect developer and power-user workflows. Consider restricting the force-download rule to MIME types WebKit cannot usefully render (application/octet-stream, application/zip, application/gzip, application/x-gzip, application/x-zip-compressed, text/csv) and deferring application/json until there is a confirmed need.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Adjusted in 14b16e7: application/json was removed from the force-download MIME list and is covered as an inline-renderable type in the policy test.
— Claude Code
There was a problem hiding this comment.
Addressed in 14b16e7: application/json was removed from the force-download MIME set and the unit test now asserts JSON remains inline.
— Claude Code
| func installScriptedDownloadInterception() { | ||
| let userContentController = configuration.userContentController | ||
| if objc_getAssociatedObject( | ||
| userContentController, | ||
| &Self.scriptedDownloadHandlerInstalledKey | ||
| ) != nil { | ||
| return | ||
| } | ||
|
|
||
| let token = UUID().uuidString | ||
| objc_setAssociatedObject( | ||
| self, | ||
| &Self.scriptedDownloadTokenKey, | ||
| token, | ||
| .OBJC_ASSOCIATION_COPY_NONATOMIC | ||
| ) | ||
| userContentController.addUserScript( | ||
| WKUserScript( | ||
| source: Self.scriptedDownloadInterceptionBootstrapScriptSource(token: token), | ||
| injectionTime: .atDocumentStart, | ||
| forMainFrameOnly: true | ||
| ) | ||
| ) | ||
| userContentController.add( | ||
| Self.sharedScriptedDownloadMessageHandler, | ||
| name: Self.scriptedDownloadMessageHandlerName | ||
| ) | ||
| objc_setAssociatedObject( | ||
| userContentController, | ||
| &Self.scriptedDownloadHandlerInstalledKey, | ||
| NSNumber(value: true), | ||
| .OBJC_ASSOCIATION_RETAIN_NONATOMIC | ||
| ) | ||
| } |
There was a problem hiding this comment.
WKUserContentController causes token-never-set for second web view
The "installed" guard key is on userContentController, but the token is stored on self (the individual CmuxWebView). If two CmuxWebView instances share a WKWebViewConfiguration (and therefore the same userContentController), the second web view hits the early-return and never sets its scriptedDownloadTokenKey. When a scripted download message arrives for that second web view, objc_getAssociatedObject(self, &Self.scriptedDownloadTokenKey) returns nil, the guard let expectedToken fails, and all scripted downloads are silently rejected. The guard and the token should be keyed consistently — either both on self or both on userContentController — and the token should be stored whenever a new CmuxWebView joins the same content controller.
There was a problem hiding this comment.
Fixed in 14b16e7: the scripted-download token is now associated with the shared WKUserContentController, matching the installed-script guard.
— Claude Code
There was a problem hiding this comment.
Addressed in 14b16e7: the install guard and token are now both stored/read on the shared WKUserContentController, so later web views sharing that controller validate against the same token instead of missing a per-view value.
— Claude Code
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Sources/Panels/CmuxWebView+ScriptedDownloads.swift (2)
318-331: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueDocument or assert main-thread assumption for
assumeIsolated.
MainActor.assumeIsolatedrelies on WebKit's contract thatWKScriptMessageHandlercallbacks are always dispatched on the main thread. A comment or debug assertion would make this assumption explicit and catch regressions if WebKit behavior ever changes.📝 Optional improvement
private final class ScriptedDownloadMessageHandler: NSObject, WKScriptMessageHandler { func userContentController( _ userContentController: WKUserContentController, didReceive message: WKScriptMessage ) { guard let webView = message.webView as? CmuxWebView, let body = message.body as? [String: Any] else { return } + // WKScriptMessageHandler callbacks are always dispatched on the main thread per WebKit contract. + assert(Thread.isMainThread, "WKScriptMessageHandler callback expected on main thread") MainActor.assumeIsolated { webView.handleScriptedDownloadMessage(body) } } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift around lines 318 - 331, The `userContentController(_:didReceive:)` method in `ScriptedDownloadMessageHandler` uses `MainActor.assumeIsolated` without documenting the assumption that this WebKit callback always runs on the main thread. Add a comment above or within the method explaining that WebKit guarantees this callback is dispatched on the main thread, and optionally add a debug assertion (such as asserting that the current thread is the main thread) to catch any regressions if WebKit behavior changes in the future.
150-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject
file:URLs from scripted download messages.The prior review flagged that
file:URLs should be rejected from scripted messages since legitimate web pages cannot link to local files. Line 161 still allowsfile:scheme throughinterceptAnchorDownload, and the Swift-sidestartScriptedDownloaddoesn't filter it either. This exposes local file reads to page-initiated scripts if the token were ever compromised.🔒 Proposed fix to reject file: URLs in JS
if (scheme === "blob") { return postBlobURLDownload(href, suggestedFilename); } - if (scheme === "data" || scheme === "http" || scheme === "https" || scheme === "file") { + if (scheme === "data" || scheme === "http" || scheme === "https") { if (scheme === "data" && href.length > maxDataURLCharacters) return false; postURLDownload(href, suggestedFilename); return true; }Alternatively, add a guard in Swift-side
startScriptedDownload:private func startScriptedDownload(_ url: URL, suggestedFilename: String?) { let scheme = url.scheme?.lowercased() ?? "" guard scheme != "file" else { debugContextDownload("browser.scriptdl.start rejected=file_scheme") return } // ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift around lines 150 - 168, Remove the `file:` scheme from the allowed URL schemes in the interceptAnchorDownload function at line 161 where the scheme check currently includes "file" alongside "data", "http", "https", and "http" - this prevents locally stored files from being accessed via scripted downloads. Additionally, add a guard statement in the Swift-side startScriptedDownload method to explicitly reject file:// scheme URLs before processing, as shown in the proposed fix example, to provide defense-in-depth protection against page-initiated script access to local files.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift:
- Around line 318-331: The `userContentController(_:didReceive:)` method in
`ScriptedDownloadMessageHandler` uses `MainActor.assumeIsolated` without
documenting the assumption that this WebKit callback always runs on the main
thread. Add a comment above or within the method explaining that WebKit
guarantees this callback is dispatched on the main thread, and optionally add a
debug assertion (such as asserting that the current thread is the main thread)
to catch any regressions if WebKit behavior changes in the future.
- Around line 150-168: Remove the `file:` scheme from the allowed URL schemes in
the interceptAnchorDownload function at line 161 where the scheme check
currently includes "file" alongside "data", "http", "https", and "http" - this
prevents locally stored files from being accessed via scripted downloads.
Additionally, add a guard statement in the Swift-side startScriptedDownload
method to explicitly reject file:// scheme URLs before processing, as shown in
the proposed fix example, to provide defense-in-depth protection against
page-initiated script access to local files.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 108bc4dc-4fda-4003-bbd3-b947ebf85428
📒 Files selected for processing (2)
Sources/Panels/CmuxWebView+ScriptedDownloads.swiftSources/Panels/CmuxWebView.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Panels/CmuxWebView+ScriptedDownloads.swift (1)
387-389: 🧹 Nitpick | 🔵 Trivial | 💤 Low value
MainActor.assumeIsolatedrelies on WebKit's undocumented threading guarantee.
WKScriptMessageHandlercallbacks are expected on the main thread per WebKit convention, but this isn't formally guaranteed in the API contract. If WebKit ever delivers messages on a background thread, this will crash.Consider using
Task {@mainactorin ... }for defensive dispatch, or add a comment documenting the WebKit threading assumption for future maintainers.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift around lines 387 - 389, The code uses MainActor.assumeIsolated to call webView.handleScriptedDownloadMessage(body), which assumes WebKit delivers callbacks on the main thread without documented guarantee. Replace the assumeIsolated block with a defensive Task { `@MainActor` in ... } wrapper to safely dispatch the webView.handleScriptedDownloadMessage call to the main actor, ensuring it will work correctly even if WebKit ever delivers the message on a background thread. Alternatively, if keeping assumeIsolated, add a clear comment documenting the WebKit threading assumption for future maintainers.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/Panels/CmuxWebView`+ScriptedDownloads.swift:
- Around line 387-389: The code uses MainActor.assumeIsolated to call
webView.handleScriptedDownloadMessage(body), which assumes WebKit delivers
callbacks on the main thread without documented guarantee. Replace the
assumeIsolated block with a defensive Task { `@MainActor` in ... } wrapper to
safely dispatch the webView.handleScriptedDownloadMessage call to the main
actor, ensuring it will work correctly even if WebKit ever delivers the message
on a background thread. Alternatively, if keeping assumeIsolated, add a clear
comment documenting the WebKit threading assumption for future maintainers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e5db2998-98ec-4898-93ab-2b850b97e876
📒 Files selected for processing (3)
Sources/Panels/BrowserDownloadFilenameResolver.swiftSources/Panels/CmuxWebView+ScriptedDownloads.swiftcmuxTests/BrowserDownloadFilenameResolverTests.swift
💤 Files with no reviewable changes (1)
- Sources/Panels/BrowserDownloadFilenameResolver.swift
…ill be re-implemented on the current download architecture The pre-#6258 implementation (save-panel based BrowserDownloadDelegate extensions) no longer matches the download system on main, so this merge takes main's tree wholesale rather than rescuing stale hunks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Closes #6255
Also fixes the PDF download button portion of #4266 by routing
<a download>/ blob-backed downloads through cmux's native save flow. #4266 should remain open for the PDF print button.Testing / proof so far:
python3 scripts/swift_file_length_budget.pyscripts/check-pbxproj.shscripts/lint-pbxproj-test-wiring.shNot run yet: local tagged app build and manual CSV/PDF download verification. Per workspace build-serialization instructions, that waits until CI is green and the user explicitly tells me to launch the dev build.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Aligns our in-app browser’s download behavior with Chrome. Adds native handling for
<a download>links, classifies more responses as downloads, and requires a user gesture for scripted downloads.<a download>; allowblob:/data:only (blobs converted to data URLs); require user activation, per‑tab tokens, 100 MB payload and 140 MB data‑URL caps, 500 ms post rate limit.shouldPerformDownloadin both main and popup navigation delegates by early‑returning.download.Content-Disposition: attachmentand common archive types (text/csv,zip/gzip,octet-stream) as downloads; fall back when MIME can’t render.BrowserDownloadFilenameResolverwith reasoned logs; send only domain/path/secure‑matching cookies on download requests, with tests for cookie scoping.Closes #6255. Fixes the PDF download button portion of #4266 (print button still tracked there).
Written for commit a053488. Summary will update on new commits.
Summary by CodeRabbit
Content-Disposition, includingattachmenthandling and MIME normalization.downloadlinks, including blob/data-url capture, native routing, token validation, and safer filename resolution.Content-Disposition: attachment, and cookie matching/filtering behavior.