HTTP basic authentication password not being asked when using "save as"
Categories
(Toolkit :: Downloads API, defect, P2)
Tracking
()
People
(Reporter: torben.tietze, Assigned: Gijs)
References
(Depends on 1 open bug, Blocks 1 open bug)
Details
(Keywords: multiprocess, regression)
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
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.
Comment 2•6 years ago
|
||
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?
Updated•6 years ago
|
Comment 3•6 years ago
|
||
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]
Comment 4•6 years ago
|
||
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 :(
Comment 5•6 years ago
|
||
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
Updated•6 years ago
|
Updated•6 years ago
|
Assignee | ||
Comment 7•6 years ago
|
||
(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.
Assignee | ||
Comment 8•6 years ago
|
||
Assignee | ||
Comment 9•6 years ago
|
||
So there are a few issues here:
- 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.
- 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.
- 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?
Assignee | ||
Updated•6 years ago
|
Comment 10•6 years ago
|
||
Gijs, I talked things over with Jonathan, here are our findings.
(In reply to :Gijs (he/him) from comment #9)
- 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.
- 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.
- 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().
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.
Comment 11•6 years ago
|
||
Comment 12•6 years ago
|
||
bugherder |
Assignee | ||
Comment 13•6 years ago
|
||
(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)
- 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.
Assignee | ||
Comment 14•6 years ago
|
||
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)
- 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().
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.
Assignee | ||
Comment 15•6 years ago
|
||
Considering this is an older regression, let's not uplift to 66 this late in the cycle.
Comment 16•6 years ago
|
||
(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().
Comment 17•6 years ago
|
||
(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.
Updated•6 years ago
|
Assignee | ||
Comment 18•6 years ago
|
||
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?
Comment 19•6 years ago
|
||
(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?
Assignee | ||
Comment 20•6 years ago
|
||
(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?
- open https://jigsaw.w3.org/HTTP/ (see comment #0
- right click "Basic authentication test"
- click "save link as"
Comment 21•6 years ago
|
||
(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?
- open https://jigsaw.w3.org/HTTP/ (see comment #0
- right click "Basic authentication test"
- click "save link as"
I have try that one but I cannot reproduce it. I see an auth dialog.
Assignee | ||
Comment 22•6 years ago
|
||
(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?
- open https://jigsaw.w3.org/HTTP/ (see comment #0
- right click "Basic authentication test"
- 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:
- comment out https://searchfox.org/mozilla-central/rev/fbb251448feb7276f9b1d0a88f9c0cb1cd144ce4/browser/base/content/nsContextMenu.js#1150-1151 and rebuild (
./mach build faster
will do) - open https://jigsaw.w3.org/HTTP/
- right click "Basic authentication test"
- click "save link as"
Comment 23•6 years ago
|
||
https://searchfox.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1169
we are closing channels with timeout.
Assignee | ||
Comment 24•6 years ago
|
||
(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!
Updated•6 years ago
|
Updated•6 years ago
|
Comment 25•6 years ago
|
||
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.
Assignee | ||
Comment 26•6 years ago
|
||
(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:
- clean profile
- open https://jigsaw.w3.org/HTTP/
- right click link "Basic Authentication Test"
- click "save link as"
- put in a filename and accept the filepicker dialog
- 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.
Comment 27•6 years ago
|
||
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.
Description
•