Closed Bug 1453560 Opened 6 years ago Closed 6 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: vinoth, Assigned: vinoth)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Whiteboard: [domsecurity-active])

Attachments

(1 file)

      No description provided.
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
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.
Attachment #8967257 - Flags: review?(ckerschb)
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
Attachment #8967257 - Flags: review+
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!
Attachment #8967257 - Flags: review?(ckerschb)
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.
Attachment #8967257 - Flags: review?(gijskruitbosch+bugs)
Attachment #8967257 - Flags: review?(gijskruitbosch+bugs) → review-
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
Attachment #8967257 - Flags: review+
Summary: Apply Meta CSP to Content Privileged about:certerror → Apply Meta CSP to Content Privileged about:certerror and about:neterror
Attachment #8967257 - Attachment description: Bug 1453560 - Meta CSP applied to content privileged about:certerror → Bug 1453560 - Apply Meta CSP to Content Privileged about:certerror and about:neterror
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.
Attachment #8967257 - Flags: review- → review?(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
Flags: needinfo?(gijskruitbosch+bugs)
(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...
Flags: needinfo?(gijskruitbosch+bugs)
(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 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.
Attachment #8967257 - Flags: review?(ckerschb)
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
Attachment #8967257 - Flags: review+
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
Attachment #8967257 - Flags: review+
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.
Attachment #8967257 - Flags: review?(gijskruitbosch+bugs)
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
Attachment #8967257 - Flags: review+
Attachment #8967257 - Flags: review?(gijskruitbosch+bugs)
Blocks: 1456920
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac371c7f3ebc
Apply Meta CSP to Content Privileged about:certerror and about:neterror. r=ckerschb, r=Gijs
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ac371c7f3ebc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1486208
You need to log in before you can comment on or make changes to this bug.