I'd like to remove CheckLoadURI(), which generally speaking gives the wrong answers anyway if you have principal around. List of callers: /browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp, line 910 /browser/components/places/src/nsBookmarksFeedHandler.cpp, line 841

*** This bug has been marked as a duplicate of 342485 ***

Sorry, different components.

(In reply to comment #0) > I'd like to remove CheckLoadURI(), which generally speaking gives the wrong > answers anyway if you have principal around. > > List of callers: > > /browser/components/bookmarks/src/nsBookmarksFeedHandler.cpp, line 910 obsolete, being removed from the build in bug 386392. > /browser/components/places/src/nsBookmarksFeedHandler.cpp, line 841 > this file no longer exists. however, it's morphed into this: http://lxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsLivemarkService.js#487 does that have the problem this bug is referring to?

Yes. You want to use checkLoadURIWithPrincipal or checkLoadURIStrWithPrincipal. checkLoadURI and checkLoadURIStr have basically identical issues and should both go away.

Rob, is there a way to get back to a principal for the original feed document from nsIFeedResult?

Note that there are pending patches to make it possible to serialize and deserialize nsIPrincipal objects...

So this bug is blocking core API changes, which are going to get harder and harder to make as we move along. I may not be able to get them approved at all by the time we get to M10...

(In reply to comment #5) > Rob, is there a way to get back to a principal for the original feed document > from nsIFeedResult? > iirc, sayrer said there's not currently a way to get the principal via the feeds apis. i'm not familiar w/ the feeds code, so not sure how big this change would be. (In reply to comment #6) > Note that there are pending patches to make it possible to serialize and > deserialize nsIPrincipal objects... > have these landed? bug #s?

Bug 369566. Waiting on approval. As a transitional step, this code could use getCodebasePrincipal() on the URIs it has, until it starts storing actual principals.

It's probably my failure to understand principals, but I don't see what we would gain with a more real one - the situation is that a string fell out of the sky on a user's head, and he said "hey, fetch this up for me and create bookmarks out of the contents" and we don't want icky ones, but as every bug around this check has shown, we really don't know what does and doesn't amount to icky. If getCodebasePrincipal gives us a principal equivalent to the URL being loaded in the browser, that's probably roughly what we want. Note that testing this is a little interesting thanks to bug 396318, but with the resulting weird expectation from that, that a link we don't like should produce a livemark item with the URL from the previous item, adding http://dev.philringnalda.com/rsstests/badlinks2.xml as a livemark with this patch still gives me the results I expect.

> It's probably my failure to understand principals They're just a way of encapsulating the concept of "someone who does stuff". Typically, a URI (or rather just the scheme+host+port part) will identify the principal, but there are cases when a page has one URI but a principal for a different URI. Obvious examples are javascript: and data: URIs, and about:blank. There are also principals that have information about the certificate used to sign the page. That information clearly cannot be captured in a URI. > If getCodebasePrincipal gives us a principal equivalent to the URL > being loaded in the browser It doesn't, per above. What it does give you is something equivalent to what the code is doing right now. You'll want a separate bug on actually saving where data came from (which is the principal, NOT the page URI) and using it for the check here. Looking at the patch, how does result.uri compare to this._livemark.feedURI? You seem to have switched from one to the other, right?

Comment on attachment 281058 [details] [diff] [review] Like so? The more we talk about this, the more I think we shouldn't be trusting me to patch it. I think I can answer the "result.uri versus this._livemark.feedURI" question, not that it matters based on the IRC conversation about wanting to get the principal off the channel in onStartRequest - result.uri and this._livemark.feedURI better be the same thing, since this._livemark.feedURI is set by getting a string out of the db and creating a URI from it, and result.uri is set by the feed processor's parseAsync() being called with this._livemark.feedURI as an argument, and passing it back in result.

But, give me long enough, eventually I'll ask the question the right way... <bz> philor: so basically the real question is "is this URI safe to load from chrome on top of some other site?" <bz> philor: right? <philor> bz: exactly! * bz thinks <bz> you could explicitly check whether the URI inherits its principal <bz> if it does, it's not safe <bz> Since really, you don't care _who_ is loading it <bz> which is what the principal in CheckLoadURIWithPrincipal would represent <bz> you care about the properties of the URI itself, and nothing else <bz> http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsINetUtil.idl#77 <bz> Use that? <bz> That should make it clear what you're doing <philor> wonderful. *that* makes perfect sense to me, at last. thanks!

Well, except that was only supposed to apply to bug 405944, not this bug.

Okay, back around again. The code changes aren't terribly interesting, unless there's some magic heritage that clings to an nsIURI which is used in getCodebasePrincipal - right now, we rather oddly start with a URI for the feed, and a URI for the link, and call CheckLoadURIStr with the .spec for each, and then SSM calls NS_NewURI on the source string, calls GetCodebasePrincipal on that URI, passes off to CheckLoadURIStrWithPrincipal which calls NS_NewURI on the target and passes off to CheckLoadURIWithPrincipal, so unless this._livemark.feedURI is somehow different than NS_NewURI(its.spec), this is doing the same thing, just cutting out some extra steps and calling getCodebasePrincipal instead of GetCodebasePrincipal. The test is... gross. You can test livemark items by listening for onItemAdded, but only if you are willing to depend on the order that they are added, and willing to have the failure case involve waiting for the test to time out. Unfortunately, onEndUpdateBatch happens some time before what I would call the actual end, so without a setTimeout, I was winding up getting back just the about:livemark-loading item.

Comment on attachment 292281 [details] [diff] [review] Thusly? Thank you!

Comment on attachment 292281 [details] [diff] [review] Thusly? a=mconnor on behalf of drivers, thanks Phil!

toolkit/components/places/src/nsLivemarkService.js 1.29 toolkit/components/places/tests/chrome/Makefile.in 1.6 toolkit/components/places/tests/chrome/test_342484.xul 1.1 toolkit/components/places/tests/chrome/bad_links.atom 1.1 I've sort of lost track: can I declare victory now, or does this bug own the rather... interesting use in http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/browser/components/feeds/src/FeedWriter.js&rev=1.47&mark=133#117 too?

Separate bug for that (blocking bug 327244) probably makes the most sense. I don't think that code is "Places" by any stretch of the imagination.

Bug 408328 it is, then.

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv