Closed Bug 1529901 Opened 6 years ago Closed 6 years ago

HTTP basic authentication password not being asked when using "save as"

Categories

(Toolkit :: Downloads API, defect, P2)

Unspecified
All
defect

Tracking

()

VERIFIED FIXED
mozilla67
Tracking Status
firefox-esr60 --- wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- verified
firefox68 --- verified

People

(Reporter: torben.tietze, Assigned: Gijs)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: multiprocess, regression)

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36

Steps to reproduce:

Right click on a hyperlink that is protected by HTTP basic authentication and press "save page as..."

For example on https://jigsaw.w3.org/HTTP/ the link "Basic Authentication test".

OS: Ubuntu 16.04
Firefox: 65

OS: Ubuntu 18.04
Firefox: 65

OS: MacOS High Sierra 10.13
Firefox: 65

Actual results:

I get asked where to save the file. After confirming, the download is listed in the download manager. It isn't finishing but the progress animation is going. The values are showing 0 Bytes and 0 Bytes/sec

Expected results:

I should have been asked where to save the file and after confirming. There should have been a dialog to ask for credentials for the basic authenticated file. Like other Browsers do.

Hi reporter,

Thank you for taking the time to add this report.

I was able to reproduce the behavior you mentioned across the latest Firefox Versions, on Windows 10, Mac OS 10.14 and Ubuntu 16.04. As an addition to the actual result listed above, the download cannot be canceled from the download manager as the "Cancel Download" button is no longer responsive.

I'm going to add this to Core:Networking: HTTP component for a more advised input. If this is not the proper component for this issue please feel free to change it to a more appropriate one.

Status: UNCONFIRMED → NEW
Component: Untriaged → Networking: HTTP
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → Core
Version: 65 Branch → Trunk

The download is kinda success in esr 60 (download the forbidden page without popping a prompt), but it should ask for auth after bug 315227 and bug 597374.

Do you mind to take a look, Honza?

Flags: needinfo?(honzab.moz)
Component: Networking: HTTP → Downloads API
Flags: needinfo?(honzab.moz)
Product: Core → Toolkit

I see a [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIWebBrowserPersist.savePrivacyAwareURI]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 536" data: no]

Priority: -- → P2

I guess "Save Link As..." instead of "save page as..." in comment #0.

I can reproduce the issue on Nightly67.0a1 Windows10.
However I can not reproduce the issue if e10s is disabled.

So, this is implementation bug of e10s :(

There are three regression bug.
#1 No prompt. But, download finished.
#2 No prompt. But, browser crashed.
#3 No prompt. No crash. But, download is never finished.

#1 Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b3b42ca813513fa7c30eaca07cb84bffcaf933b6&tochange=93b37aa497c48a6e28a9463eeb753b2ce3964f42

Suspect: c31b663b4dd2 Dragana Damjanovic — Bug 1409449 - Do not show auth-dialog for triggeringPrincipal==SystemPrincipal. r=ckerschb r=valentin r=francois

#2 Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=5d23b6e0b74d74eee52d7af5fa47573ef8afe979&tochange=9a2b02fe351bd65f6850a5c80b91dd0eec4a878a

Crash: 9a2b02fe351b Gijs Kruitbosch — Bug 1469916, r=ckerschb,jkt

#3 Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=8a7fd632104453b557ca73a335626d4f24427b50&tochange=ddb29b481834c16a608ff6e4dd8382155239a94f

Fixed only crash: ddb29b481834 Gijs Kruitbosch — Bug 1473507 - fix crash in nsILoadInfo::GetOriginAttributes when passing no principal to SavePrivacyAwareURI, r=mccr8

Has Regression Range: --- → yes
Has STR: --- → yes

Gijs any idea what's going on here?

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Julien Cristau [:jcristau] from comment #6)

Gijs any idea what's going on here?

[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsIWebBrowserPersist.savePrivacyAwareURI]" nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: chrome://global/content/contentAreaUtils.js :: internalPersist :: line 536" data: no]
internalPersist chrome://global/content/contentAreaUtils.js:536
continueSave chrome://global/content/contentAreaUtils.js:451
internalSave chrome://global/content/contentAreaUtils.js:405

Is what's going on (stack from 66 beta 10).

I'll see if I can find time to have a more in-depth look tomorrow.

So there are a few issues here:

  1. we used to always use system principal for saves. bug 1398229 made us use the node's principal. That seems to have just not worked for the http auth case. I don't really know why. I don't even know if the preflight request ever did anything better when it did use system principal (ie maybe it didn't work before we started passing the node's principal). Anyway, now we're passing loading principal. I find this a little odd; is loading principal really the best idea if we have webpage 1 (html) linking to webpage 2 (also html) ? Shouldn't it be triggering principal? It's just a link, not an included image or other included resource.
  2. there was a fallback that "just" called saveURL without using a principal. That used to use system principal because no principal was present. Then bug 1409449 made us not show an http auth prompt for that.
  3. then I made webbrowserpersist / saving anything require a principal, in bug 1469916. But this code didn't pass one, so it crashed, until I fixed bug 1473507. I thought I got all the consumers there (ie added principals everywhere), clearly I was wrong. I blame the insane amount of indirection we have for saving anything - there must be at least 10 different utility functions that eventually call internalSave, and they get called all over the shop.

Anyway... the trivial fix is to just add a principal to the fallback case. I've added a patch for that. But I'm confused about why the preflight request doesn't handle the http auth prompt itself, ie why are we hitting the fallback? Christoph/Baku, can you check both why we're not handling http auth for the initial request and whether loading principal is really right here, or should the initial request use triggering principal, too?

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(ckerschb)
Flags: needinfo?(amarchesini)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED

Gijs, I talked things over with Jonathan, here are our findings.

(In reply to :Gijs (he/him) from comment #9)

  1. we used to always use system principal for saves. bug 1398229 made us use the node's principal. That seems to have just not worked for the http auth case. I don't really know why. I don't even know if the preflight request ever did anything better when it did use system principal (ie maybe it didn't work before we started passing the node's principal). Anyway, now we're passing loading principal. I find this a little odd; is loading principal really the best idea if we have webpage 1 (html) linking to webpage 2 (also html) ? Shouldn't it be triggering principal? It's just a link, not an included image or other included resource.

Right, so good we are not using the SystemPrincipal anymore, that was definitely wrong. Jonathan and I are with you, we should in fact use the triggeringPrincipal (which we suppose is this.principal) instead of the node's principal.

  1. there was a fallback that "just" called saveURL without using a principal. That used to use system principal because no principal was present. Then bug 1409449 made us not show an http auth prompt for that.

Right - that sounds good as well - using SystemPrincipal was definitely wrong.

  1. then I made webbrowserpersist / saving anything require a principal, in bug 1469916. But this code didn't pass one, so it crashed, until I fixed bug 1473507. I thought I got all the consumers there (ie added principals everywhere), clearly I was wrong. I blame the insane amount of indirection we have for saving anything - there must be at least 10 different utility functions that eventually call internalSave, and they get called all over the shop.

So, that quirks we have accumulated over time within internalSave() to query the right principal [1] makes me nervous quite a bit. We should file a follow up bug, adding some kind of assertion (similar to what we did for providing the top-level triggeringPrincipal) that we always pass an explicit TriggeringPrincipal to internalSave().

[1] https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/toolkit/content/contentAreaUtils.js#432-435

Anyway... the trivial fix is to just add a principal to the fallback case. I've added a patch for that. But I'm confused about why the preflight request doesn't handle the http auth prompt itself, ie why are we hitting the fallback? Christoph/Baku, can you check both why we're not handling http auth for the initial request and whether loading principal is really right here, or should the initial request use triggering principal, too?

The patch you have looks good, but I can't help you with the http auth problem - I have no idea why we are hitting the fallback. Probably Dragana or someone from the Necko team can be of more help with that problem.

Flags: needinfo?(ckerschb)
Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/autoland/rev/0aee72fddeb9 saving resources behind http auth should work, r=ckerschb
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)

Gijs, I talked things over with Jonathan, here are our findings.

(In reply to :Gijs (he/him) from comment #9)

  1. we used to always use system principal for saves. bug 1398229 made us use the node's principal. That seems to have just not worked for the http auth case. I don't really know why. I don't even know if the preflight request ever did anything better when it did use system principal (ie maybe it didn't work before we started passing the node's principal). Anyway, now we're passing loading principal. I find this a little odd; is loading principal really the best idea if we have webpage 1 (html) linking to webpage 2 (also html) ? Shouldn't it be triggering principal? It's just a link, not an included image or other included resource.

Right, so good we are not using the SystemPrincipal anymore, that was definitely wrong. Jonathan and I are with you, we should in fact use the triggeringPrincipal (which we suppose is this.principal) instead of the node's principal.

No, my point is that we're passing a principal to channel creation, but we pass it as the loadingPrincipal, and I think we should be passing as the triggering principal. It'd be the same principal. I wonder if that's why we're not getting a dialog and so on. Though I haven't tested it, and I'm not sure I fully understand what the consequence of passing triggering instead of loading principal would be. Ni for this.

Flags: needinfo?(gijskruitbosch+bugs)

Sigh. Ni for comment 13 which I was in the process of writing when I accidentally submitted...

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #10)

  1. then I made webbrowserpersist / saving anything require a principal, in bug 1469916. But this code didn't pass one, so it crashed, until I fixed bug 1473507. I thought I got all the consumers there (ie added principals everywhere), clearly I was wrong. I blame the insane amount of indirection we have for saving anything - there must be at least 10 different utility functions that eventually call internalSave, and they get called all over the shop.

So, that quirks we have accumulated over time within internalSave() to query the right principal [1] makes me nervous quite a bit. We should file a follow up bug, adding some kind of assertion (similar to what we did for providing the top-level triggeringPrincipal) that we always pass an explicit TriggeringPrincipal to internalSave().

[1] https://searchfox.org/mozilla-central/rev/92d11a33250a8e368c8ca3e962e15ca67117f765/toolkit/content/contentAreaUtils.js#432-435

So we just don't do anything if we don't get a triggering principal. Not sure how much an assertion (from JS?) would help, but ni for me to file a followup for this, I guess.

Anyway... the trivial fix is to just add a principal to the fallback case. I've added a patch for that. But I'm confused about why the preflight request doesn't handle the http auth prompt itself, ie why are we hitting the fallback? Christoph/Baku, can you check both why we're not handling http auth for the initial request and whether loading principal is really right here, or should the initial request use triggering principal, too?

The patch you have looks good, but I can't help you with the http auth problem - I have no idea why we are hitting the fallback. Probably Dragana or someone from the Necko team can be of more help with that problem.

And ni for me to also investigate this further unless someone else comments who knows (maybe baku, who wrote some of this helper code). But I have too much stuff to juggle to do it right now.

Flags: needinfo?(ckerschb)

Considering this is an older regression, let's not uplift to 66 this late in the cycle.

(In reply to :Gijs (he/him) from comment #14)

So we just don't do anything if we don't get a triggering principal. Not sure how much an assertion (from JS?) would help, but ni for me to file a followup for this, I guess.

FWIW, I filed https://bugzilla.mozilla.org/show_bug.cgi?id=1532723 to actually make sure we always pass a non null principal to internalSave().

Flags: needinfo?(ckerschb)

(In reply to :Gijs (he/him) from comment #13)

No, my point is that we're passing a principal to channel creation, but we pass it as the loadingPrincipal, and I think we should be passing as the triggering principal. It'd be the same principal. I wonder if that's why we're not getting a dialog and so on. Though I haven't tested it, and I'm not sure I fully understand what the consequence of passing triggering instead of loading principal would be. Ni for this.

In most cases the triggeringPrincipal and the LoadingPrincipal are identical. If you pass a loadingPrincipal and keep the triggeringPrincipal empty, then internally we set the LoadingPrincipal as the triggeringPrincipal as well.

Dragana, do you know why the code in https://searchfox.org/mozilla-central/rev/fbb251448feb7276f9b1d0a88f9c0cb1cd144ce4/browser/base/content/nsContextMenu.js#1105-1163 doesn't pop up auth prompts (or at least, doesn't manage to do so within 4 seconds on anyone's machine who's tried, triggering the fallback in onStopRequest as a result of the timer firing) ? Are we missing some load flag or passing one that implicitly inhibits the auth prompt or something?

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dd.mozilla)

(In reply to :Gijs (he/him) from comment #18)

Dragana, do you know why the code in https://searchfox.org/mozilla-central/rev/fbb251448feb7276f9b1d0a88f9c0cb1cd144ce4/browser/base/content/nsContextMenu.js#1105-1163 doesn't pop up auth prompts (or at least, doesn't manage to do so within 4 seconds on anyone's machine who's tried, triggering the fallback in onStopRequest as a result of the timer firing) ? Are we missing some load flag or passing one that implicitly inhibits the auth prompt or something?

do you have a test case?

Flags: needinfo?(dd.mozilla) → needinfo?(gijskruitbosch+bugs)

(In reply to Dragana Damjanovic [:dragana] from comment #19)

(In reply to :Gijs (he/him) from comment #18)

Dragana, do you know why the code in https://searchfox.org/mozilla-central/rev/fbb251448feb7276f9b1d0a88f9c0cb1cd144ce4/browser/base/content/nsContextMenu.js#1105-1163 doesn't pop up auth prompts (or at least, doesn't manage to do so within 4 seconds on anyone's machine who's tried, triggering the fallback in onStopRequest as a result of the timer firing) ? Are we missing some load flag or passing one that implicitly inhibits the auth prompt or something?

do you have a test case?

  1. open https://jigsaw.w3.org/HTTP/ (see comment #0
  2. right click "Basic authentication test"
  3. click "save link as"
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(dd.mozilla)

(In reply to :Gijs (he/him) from comment #20)

(In reply to Dragana Damjanovic [:dragana] from comment #19)

(In reply to :Gijs (he/him) from comment #18)

Dragana, do you know why the code in https://searchfox.org/mozilla-central/rev/fbb251448feb7276f9b1d0a88f9c0cb1cd144ce4/browser/base/content/nsContextMenu.js#1105-1163 doesn't pop up auth prompts (or at least, doesn't manage to do so within 4 seconds on anyone's machine who's tried, triggering the fallback in onStopRequest as a result of the timer firing) ? Are we missing some load flag or passing one that implicitly inhibits the auth prompt or something?

do you have a test case?

  1. open https://jigsaw.w3.org/HTTP/ (see comment #0
  2. right click "Basic authentication test"
  3. click "save link as"

I have try that one but I cannot reproduce it. I see an auth dialog.

Flags: needinfo?(dd.mozilla)

(In reply to Dragana Damjanovic [:dragana] from comment #21)

(In reply to :Gijs (he/him) from comment #20)

(In reply to Dragana Damjanovic [:dragana] from comment #19)

(In reply to :Gijs (he/him) from comment #18)

Dragana, do you know why the code in https://searchfox.org/mozilla-central/rev/fbb251448feb7276f9b1d0a88f9c0cb1cd144ce4/browser/base/content/nsContextMenu.js#1105-1163 doesn't pop up auth prompts (or at least, doesn't manage to do so within 4 seconds on anyone's machine who's tried, triggering the fallback in onStopRequest as a result of the timer firing) ? Are we missing some load flag or passing one that implicitly inhibits the auth prompt or something?

do you have a test case?

  1. open https://jigsaw.w3.org/HTTP/ (see comment #0
  2. right click "Basic authentication test"
  3. click "save link as"

I have try that one but I cannot reproduce it. I see an auth dialog.

Yes, courtesy of the fallback mechanism... the question is, why do we need the fallback mechanism to get an auth dialog?

Perhaps this is more clear:

  1. comment out https://searchfox.org/mozilla-central/rev/fbb251448feb7276f9b1d0a88f9c0cb1cd144ce4/browser/base/content/nsContextMenu.js#1150-1151 and rebuild (./mach build faster will do)
  2. open https://jigsaw.w3.org/HTTP/
  3. right click "Basic authentication test"
  4. click "save link as"
Flags: needinfo?(dd.mozilla)
Flags: needinfo?(dd.mozilla)

(In reply to Dragana Damjanovic [:dragana] from comment #23)

https://searchfox.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1169

we are closing channels with timeout.

Thanks!

Blocks: 597374
Flags: needinfo?(amarchesini)
Depends on: 1534311
Flags: qe-verify+

I managed to reproduce the issue on Windows 10 x64 on an older Nightly (2019-02-22).
However, I can still reproduce this issue using the latest Nightly 68.0a1 and beta 67.0b8 on Windows 10x64, Ubuntu 18.04 x64.
On macOS 10.13, I can reproduce the issue only on beta 67.0b8, on Latest Nightly 68.0a1 it seems to be fixed.

Flags: needinfo?(gijskruitbosch+bugs)

(In reply to Oana Botisan, Desktop Release QA from comment #25)

I managed to reproduce the issue on Windows 10 x64 on an older Nightly (2019-02-22).
However, I can still reproduce this issue using the latest Nightly 68.0a1 and beta 67.0b8 on Windows 10x64, Ubuntu 18.04 x64.
On macOS 10.13, I can reproduce the issue only on beta 67.0b8, on Latest Nightly 68.0a1 it seems to be fixed.

It wfm on beta on mac. Here are my steps:

  1. clean profile
  2. open https://jigsaw.w3.org/HTTP/
  3. right click link "Basic Authentication Test"
  4. click "save link as"
  5. put in a filename and accept the filepicker dialog
  6. put guest as both username and password in the auth dialog that comes up and accept the auth dialog.

Then the file gets saved, the downloads panel opens (because it's the first download) and shows the file as saved. Opening the file shows a "Your browser made it" message, indicating http auth was successful.

Please clarify if you're doing something different or are seeing different results with the same steps, or something else.

Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(oana.botisan)

I misunderstood. I thought that the authentication password should appear before you select a folder where you save the file. I don't know why I thought that was the logical course of action.

Thank you for the complete steps. I managed to reproduce the issue using an older build of Nightly from 2019-02-22.

I managed to verify the fix by using the steps in comment 26 on latest Nightly 68.0a1 and beta 67.0b8 on Windows 10 x64, macOS 10.13 and Ubuntu 18.04 x64. The bug is not reproducing anymore.

Status: RESOLVED → VERIFIED
Flags: needinfo?(oana.botisan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: