Last Comment Bug 786009 - js has to be enabled to parse rss-feeds (regression?)
: js has to be enabled to parse rss-feeds (regression?)
Status: RESOLVED FIXED
: regression, reproducible
Product: Core
Classification: Components
Component: Security: CAPS (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla19
Assigned To: Jonas Sicking (:sicking) PTO Until July 5th
: Manuela Muntean [Away]
Mentors:
Depends on:
Blocks: 774585 804411
  Show dependency treegraph
 
Reported: 2012-08-27 12:45 PDT by matthias koplenig
Modified: 2012-12-21 13:51 PST (History)
11 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
fixed
+
verified
fixed


Attachments
Patch to fix (1.80 KB, patch)
2012-10-22 01:24 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bzbarsky: review+
akeybl: approval‑mozilla‑aurora+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review
Alternative patch (only for m-c) (7.94 KB, patch)
2012-10-24 10:29 PDT, Jonas Sicking (:sicking) PTO Until July 5th
bzbarsky: review+
gavin.sharp: review+
Details | Diff | Splinter Review

Description matthias koplenig 2012-08-27 12:45:50 PDT
str:

1) disable js
2) open: http://www.heise.de/newsticker/heise-atom.xml

result: feeds doesn't get parsed.

expected result: feed should be displayed. possible regression, as it works as expected in the current stable release.
Comment 1 XtC4UaLL [:xtc4uall] 2012-08-28 01:23:17 PDT
FWIW, Aurora (16) is WFM.
Comment 2 matthias koplenig 2012-08-30 12:52:42 PDT
this build works as expected:
20120718030544
http://hg.mozilla.org/mozilla-central/rev/ae22909cef5a

this one doesn't anymore:
20120719030543
http://hg.mozilla.org/mozilla-central/rev/6d8456a77e57


hope this helps.
Comment 3 Boris Zbarsky [:bz] 2012-08-30 13:08:45 PDT
Relevant range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ae22909cef5a&tochange=6d8456a77e57

Yes, that helps.  Jonas, I will bet this is a regression from bug 774585: the script security manager looks at the principal URI in CanExecuteScripts() and if it's an about: URI checks for the ALLOW_SCRIPT flag on the about module.  I assume in the feeds case the channel's original URI is not about:feeds (perhaps it's the feed URI) and hence the principal's URI has changed with the patch.

That actually seems like a bad idea to start with, because we suddenly changed permissions on the feeds page....
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-30 14:32:55 PDT
Boris: Any ideas for how to fix this? I'm really not a fan of how we used to hardcode the principal based on calling GetCodebasePrincipal. In general we have *way* too many callers of GetCodebasePrincipal, any time we call that we're basically doing security checks based on URIs rather than on principals.
Comment 5 Boris Zbarsky [:bz] 2012-08-30 19:42:25 PDT
So the problem is that this case is weird.  The original URI for this channel is whatever it is.  Then the feed reader redirects to about:feeds which is itself a redirect.

We have a way to use the original URI for the principal, or the final URI, but not some intermediate URI....

We could add some explicit way of setting the URI to use for the principal, I guess.  But if we're going to do that, it's saner to just set the owner: overriding the principal is what it's for.

What would break if we did that?
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2012-08-30 20:07:44 PDT
Arg, lost a longer comment.

Basically I don't know of anything that would break. In general I'd like to reduce the number of callsites that we have that create principals so that we can clean them up a bit. Right now they are a DOM-specific and not much more than a wrapper around a URI, and making them anything else is pretty hard. For example if we want to add cookie jars to firefox it'd be pretty hard to make sure that we're using the right principal everywhere. Likewise we end up losing the CSP policy which is stored in the principal unless we go directly to the document and get the principal from there.

I'd like to be able to pass around principals to more than DOM code. For example necko really should use principals more often. But right now that's not terribly reliable. The extra appid/browserflag information we've added to principals is pretty B2G specific. There's simply too much firefox code where we simply didn't have enough context and just wrapped a principal around a URI instead.

None of this is very easy of course. And definitely for other bugs.

Ssetting a specific principal on about:feeds would be an ok solution for now. I don't think anything would break.
Comment 7 Boris Zbarsky [:bz] 2012-08-30 20:13:46 PDT
OK.  Note that it's not just about:feeds.  It's everything that has ALLOW_SCRIPT and also has URI_SAFE_FOR_UNTRUSTED_CONTENT.  That seems to be "feeds", "neterror", "blocked", "certerror", "rights", "robots", "home".

I'm actually kinda surprised we haven't had more problems reported.....
Comment 8 Alex Keybl [:akeybl] 2012-09-05 16:10:40 PDT
Knowing how slammed Jonas is right now with B2G, assigning to Andrew to help find somebody to look at this regression from bug 774585.
Comment 9 Jonas Sicking (:sicking) PTO Until July 5th 2012-09-05 17:43:00 PDT
Boris: I don't understand why we have to do this for other URLs than about:feed?

Isn't about:feed the only URL where we're starting with http URL (or other "normal protocol) redirect to an about: url, which redirects to a chrome: (or resource:?) URL, and then want to use the about: URL for the principal?

All other examples you listed start with about: URLs and that's what we want to use for the principal, no?
Comment 10 Boris Zbarsky [:bz] 2012-09-05 18:09:55 PDT
That's a good question.  I guess for the neterror and certerror cases we do start with those right now and fake the url bar via another mechanism?  They do sort of start with an http:// URI, but we do some weirdness after that..

What about about:blocked?  In that case we _do_ start with an http:// URI, I believe.  Is it handled like neterror/certerror?

But mainly, I wanted us to do something consistent so we wouldn't have to do fragile reasoning about exactly how all sorts of weird cases are loaded....
Comment 11 neil@parkwaycc.co.uk 2012-09-11 02:12:58 PDT
FeedConverter.js opens about:feeds to handle the preview, which the about redirector then redirects to a chrome: URL, but then FeedConverter.js sets the channel's original URI to be that of the feed. This means that the document's URI is that of the feed, not about:feeds or the chrome: URL.

Error pages never change the docshell's idea of the current URI. This means that the document's URI is an about: page, but the UI still displays the originally attempted URI.
Comment 12 Andrew Overholt [:overholt] 2012-09-11 12:21:19 PDT
Jonas says he thinks he knows what to do so assigning to him.
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-26 14:45:02 PDT
Jonas - are you going to be able to work on this in the next few weeks?  This regression will go to our Beta audience in just over a week and we might start seeing more reports about this issue.
Comment 14 Lukas Blakk [:lsblakk] use ?needinfo 2012-10-19 16:26:52 PDT
Also - can someone comment on how this affects users with disabled js outside of this one reproducible issue?  Would this have potential for wide impact in a variety of XML-related web interactions?
Comment 15 Boris Zbarsky [:bz] 2012-10-19 17:52:26 PDT
This issue is specific to about: URIs that are loaded as "untrusted" but include script.  And not only that, but you have to have a redirect to such a URI from another URI.

So we're pretty sure that this only impacts about:feeds in practice.

sicking, are you fixing this?
Comment 16 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-22 01:22:48 PDT
Try push: https://tbpl.mozilla.org/?tree=Try&rev=d9fe5cc2b919
Comment 17 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-22 01:24:02 PDT
Created attachment 673788 [details] [diff] [review]
Patch to fix

Given that we haven't heard of any other features than feeds breaking. I'd prefer to only change the logic here for feeds.
Comment 18 neil@parkwaycc.co.uk 2012-10-22 01:54:23 PDT
Comment on attachment 673788 [details] [diff] [review]
Patch to fix

>+        if (!strcmp(path.get(), "feeds")) {
ITYM if (path.EqualsLiteral("feeds")) {
Comment 19 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-22 02:05:11 PDT
Doh! Yes!
Comment 20 Boris Zbarsky [:bz] 2012-10-22 08:25:03 PDT
Comment on attachment 673788 [details] [diff] [review]
Patch to fix

r=me with Neil's comment addressed.
Comment 21 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-22 11:26:43 PDT
Comment on attachment 673788 [details] [diff] [review]
Patch to fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 774585

User impact if declined: The feed reader would be broken for users that turn off Javascript (which is rare). All in all I don't think this would affect a lot of users since I suspect the intersection of disabling JS and using feed reader is small.

Testing completed (on m-c, etc.): Try run seems to have passed with only known oranges: https://tbpl.mozilla.org/?tree=Try&rev=d9fe5cc2b919

Risk to taking this patch (and alternatives if risky): The patch is very safe. It simply reverts behavior to what we had in Firefox 16 when handling about:feed URLs. Backing out the original patch would be a bigger change and likely a bigger risk given how much other changes were made in the FF17 timeframe around principal handling.

String or UUID changes made by this patch: None
Comment 22 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-22 18:06:35 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2594523cb92
Comment 23 Ryan VanderMeulen [:RyanVM] 2012-10-23 03:57:11 PDT
https://hg.mozilla.org/mozilla-central/rev/e2594523cb92
Comment 24 neil@parkwaycc.co.uk 2012-10-23 06:10:51 PDT
So, I've been looking at this to port it to SeaMonkey, and it has just dawned on me that a better place to set the owner might be in FeedConverter.js:
http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/src/FeedConverter.js#248
(Does it matter if you set the owner before or after the originalURI?)
Comment 25 Jonas Sicking (:sicking) PTO Until July 5th 2012-10-24 10:29:12 PDT
Created attachment 674728 [details] [diff] [review]
Alternative patch (only for m-c)

Yes, that's a much better approach!

This patch uses that approach. I think we should still stick with the previous patch for the release branches since that is a safer approach as it just reverts us to previous behavior.

But this patch would be a better way to fix things for future releases.
Comment 26 Boris Zbarsky [:bz] 2012-10-24 12:13:52 PDT
Comment on attachment 674728 [details] [diff] [review]
Alternative patch (only for m-c)

Hrm.  So "chromeURI" is horribly misnamed, eh?

r=me on the owner bit; the rest could use review from someone who actually knows this code.
Comment 28 neil@parkwaycc.co.uk 2012-10-28 09:28:33 PDT
Comment on attachment 674728 [details] [diff] [review]
Alternative patch (only for m-c)

Sorry for the delay in redirecting this but I'm not a browser peer, although the patch looks reasonable.

>-      var ios = 
>-          Cc["@mozilla.org/network/io-service;1"].
>-          getService(Ci.nsIIOService);
>+      var ios = Services.io;
[Not sure whether it's worth keeping the temporary variable.]
Comment 29 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-10-28 09:52:51 PDT
Comment on attachment 674728 [details] [diff] [review]
Alternative patch (only for m-c)

Can you land the service.jsm cleanup separately from the actual functional change here?
Comment 30 Manuela Muntean [Away] 2012-12-14 06:17:51 PST
Verified fixed on Firefox 18 beta 4.

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:18.0) Gecko/20100101 Firefox/18.0
Build ID: 20121212073002
Comment 31 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-12-21 13:51:02 PST
Do we need a new bug to track the "alternative patch"? Looks like it was forgotten.

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