Any content to be added by an XSLT stylesheet must be permitted by that document's Content Security Policy.

Is this still a valid issue, Sid ? (if you know off hand)

Oh boy... hm. I don't have a test for this lying around, so no idea if it's an issue. Brandon: do you have a test case (reduced or not) for this?

I don't, sorry.

I'm pretty sure Brandon will be ok with me taking this.

This needs testing to see if it's actually a problem ...

It seems the CSP for content loaded by XSL behaves as expected. It loads the XSP if loaded from the same domain, but prevents the XSL loaded from example.com. Confirming using the debug output: CSP debug: blocking request for http://example.org/tests/content/base/test/file_CSP_bug663567_blocks.xsl

Comment on attachment 774136 [details] [diff] [review] mochitest confirming content loaded by XSL is subject to CSP Review of attachment 774136 [details] [diff] [review]: ----------------------------------------------------------------- Looks pretty good, but I'm curious about a few things. ::: content/base/test/file_CSP_bug663567_allows.xml^headers^ @@ +1,1 @@ > +X-Content-Security-Policy: default-src 'self' \ No newline at end of file Please use the non-prefixed header. We're gonna get rid of the prefixed ones soon and "Content-Security-Policy" header payloads are intended to be what's used in the future. ::: content/base/test/test_CSP_bug663567.html @@ +29,5 @@ > + * we load the xsl file using: > + * <?xml-stylesheet type="text/xsl" href="file_CSP_bug663467_allows.xsl"?> > + */ > + var cspframe = document.getElementById('xsltframe'); > + var xsltAllowedHeader = cspframe.contentWindow.document.getElementById('xsltheader').innerHTML; Won't calling .innerHTML throw if getElementById returns null (like if the XSLT doesn't load)? Why don't you just check if the element is null since if the XSLT isn't loaded the <h2 id="xsltheader"> element won't exist...

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #8) > Comment on attachment 774136 [details] [diff] [review] > mochitest confirming content loaded by XSL is subject to CSP > > Review of attachment 774136 [details] [diff] [review]: > ----------------------------------------------------------------- > > Looks pretty good, but I'm curious about a few things. > > ::: content/base/test/file_CSP_bug663567_allows.xml^headers^ > @@ +1,1 @@ > > +X-Content-Security-Policy: default-src 'self' > \ No newline at end of file > > Please use the non-prefixed header. We're gonna get rid of the prefixed > ones soon and "Content-Security-Policy" header payloads are intended to be > what's used in the future. ok, i'll incorporate that. > > ::: content/base/test/test_CSP_bug663567.html > @@ +29,5 @@ > > + * we load the xsl file using: > > + * <?xml-stylesheet type="text/xsl" href="file_CSP_bug663467_allows.xsl"?> > > + */ > > + var cspframe = document.getElementById('xsltframe'); > > + var xsltAllowedHeader = cspframe.contentWindow.document.getElementById('xsltheader').innerHTML; > > Won't calling .innerHTML throw if getElementById returns null (like if the > XSLT doesn't load)? Why don't you just check if the element is null since > if the XSLT isn't loaded the <h2 id="xsltheader"> element won't exist... that's true, i'll fix that.

Comment on attachment 774136 [details] [diff] [review] mochitest confirming content loaded by XSL is subject to CSP Same comment on using the unprefixed header I think it's not totally bad to check the text to see if things loaded all the way.. maybe a try/catch ? Otherwise looks good, thanks for writing the test !

I just happened to be catching up on the w3c webappsec list today and noticed this post : http://lists.w3.org/Archives/Public/public-webappsec/2013Jun/0079.html In particular, from Adam Barth : "You might think that XSLT is controlled by style-src because it's styling information, but we've actually put it into the script-src bucket because it's just as powerful as script. You can see some discussion of this topic if you search for XSLT in https://dvcs.w3.org/hg/content-security-policy/raw-file/tip/csp-specification.dev.html."

Comment on attachment 776612 [details] [diff] [review] updated mochi test verifying CSP restricts EventSource using the connect-src directive Review of attachment 776612 [details] [diff] [review]: ----------------------------------------------------------------- The title of this attachment is totally different from the title and content of the bug. Was this an accidental copy/paste from another bug? This test looks good, and it is good to see that CSP is doing the right thing for XSLT. The only changes I would make would be to remove some ok()'s that are not really testing anything. With that, r+. Please unbitrot Makefile.in when you make the other changes. ::: content/base/test/test_CSP_bug663567.html @@ +68,5 @@ > + document.getElementById('xsltframe2').addEventListener('load', checkBlocked, false); > + next(); > + }, > + function () { > + ok(true, "All tests finished!\n"); Remove this line; this is not a test. @@ +74,5 @@ > + } > +]; > + > +function next() { > + ok(true, "Begin!"); Remove this line; this is not a test.

removed unnecessary checks, and also renamed the patch-file.

Comment on attachment 789146 [details] [diff] [review] bug_663567_v2_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch carrying over r=grobinson from previous review.

setting up the patch so someone else can check in my code!

Comment on attachment 789232 [details] [diff] [review] bug_663567_v3_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch I can't get to it in the next few hours... added checkin-needed keyword so it's fair game for anyone.

Backed out for B2G mochitest-1 timeouts. https://hg.mozilla.org/integration/mozilla-inbound/rev/aa55dcb8ed4c https://tbpl.mozilla.org/php/getParsedLog.php?id=26492003&tree=Mozilla-Inbound

fixed problem with async calls!

Comment on attachment 790817 [details] [diff] [review] bug_663567_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch :sstamm already reviewed this, ready to be checked in!

Comment on attachment 790817 [details] [diff] [review] bug_663567_verify_that_content_added_by_xslt_stylesheet_is_subject_to_csp.patch I guess it has to be the '?' in the checkin-box so someone else can check it in!

If you update the file file_CSP_bug663567_blocks.xml^headers^ and set the CSP to: Content-Security-Policy: default-src 'self' you get the behavior/error you expect. The setup for this testcase should have been set up like it is now from the beginning.

Comment on attachment 799602 [details] [diff] [review] bug_663567_xslt_test_update.patch Review of attachment 799602 [details] [diff] [review]: ----------------------------------------------------------------- This patch updates the test so it checks default-src must authorize xslt loads: allowed: "default-src 'self'", relative path XSLT blocked: "default-src *.example.com", relative path XSLT not on example.com What about the situation where the XSLT is loaded cross-origin and should be allowed/blocked? From some manual testing, it looks like when it CSP should allow a cross-origin request for XSLT it gets an NS_ERROR_DOM_BAD_URI, which means we're not supposed to load xslt files cross-origin and probably needs CORS to work properly. So I think this test works fine. Thanks for the turnaround. Should we be testing the script-src directive (not default-src) given comment 11 (or is that a follow-up bug)?

(In reply to Sid Stamm [:geekboy or :sstamm] from comment #26) > Should we be testing the script-src directive (not default-src) given > comment 11 (or is that a follow-up bug)? I think this test should only verify that XSLT is subject to the CSP. We should test that XSLT a subject to the script-src directive in a follow up bug.

inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/49a194c84ab0

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27) > (In reply to Sid Stamm [:geekboy or :sstamm] from comment #26) > > Should we be testing the script-src directive (not default-src) given > > comment 11 (or is that a follow-up bug)? > > I think this test should only verify that XSLT is subject to the CSP. We > should test that XSLT a subject to the script-src directive in a follow up > bug. Follow up in Bug 910139.