User Agent: Mozilla/5.0 (X11; Linux i686; rv:5.0) Gecko/20100101 Firefox/5.0 Build ID: 20110615151330 Steps to reproduce: anchor href="javascript:..." syntax does not work anymore for apps in Facebook Iframe with Firefox TEST CASE: 1) Go to http://apps.facebook.com/nuxfootprints/ (no Facebook account is needed, you can be completely logged out of Facebook) 2) Click on the test link "anchor href=javascript test" Actual results: Javascript code does not run. Nothing is reported in firebug either. It simply does nothing. This issue repros in Firefox 5.0 in Linux ,and Firefox 8.0 in Windows. Note: before upgrading to Firefox 8.0 in Windows, I tried this in Firefox 3.something in Windows and it worked. It also works in IE and Chrome. Expected results: Javascript code should be executed. This issue basically breaks my app in Facebook with Firexfox.

For what it's worth, it's working fine for me on current nightlies (FF11 nightlies)

Reproduced: Mozilla/5.0 (X11; Linux x86_64; rv:8.0) Gecko/20100101 Firefox/8.0 WFM: Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9.2.24) Gecko/20111103 Firefox/3.6.24 Mozilla/5.0 (X11; Linux x86_64; rv:9.0) Gecko/20100101 Firefox/9.0 Mozilla/5.0 (X11; Linux x86_64; rv:10.0a2) Gecko/20111114 Firefox/10.0a2 Mozilla/5.0 (X11; Linux x86_64; rv:11.0a1) Gecko/20111114 Firefox/11.0a1 Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/14.0.835.202 Safari/535.1 Opera/9.80 (X11; Linux x86_64; U; en) Presto/2.9.168 Version/11.52

Thomas, thank you for testing that! I was about to ask whether it worked on beta. Since it does, it should be fixed once we ship Firefox 9, on Dec 20. On nightlies, I see this start working in this range: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=630e28e90986&tochange=f69a10f23bf3 That's the same fix range as bug 699893, and similar symptoms. Sicking, any idea what might have fixed this?

No idea, no.

My bisect ended up on the versio bump. And indeed, if I take Firefox 8 and load the testcase in it, I see the bug. If I then change the UA string of Firefox 8 to this: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:9.0) Gecko/20100101 Firefox/9.0 (no code changes; just change the UA string) then the bug disappears. So there's some sort of broken UA sniffing going on on the Facebook end.

And ccing the browser-bugs address, in hopes that the Facebook folks will actually notice this.

Added comments to Facebook's bug reports about this issue and linked back to this bug report: http://developers.facebook.com/bugs/197517616988383 http://developers.facebook.com/bugs/282419221777641

This is being caused by our X-Content-Security-Policy-Report-Only header, which we are sending to Firefox <= 8, but not Firefox 9 until we've tested Firefox 9. Doesn't make sense that the *-Report-Only header is blocking anything, though. The content of the header is: allow *;script-src https://*.facebook.com http://*.facebook.com https://*.fbcdn.net http://*.fbcdn.net *.facebook.net *.google-analytics.com *.virtualearth.net *.google.com 127.0.0.1:* *.spotilocal.com:*;options inline-script eval-script;report-uri http://www.facebook.com/csp.php

Marshall, thanks! That's enough info for me to fix on our end. Though you may still want to stop sending that header for down-rev Firefox.... :( bsterne, nsJSChannel is not a writable property bag, and NS_NewChannel currently errors out if there is a non-null channelPolicy and the channel is not a writable property bag. Is that desired/expected behavior (in that nsJSChannel needs to implement this interface), or just an accidental set of |rv| in NS_NewChannel?

(In reply to Boris Zbarsky (:bz) from comment #10) > bsterne, nsJSChannel is not a writable property bag, and NS_NewChannel > currently errors out if there is a non-null channelPolicy and the channel is > not a writable property bag. Is that desired/expected behavior (in that > nsJSChannel needs to implement this interface), or just an accidental set of > |rv| in NS_NewChannel? The JSChannel is not a writable property bag, but the channel it creates is: http://hg.mozilla.org/mozilla-central/annotate/30161b298513/dom/src/jsurl/nsJSProtocolHandler.cpp#l531 That's the thing CSP cares about. CSP needs to add the channelPolicy object to the channel's property bag to handle redirects. When a channel redirects, the channelPolicy object is forwarded from the old channel to the new channel. So my assumption that every channel that matters within the context of CSP will implement nsIWritablePropertyBag, and create their channels using NS_NewChannel. I believe the fix is simply to make nsJSChannel implement the writable property bag. I'm not sure how we didn't catch this sooner :-(

Brandon, you taking this bug? /be

Yes, I'll take this. Boris was exactly right when he asked: (In reply to Boris Zbarsky (:bz) from comment #10) > bsterne, ... Is that desired/expected behavior ... or just an accidental set of > |rv| in NS_NewChannel? This is in fact a misguided setting of rv from within that QI, since I don't check rv anyway and simply pass it through. So we don't actually need to make nsJSChannel implement nsIWritablePropertyBag. Before I write the patch to remove the rv setting, is it safe to assume that any channel that doesn't implement nsIWritablePropertyBag isn't capable of redirecting? If this isn't true, than we have other potential hazards where a redirect can be used to bypass CSP. One way to address the latter would be to add an assertion, or even return an error from our redirect listener if the channel doesn't implement writable property bag. This would be a change to our current code which simply allows the redirect (from CSPs perspective) if the channel isn't a WPB: http://hg.mozilla.org/mozilla-central/annotate/53f4c8abf558/content/base/src/nsCSPService.cpp#l220 That code essentially makes the assumption that I am questioning above.

> is it safe to assume that any channel that doesn't implement nsIWritablePropertyBag isn't > capable of redirecting? For extension-implemented channels, probably not. If the writable property bit is only needed to handle redirects securely, then failing redirect of non-writable-property-bag channels when there is a CSP seems much better than failing their creation when there is a CSP to me. And logging something to the error console in that situation would be _really_ nice.

Daniel: isn't the problem that it's blocking while in Report-Only mode?

(In reply to Marshall Roch from comment #15) > Daniel: isn't the problem that it's blocking while in Report-Only mode? Not really. The bug is that all javascript: links are broken when any CSP policy is sent: report-only or regular, as well as policies that allow inline-script. I'll put up a patch shortly.

Here's the meat of the patch, which I built and verified fixes the bug (allows the javascript: link in the subframe to load). Requesting bz's review on the substance of the patch and in the meantime I'll write a test and add a console warning when we kill the redirect to a non-writable property bag.

(In reply to Marshall Roch from comment #9) > The content of the header is: > > allow *;script-src https://*.facebook.com http://*.facebook.com > https://*.fbcdn.net http://*.fbcdn.net *.facebook.net *.google-analytics.com > *.virtualearth.net *.google.com 127.0.0.1:* *.spotilocal.com:*;options > inline-script eval-script;report-uri http://www.facebook.com/csp.php FYI, this policy will not generate any reports for you if the top-level page is served from https. The report-uri must share the same scheme, port and public suffix +1 as the top-level document: https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html#report-uri A relative URI is valid there, so you may want to just remove the scheme so the report-uri works for both http and https pages.

Comment on attachment 575541 [details] [diff] [review] patch v1 r=me

Given the lack of motion on the Facebook side on this, we probably need to backport this to aurora and maybe even beta if we can....

Same patch as before but adds a test, and a console warning when the redirect is canceled due to no writable property bag. Boris, what's your preference for the content of the error message? Right now it just says "Warning: Unable to redirect to <URL>".

I am going to push this to Try while I wait for review, but I don't expect any surprises.

Comment on attachment 576645 [details] [diff] [review] patch with test You may be able to use the logging facilities in nsContentUtils to simplify your logging code (and incidentally make it localizable!). Also, mentioning the need to implement nsIWritablePropertyBag2 is a good idea. Something like: Unable to redirect to %S because the channel doesn't implement nsIWritablePropertyBag2. One other thing... You should check the return value of SetPropertyAsInterface. A JS-implemented channel might QI to nsIWritablePropertyBag2 but throw on attempts to invoke any method from the interface. r=me with those nits.

Thanks, Boris. Pushed to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/79c9926f0f45

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/d7a9e843e573 - somebody's leaking in browser-chrome, and with all the burning from the missing test and burning from ftp.m.o being broken, it's hard to tell who. I picked you to back out first because the dom.properties hunk of https://hg.mozilla.org/integration/mozilla-inbound/rev/79c9926f0f45 is pretty broken looking, missing a newline after your added string, but... I don't see that in either of the patches attached to this bug. Was it from some other bug, or not the patch you meant to push?

Off the hook for the leak, it persisted past your backout.

Push to m-i, take 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/0706bf7b4508

This patch introduced an unused variable...

https://hg.mozilla.org/mozilla-central/rev/0706bf7b4508 (Leaving open for comment 28's patch)

(In reply to Ms2ger from comment #28) > Created attachment 579334 [details] [diff] [review] > Remove unused variable > > This patch introduced an unused variable... Thanks, pushed to m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/87a61be93cb7

Comment on attachment 579334 [details] [diff] [review] Remove unused variable http://hg.mozilla.org/mozilla-central/rev/87a61be93cb7