Closed
Bug 786009
Opened 12 years ago
Closed 12 years ago
js has to be enabled to parse rss-feeds (regression?)
Categories
(Core :: Security: CAPS, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: metasieben, Assigned: sicking)
References
Details
(Keywords: regression, reproducible)
Attachments
(2 files)
1.80 KB,
patch
|
bzbarsky
:
review+
akeybl
:
approval-mozilla-aurora+
akeybl
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
7.94 KB,
patch
|
bzbarsky
:
review+
Gavin
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Keywords: qawanted,
regressionwindow-wanted
Reporter | ||
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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....
Blocks: 774585
Status: UNCONFIRMED → NEW
tracking-firefox17:
--- → ?
Component: General → Security: CAPS
Ever confirmed: true
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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?
Assignee | ||
Comment 6•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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
Assignee | ||
Comment 9•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Jonas says he thinks he knows what to do so assigning to him.
Assignee: overholt → jonas
Comment 13•12 years ago
|
||
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.
status-firefox16:
--- → unaffected
status-firefox17:
--- → affected
status-firefox18:
--- → affected
tracking-firefox18:
--- → +
Updated•12 years ago
|
Keywords: reproducible
Comment 14•12 years ago
|
||
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•12 years ago
|
||
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?
Assignee | ||
Comment 16•12 years ago
|
||
Assignee | ||
Comment 17•12 years ago
|
||
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 18•12 years ago
|
||
Comment on attachment 673788 [details] [diff] [review]
Patch to fix
>+ if (!strcmp(path.get(), "feeds")) {
ITYM if (path.EqualsLiteral("feeds")) {
Assignee | ||
Comment 19•12 years ago
|
||
Doh! Yes!
Comment 20•12 years ago
|
||
Comment on attachment 673788 [details] [diff] [review]
Patch to fix
r=me with Neil's comment addressed.
Attachment #673788 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 21•12 years ago
|
||
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
Attachment #673788 -
Flags: approval-mozilla-beta?
Attachment #673788 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 22•12 years ago
|
||
Comment 23•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 24•12 years ago
|
||
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?)
Assignee | ||
Comment 25•12 years ago
|
||
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 26•12 years ago
|
||
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+
Updated•12 years ago
|
Attachment #673788 -
Flags: approval-mozilla-beta?
Attachment #673788 -
Flags: approval-mozilla-beta+
Attachment #673788 -
Flags: approval-mozilla-aurora?
Attachment #673788 -
Flags: approval-mozilla-aurora+
Assignee | ||
Updated•12 years ago
|
Attachment #674728 -
Flags: review?(neil)
Comment 27•12 years ago
|
||
Comment 28•12 years ago
|
||
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.]
Attachment #674728 -
Flags: review?(neil) → review?(gavin.sharp)
Comment 29•12 years ago
|
||
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?(gavin.sharp) → review+
Comment 30•12 years ago
|
||
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
Comment 31•12 years ago
|
||
Do we need a new bug to track the "alternative patch"? Looks like it was forgotten.
status-firefox19:
fixed → ---
Target Milestone: mozilla19 → ---
Updated•12 years ago
|
status-firefox19:
--- → fixed
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•