267367 - image loading from chrome results in auth prompts if src is protected

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Darin Fisher]

Favicon loading results in annoying auth prompts

On some intranets, where http auth is used, favicon loading can result in a
large number of annoying auth prompt.  It is a major problem if the user does
not have a way to login to content under "/", resulting in a user receiving an
endless stream of auth prompts for the site's favicon.

We should:
1) not be prompting users when loading favicons.
2) remembering that a favicon cannot be loaded and not flooding the network with
pointless favicon requests.

Apparently, IE does not request favicons for https:// links.  Perhaps we should
be doing the same.

This could be a major problem for deployment of firefox at some companies. (hint
hint)

This problem only appears in Firefox 1.0rc1.  It was not a problem in Firefox 1.0PR.

Comment 1

[Mike Kaply [:mkaply]]

I'm going to take the initiative mark this blocking.

This will hold up Firefox usage in IBM.

Comment 2

[Darin Fisher]

ben suggested that this may have been caused by the patch for bug 262262.

Comment 3

[Darin Fisher]

ok, mkaply says that bug 262272 is to blame for this.

Comment 4

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #3)
> ok, mkaply says that bug 262272 is to blame for this.

If backing out that patch fixes this, then it's incidental -- without that patch
there is other broken behaviour (such as not requesting a favicon in many
cases).  I need a test site where I can reproduce this.

Also from darin: the bug appears in PR1 in the case that doesn't cause the bug
where the patch for 262272 fixes it, e.g. a tab with no favicon loading the
site.  This seems to be related to the loading of <image> elements in chrome; at
some point they didn't prompt for auth, now they seem to..

Comment 5

[Darin Fisher]

Attachment: apache test case

This tarball contains files for a simple apache testcase.  Just untar the
contents of these into apache's root content directory (/var/www/html on Fedora
Core 2), and add 'AllowOverride AuthConfig' to apache's httpd.conf file.  Then
restart apache, and visit http://localhost/a/test.html.  You should be prompted
once for test.html (enter "foo" for the username and password).  Next, you will
be prompted for "/favicon.ico" :-(

Comment 6

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Morphing this bug.

The underlying cause is that we prompt for auth for images in chrome whose src
points to content that is on a protected site.  The particular favicon issue
didn't appear in 0.9, because we never even requested the favicon due to various
favicon bugs.  Now that those are fixed, we're seeing this.

Comment 7

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

I suppose this could get fixed by implementing an interface requestor, which
gives out an nsIAuthPrompt, which is implemented as a no-op for the various
methods; and setting that one on the <image>'s channel.

>2) remembering that a favicon cannot be loaded and not flooding the network with
>pointless favicon requests.

hmm, I thought we did that...

Comment 8

[Darin Fisher]

-> me (i have a patch)

Comment 9

[Darin Fisher]

> I suppose this could get fixed by implementing an interface requestor, which
> gives out an nsIAuthPrompt, which is implemented as a no-op for the various
> methods; and setting that one on the <image>'s channel.

we already have nsIAuthPromptProvider for this :-)

my patch extends GetAuthPrompt to not give out an nsIAuthPrompt when it is of
type chrome and has a chrome URI.  jst suggested this check, and it seems to
work well.


> hmm, I thought we did that...

yeah, you're right.

Comment 10

[Darin Fisher]

Attachment: v1 patch

Comment 11

[Darin Fisher]

-> docshell component

Comment 12

[Scott MacVicar]

Isn't this a duplicate of bug #133755

Comment 13

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

Comment on attachment 164334 [details] [diff] [review]
v1 patch

ah, interesting. yeah, I think we may want to do this.

though, what if I have an <html:iframe> in my chrome-privileged extension, and
point that to some pw-protected server (I'm assuming the chrome docshell type
gets inherited...), would it work? if not, do we care? :)

Comment 14

[Johnny Stenback (:jst)]

Comment on attachment 164334 [details] [diff] [review]
v1 patch

+    if (mItemType == typeChrome) {
+	 PRBool isChromeURI;
+	 mCurrentURI->SchemeIs("chrome", &isChromeURI);

Make that if check also check that mCurrentURI is non-null, just so we don't
crash if this is ever called when we have no URI in a docshell.

sr=jst with that change.

Comment 15

[Darin Fisher]

> though, what if I have an <html:iframe> in my chrome-privileged extension, and
> point that to some pw-protected server (I'm assuming the chrome docshell type
> gets inherited...), would it work? if not, do we care? :)

I think that a <html:iframe> would be of type content and not chrome.  Moreover,
this patch checks the URI scheme, so it should not be an issue.

The real issue is <html:img> or even XUL <image> for that matter.  It might be
the case that someone wants to load a image from a remote server into a chrome
XUL document.  But, that's hopefully not too common.  It could be worked around
by putting the image into a HTML frame.

Comment 16

[Darin Fisher]

> Make that if check also check that mCurrentURI is non-null, just so we don't
> crash if this is ever called when we have no URI in a docshell.

will do, and i was also thinking of checking the return value of SchemeIs just
to be safe.

another thing: it might be good to have GetInterface call
GetAuthPrompt(PROMPT_NORMAL) just so we are consistent in our checking.

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 164334 [details] [diff] [review]
v1 patch

a=[e-mail]

Comment 18

[Darin Fisher]

fixed-on-trunk, fixed-aviary1.0

we should probably try to land this on the 1.7 branch too for consistency. 
there may be a way to trigger a similar bug in seamonkey.

Comment 19

[Darin Fisher]

Attachment: v2 patch - the version i actually checked in

Comment 20

[Mike Kaply [:mkaply]]

Comment on attachment 164364 [details] [diff] [review]
v2 patch - the version i actually checked in

a=mkaply for 1.7

nicde job darin

Comment 21

[Boris Zbarsky [:bzbarsky]]

See bug 266554 comment 27.  In brief, this breaks mailnews (because there chrome
docshells need to prompt for login to mail servers).

Comment 22

[Scott MacGregor]

Copying my comments from the wrong bug to this one:

Ack! I think this caused some pretty serious problems with today's Thunderbird
0.9 release. cc'ing myself. In the bits from this morning that we were going to
release, Thunderbird has all sorts of weird wallet problems. 

Between yesterday and today, Thunderbird can no longer log onto mail servers.
Wallet fails to bring up UI for prompting for passwords instead the attempt to
bring up the UI fails (I think) and we end up sending an empty password for
everything.

This effects trying to read IMAP, POP and news that requires auth dialogs.

This was the only checkin for Firefox between yesterday and today that would
have effected Thunderbird as well which is why I'm starting off by pointing the
finger here even if that may not be the case. 

Comment 23

[Darin Fisher]

scott:

yeah, i see that the v2 patch is the culprit.  the change to
nsDocShell::GetInterface prevents mailnews from getting a nsIAuthPrompt.  i
didn't realize that mailnews was using this mechanism to get at the auth prompt :-(

the fix should be simple.  we can just backout my changes to
nsDocShell::GetInterface since that part of the patch is not required to fix
this bug.

Comment 24

[Darin Fisher]

ok, i backed out the changed to nsDocShell::GetInterface w/ a=mscott.

long term, we should probably teach mailnews (and any other chrome app) to use
nsIWindowWatcher instead.

Comment 25

[Darin Fisher]

mkaply: actually, we have to land the patch for bug 205947 on the 1.7 branch as
well if we want to take this one on the 1.7 branch.

Comment 26

[Mike Kaply [:mkaply]]

bug 205947 was already approved for 1.7

Comment 27

[Darin Fisher]

partial backout performed on trunk too

Comment 28

[Mike Kaply [:mkaply]]

205947 landed on 1.7.

The portion of this patch that makes it match what is in aviary need to go in.

Comment 29

[Mike Kaply [:mkaply]]

This patch already went in accidentally with another checkin.

I only took the second part of the patch.

Comment 30

[Boris Zbarsky [:bzbarsky]]

This caused bug 277322 (XMLHttpRequest no longer working in chrome if it needs
auth).