Closed Bug 358878 Opened 13 years ago Closed 13 years ago

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

Categories

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

2.0 Branch
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 3 alpha1

People

(Reporter: Pike, Assigned: mano)

References

()

Details

(Keywords: fixed1.8.1.1, privacy)

Attachments

(2 files, 2 obsolete files)

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.
Should this be fixed for 1.8.1.1 as this is a privacy issue?
Flags: blocking1.8.1.1?
Group: security
Keywords: privacy
This is bad. We should do some kine of announcement that this is a bug and not a desired feature. 
 -> my overloaded plate.
Assignee: nobody → mano
Priority: -- → P1
Target Milestone: --- → Firefox 3 alpha1
Attached patch use data uris (obsolete) — Splinter Review
Attachment #244253 - Flags: review?(gavin.sharp)
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.
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.
Summary: feed preview's favicon should not send referer → Feed preview's request for favicon.ico should not send Referer or cookies
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.
*** Bug 359060 has been marked as a duplicate of this bug. ***
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 :(
Attachment #244253 - Flags: review?(gavin.sharp) → review-
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.
Attached patch patch (obsolete) — Splinter Review
Note this does not address the cookies thing.
Attachment #244253 - Attachment is obsolete: true
Attachment #244511 - Flags: review?(gavin.sharp)
And please separate this out to another bug (which also applies to the feed preferences pane, by the way).
Summary: Feed preview's request for favicon.ico should not send Referer or cookies → Feed preview's request for favicon.ico should not send Referer
Attachment #244511 - Flags: review?(gavin.sharp) → review+
Attached patch as checked inSplinter Review
mozilla/browser/components/feeds/src/FeedWriter.js 1.24
Attachment #244511 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
(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?)
(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.
Flags: blocking1.8.1.1? → blocking1.8.1.1+
Attached patch 1.8 branch patchSplinter Review
Use "src" attribute and workaround bug 360220.
Attachment #246221 - Flags: review?(gavin.sharp)
Attachment #246221 - Flags: approval1.8.1.1?
Attachment #246221 - Flags: review?(gavin.sharp) → review+
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!
Attachment #246221 - Flags: approval1.8.1.1? → approval1.8.1.1+
1.8 branch: mozilla/browser/components/feeds/src/FeedWriter.js  1.2.2.26
Keywords: fixed1.8.1.1
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!
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
Has this been fixed in the 2.x branch?
Martin: yes it did, see comment 21 and the keywords above.
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.