Closed Bug 786009 Opened 7 years ago Closed 7 years ago

js has to be enabled to parse rss-feeds (regression?)


(Core :: Security: CAPS, defect)

Not set



Tracking Status
firefox16 --- unaffected
firefox17 + fixed
firefox18 + verified
firefox19 --- fixed


(Reporter: metasieben, Assigned: sicking)



(Keywords: regression, reproducible)


(2 files)


1) disable js
2) open:

result: feeds doesn't get parsed.

expected result: feed should be displayed. possible regression, as it works as expected in the current stable release.
FWIW, Aurora (16) is WFM.
OS: Mac OS X → All
this build works as expected:

this one doesn't anymore:

hope this helps.
Relevant range:

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....
Blocks: 774585
Component: General → Security: CAPS
Ever confirmed: true
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.
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?
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.
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.....
Knowing how slammed Jonas is right now with B2G, assigning to Andrew to help find somebody to look at this regression from bug 774585.
Assignee: nobody → overholt
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?
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....
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.
Jonas says he thinks he knows what to do so assigning to him.
Assignee: overholt → jonas
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.
Keywords: reproducible
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?
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?
Attached patch Patch to fixSplinter Review
Given that we haven't heard of any other features than feeds breaking. I'd prefer to only change the logic here for feeds.
Attachment #673788 - Flags: review?(bzbarsky)
Comment on attachment 673788 [details] [diff] [review]
Patch to fix

>+        if (!strcmp(path.get(), "feeds")) {
ITYM if (path.EqualsLiteral("feeds")) {
Comment on attachment 673788 [details] [diff] [review]
Patch to fix

r=me with Neil's comment addressed.
Attachment #673788 - Flags: review?(bzbarsky) → review+
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:

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
Attachment #673788 - Flags: approval-mozilla-beta?
Attachment #673788 - Flags: approval-mozilla-aurora?
Blocks: 804411
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
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:
(Does it matter if you set the owner before or after the originalURI?)
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.
Attachment #674728 - Flags: review?(bzbarsky)
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.
Attachment #674728 - Flags: review?(bzbarsky) → review+
Attachment #673788 - Flags: approval-mozilla-beta?
Attachment #673788 - Flags: approval-mozilla-beta+
Attachment #673788 - Flags: approval-mozilla-aurora?
Attachment #673788 - Flags: approval-mozilla-aurora+
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[";1"].
>-          getService(Ci.nsIIOService);
>+      var ios =;
[Not sure whether it's worth keeping the temporary variable.]
Attachment #674728 - Flags: review?(neil) → review?(
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?
Attachment #674728 - Flags: review?( → review+
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
QA Contact: manuela.muntean
Do we need a new bug to track the "alternative patch"? Looks like it was forgotten.
Target Milestone: mozilla19 → ---
You need to log in before you can comment on or make changes to this bug.