Last Comment Bug 358878 - Feed preview's request for favicon.ico should not send Referer
: Feed preview's request for favicon.ico should not send Referer
Status: RESOLVED FIXED
: fixed1.8.1.1, privacy
Product: Firefox
Classification: Client Software
Component: RSS Discovery and Preview (show other bugs)
: 2.0 Branch
: All All
: P1 normal with 3 votes (vote)
: Firefox 3 alpha1
Assigned To: Mano (::mano, needinfo? for any questions; not reading general bugmail)
:
Mentors:
http://lxr.mozilla.org/mozilla/source...
: 359060 (view as bug list)
Depends on: 360220 363318 365570
Blocks:
  Show dependency treegraph
 
Reported: 2006-10-31 04:21 PST by Axel Hecht [:Pike]
Modified: 2011-08-05 22:28 PDT (History)
23 users (show)
dveditz: blocking1.8.1.1+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use data uris (8.52 KB, patch)
2006-10-31 14:45 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
gavin.sharp: review-
Details | Diff | Splinter Review
patch (11.21 KB, patch)
2006-11-02 16:01 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
gavin.sharp: review+
Details | Diff | Splinter Review
as checked in (10.97 KB, patch)
2006-11-04 00:40 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
no flags Details | Diff | Splinter Review
1.8 branch patch (9.85 KB, patch)
2006-11-21 15:49 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
gavin.sharp: review+
jaymoz: approval1.8.1.1+
Details | Diff | Splinter Review

Description Axel Hecht [:Pike] 2006-10-31 04:21:45 PST
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] 2006-10-31 07:08:55 PST
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] 2006-10-31 07:21:23 PST
Should this be fixed for 1.8.1.1 as this is a privacy issue?
Comment 3 Asa Dotzler [:asa] 2006-10-31 12:18:41 PST
This is bad. We should do some kine of announcement that this is a bug and not a desired feature. 
Comment 4 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-10-31 12:44:04 PST
 -> my overloaded plate.
Comment 5 Asa Dotzler [:asa] 2006-10-31 14:19:41 PST
now reported at http://www.heise-security.co.uk/news/80310
Comment 6 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-10-31 14:45:00 PST
Created attachment 244253 [details] [diff] [review]
use data uris
Comment 7 Jesse Ruderman 2006-10-31 15:43:39 PST
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 2006-10-31 15:47:43 PST
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 2006-10-31 18:13:42 PST
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: gavin@gavinsharp.com] 2006-11-01 10:00:04 PST
*** Bug 359060 has been marked as a duplicate of this bug. ***
Comment 11 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-11-02 06:43:41 PST
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: gavin@gavinsharp.com] 2006-11-02 06:48:10 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-02 16:01:11 PST
Created attachment 244511 [details] [diff] [review]
patch

Note this does not address the cookies thing.
Comment 14 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-02 16:03:19 PST
And please separate this out to another bug (which also applies to the feed preferences pane, by the way).
Comment 15 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-04 00:40:21 PST
Created attachment 244661 [details] [diff] [review]
as checked in

mozilla/browser/components/feeds/src/FeedWriter.js 1.24
Comment 16 Christian :Biesinger (don't email me, ping me on IRC) 2006-11-04 07:43:29 PST
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-04 07:46:54 PST
nsSearchService.js
Comment 18 :Gavin Sharp [email: gavin@gavinsharp.com] 2006-11-04 09:57:53 PST
(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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-21 15:49:26 PST
Created attachment 246221 [details] [diff] [review]
1.8 branch patch

Use "src" attribute and workaround bug 360220.
Comment 20 Jay Patel [:jay] 2006-11-22 14:35:05 PST
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 Mano (::mano, needinfo? for any questions; not reading general bugmail) 2006-11-23 04:25:40 PST
1.8 branch: mozilla/browser/components/feeds/src/FeedWriter.js  1.2.2.26
Comment 22 Jay Patel [:jay] 2006-11-29 18:05:18 PST
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] 2006-12-11 11:50:01 PST
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 2007-03-07 06:14:07 PST
Has this been fixed in the 2.x branch?
Comment 25 Marek Stępień [:marcoos, inactive] 2007-03-07 06:16:58 PST
Martin: yes it did, see comment 21 and the keywords above.

Note You need to log in before you can comment on or make changes to this bug.