Closed Bug 1447384 Opened 4 years ago Closed 4 years ago

CSP is blocking bits of about:reader

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 --- wontfix
firefox61 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

STR:

1. open news website like BBC , CNN, whatever
2. pick news article and click it (I used http://money.cnn.com/2018/03/20/technology/facebook-data-scandal-deepens/index.html )
3. enter reader mode once it's loaded
4. open web console

ER:
no errors

AR:

17:45:35.878 Content Security Policy: The page’s settings blocked the loading of a resource at self (“default-src”). Source: 
            @keyframes grow {
         ....
reader:1
17:45:36.028 Content Security Policy: The page’s settings blocked the loading of a resource at self (“default-src”). Source: onclick attribute on DIV element.


Not 100% sure why we're blocking keyframes but not other bits of CSS, apparently? Or where the onclick is coming from. Need to investigate further.

Christoph, I don't suppose you have time to look at this, or ideas about what's up with the keyframes thing?
Flags: needinfo?(ckerschb)
I can only have a wild guess here. Within Bug 1185351 [1] we fixed a problem where CSP was applied even though we were operating within an anonymous subtree. We perform that IsInAnonymousSubtree()-check within nsStyledElement.cpp but not in others [2], like e.g. we don't do that anonymous subtree check within nsStyleLinkElement.cpp or also not within nsSMILCSSValueType.cpp. I don't know if the |@keyframes grow| part falls in any of those two and also not if the reader mode uses anonymous subtrees, but that might be worth checking.

Unfortunately I don't have the cycles to look into that in more detail at the moment. If you think it's critical to fix we could ask :vino to take a closer look.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1185351
[2] https://dxr.mozilla.org/mozilla-central/search?q=CSPAllowsInlineStyle&redirect=false
Flags: needinfo?(ckerschb)
Flags: needinfo?(gijskruitbosch+bugs)
The <div> stuff seems to be an attribute that is in the CNN markup that the sanitizer correctly removes, but somehow still triggers a CSP error. I don't really know why, but that can be a separate bug. The keyframes stuff only breaks the animation of the waveform on the narrate button. That's unfortunate but not the end of the world, so let's not worry about that for 60.
Flags: needinfo?(gijskruitbosch+bugs)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Depends on: 1449246
Comment on attachment 8962784 [details]
Bug 1447384 - fix narrate styling falling foul of CSP,

https://reviewboard.mozilla.org/r/231634/#review238730

Sorry for the delay. This is much better, thank you!
Attachment #8962784 - Flags: review?(jhofmann) → review+
We should probably file a follow-up bug for removing innerHTML entirely there :)

Also note that I still get this error when opening about:reader:

> Content Security Policy: The page’s settings blocked the loading of a resource at self (“default-src”). Source: onclick attribute on A element.
(In reply to Johann Hofmann [:johannh] from comment #5)
> Also note that I still get this error when opening about:reader:
> 
> > Content Security Policy: The page’s settings blocked the loading of a resource at self (“default-src”). Source: onclick attribute on A element.

Yeah, I filed bug 1449246 about this - it seems that when we use the sanitizer to remove stuff, it still throws CSP errors (we pass the sanitizer a reference to a node in which we want to insert things so it presumably gets the document's CSP that way).
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/8a98c1c77c43
fix narrate styling falling foul of CSP, r=johannh
Blocks: 1450924
https://hg.mozilla.org/mozilla-central/rev/8a98c1c77c43
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.