Properly handle responses that should trigger download action #3407

miasma13 · 2024-09-27T13:39:06Z

Task/Issue URL: https://app.asana.com/0/414709148257752/1208359719493827/f

Description:
We should be more deliberate on handling download triggers e.g. with links with download attribute, BLOB content, responses with Content-Disposition: attachment header.

Steps to test this PR:
See parent task for set of download scenarios to test. Please compare behavior with version from before changes.

Definition of Done (Internal Only):

Does this PR satisfy our Definition of Done?

Internal references:

Software Engineering Expectations
Technical Design Template

…d set to true

miasma13 · 2024-09-27T13:39:33Z

DuckDuckGo.xcodeproj/project.pbxproj

@@ -816,7 +817,6 @@
 		B6BA95C328891E33004ABA20 /* BrowsingMenuAnimator.swift in Sources */ = {isa = PBXBuildFile; fileRef = B6BA95C228891E33004ABA20 /* BrowsingMenuAnimator.swift */; };
 		B6BA95C528894A28004ABA20 /* BrowsingMenuViewController.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = B6BA95C428894A28004ABA20 /* BrowsingMenuViewController.storyboard */; };
 		B6BA95E828924730004ABA20 /* JSAlertController.storyboard in Resources */ = {isa = PBXBuildFile; fileRef = B6BA95E728924730004ABA20 /* JSAlertController.storyboard */; };
-		B6CB93E5286445AB0090FEB4 /* Base64DownloadSession.swift in Sources */ = {isa = PBXBuildFile; fileRef = B6CB93E4286445AB0090FEB4 /* Base64DownloadSession.swift */; };


No longer used anywhere.

miasma13 · 2024-09-27T13:40:20Z

Core/PixelEvent.swift

@@ -1326,7 +1325,6 @@ extension Pixel.Event {
        case .cachedTabPreviewRemovalError: return "m_d_tpre"

        case .missingDownloadedFile: return "m_d_missing_downloaded_file"
-        case .unhandledDownload: return "m_d_unhandled_download"


Looking at the code path and confirming in Kibana this pixel is/was not being fired.

miasma13 · 2024-09-27T13:43:26Z

DuckDuckGo.xcodeproj/project.pbxproj

@@ -6517,6 +6516,7 @@
 				F1D43AF92B99C1D300BAB743 /* BareBonesBrowserKit */,
 				9F8FE9482BAE50E50071E372 /* Lottie */,
 				9F96F73A2C9144D5009E45D5 /* Onboarding */,
+				1E5918462CA422A7008ED2B3 /* Navigation */,


Required for WKNavigationAction, HTTPURLResponse and other navigation related extensions.

miasma13 · 2024-09-27T13:44:42Z

DuckDuckGo/TabViewController.swift


-                } else {


Logical order of if-else has been reversed here.

miasma13 · 2024-09-27T13:50:22Z

DuckDuckGo/TabViewController.swift

@@ -167,6 +168,9 @@ class TabViewController: UIViewController {
    var mostRecentAutoPreviewDownloadID: UUID?
    private var blobDownloadTargetFrame: WKFrameInfo?

+    // Recent request's URL if its WKNavigationAction had shouldPerformDownload set to true
+    private var recentNavigationActionShouldPerformDownloadURL: URL?


This is used to hold and pass information from WKNavigationAction's shouldPerformDownload read in:

func webView(_ webView: WKWebView, decidePolicyFor navigationAction: WKNavigationAction, decisionHandler: @escaping (WKNavigationActionPolicy) -> Void)

to be used for decision logic later in:

func webView(_ webView: WKWebView, decidePolicyFor navigationResponse: WKNavigationResponse, decisionHandler: @escaping (WKNavigationResponsePolicy) -> Void)

and

func webView(_ webView: WKWebView, navigationResponse: WKNavigationResponse, didBecome download: WKDownload)

miasma13 · 2024-09-27T13:53:55Z

DuckDuckGo/TabViewController.swift

@@ -1742,6 +1758,8 @@ extension TabViewController: WKNavigationDelegate {
        }

        if navigationAction.isTargetingMainFrame(),
+           !navigationAction.isSameDocumentNavigation,
+           !navigationAction.shouldDownload,


These are required changes for GPC protected sites not to break IFrame triggered downloads, as canceling and re-triggering request breaks the download attribute. This matches the logic on macOS where for page sub-requests we do not trigger the full distributed navigation chain execution. Aligned and accepted by the privacy team.

github-actions · 2024-10-05T00:19:01Z

This PR has been inactive for more than 7 days and will be automatically closed 7 days from now.

github-actions · 2024-10-13T00:21:02Z

This PR has been closed after 14 days of inactivity. Feel free to reopen it if you plan to continue working on it or have further discussions.

brindy · 2024-10-15T12:48:26Z

DuckDuckGo/TabViewController.swift

+        let hasContentDispositionAttachment = httpResponse?.shouldDownload ?? false
+        // If preceding WKNavigationAction requested to start the download
+        let hasNavigationActionRequestedDownload = (recentNavigationActionShouldPerformDownloadURL != nil) && recentNavigationActionShouldPerformDownloadURL == navigationResponse.response.url
+        // File can be rendered by web view or in custom preview handled by FilePreviewHelper
+        let canLoadOrPreviewTheFile = navigationResponse.canShowMIMEType || FilePreviewHelper.canAutoPreviewMIMEType(mimeType)
+
+        let shouldTriggerDownloadAction = hasContentDispositionAttachment || hasNavigationActionRequestedDownload || !canLoadOrPreviewTheFile


None of this is used more than once and the shouldTriggerDownloadAction is used after a bunch of other evaluations. How about making that a private function that is called instead at the time it's relevant?

brindy · 2024-10-15T12:49:04Z

DuckDuckGo/TabViewController.swift

+            Pixel.fire(pixel: .downloadStarted,
+                       withAdditionalParameters: [PixelParameters.canAutoPreviewMIMEType: "1"])
+        } else if shouldTriggerDownloadAction,
+                  let downloadMetadata = AppDependencyProvider.shared.downloadManager.downloadMetaData(for: navigationResponse.response) {


Now would be a really nice time to refactor out this singleton 😇

brindy

~~Something isn't quite right. I happened to be on this page and it triggers a download~~

~~https://duckduckgo.com/email/~~

~~But also doing a search triggers the download.~~

Edit: my fault, I changed some code when testing.

…ension

brindy

LGTM, thanks!

…p-dependency-provider * main: bump bsk for content blocker rules fix (#3445) speculative fix for set bars visibility crashes (#3442) Release 7.141.0-1 (#3443) Fix browsing menu bottom offset when bar location set to bottom (#3440) Properly handle responses that should trigger download action (#3407)

* main: Remove `SubscriptionFeatureAvailability` from `AppDependencyProvider` (#3447) Release 7.141.0-2 (#3451) Do not notify the FE on experiment activation (#3450) point to bsk branch (#3444) bump bsk for content blocker rules fix (#3445) speculative fix for set bars visibility crashes (#3442) Release 7.141.0-1 (#3443) Fix browsing menu bottom offset when bar location set to bottom (#3440) Properly handle responses that should trigger download action (#3407)

# By Daniel Bernal (4) and others # Via Daniel Bernal (1) and others * main: Pixel retrying (#3358) Remove `voiceSearchHelper` from `AppDependencyProvider` (#3452) Update AutoClearSettingsViewController to use DI for app settings (#3448) Bump BSK (#3441) Remove `SubscriptionFeatureAvailability` from `AppDependencyProvider` (#3447) Release 7.141.0-2 (#3451) Do not notify the FE on experiment activation (#3450) point to bsk branch (#3444) bump bsk for content blocker rules fix (#3445) speculative fix for set bars visibility crashes (#3442) Release 7.141.0-1 (#3443) Fix browsing menu bottom offset when bar location set to bottom (#3440) Properly handle responses that should trigger download action (#3407) Add Events Firing for Phishing Detection Settings: Point to BSK (#3423) DuckPlayer: Temporary Fix for Watch In Youtube (#3437) Add 'Open in New Tab' support for DuckPlayer (#3431) update BSK dependency (#3434) Release 7.141.0-0 (#3435) Add error handling to contrainer removal (#3424) Prevent autofill prompt crash for edge case where a context menu is also visible on screen (#3417) # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

miasma13 added 9 commits September 23, 2024 14:17

Remove unused session

f6b0728

Do not trigger GPC flow for same document navigation

8424a6e

Use safely unwrapped requests

df92b4b

Store URL for recent WKNavigationAction that had shouldPerformDownloa…

2ac18a7

…d set to true

Fix triggering the downloads when appropriate

7c77365

Blob type URLs need to be downloaded first

0bb7a0d

Fix checks order

5b7429a

Clean up naming

780d323

Clean up path for handling BLOB downloads

7e731fd

miasma13 commented Sep 27, 2024

View reviewed changes

DuckDuckGo/TabViewController.swift

} else {

Copy link

Contributor Author

miasma13 Sep 27, 2024

Choose a reason for hiding this comment

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

Logical order of if-else has been reversed here.

miasma13 commented Sep 27, 2024

View reviewed changes

github-actions bot added the stale label Oct 5, 2024

github-actions bot closed this Oct 13, 2024

Make showing custom preview for specific mimetypes a higher priority

5f83fc8

miasma13 reopened this Oct 14, 2024

miasma13 changed the base branch from main to release/7.141.0 October 14, 2024 16:46

miasma13 requested a review from brindy October 14, 2024 16:49

github-actions bot removed the stale label Oct 15, 2024

brindy reviewed Oct 15, 2024

View reviewed changes

brindy requested changes Oct 15, 2024

View reviewed changes

miasma13 added 2 commits October 16, 2024 11:12

Share logic for determining if should trigger download

83cdecf

Extend logic to resolve download MIMEtype by also looking at file ext…

8a359fb

…ension

brindy approved these changes Oct 16, 2024

View reviewed changes

miasma13 merged commit 70b3d55 into release/7.141.0 Oct 16, 2024
13 checks passed

miasma13 deleted the michal/improve-downloads branch October 16, 2024 12:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Properly handle responses that should trigger download action #3407

Properly handle responses that should trigger download action #3407

miasma13 commented Sep 27, 2024 •

edited

Loading

miasma13 Sep 27, 2024

miasma13 Sep 27, 2024

miasma13 Sep 27, 2024

miasma13 Sep 27, 2024

miasma13 Sep 27, 2024

miasma13 Sep 27, 2024

github-actions bot commented Oct 5, 2024

github-actions bot commented Oct 13, 2024

brindy Oct 15, 2024

brindy Oct 15, 2024

brindy left a comment •

edited

Loading

brindy left a comment

Properly handle responses that should trigger download action #3407

Properly handle responses that should trigger download action #3407

Conversation

miasma13 commented Sep 27, 2024 • edited Loading

Internal references:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 5, 2024

github-actions bot commented Oct 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brindy left a comment • edited Loading

Choose a reason for hiding this comment

brindy left a comment

Choose a reason for hiding this comment

miasma13 commented Sep 27, 2024 •

edited

Loading

brindy left a comment •

edited

Loading