1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Comment 1

[Phabricator Automation]

Attachment: Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Comment 2

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Tested on android device.
Please review the patch.

Comment 3

[Phabricator Automation]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D918

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

already reviewed this patch - clearing additional flag!

Comment 5

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Please review the patch and let me know if changes are needed.

Comment 6

[Phabricator Automation]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Flag set by Christoph Kerschbaumer [:ckerschb] is no longer active.

https://phabricator.services.mozilla.com/D918

Comment 8

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Previous patch was for android,so hence removed it and now made changes for desktop for about:certerror and about:neterror.
Please review the patch and let me know if changes are needed.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Hey Gijs,

it seems about:certerror as well as about:neterror end up loading aboutNetError.xhtml, probably because of [1]:
> % override chrome://global/content/netError.xhtml chrome://browser/content/aboutNetError.xhtml

Would it make sense to remove that line and just update the individual entries within aboutRedirector to reflect that change:

> "certerror", "chrome://browser/content/aboutNetError.xhtml",
> "neterror", "chrome://global/content/netError.xhtml",

Or am I missing something?

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/jar.mn#135

Comment 10

[:Gijs (he/him)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #9)
> Hey Gijs,
> 
> it seems about:certerror as well as about:neterror end up loading
> aboutNetError.xhtml, probably because of [1]:
> > % override chrome://global/content/netError.xhtml chrome://browser/content/aboutNetError.xhtml
> 
> Would it make sense to remove that line and just update the individual
> entries within aboutRedirector to reflect that change:
> 
> > "certerror", "chrome://browser/content/aboutNetError.xhtml",
> > "neterror", "chrome://global/content/netError.xhtml",
> 
> Or am I missing something?

I think there's some confusion. The jar.mn line means that when we load netError.xhtml, we end up loading aboutNetError.xhtml instead. certerror points directly to the latter, neterror is defined in docshell/ and points to the former, which then points to the latter.


netError.xhtml lives in Core, but aboutNetError.xhtml lives in browser/ . the about redirector that sets these neterror URL is in docshell/ . So we can't just unify this, unfortunately, because desktop Firefox isn't the only user of docshell/netwerk/Core.

I don't really understand how this is related to this bug. We should just update *all* of the network error pages. In fact, including mobile, even if we don't complain about it not having a CSP because we don't do that on mobile...

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to :Gijs (he/him) from comment #10)
> I don't really understand how this is related to this bug. We should just
> update *all* of the network error pages.

Ah that makes sense now. I haven't realized the distinction between docshell/ and browser/. Thanks for clarification.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

I am fine with that change. Please verify with Gijs that we are not missing anything.

Comment 13

[Phabricator Automation]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D918

Comment 14

[Phabricator Automation]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Flag set by Christoph Kerschbaumer [:ckerschb] is no longer active.

https://phabricator.services.mozilla.com/D918

Comment 15

[Vinothkumar Nagasayanan [:vinoth]]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Yes I added the commands you mentioned.
I added indentation to the newly added file, so it shows it as new changes.
Please review by enabling "Whitespace changes ignore all" in History tab of phabricator, to see the new changes I added clearly.
Or use the URL like this,
https://phabricator.services.mozilla.com/D918?whitespace=ignore-all

So that it shows the new changes alone.

Please verify and let me know the comments.

Comment 16

[Phabricator Automation]

Comment on attachment 8967257 [details]
Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror

Christoph Kerschbaumer [:ckerschb] has approved the revision.
:Gijs (he/him) has approved the revision.

https://phabricator.services.mozilla.com/D918

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac371c7f3ebc
Apply Meta CSP to Content Privileged about:certerror and about:neterror. r=ckerschb, r=Gijs

Comment 18

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/ac371c7f3ebc