It shouldn't load those files. (Btw, why do you parse responseText and not just load HTML using XHR?)
Olli Pettay Because I have a problem with some sites which I describe in this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=770684 And I have been advised to use DOMParser in my extension untill this bug will be fixed.
Sorry, wrong link. See: https://bugzilla.mozilla.org/show_bug.cgi?id=765620
I'm not convinced that this is a bug. If you parse the markup as HTML, you can adopt the nodes into the document and they should behave like normal HTML nodes. It seems weird to only kick off the loads on adoption.
Kyle Huey But DOMParser does not load all the images and so on. Just some.
Kyle Huey And I think we must discern page rendering and DOM parsing. For background parsing we don't need any image loading or styles.
If I run the code from comment 0 in the scratchpad, I get an exception because xhr.channel is undefined, as expected. I assume that the "in browser context" bit hides some crucial part of the steps to reproduce? I'd really appreciate actually being told what that is. Kyle, DOMParser is not supposed to do subresource loads. See bug 421228. ;)
2 Boris Zbarsky http://blog.mozilla.org/devtools/2011/08/15/introducing-scratchpad/ See the part "A Word about Scopes", the last paragraph.
Ah, looks like the "devtools.chrome.enable" pref needs to be set to true. This is a bad bug in the patch for bug 102699. Before that patch, the only codepath that could lead to parsing looked like this, in order: 1) Create a document with the DOMParser's mOriginalPrincipal. 2) Call EnableXULXBL() on the document if needed 3) Call StartDocumentLoad() 4) Set the document's base URI 5) Reset the document's principal to mPrincipal. 6) Feed data into the parser. That sequence of steps was pretty clearly documented (at least in terms of the whole principal dance) and _very_ critical. When that bug was fixed, the XML codepath stayed as above, but HTML codepath was written more like this: 1) Create a document with the DOMParser's mOriginalPrincipal. 2) Feed data into the parser. 3) Call EnableXULXBL() on the document if needed 4) Set the document's base URI 5) Reset the document's principal to mPrincipal. But the whole point of resetting to mPrincipal is that it MUST happen before any data goes in. Otherwise you're parsing with the system principal. Also, this is never calling StartDocumentLoad, so afaict it's not setting up whatever state that would normally set up (e.g. the document URI) the same way as the XML path. And it's calling EnableXULXBL() too late, of course. Not like this matters much for text/html. This bug means that using DOMParser on text/html is pretty unsafe from chrome: It allows whatever string you're parsing to poke any URI it wants, including ones that web content normally can't access. (On a Unix system it allows at minimum a DoS attack by reading from file:///dev/tty.) Henri, Olli, can one of you grab this? Or should I try to?
5 years ago
Our final beta build is tomorrow EOD pacific time. Is there any reason to consider blocking that build on this FF12 regression?
No, though this might be worth taking in a chemspill if we do one for some reason: every extension using DOMParser is basically insecure at the moment.
I think Henri will be back from vacation real soon.
What is the critical part of this bug? The created document is a data document, so it itself shouldn't load anything. HTML parser may speculatively load something. (It shouldn't enable speculative loads for data documents) Or what am I missing here.
Though, I really don't understand why anything is loaded. Document is a data document, so the data document content policy should prevent all the loads.
Content policy is never consulted for the system principal. See the CHECK_PRINCIPAL bit in NS_CheckContentLoadPolicy. So the document is in fact loading things here.
bug 388597 added a bizarre inconsistency to content policy handling. (Another option would be to not have data document cp at all, but prevent such loads in some other way.)
I guess I've looked at the code enough to write a patch.
//every extension using DOMParser is basically insecure at the moment I use it only because of the bug 765620. I just have given a simple testcase in the last comment there. May be somebody can confirm the bug.
Created attachment 640106 [details] [diff] [review] v1 Like this?
Comment on attachment 640106 [details] [diff] [review] v1 Unfortunately this seems to leak. We're not supposed to call StartDocumentLoad in this case for HTML documents.
Created attachment 640113 [details] [diff] [review] v2
Created attachment 640187 [details] [diff] [review] simple approach
Comment on attachment 640187 [details] [diff] [review] simple approach r=me
Tracking for FF14, for possible 14.0.1 consideration.
Thank you, people.
(In reply to Olli Pettay [:smaug] (limited connectivity July 26-29) from comment #24) > https://hg.mozilla.org/mozilla-central/rev/15f4ed9607b3 Is this low risk enough to uplift to FF15 on mozilla-beta?
(In reply to Alex Keybl [:akeybl] from comment #27) > (In reply to Olli Pettay [:smaug] (limited connectivity July 26-29) from > comment #24) > > https://hg.mozilla.org/mozilla-central/rev/15f4ed9607b3 > > Is this low risk enough to uplift to FF15 on mozilla-beta? Seems so to me. (Looks like smaug is away right now.)
Comment on attachment 640187 [details] [diff] [review] simple approach [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 102699, bug 388597 User impact if declined: security issues Testing completed (on m-c, etc.): Landed m-c 2012-07-09 Risk to taking this patch (and alternatives if risky): Should be very low risk String or UUID changes made by this patch: NA
Comment on attachment 640187 [details] [diff] [review] simple approach [Triage Comment] Low risk sg:moderate fix - let's take this for the next beta (going to build tomorrow). Please land as soon as possible.
Reporter, can you see if this is fixed for you now? Can you try the latest Firefox 15 release, 16 beta, and 17 aurora? Thanks.
Yes, it is fixed in these versions. Thank you.
Thanks vvsemozhetbyt@. Marking this bug verified based on comment 33.