1477458 - Style is not applied in XML Pretty Print when page CSP forbids inline style

Status: VERIFIED FIXED

(Core :: XML, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

STR: load URL with Nightly.

This bug does not affect Beta because the feature is disabled there.

(The URL was from bug 1401793 and this is reported by mib_bwgabp over IRC)

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I can confirm that the XML itself is not a problem; download it and open it up locally have caused the style to apply.

Comment 2

[Ian Moody [:Kwan] (UTC+0)]

Is this just CSP combined with the use of <style>? So switching back to <link> now that bug 1410578 is fixed will take care of this?  I'll try it out.

Comment 3

[Ian Moody [:Kwan] (UTC+0)]

Yep that handled it.  Do we care about being able to use inline styles etc. in UA shadow roots?  Or is being able to use chrome:// links to external resources good enough?

Also what'd be the best way to test this?  Reftest with an .sjs serving a file with a CSP header?

Comment 4

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Bug 1477458 - Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP.

Review commit: https://reviewboard.mozilla.org/r/258550/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258550/

Comment 5

[Ian Moody [:Kwan] (UTC+0)]

Attachment: Bug 1477458 - Add test for XML Pretty Print with restrictive CSP.

Review commit: https://reviewboard.mozilla.org/r/258556/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/258556/

Comment 6

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8993900 [details]
Bug 1477458 - Add test for XML Pretty Print with restrictive CSP.

https://reviewboard.mozilla.org/r/258556/#review265566

::: dom/base/test/reftest/reftest.list:2
(Diff revision 1)
>  == test_bug920877.html test_bug920877-ref.html
> +HTTP == test_xmlPrettyPrint_csp.xml test_xmlPrettyPrint_csp-ref.xml

Is this alright where it is or should it be in a new reftest directory under dom/xml?

Comment 7

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Ian Moody [:Kwan] (UTC+0) from comment #3)
> Yep that handled it.  Do we care about being able to use inline styles etc.
> in UA shadow roots?  Or is being able to use chrome:// links to external
> resources good enough?

Oh right, we'd probably need _actual_ UA shadow roots first: bug 1431255.  Presumably CSP wouldn't affect stuff inside those.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8993896 [details]
Bug 1477458 - Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP.

https://reviewboard.mozilla.org/r/258550/#review265950

::: dom/xml/resources/XMLPrettyPrint.xsl:21
(Diff revision 1)
>  
>    <xsl:output method="xml"/>
>  
>    <xsl:template match="/">
>      <div id="top">
> -      <style>
> +      <link rel="stylesheet" href="chrome://global/content/xml/XMLPrettyPrint.css"/>

If site CSP affects <style>, wouldn't it affect <link> as well?  And if it's not affecting the <link>, why not and why does that not apply to <style>?

Or is the <style> OK but the @import is what ends up getting CSP applying to it?  I would expect that to work too, honestly, if the mTriggeringPrincipal of the <style> is set right.  And if it's not set right, then I'd expect the <link> to have the same problem.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8993900 [details]
Bug 1477458 - Add test for XML Pretty Print with restrictive CSP.

https://reviewboard.mozilla.org/r/258556/#review265952

r=me assuming this fails without the patch for the bug.

Comment 10

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8993896 [details]
Bug 1477458 - Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP.

https://reviewboard.mozilla.org/r/258550/#review265950

> If site CSP affects <style>, wouldn't it affect <link> as well?  And if it's not affecting the <link>, why not and why does that not apply to <style>?
> 
> Or is the <style> OK but the @import is what ends up getting CSP applying to it?  I would expect that to work too, honestly, if the mTriggeringPrincipal of the <style> is set right.  And if it's not set right, then I'd expect the <link> to have the same problem.

My naive assumption (once I'd confirmed that it does in fact work) was that the <style> is classified as unsafe-inline, and thus disallowed before even getting to the import, whereas since the <link> is a chrome:// URL it bypasses CSP.
(And presumably the UA shadow root work in bug 1431255 might allow <style> to bypass it as well)

And after some clueless debugging I think that's right.  (Thanks to Mossop and froydnj!)  chrome://global/content/xml/XMLPrettyPrint.css has an mInternalContentPolicyType of TYPE_INTERNAL_STYLESHEET, so I'm guessing that means CSP restrictions don't apply to it.  However since the <style> isn't special in anyway at the moment is isn't marked as such (though since it seems to use a different code path I can't check in the same place).

Comment 11

[Boris Zbarsky [:bzbarsky]]

> as that the <style> is classified as unsafe-inline

Sure.

> whereas since the <link> is a chrome:// URL it bypasses CSP.

The fact that it's a chrome:// URL suggests that it's getting loaded with the system principal as triggering principal, which would in fact bypass CSP.  But then why is that principal not being used for the <style> (which would also bypass CSP)?

This is the part I am trying to understand.

Can you check what principals (aPrincipal and aTriggeringPrincipal) nsStyleUtil::CSPAllowsInlineStyle sees for your sheet in the inline style case?  What principals (aLoadingPrincipal and aTriggeringPrincipal) does Loader::CheckContentPolicy see for your sheet in the <link> case?

> has an mInternalContentPolicyType of TYPE_INTERNAL_STYLESHEET

Sure, all <link> loads do.  We apply CSP to that content policy type.

Comment 12

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #11)
> Can you check what principals (aPrincipal and aTriggeringPrincipal)
> nsStyleUtil::CSPAllowsInlineStyle sees for your sheet in the inline style
Principal: Content, Triggering: 0x0
> case?  What principals (aLoadingPrincipal and aTriggeringPrincipal) does
> Loader::CheckContentPolicy see for your sheet in the <link> case?
Both ContentPrincipal

So now I'm out of my depth, and puzzled.
(But thanks for the pointers on where to break, much easier when one knows where to look)

> > has an mInternalContentPolicyType of TYPE_INTERNAL_STYLESHEET
> 
> Sure, all <link> loads do.  We apply CSP to that content policy type.

Bah, I wish I'd remember publishing mozreview comments is two clicks.

(Ian Moody, ~3 hours ago)
>Hmm, except INTERNAL doesn't mean what I thought it did, so I'm still not sure why it's loading.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Principal: Content, Triggering: 0x0

OK, so that will do the CSP check against that content principal...

> Both ContentPrincipal

For the random XML page?  Why is the CSP check there passing?

Comment 14

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #13)
> > Principal: Content, Triggering: 0x0
> 
> OK, so that will do the CSP check against that content principal...
> 
> > Both ContentPrincipal
> 
> For the random XML page?  Why is the CSP check there passing?

Why indeed
http://kwan.perix.co.uk/mozilla/chromeCSS/test.html
(Are web pages supposed to be able to import chrome:// CSS?  I found it... surprising.  Even if they can't access the .cssRules.)

Comment 15

[Boris Zbarsky [:bzbarsky]]

> Are web pages supposed to be able to import chrome:// CSS?

Yes, if the relevant chrome package exports it to web pages.

Comment 16

[Boris Zbarsky [:bzbarsky]]

OK.  So apparently https://searchfox.org/mozilla-central/rev/bdfd20ef30d521b57d5b6feeda71325e8b4cad66/dom/security/nsCSPService.cpp#89-115 is the key: we skip CSP for URI_IS_LOCAL_RESOURCE (including chrome:) for image and stylesheet loads...

So messed up.  :(

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8993896 [details]
Bug 1477458 - Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP.

https://reviewboard.mozilla.org/r/258550/#review266242

::: commit-message-5a810:1
(Diff revision 1)
> +Bug 1477458 - Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP.

Worth pointing out in the commit message that our CSP implementation special-cases `<link rel="stylesheet">` with a chrome:// URL to ignore CSP, which is why this works.

Comment 18

[Ian Moody [:Kwan] (UTC+0)]

Changing summary since the bustage isn't restricted to shadow DOM being on 

[Tracking Requested - why for this release]:
broken XML view for some sites with CSPs forbidding inline style.


(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> OK.  So apparently
> https://searchfox.org/mozilla-central/rev/
> bdfd20ef30d521b57d5b6feeda71325e8b4cad66/dom/security/nsCSPService.cpp#89-
> 115 is the key: we skip CSP for URI_IS_LOCAL_RESOURCE (including chrome:)
> for image and stylesheet loads...
> 
> So messed up.  :(
Ah, which was done in bug 1439444 specifically because the new CSP behaviour from bug 1432358 busted this exact stylesheet.
But I wonder why it did, that was with it added by XBL, I'd have thought that would have given it a non-content principal.  videocontrols.css seems to get 0x0 (NullPrincipal?) for both.  Guess it's weirdness of the insertion.
Maybe bug 1431255 will lead to this having a system principal and then the exception (at least for stylesheets) could go?

Comment 19

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8993896 [details]
Bug 1477458 - Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258550/diff/1-2/

Comment 20

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8993900 [details]
Bug 1477458 - Add test for XML Pretty Print with restrictive CSP.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/258556/diff/1-2/

Comment 21

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8993896 [details]
Bug 1477458 - Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP.

https://reviewboard.mozilla.org/r/258550/#review266242

> Worth pointing out in the commit message that our CSP implementation special-cases `<link rel="stylesheet">` with a chrome:// URL to ignore CSP, which is why this works.

Done.

Comment 22

[Ian Moody [:Kwan] (UTC+0)]

Hmm, mach try fuzzy [path] doesn't seem to know my new test (or its existing sibling) exists (perhaps for the same reason mach test can't run reftests?) so I'm not sure of a good way to do a try push without excessive tests.
Given that this is pretty much just reverting to the old style insertion behaviour it shouldn't be that risky.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eefeae0deacf
Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP. r=bz
https://hg.mozilla.org/integration/autoland/rev/e6d8a8e93db2
Add test for XML Pretty Print with restrictive CSP. r=bz

Comment 24

[Mozilla Autoland]

We're sorry - something has gone wrong while rewriting or rebasing your commits. The commits being pushed no longer match what was requested. Please file a bug.

Comment 25

[Boris Zbarsky [:bzbarsky]]

> I'd have thought that would have given it a non-content principal.

The principal at the time was the principal of the node, which was just the principal of the document.  XBL did not affect this.

> Maybe bug 1431255 will lead to this having a system principal

Possibly.  We'll see how it actually gets implemented.

Comment 26

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Tracking for 62. If a fix lands for 63, we may want to consider uplift.

Comment 27

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/eefeae0deacf
https://hg.mozilla.org/mozilla-central/rev/e6d8a8e93db2

Comment 28

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sorry about incorrectly assuming that this won’t affect 62 :’(

Comment 29

[Ian Moody [:Kwan] (UTC+0)]

Comment on attachment 8993896 [details]
Bug 1477458 - Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1437956

[User impact if declined]:
XML files on sites that have a Content Security Policy (CSP) forbidding inline styles will have a broken Pretty Print.

[Is this code covered by automated tests?]:
Yes.  An existing test covers Pretty Printing working generally (dom/base/test/file_bug590812.xml), and the 2nd commit here adds a test for this style under CSP scenario.

[Has the fix been verified in Nightly?]:
Yes, working in 63.0a1 (2018-07-26) 8f2f847b2f9d, both with and without Shadow DOM.

[Needs manual test from QE? If yes, steps to reproduce]:
1) Open bug's URL
2) Page should have XML syntax highlighting

[List of other uplifts needed for the feature/fix]:
None.

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Essentially just restoring the old style import behaviour from before bug 1437956.  It wouldn't have changed at all except for bug 1410578 meant it didn't work in the new (currently Nightly-only) Shadow DOM path.  Now that that bug is fixed we can go back to the old way.

[String changes made/needed]:
None

Comment 30

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8993896 [details]
Bug 1477458 - Use <link/> instead of <style/> to insert CSS for XML Pretty Printing to avoid site CSP.

Recent regression in 62, seems like a good idea to uplift, Beta62+

Comment 31

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0d2d99c8e40
https://hg.mozilla.org/releases/mozilla-beta/rev/7f9a3b5d4316

Comment 32

[Cristian Craciun]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:64.0) Gecko/20100101 Firefox/64.0
Build ID: 20180913100107

Verified as fixed on the latest Nightly build (64.0a1), on the latest Beta build (63.0b5) and on Release (62).