Closed Bug 1745650 Opened 2 years ago Closed 2 years ago

If a download upgrades to https via httpsFirst-/httpsOnly - mode it fails

Categories

(Core :: DOM: Security, defect, P2)

Product:
Core
Component:
DOM: Security
Version:
Firefox 95
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
98 Branch
Tracking Flags:
Tracking Status
firefox98 --- fixed

People

(Reporter: sworddragon2, Assigned: t.yavor)

References

(Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(3 files, 1 obsolete file)

Time of a failed upgraded download
14.63 KB, image/png
Details
Bug 1745650 - If a download upgrades to https via httpsFirst-/httpsOnly - mode it fails. r=ckerschb
48 bytes, text/x-phabricator-request
Details | Review
Bug 1745650 - Test case: https - only/ first completes slow download from upgraded site.r=ckerschb
48 bytes, text/x-phabricator-request
Details | Review

Steps to reproduce:

  1. Enable https-only - mode or https-first - mode (which is the default for private browsing).
  2. Download something that is accessible via http and https (for example http://de.download.nvidia.com/Windows/497.09/497.09-desktop-win10-win11-64bit-international-dch-whql.exe ).

Actual results:

The download upgrades to https but an error message pops up claiming that the source file can't be read.

Expected results:

The download should work as usual (e.g. which is the case when explicitely requested via https).

Component: Untriaged → DOM: Security
Product: Firefox → Core
Blocks: 1725390
Flags: needinfo?(lyavor)
Assignee: nobody → lyavor
Flags: needinfo?(lyavor)
Status: UNCONFIRMED → NEW
Ever confirmed: true

After checking multiple requests and their times I guess it is related to the background request.
If that is right, what happens is for https-only and https-first:

  1. Request e.g. http://de.download.nvidia.com/Windows/497.09/497.09-desktop-win10-win11-64bit-international-dch-whql.exe
  2. http://de.download.nvidia.com/Windows/497.09/497.09-desktop-win10-win11-64bit-international-dch-whql.exe gets upgraded to https.
  3. The background timer starts
  4. background timer was successful before download was completed (before https-site loaded completely )
  5. Loading process stops and so the download.

That bug doesn't occur if we aren't upgrading the download link since the background timer only runs for upgraded sites.

Hey Smaug, do you know where in the codebase would be the best place to set an indicator that a download has started loading bits?

Some Background Info:

  • Our https-first-mode tries to upgrade top-level connections from http to https. To avoid long time-outs in case a top-level connection is not available over https, we fire an http background request after 3000 ms to the host we are trying to connect to. That happens within PotentiallyFireHttpRequestToShortenTimout
  • If the http background request returns faster than we start loading bits using the upgraded top-level connection, then we see that as a strong indicator that the upgraded top-level connection will result in a timeout and we fall back to connecting to the top-level page using http. FWIW, that part seems to work fine for top-level requests.
  • Now problem being is that downloads look completely identical to top-level loads in our code, because we only know a load of TYPE_DOCUMENT will result in a download once we would start rendering the content.
  • For top-level loads we set the bit HTTPS_ONLY_TOP_LEVEL_LOAD_IN_PROGRESS within DocumentLoadListener::OnStartRequest to indicate that a top-level load has started.
  • Apparently for downloads we do not do that and we would need to set a similar bit once we know that download has started, so that we do not fire the background request which could kill our download.

I remember that for our data: URI blocking mechanism, we were facing a similar problem, and we did not want to block data: URIs which result in a download. To compensate, we added the check AllowTopLevelNavigationToDataURI() within nsDSURIContentListener::DoContent. Is that a good place, or can you suggest a better place in our codebase where we would know that a download has started and we are about to load bits?

Flags: needinfo?(bugs)

Basti mentioned that we could set the flag within nsExternalAppHandler::OnStartRequest which I think could be a good place.

Thank you both!
Sounds good.

Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]

yeah, externalapphandler should be able to handle this.

AllowTopLevelNavigationToDataURI() case wasn't about download, but whether or not load top level pages, no?

Flags: needinfo?(bugs)

The severity field is not set for this bug.
:ckerschb, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(ckerschb)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/dd8809f8bb92
If a download upgrades to https via httpsFirst-/httpsOnly - mode it fails. r=ckerschb,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/f94ea51101a1
Test case: https - only/ first completes slow download from upgraded site.r=ckerschb

Backed out for causing failures on browser_slow_download.js

[task 2022-01-03T18:12:42.588Z] 18:12:42     INFO - TEST-PASS | dom/security/test/https-first/browser_slow_download.js | File should be downloaded. - 
[task 2022-01-03T18:12:42.588Z] 18:12:42     INFO - Buffered messages finished
[task 2022-01-03T18:12:42.589Z] 18:12:42     INFO - TEST-UNEXPECTED-FAIL | dom/security/test/https-first/browser_slow_download.js | Uncaught exception - DownloadError: Download blocked.
[task 2022-01-03T18:12:42.590Z] 18:12:42     INFO - Stack trace:
[task 2022-01-03T18:12:42.591Z] 18:12:42     INFO - DownloadError@resource://gre/modules/DownloadCore.jsm:1819:16
[task 2022-01-03T18:12:42.592Z] 18:12:42     INFO - DownloadError.fromSerializable@resource://gre/modules/DownloadCore.jsm:1928:11
[task 2022-01-03T18:12:42.592Z] 18:12:42     INFO - Download.fromSerializable@resource://gre/modules/DownloadCore.jsm:1352:36
[task 2022-01-03T18:12:42.593Z] 18:12:42     INFO - D_createDownload@resource://gre/modules/Downloads.jsm:108:39
[task 2022-01-03T18:12:42.594Z] 18:12:42     INFO - _nsITransferInitInternal@resource://gre/modules/DownloadLegacy.jsm:413:15
[task 2022-01-03T18:12:42.595Z] 18:12:42     INFO - initWithBrowsingContext@resource://gre/modules/DownloadLegacy.jsm:315:17
[task 2022-01-03T18:12:42.596Z] 18:12:42     INFO - promptForSaveToFileAsync/<@resource://gre/modules/HelperAppDlg.jsm:317:23
[task 2022-01-03T18:12:42.597Z] 18:12:42     INFO - Leaving test bound test_slow_download
[task 2022-01-03T18:12:42.597Z] 18:12:42     INFO - GECKO(2493) | [Child 2565: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 12 (7fd4ebfc1400) [pid = 2565] [serial = 25] [outer = 0] [url = about:blank]
[task 2022-01-03T18:12:42.598Z] 18:12:42     INFO - GECKO(2493) | [Child 2565: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 11 (7fd4ebfc3800) [pid = 2565] [serial = 27] [outer = 0] [url = about:blank]
[task 2022-01-03T18:12:42.601Z] 18:12:42     INFO - GECKO(2493) | [Child 2565: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 10 (7fd4ebfc5000) [pid = 2565] [serial = 28] [outer = 0] [url = https://example.com/browser/dom/security/test/https-first/file_mixed_content_console.html]
[task 2022-01-03T18:12:42.602Z] 18:12:42     INFO - GECKO(2493) | [Child 2565: Main Thread]: I/DocShellAndDOMWindowLeak --DOMWINDOW == 9 (7fd4ebfcc800) [pid = 2565] [serial = 30] [outer = 0] [url = about:blank]
[task 2022-01-03T18:12:42.603Z] 18:12:42     INFO - GECKO(2493) | [Child 2565: Main Thread]: I/DocShellAndDOMWindowLeak --DOCSHELL 7fd4ebfcb800 == 1 [pid = 2565] [id = 8] [url = https://example.com/browser/dom/security/test/https-first/download_page.html]
[task 2022-01-03T18:12:42.604Z] 18:12:42     INFO - TEST-PASS | dom/security/test/https-first/browser_slow_download.js | Download start page is https -
Flags: needinfo?(lyavor)
Severity: -- → S2
Flags: needinfo?(ckerschb)

We have this one on TRY again with some extended timeout requests for the test itself, which seems to work fine now:
https://treeherder.mozilla.org/jobs?repo=try&revision=8d1aab44319a2fe418ec461c1e3a594530602e47

Going to re-land ...

Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/23081f3b923f
If a download upgrades to https via httpsFirst-/httpsOnly - mode it fails. r=ckerschb,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/7f7864031ae3
Test case: https - only/ first completes slow download from upgraded site.r=ckerschb

Backed out for causing mochitest failures on browser_slow_download.js

Flags: needinfo?(lyavor)

That is related to a defect in another test file: https://bugzilla.mozilla.org/show_bug.cgi?id=1749953

Flags: needinfo?(lyavor)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/efb69ab57dc9
If a download upgrades to https via httpsFirst-/httpsOnly - mode it fails. r=ckerschb,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/98712a0ace1e
Test case: https - only/ first completes slow download from upgraded site.r=ckerschb

Backed out for causing mochitest failures on browser_slow_download.js

Flags: needinfo?(lyavor)
Attachment #9255694 - Attachment is obsolete: true
Flags: needinfo?(lyavor)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/336d3cfecad2
If a download upgrades to https via httpsFirst-/httpsOnly - mode it fails. r=ckerschb,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/32bac3160aa5
Test case: https - only/ first completes slow download from upgraded site.r=ckerschb

Backed out for causing build bustages on browser_download_slow. CLOSED TREE

Backout link : https://hg.mozilla.org/integration/autoland/rev/a5264b7356e480a7bd5850ed649804893daa0ed9

Push with failures : https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=32bac3160aa5053b8c46e82119e5b445ac9e5b6b&selectedTaskRun=UdVVP1b9Tv22ucvWjEmyHw.0

Link to failure log : https://treeherder.mozilla.org/logviewer?job_id=364356808&repo=autoland&lineNumber=64282

Failure message : package-tests> Error: Symlink target path does not exist: /builds/worker/checkouts/gecko/dom/security/test/https-first/browser_download_slow.html

Flags: needinfo?(lyavor)
Flags: needinfo?(lyavor)
Pushed by mozilla@christophkerschbaumer.com:
https://hg.mozilla.org/integration/autoland/rev/34a08545bf69
If a download upgrades to https via httpsFirst-/httpsOnly - mode it fails. r=ckerschb,necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/2f5a2ce06906
Test case: https - only/ first completes slow download from upgraded site.r=ckerschb

https://hg.mozilla.org/mozilla-central/rev/34a08545bf69
https://hg.mozilla.org/mozilla-central/rev/2f5a2ce06906

Status: ASSIGNED → RESOLVED
Closed: 2 years ago
status-firefox98: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 98 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: