Downloading of a file from a password protected directory fails

RESOLVED FIXED in mozilla2.0b7

Status

()

Core
Networking
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Thomas Braun, Assigned: mayhemer)

Tracking

({regression})

Trunk
mozilla2.0b7
x86
Windows XP
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+, blocking1.9.2 -, status1.9.2 wanted, status1.9.1 unaffected)

Details

(Whiteboard: [has workaround], URL)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

7 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; de-DE; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10 ( .NET CLR 3.5.30729)

When trying to download a file via a https:// from a password protected directory, Firefox asks for the user name and password. After entering the user credentials, nothing more happens - also copying the link into the browsers address bar does not work.

When putting the link in the address bar after *restarting* the browser, the download is working as expected.

I already checked with CharlesProxy and it seems as if Firefox does not send any credentials (or anything else) at all to the web server after entering username/password.

The supplied URL demonstrates the problem.

Reproducible: Always

Steps to Reproduce:
1. go to the URL
2. right-click on the link -> "save file as..."
3. enter the following username and password: mozilla / test
Actual Results:  
nothing happens

Expected Results:  
File should be downloaded

I already talked to moo / Thomas B. Ruecker (dm8tbr@afthd.tu-darmstadt.de) on the #firefox.de IRC channel and he could reproduce the problem as well.
For completeness: here's my build ID
Mozilla/5.0 (X11; U; Linux i686; de; rv:1.9.2.10) Gecko/20100914 Firefox/3.6.10
(Reporter)

Comment 2

7 years ago
using a newly created user profile does not help either
(Reporter)

Comment 3

7 years ago
The problem is not limited to HTTPS-URLs, but also applies to non-SSL connections, Testcase URL is http://www.ellis-events.com/download.htm, Username and password is the same.
(Reporter)

Comment 4

7 years ago
I now checked some older Firefox versions plus the current 4.0 Beta 6:

Firefox 3.0  

   Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.0.14) Gecko/2009082707 Firefox/3.0.14

and 

Firefox 3.5.7

   Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.1.7) Gecko/20091221 Firefox/3.5.7

behave both as expected.

The current 4.0 beta:

   Mozilla/5.0 (Windows NT 5.1; rv:2.0b6) Gecko/20100101 Firefox/4.0b6

shows the same problem as 3.6.10
I think this is enough info for the bug to be reproducible.

Tested latest trunk.
Status: UNCONFIRMED → NEW
Ever confirmed: true
As this is obviously a regression, we could also need a regressionwindow.
Keywords: regression, regressionwindow-wanted
(Reporter)

Updated

7 years ago
Summary: Downloading of a file via HTTPS from a password protected directory fails → Downloading of a file from a password protected directory fails

Comment 7

7 years ago
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/247524e19d0c
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090720 Minefield/3.6a1pre
Fails:
http://hg.mozilla.org/mozilla-central/rev/f2a58ffcd00c
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090721 Minefield/3.6a1pre
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=247524e19d0c&tochange=f2a58ffcd00c

Comment 8

7 years ago
In local build,
The following changeset causes the probrem.
d6895cb5ac3b	Honza Bambas — Bug 475053 - Implement asyncPromptAuth to fix multiple password prompt overlap

Updated

7 years ago
Keywords: regressionwindow-wanted
Blocks: 475053
blocking2.0: --- → ?
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
Version: unspecified → Trunk
blocking1.9.2: --- → ?
(Reporter)

Comment 9

7 years ago
If I save the user credentials for the downloaded resource, I get the following, slightly different behaviour:

- the first attempt to "save as..." shows the user/password dialog with the username and password already filled in, but the download fails.

- A second attempt (*without* closing the browser) succeeds, the file gets downloaded then.
Did this work in Firefox 3.6, or 3.5? I.e. is it only a problem in Firefox 4 betas or is this an older problem...?

Comment 11

7 years ago
This problem happens on 3.6 and 4.0b7pre.
3.5 works fine.

And See comment #4
And comment 7.  I suspect the sync prompt thing is a good regression candidate here.... and that we do want to fix this on 1.9.2 if we can.
(Reporter)

Comment 13

7 years ago
Am I right to assume that a fix probably will not make it into 3.6.11 as it is only 4 days to go until code freeze and only 3 blocking issues left?
Unless some sort of magic happens, that's probably true....
Not going to "block" a branch release (we've managed to live with it for 10 updates), but we'll look at the patch when it's done for trunk. It probably should block Firefox 4.
blocking1.9.2: ? → -
status1.9.1: --- → unaffected
status1.9.2: --- → wanted
Honza, please see comment 8. Blocking.
Assignee: nobody → honzab.moz
blocking2.0: ? → betaN+
(Assignee)

Comment 17

7 years ago
Looking into this.
Status: NEW → ASSIGNED
(Assignee)

Comment 18

7 years ago
nsContextMenu (in its js file) in saveAs command assigns its own listener to the channel.  Before nsContextMenu asyncOpens the channel it starts a timer that fires in one second and cancels the channel with NS_ERROR_SAVE_LINK_AS_TIMEOUT.  The timer is canceled in the listener's OnStartRequest.

Workaround: set browser.download.saveLinkAsFilenameTimeout to 60000.  It will make the code wait for one minute instead of just a single second.

Going to investigate why this worked before the patch for bug 475053.
Whiteboard: [has workaround]
(Assignee)

Comment 19

7 years ago
Here's why: when the timer cancels the channel, nsHttpChannel::Cancel cancels wait for the auth prompt and gets OnAuthCancelled with userCancel=false.  In case of userCancel=false it doesn't cancel+respin the transaction and therefor it hangs forever suspended.  And, it keeps the cache entry and therefor the download is no more possible until firefox restart.  nsHttpChannel needs to be patched.

However, there is also a different bug.  In bug 299372 we introduced a code that waits a second for the server response to get the filename from content-disposition headers, if this times out, we do it an older way, w/o waiting for the header to do not make user wait.  This new code, however, doesn't count with a pop-up of the authentication dialog.  Nor it counts with many other things like url classification, async cache lookup, all that may delay OnStartRequest.  Starting the timer before AsyncOpen is wrong IMO.

I have to think of some API we already have to use it to start the timer.
See Also: → bug 299372
(Assignee)

Comment 20

7 years ago
Patching nsHttpChannel doesn't help to fix this.  We get both the auth prompt and save dialog (from the legacy save uri system) on the screen.
(Assignee)

Comment 21

7 years ago
Created attachment 478561 [details] [diff] [review]
v1

Fix for both problems.  Not sure of the reviewer for the nsContextMenu.js changes.
Attachment #478561 - Flags: review?(mconnor)
Attachment #478561 - Flags: review?(bzbarsky)
Comment on attachment 478561 [details] [diff] [review]
v1

the magic wheel of "not actually me" lands on.... dolske!
Attachment #478561 - Flags: review?(mconnor) → review?(dolske)
Comment on attachment 478561 [details] [diff] [review]
v1

>+++ b/browser/base/content/nsContextMenu.js
...
>   saveLink: function() {
>     // canonical def in nsURILoader.h
>     const NS_ERROR_SAVE_LINK_AS_TIMEOUT = 0x805d0020;
>+    // defined in nsISocketTransport.idl
>+    const NS_NET_STATUS_WAITING_FOR = 0x804b000a;

The previous line has the excuse that the value is in a .h (and, sadly, not in Components.results), but for the one you're adding you should just use Ci.nsISocketTransport.STATUS_WAITING_FOR.

>+      onStatus: function sLA_callbacks_onStatus(aRequest, aContext, aStatus, aHost) {
>+        if (aStatus != NS_NET_STATUS_WAITING_FOR)
>+          return;

I don't think this is right (2nd half of comment 19, too).

The problem bug 299372 was fixing (well, improving what had earlier been fixed) was to not have clicking "Save As" do nothing for a long period of time before showing UI. EG, consider being on a terrible connection where it takes a minute to resolve the host and successfully connect. AIUI, we won't get NS_NET_STATUS_WAITING_FOR until after that point, so this reverts us back to the original bug. Thus, I think we really need to start the timer immediately.

It's a little sucky to have the auth prompt come up while we're showing the Save As dialog, but I don't see an easy way around that. Followup fodder for finding a way to suppress the auth prompt until after the Save As has been responded to? I suppose we could try suspending the channel while the dialog is up (if the timeout fired)? I'm a bit wary of making non-authenticated downloads slower (because we wouldn't be slurping down data while waiting for the user), but maybe that's ok since the site is already slow to respond to the request...

I think we need a test here too. Should be testable via a .sjs that deliberately does not respond to a request?
Attachment #478561 - Flags: review?(dolske) → review-
(Assignee)

Comment 24

7 years ago
OK, if we do not authenticate to the site, we don't have a chance to get the proper header anyway.  So, I suggest to cancel the channel and fall back to the old way when the channel asks for an auth prompt.  We then can run the timer before we open the channel (what I really don't like, but understand why we need it).
Honza, what's the nsHttpChannel change (which I assume is what you want me to review) actually doing?
(Assignee)

Comment 26

7 years ago
(In reply to comment #25)
> Honza, what's the nsHttpChannel change (which I assume is what you want me to
> review) actually doing?

The 'userCancel' argument is true when user presses the cancel button in the auth dialog (refuse to provide credentials).  It is false when the queued authentication task has been canceled by something else then user, e.g. cancellation of the channel, as in our case.

However, while we wait for the auth prompt result (result here is also user or internal cancellation of the dialog) the transaction is always suspended.  So, we always have to resume it after we get the result, whatever it is.

We was not doing that for internal cancellation, that was making the channel hang forever, also with the cache entry.  I changed then the condition to just check the transaction is non-null.

We are apparently missing a test for this.
(Assignee)

Comment 27

7 years ago
Created attachment 481178 [details] [diff] [review]
v2 [Check in comment 30]

- the channel is canceled after it demands an auth prompt
- other remains unchanged

To preserve the file name determination even we must prompt for credentials would be to cancel the timer when the channel wants an auth prompt, provide an auth prompt, and start the timer again after a notification comes to the progress listener, whatever notification it is - the only way to figure out the credentials were provided.  This is relatively complex, but if we really want it, I can have a patch and a good test for it.

I'll add a test anyway and update the patch with a test ones again.
Attachment #478561 - Attachment is obsolete: true
Attachment #481178 - Flags: superreview?(bzbarsky)
Attachment #481178 - Flags: review?(dolske)
Attachment #478561 - Flags: review?(bzbarsky)
Comment on attachment 481178 [details] [diff] [review]
v2 [Check in comment 30]

r=me for the nsContextMenu.js change.
Attachment #481178 - Flags: review?(dolske) → review+
Comment on attachment 481178 [details] [diff] [review]
v2 [Check in comment 30]

sr=me
Attachment #481178 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 30

7 years ago
Comment on attachment 481178 [details] [diff] [review]
v2 [Check in comment 30]

http://hg.mozilla.org/mozilla-central/rev/d9fdddb0a30e
Attachment #481178 - Attachment description: v2 → v2 [Check in comment 30]
(Assignee)

Updated

7 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
(Reporter)

Comment 31

7 years ago
Now that this is fixed (thanks to everybody involved) - what are the chances that this fix will find its way into 3.6.12 and not only 4.x?
Blocks: 467530
Blocks: 607560
No longer blocks: 467530
Target Milestone: --- → mozilla2.0b8
Comment on attachment 481178 [details] [diff] [review]
v2 [Check in comment 30]

>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
>@@ -953,24 +953,27 @@ nsContextMenu.prototype = {
>     function callbacks() {}
>     callbacks.prototype = {
>       getInterface: function sLA_callbacks_getInterface(aIID) {
>         if (aIID.equals(Ci.nsIAuthPrompt) || aIID.equals(Ci.nsIAuthPrompt2)) {
>-          var ww = Cc["@mozilla.org/embedcomp/window-watcher;1"].
>-                   getService(Ci.nsIPromptFactory);
>-          return ww.getPrompt(doc.defaultView, aIID);
>+          // If the channel demands authentication prompt, we must cancel it
>+          // because the save-as-timer would expire and cancel the channel
>+          // before we get credentials from user.  Both authentication dialog
>+          // and save as dialog would appear on the screen as we fall back to
>+          // the old fashioned way after the timeout.
>+          timer.cancel();
>+          channel.cancel(NS_ERROR_SAVE_LINK_AS_TIMEOUT);
>         }
>         throw Cr.NS_ERROR_NO_INTERFACE;
>       } 
>     }

Is it actually intended that this code/case now throws instead of returning (something)?
If so, I'd say it lacks an explicit comment.
(Assignee)

Comment 33

7 years ago
(In reply to comment #32)
> Is it actually intended that this code/case now throws instead of returning
> (something)?

It's actually the fix for this bug.  

> If so, I'd say it lacks an explicit comment.

And I'd say the comment is enough to explain what we are doing here, isn't it?
(In reply to comment #33)

> It's actually the fix for this bug.  

Ah, unusual but if it is then fine.
Thanks for the confirmation.

> And I'd say the comment is enough to explain what we are doing here, isn't it?

Well, I know little about this code and the comment seemed related to the 2 .cancel(), not to the s/return/throw/.
I would have expected at least something like a '// fall through' to be (even more) explicit.
I guess NS_ERROR_NO_INTERFACE after succeeding .equals() confuses me by default :-|
Target Milestone: mozilla2.0b8 → mozilla2.0b7
You need to log in before you can comment on or make changes to this bug.