Closed
Bug 408328
Opened 17 years ago
Closed 17 years ago
Replace FeedWriter.js's CheckLoadURIStr with CheckLoadURIStrWithPrincipal
Categories
(Firefox Graveyard :: RSS Discovery and Preview, defect)
Firefox Graveyard
RSS Discovery and Preview
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 3 beta3
People
(Reporter: philor, Assigned: philor)
References
()
Details
Attachments
(1 file)
9.05 KB,
patch
|
sayrer
:
review+
bzbarsky
:
superreview+
sayrer
:
approval1.9+
|
Details | Diff | Splinter Review |
Passing a principal gets better results, the withoutprincipal versions need to go away, if you haven't heard this song before you just haven't been paying attention. The fact that we want to pretend that we're the original window, not the scary mixture of chrome and content we really are, ought to make it more fun, and seems likely to make it more of a case where we'd rather explicitly pass the right principal rather than hope getCodebasePrincipal does the right thing.
Flags: blocking-firefox3?
Assignee | ||
Comment 1•17 years ago
|
||
Fun, fun. Since the "this._window, this._document" stuff acts like it's saving off the state before the "you got your chrome in my content!" stage, I naively thought that this._document.nodePrincipal would be just the ticket. Unfortunately, while it gives me the results I want and expect, it tells the error console that "Content at about:feeds may not load or link to file:///var/." which not only isn't what I want it saying, it makes me think I'm likely to get the wrong answer to some question I'm not thinking to ask. I sure hope someone who understands this plans on taking this bug.
Comment 2•17 years ago
|
||
bz: can we get an opine, here?
Comment 3•17 years ago
|
||
On what, exactly?
Assignee | ||
Comment 4•17 years ago
|
||
If one had to guess (and one quite often does), "bz: can we get an opinion on whether this should block Fx 3, or more particularly, have you given up on getting bug 327244 into 1.9 yet?" would be a likely guess at the desired opinion. While it hasn't helped me figure out what to do about it here, I did finally see where the about:feeds principal, which apparently seemed odd to you in the test for bug 395533, comes from: FeedConverter.handleResult creates a channel for about:feeds, which in http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/feeds/src/nsAboutFeeds.cpp&rev=1.1&mark=57-74#45 creates a channel for the chrome URI of the subscribe.xhtml page, and calls SetOwner on it with an about:feeds principal.
Comment 5•17 years ago
|
||
I really have no idea on bug 327244. I'd love to get it in, since it's likely to improve our security story, but it's sort of out of my hands now...
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → philringnalda
Assignee | ||
Comment 6•17 years ago
|
||
One sure way to duck the "blocking?" question: just fix it.
Attachment #294397 -
Flags: superreview?(bzbarsky)
Attachment #294397 -
Flags: review?(sayrer)
Comment 7•17 years ago
|
||
Comment on attachment 294397 [details] [diff] [review] Fix v.1 This looks OK if we're guaranteed that there is always a principal (e.g. if we always go through the getCodebasePrincipal codepath and an exception there makes sure we don't reach the security chek). Otherwise we would crash when we do the security check on a null principal... perhaps we should fix CAPS to not do that.
Attachment #294397 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 8•17 years ago
|
||
We do always go through FeedWriter.Init(), but I don't see where we crash with a null principal: I thought that the NS_ENSURE_ARG_POINTER(aPrincipal) in CheckLoadURIWithPrincipal was supposed to protect against that. And indeed, if I just comment out the line setting this._feedPrincipal, I don't crash, I just get what I would expect, nothing at all linked in the preview page because every security check call fails.
Updated•17 years ago
|
Attachment #294397 -
Flags: review?(sayrer) → review+
Comment 9•17 years ago
|
||
> I thought that the NS_ENSURE_ARG_POINTER(aPrincipal) in
> CheckLoadURIWithPrincipal
Ah, I'd missed it because it came after the various asserts. OK.
Assignee | ||
Updated•17 years ago
|
Attachment #294397 -
Flags: approval1.9?
Assignee | ||
Updated•17 years ago
|
Status: NEW → ASSIGNED
Updated•17 years ago
|
Attachment #294397 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 10•17 years ago
|
||
browser/components/feeds/src/FeedWriter.js 1.48 browser/components/feeds/Makefile.in 1.3 browser/components/feeds/test/Makefile.in 1.1 browser/components/feeds/test/test_bug408328.html 1.1 browser/components/feeds/test/bug408328-data.xml 1.1
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: blocking-firefox3? → in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3 M11
Updated•5 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•