315227 - Failed download when using "Save Link As..." on a link to an page / file that uses HTTP/FTP authentication.

Status: VERIFIED FIXED

(Toolkit :: Downloads API, defect)

Description

[N/A]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20051025 Firefox/1.5

When using "Save Link As..." on a link to a page or file (PDF, etc.) that uses HTTP authentication (not yet authenticated) simply brings up the "Save As" dialog instead of the "Enter User Name and Password" dialog. This results in failed download (saves 401 error page).

Works as expected in 1.0x (prompts user for Authentication, then "Save As").

Reproducible: Always

Steps to Reproduce:
1.Find a link to a HTTP authenticated page.
2.Right Click on link -> "Save Link As..."


Actual Results:  
"Save As" dialog is displayed and an 401 error page is saved no matter the file type.

Expected Results:  
"Enter User Name and Password" dialog is displayed for authentication before the "Save As" dialog.

Possibly related to: 308715 (404 page is saved).

Works as expected in 1.0x (prompts user for Authentication, then "Save As").

Comment 1

[Mike Connor [:mconnor]]

Jesse, can you provide a testcase and confirmation before asking for blocking? Thanks!

Comment 2

[mails4andy]

I can confirm this with
Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060224 Firefox/1.6a1

A testcase cannot be applied as an attachment, so I have created an online testcase under http://www.andy-online.de/bug315227/index.html

I hope that helps.

Comment 3

[maenneph]

Confirming this with nightly build.

(Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a1) Gecko/20060521 Minefield/3.0a1)

Comment 4

[Jackie Liu]

I can also confirm with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1) Gecko/20060925 BonEcho/2.0.  Showstopper for Firefox 2?

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

This isn't a 1.5->2.0 regression, and while we're sorry it didn't get fixed, it's too late to deal with it now and not serious enough to block.

Putting it on the radar for Firefox 3, though.

Comment 6

[Jesse Ruderman]

*** Bug 352810 has been marked as a duplicate of this bug. ***

Comment 7

[Jesse Ruderman]

*** Bug 355861 has been marked as a duplicate of this bug. ***

Comment 8

[OstGote!]

Same thing for SeaMonkey. Is there any reason why this is declared as FF only? I did not find any Suite or Core bug for this problem. IMHO this bug should be Core/File Handling (or Network...). 

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

note that ff and seamonkey have different code here, though I'm going to fix it for both

Comment 10

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch (firefox)

I don't think there's a good way to unit test this, unfortunately...

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch (seamonkey)

Comment 12

[[e-mail]]

Comment on attachment 266250 [details] [diff] [review]
patch (seamonkey)

r- because cancelling the prompt doesn't cancel the download. Instead it gets stuck in a limbo state, and you have to cancel it manually.

Note that the transfer tries to open the download manager / progress window while the persist is trying to prompt, which is suboptimal.

>+  this.window = win;
You don't use this at all, because you always pass the global window.

>+  this.transfer = transfer;
You don't use this either, except via makeClosure.

>+  // Now... we need to forward all calls to our transfer
If only __noSuchMethod__ worked... but it doesn't :-(

>+  QueryInterface: function dl_qi(iid)
Nit: aIID

Comment 13

[[e-mail]]

(In reply to comment #12)
>r- because cancelling the prompt doesn't cancel the download.
Looks like an issue with the tree I tested it with :-\ Two other trees are ok.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v1.1

nits fixed, except for the this.window one. I prefer passing this in from the caller.

Comment 15

[[e-mail]]

Comment on attachment 268856 [details] [diff] [review]
patch v1.1

I guess this is OK although I had a look into the source and it looks as if it would be much neater to add this to nsDownloadProxy instead.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

nsDownloadProxy doesn't have access to the nsIDOMWindow

Comment 17

[[e-mail]]

(In reply to comment #16)
>nsDownloadProxy doesn't have access to the nsIDOMWindow
I was expecting you to pass it in from the caller :-P

Comment 19

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 268856 [details] [diff] [review]
patch v1.1

>+  getInterface: function dl_gi(aIID)
>+  {
>+    if (aIID.equals(Components.interfaces.nsIAuthPrompt)) {
>+      return ww.getNewAuthPrompter(this.window);

>+    if (aIID.equals(Components.interfaces.nsIAuthPrompt2)) {
>+      return ww.getPrompt(this.window, aIID);

Is there a reason you didn't just use getPrompt for both of these, and factor out the common code? getPrompt(window, nsIAuthPrompt) seems to be equivalent to getNewAuthPrompter(window).

Shouldn't we fail the download on a 403, rather than save the error (e.g. if you cancel the password prompt)?

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

sure, I can use getPrompt for both

Yes, we probably should fail. But we should also fail 404 pages, etc. Do you think I should do that as part of this patch?

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 268856 [details] [diff] [review]
patch v1.1

(In reply to comment #20)
> Yes, we probably should fail. But we should also fail 404 pages, etc. Do you
> think I should do that as part of this patch?

No, I didn't realize we didn't fail for other HTTP errors.

Comment 22

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: final patch

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Checking in suite/common/contentAreaUtils.js;
/cvsroot/mozilla/suite/common/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.144; previous revision: 1.143
done
Checking in toolkit/content/contentAreaUtils.js;
/cvsroot/mozilla/toolkit/content/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.90; previous revision: 1.89
done
Checking in xpfe/communicator/resources/content/contentAreaUtils.js;
/cvsroot/mozilla/xpfe/communicator/resources/content/contentAreaUtils.js,v  <--  contentAreaUtils.js
new revision: 1.141; previous revision: 1.140
done

Comment 25

[Stephen Donner [:stephend] Not actively reading bugmail]

http://litmus.mozilla.org/manage_testcases.cgi

in-litmus+

Comment 27

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED using:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.9a9pre) Gecko/2007102704 Minefield/3.0a9pre

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a9pre) Gecko/2007102705 Minefield/3.0a9pre

and

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007102704 Minefield/3.0a9pre