2.51 KB, patch
Darin Fisher: review+
|Details | Diff | Splinter Review|
9.87 KB, patch
|Details | Diff | Splinter Review|
28.50 KB, patch
Darin Fisher: superreview+
Ben Goodger (use ben at mozilla dot org for email): approval-aviary+
|Details | Diff | Splinter Review|
28.73 KB, patch
|Details | Diff | Splinter Review|
No referrer is sent when a document is loaded as a result of a <META HTTP-EQUIV="refresh">. There are two candidates for a referrer, the original referrer, or the document containing the META tag. Docshell already makes a distinction based on the refresh timer, short ones are considered a "redirect" and longer ones are a "refresh", although it would seem to make more sense to compare the URIs instead. I propose redirects: use the original document's referrer, like HTTP redirects refreshes: use the document being refreshed, i.e. it refers to itself.
What does IE do?
IE doesn't appear to send a referrer, or at least document.referrer doesn't get set. But that doesn't make it right, and I need a referrer to fix 264560.
do you really need a referrer? or do you just need to set that special docshell referrer property on the channel?
Yes, that's all I need. The part that sets that currently gets that from the referrer argument that's also used to set the referrer on the channel. And currently it gets nothing because nsDocShell::Notify() creates a nsDocShellLoadInfo with a null referrer. How can sending a referrer in this case be wrong?
pardon me, nsRefreshTimer::Notify()
> How can sending a referrer in this case be wrong? I simply worry about changing the behavior of the webplatform, especially when it deviates from established behavior (by us and IE). Though the Referer header is technically optional (according to RFC 2616), there are plenty of sites that break if it is not sent, or if it is not the expected value. Sure, you are talking about sending it when it is currently not sent, but might that not screw up web tracking software that doesn't want to count meta refreshes as link clicks? I'm just suggesting that there might be some really wanky web dependencies on this stuff, which may make this a risky change :-/
Darin's right. If this is being proposed for branches, I would be against sending a referrer in the actual HTTP request -- that's a pretty risky change. If this is just being proposed for trunk, it may be ok, but needs a lot of testing.
Anyone got any clues on how to do this cleanly w/o passing the referrer to the http request etc in the docshell code? More arguments to LoadURI etc?
Can't we just set the referrer property in the nsIProperties directly, like suggested in comment 4?
I guess the problem is that the refresh code that would set it doesn't have the channel, so no nsIProperties to set it on.
Ah, I see. Then the answer is to extend loadinfo to indicate whether the referrer should actually be set or not, perhaps? At least in the short run...
Created attachment 164218 [details] [diff] [review] Maybe something like this then? This adds an internalReferrer to nsIDocShellLoadInfo, but I had to move the code that sets it on the channel, that will now happen later, if that's not ok then we'll need to push this info further in...
Yeah, that looks ok from the docshell end. I suspect it's fine from necko too, but I don't recall the exact details of the nsIProperties stuff. Longer-term, perhaps we should think about how to do this more cleanly, but for branch that looks fine.
Created attachment 164229 [details] [diff] [review] Cleaner, I think... This adds a boolean noReferrer to nsIDocShellLoadInfo and makes the code use that to prevent the referrer from being sent to the server in the refresh case. This changes some arguments to InternalLoad() and DoURILoad() in the docshell code, but it's got the benefit of not changing when the internal referrer is set on the channel (through nsIProperties).
Oh, and with the latest diff I'll need to rev nsIDocShell's IID too, done locally...
Is changing nsIDocShellLoadInfo interface on the branch the right thing? In some ways that seems like a worse idea than sending an extra referrer (on the other hand it's probably not that useful an interface for external code). By moving the property setting up to LoadURI you miss Reload and LoadHistoryEntry, but I'm not sure how bad that might be. You could catch them all by doing the seeting in LoadInternal, by adding yet another owner-ish type argument :-) Oh, wait -- nsWebShell::OnLinkClickSync() calls InternalLoad directly, bypassing LoadURI() entirely, so the property will never be set and we'll be back to bug 265135
Previous comments were directed at the "Maybe something like this" patch, the "Cleaner, I think..." patch addresses exactly those concerns. Looks good from a quick scan.
13 years ago
> Oh, and with the latest diff I'll need to rev nsIDocShell's IID too, done locally... best not to change the IID of nsIDocShell. we have embedders (Eclipse) who are using nsIDocShell::loadStream. fortunately, i think this change can be made without rev'ing the IID of nsIDocShell since the interface isn't changing in a binary incompatible way (assuming existing callers of InternalLoad all pass 0x1 for true).
Comment on attachment 164229 [details] [diff] [review] Cleaner, I think... >+ const long INTERNAL_LOAD_NO_REFERRER = 0x2; ... >+ /** True if the no referrer should be sent, even if it's available >+ */ >+ attribute boolean noReferrer; I'd really like a better name for this attribute. noReferrer sounds odd. "If you don't want a referrer, then why are you setting the referrer attribute?" How about if we call it "sendReferrer" with a default value of true? I like "sendReferrer" because it seems more consistent with what actually happens in the implementation. sr=darin with that change.
But I guess you'd still want the sense of the InternalLoad flag to be something like: INTERNAL_LOAD_DONT_SEND_REFERRER
Comment on attachment 164229 [details] [diff] [review] Cleaner, I think... Just to be clear, this patch does not fix *this* bug (send a referrer), it fixes bug 264560. I filed this bug as one way to fix bug 264560, but even apart from that sending the referrer in the refresh case seems like the right thing to do. Just maybe not now for Firefox 1.0 since that change seems to scare smart people. r=dveditz (but please leave this bug open and close 264560 when checked in) I'm not keen on the "noReferrer" name, btw, nor the hard-to-read "!(x & FLAG)" >+ rv = DoURILoad(aURI, aReferrer, !(aFlags & INTERNAL_LOAD_NO_REFERRER), But it's correct and works. Also may want a #define for 0 to make the following clearer: > rv = InternalLoad(uri, > referrerURI, > nsnull, // No owner >- PR_FALSE, // Do not inherit owner from document (security-critical!) >+ 0, // Do not inherit owner from document (security-critical!)
Comment on attachment 164229 [details] [diff] [review] Cleaner, I think... firstname.lastname@example.org
Fix checked in. Leaving bug open, but adding fixed-aviary1.0 to get this off the 1.0 blocker radar.
(In reply to comment #21) > But I guess you'd still want the sense of the InternalLoad flag to be something > like: INTERNAL_LOAD_DONT_SEND_REFERRER Duh, forgot to do this before checking in, I made this change on the branch already tho.
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.
> This was the only checkin for Firefox between yesterday and today that would > have effected Thunderbird as well No, it was not. You're likely seeing fallout from bug 267367... that touched docshell code in a much more fundamental way than this bug did, and would have had exactly the effects you describe.
Comment on attachment 164338 [details] [diff] [review] Final diff that was checked in. this version of the patch still uses INTERNAL_LOAD_FLAGS_NO_REFERRER. was that intentional?
(In reply to comment #29) > (From update of attachment 164338 [details] [diff] [review]) > this version of the patch still uses INTERNAL_LOAD_FLAGS_NO_REFERRER. was that > intentional? > No, see comment 26.
Can sending a referer at least be a togglable option? Many websites (inlcuding my own) use the meta refresh to link people to other pages without that other page knowing where the visitor came from. IIRC Opera 7.2+ is the only browser that does send a referer on a meta refresh. The rest doesn't.
Comment on attachment 164229 [details] [diff] [review] Cleaner, I think... sr=darin for the version that was checked in.
Comment on attachment 163915 [details] [diff] [review] send referrer for meta refresh No, I really do want reviews on this, we should do this at least on the trunk. I'm happy to hear Opera supports this, it bolsters my contention that it won't break anything.
Comment on attachment 163915 [details] [diff] [review] send referrer for meta refresh dveditz: ah, right... ok. so, i'm willing to give this a try on the trunk. does a new patch need to be cut considering the recent changes?
Comment on attachment 163915 [details] [diff] [review] send referrer for meta refresh sr=jst for this change. The other change in this bug didn't land on the trunk yet (should it?), so this would apply against the trunk, me thinks.
Comment on attachment 163915 [details] [diff] [review] send referrer for meta refresh sure, let's go with this on the trunk.
Note that this bug already has one comment about a site it _will_ break... so "won't break anything" is a rather tenuous contention here. The real question is whether we think it will break few enough things that we're ok with breaking them for the theoretical purity of sending referrer on meta refreshes.
hmm, yeah.. i had overlooked comment #31 when i marked r+ on the patch. perhaps we should just take the aviary patch then.
(In reply to comment #31) > Can sending a referer at least be a togglable option? Many websites (inlcuding > my own) use the meta refresh to link people to other pages without that other > page knowing where the visitor came from. What's your site and some of the others? I found a lot of complaints about not sending the referrer in this case, but few using it for user-helpful reasons. The fact that newer Opera versions started sending the referrer in this case may mean they came to the same conclusion. I guess we should run this by WHAT-WG to see if we can get a common approach.
Whatever went in aviary needs to go in 1.7 for gecko compatibility....
Checked into 1.7 branch
What about using "network.http.sendRefererHeader" for meta redirects/refreshes? Will that have any effect on this?
Does this need to land on the trunk? If so, who can do that?
yes, and me, today
aviary fix checked in to trunk. Leaving bug open because the patch really fixes bug 264560, not the summary item (objection in comment 31 noted).
(In reply to comment #39) > What's your site and some of the others? I found a lot of complaints about not > sending the referrer in this case, but few using it for user-helpful reasons. GMail uses this technique to hide the user's authentication credentials when clicking on a hyperlink in an email. It would be a bad idea to change this feature. Currently, this technique is one of the few feasible ways to prevent sending of the Referer header and thus protect the user's privacy/security.
*** Bug 322216 has been marked as a duplicate of this bug. ***
I'm not sure what's going on after it has been a known issue for a year and still not fixed in Firefox 1.5. Opera does it right -- IE6 and Firefox do it the way that actually helps the "bad" guys. A real world problem: Stealing Bandwidth This issue can be exploited to steal bandwidth from a server and its admin won't have ANY chance to see where the hot-linked traffic is coming from. (Let me know if you need me to elaborate.) Incosistency: HTTP Redirect (301/302) does send referers. Meta Refresh redirect does not. (Only Microsoft could have created something as inconsistent as this).
(In reply to comment #48) > A real world problem: Stealing Bandwidth > > This issue can be exploited to steal bandwidth from a server and its admin > won't have ANY chance to see where the hot-linked traffic is coming from. (Let > me know if you need me to elaborate.) Who cares where it's coming from, so long as you can block it? Configure your server to send an error response if the expected Referer header is not present. Problem solved. On the other hand, if there is no way to prevent sending of a Referer header, it becomes very difficult to protect a user's privacy/security when linking from a private resource. For example, GMail relies on the meta refresh technique to hide the URL of the email containing the link. Without the meta refresh technique the target of the hyperlink gets access to the GMail URL, which contains session information (see the auth= query parameter). Changing the behaviour of meta refresh has bad consequences for access control in WWW applications. A lot of security vulnerabilities will be created. These vulnerabilities will also be very hard to close.
Ian, does WHATWG plan to specify this?
There's no "http-equiv" attribute in HTML5 at the moment. Does that answer your question? :-)
(In reply to comment #49) > Who cares where it's coming from, so long as you can block it? Configure your > server to send an error response if the expected Referer header is not present. > Problem solved. By denying traffic with an empty referer you create only a new problem. There are many user agents that do not send referers (in some of them this behaviour is optional, in some it is hard-coded). Then there are those "Internet Security Software Solutions" (firewalls surrounded by other tools) that clear the referer in order "to protect your privacy". All these people would be constantly getting errors when trying to download the "protected" file. The result would be endless hundreds of emails to webmasters saying that their downloads are broken. The bottom line, no, you cannot deny traffic with an empty referer (so your comment "Problem solved" is in fact incorrect). > On the other hand, if there is no way to prevent sending of a Referer header, > it becomes very difficult to protect a user's privacy/security when linking > from a private resource. As we all know, there is no such thing as privacy on the internet. Let's not be naive here. However, if you need to censor the referer, copy the link manually to the adress box and press Enter. Problem solved. > For example, GMail relies on the meta refresh technique to hide the URL of > the email containing the link. I seriously doubt that you heard this from a Google engineer. Why? Because he might be fired for not securing their system properly. Why? 1) Opera does send referers on meta refresh redirection. So about 7% of their visitors would be insecure (yes, according to our stats, 7% of visitors use Opera). 2) Their system (Gmail) must be secured not in this lame way, but in a proper and professional way (and I believe it is) so that it protects Opera users too. When someone gets a referer leading to your Gmail Inbox, he can't be allowed to access that URL without logging in. It is quite easy to do (the session ID may be connected with a particular IP/cookie, etc.) Ask the Yahoo Mail engineers how they did it -- they know better than I do. The bottom line, this argument is invalid too. > Changing the behaviour of meta refresh has bad consequences for access control > in WWW applications. A lot of security vulnerabilities will be created. Name some. (I already named the consequences of not sending the referer). Again, Opera has been doing this right for a long long time and nothing has broken. Your third argument is again invalid.
To summarize: If anyone designed (the security of) his web pages to count on the fact that browsers do not send referers on meta refresh redirection, then it is solely HIS FAULT BECAUSE THIS BEHAVIOUR IS NOT GUARANTEED TO WORK IN ALL BROWSERS (Opera behaves diferrently). It is solely his failure and he will face consequences for it if the default behaviour is changed. Sorry, but if Firefox does not fix this soon, I hope that Google will buy Opera and that it will become the most used browser pushing Firefox into oblivion. Not sending a referer in general is harmful to the health of the entire web infrastructure. (Only a bunch of malevolent hackers could think otherwise.)
From that bug it seems that Gecko is supposed to send a Referer header for meta refresh. However, with Firefox 220.127.116.11 I see that it doesn't. This has some security relevance: http://kuza55.blogspot.com/2007/03/non-persistent-untraceable-xss-attacks.html As to concerns about credentials leaking through referrer headers - all cases that I've seen use a neutral dereferer page that doesn't have any credentials in the URL. The target page should only get the URL of this dereferer page.
Oh, I see - the correct referrer is set internally but not sent to the web page. I think this needs to be fixed.
What is going on here? This is a security-related bug. Why hasn't it been fixed already? Why does it take years? YOU ARE ALLOWING MALEVOLENT PEOPLE TO CLEAR THE REFERRERS TO CONCEAL THEIR XSS ATTACKS, HOT LINKING, AND OTHER MALEVOLENT PRACTICES WITHOUT BEING TRACEBLE BY THE VICTIMS. DO SOMETHING ABOUT IT ALREADY!
Actually there are pages (like GMail) that now depend on this to avoid leaking private information that is stored in their URLs. So arguably this shouldn't be fixed. Note that if you believe not sending referrers is a security problem, it would be helpful to provide feedback to the HTMLWG or WHATWG where I just added rel=noreferrer to explicitly prevent a link from including the referrer header.