358878 - Feed preview's request for favicon.ico should not send Referer

Status: RESOLVED FIXED

(Firefox Graveyard :: RSS Discovery and Preview, defect, P1)

Description

[Axel Hecht [:Pike]]

Just setting the image of a menuitem is apparently sending a referer when getting that image, which may be ok in the general context of a xul menu.

In the context of feed preview though, it sends the URL of the users feed to the service, at least when that service manages to not get his favicon cached.

We should fix that, maybe by hooking a loader object in the middle explicitly and use data urls, or just a x-no-referer protocol or so.

Comment 1

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

http://www.legroom.net/modules.php?op=modload&name=News&file=article&sid=215 implies that this bug should probably be open ...

Comment 2

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

Should this be fixed for 1.8.1.1 as this is a privacy issue?

Comment 3

[Asa Dotzler [:asa]]

This is bad. We should do some kine of announcement that this is a bug and not a desired feature. 

Comment 4

[Asaf Romano (gone)]

 -> my overloaded plate.

Comment 5

[Asa Dotzler [:asa]]

now reported at http://www.heise-security.co.uk/news/80310

Comment 6

[Asaf Romano (gone)]

Attachment: use data uris

Comment 7

[Jesse Ruderman]

Does this patch also fix the cookie issue, so cookies aren't shared for this request, and so if you have the "ask me every time a site tries to set a cookie" option set you won't see the dialog?

Is it ok to treat all favicon.ico files as image/x-icon, ignoring the mime type sent by the server?  I'm guessing imagelib sniffs anyway.

Comment 8

[Jesse Ruderman]

http://www.legroom.net/modules.php?op=modload&name=News&file=article&sid=215 says:

"I find it very odd that Firefox 2.0 loads the Google favicon, and only the Google favicon, remotely for the Feed Preview page, when the favicons for three other services displayed in the same page are loaded locally."

Do we know why he saw that behavior?  To me it looks like the code loads favicon.ico remotely from all the services.

Comment 9

[Jared]

Jesse, I'm the one that posted the information on legroom.  No, I don't know why Google was the only site I saw that exhibited this behavior.  It was confirmed to happen for Yahoo as well according to the Heise article and another Firefox user that had e-mailed me privately over the weekend, but I haven't had the time to look into it any further myself.  It may very well load the icon from all other servers, though for whatever reason in the few test cases I tried on my system I only saw traffic going to Google.

If you'd like I'd be happy to test further and see what I can find.  I see a lot of big names already included on the comments in this bug, though, so I'm not sure how much information I could add at this point, but I'm certainly willing to help.  Just let me know.

Comment 10

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

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

Comment 11

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

Comment on attachment 244253 [details] [diff] [review]
use data uris

>Index: browser/components/feeds/src/FeedWriter.js

>       var ios = Cc["@mozilla.org/network/io-service;1"].
>                 getService(Ci.nsIIOService);
>       for (var i = 0; i < handlers.length; ++i) {
>         menuItem = this._document.createElementNS(XUL_NS, "menuitem");
>         menuItem.className = "menuitem-iconic";
>         menuItem.setAttribute("label", handlers[i].name);
>         menuItem.setAttribute("handlerType", "web");
>         menuItem.setAttribute("webhandlerurl", handlers[i].uri);
>-
>-        var uri = ios.newURI(handlers[i].uri, null, null);
>-        menuItem.setAttribute("image", uri.prePath + "/favicon.ico");
>-
>         handlersMenuPopup.appendChild(menuItem);
>+
>+        // For privacy reasons we cannot set the image attribute directly
>+        // to the icon url, see Bug 358878
>+        var uri = makeURI(handlers[i].uri);
>+        if (uri) 
>+          new iconDataURIGenerator(uri.prePath + "/favicon.ico", menuItem);

ios is now unused (and the one in _getContainer too).

Note that http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp&rev=1.296&mark=3820#3810 asks your notificationCallbacks object for nsIPrompt, which it currently doesn't implement, so an exception is thrown (from QI). Since that code doesn't seem to actually do anything with the prompt (why does SetCookieStringFromHttp have an aPrompt argument that it doesn't use?) you can probably just get away with adding nsIPrompt to your QI to silence the warning.

Copying this code over really sucks, but I can't think of a better solution :(

Comment 12

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

Also note that this patch doesn't fix the fact that the cookie is sent with the response - we could fix that using the technique described at http://developer.mozilla.org/en/docs/Creating_Sandboxed_HTTP_Connections#Handling_cookies to avoid receiving or sending cookies for these requests.

Perhaps Darin or biesi could answer the question in comment 11, or provide any suggestions for fixing this another way.

Comment 13

[Asaf Romano (gone)]

Attachment: patch

Note this does not address the cookies thing.

Comment 14

[Asaf Romano (gone)]

And please separate this out to another bug (which also applies to the feed preferences pane, by the way).

Comment 15

[Asaf Romano (gone)]

Attachment: as checked in

mozilla/browser/components/feeds/src/FeedWriter.js 1.24

Comment 16

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

(In reply to comment #12)
> Perhaps Darin or biesi could answer the question in comment 11, or provide any
> suggestions for fixing this another way.

You mean this part?
> Copying this code over really sucks, but I can't think of a better solution :(

Which code is that referring to? (And where was it copied from?)

Comment 17

[Asaf Romano (gone)]

nsSearchService.js

Comment 18

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

(In reply to comment #16)
> You mean this part?
> > Copying this code over really sucks, but I can't think of a better solution :(

No, I meant the question about SetCookieStringFromHttp having an aPrompt argument that it doesn't use (though if you have other suggestions for a better solution I'm all ears :). I hadn't realized that it was a public interface, I guess that explains it.

Comment 19

[Asaf Romano (gone)]

Attachment: 1.8 branch patch

Use "src" attribute and workaround bug 360220.

Comment 20

[Jay Patel [:jay]]

Comment on attachment 246221 [details] [diff] [review]
1.8 branch patch

Approved for 1.8.1 branch, a=jay for drivers.  Please land this asap, thanks!

Comment 21

[Asaf Romano (gone)]

1.8 branch: mozilla/browser/components/feeds/src/FeedWriter.js  1.2.2.26

Comment 22

[Jay Patel [:jay]]

Jared B. or anyone else that is able: Could you please help us verify this fix?  Please retest with the latest "-mozilla1.8/" nightly build from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/ and let us know if things work better for you.  Please replace "fixed1.8.1.1" with the "verified1.8.1.1" keyword if everything look good.  Thanks!

Comment 23

[Brendan Eich [:brendan]]

There's no reason to call splice in the base64 encoder -- it mutates the array gratuitously, and at linear cost, which compounds quadratically.  Walking an index through the array is much, much cheaper.  If this._bytes needs to be truncated at the end of the b64 function, then that can be done once by setting length to 0.

/be

Comment 24

[Martin Jürgens]

Has this been fixed in the 2.x branch?

Comment 25

[Marek Stępień [:marcoos, inactive]]

Martin: yes it did, see comment 21 and the keywords above.