1051847 - Add trusted identity block to remaining end-user facing about: pages

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Sevaan Franks [:sevaan]]

Attachment: Example of Identity Block

Some of the about: pages use an identity block to show the user that the page they are on
is a trusted part of the browser. The following about: pages do not use an identity block
(but should!):

about:
about:about
about:buildconfig
about:cache
about:downloads
about:license
about:logo
about:memory
about:mozilla
about:networking
about:newtab
about:plugins
about:rights
about:robots
about:sync-log
about:sync-progress
about:sync-tabs
about:telemetry
about:webrtc

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

This should be very simple since we can just add these to the whitelist that we have at http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6572.

Comment 3

[Nathan Froyd [:froydnj]]

Let's link to a specific file, so the whitelist link doesn't move around:

http://hg.mozilla.org/mozilla-central/file/4b180eaafdbf/browser/base/content/browser.js#l6379

I can mentor somebody here, even if I can't review.

Comment 4

[jaramat]

I think about:blank and about:newtab are the only internal pages that shouldn't have the identity block, since they aren't even "regular" pages from the user's point of view (they don't have a URL in the address bar).

Comment 5

[Lukas Blakk [:lsblakk] use ?needinfo]

Assigning this to Sofie as part of her Ascend project work.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to jaramat from comment #4)
> I think about:blank and about:newtab are the only internal pages that
> shouldn't have the identity block, since they aren't even "regular" pages
> from the user's point of view (they don't have a URL in the address bar).

We have to be careful though of not showing this for add-on created about: pages. So we will still need to use a whiltelist.

Comment 7

[viv1fi]

Attachment: extended about: whitelist

Kept it a regex literal...

Comment 8

[Nathan Froyd [:froydnj]]

Comment on attachment 8502077 [details] [diff] [review]
extended about: whitelist

Review of attachment 8502077 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good to me; turning final review over to Jared.

Comment 9

[Dão Gottwald [:dao]]

Comment on attachment 8502077 [details] [diff] [review]
extended about: whitelist

This whitelist intentionally doesn't contain all possible about: URIs but is supposed to contain those who are exposed to end users through some UI. Please extend it only to that end. We shouldn't let this regular expression grow liberally as this is performance-critical code.

Comment 10

[viv1fi]

Attachment: revised.patch

about:* w/ no UI:

buildconfig
cache
crashes
credits
downloads
license -- should have id block
logo
memory
mozilla
networking
newtab
plugins
rights -- should have id block
robots
sync-log
sync-progress
sync-tabs
webrtc

Any more conservative and I may as well remove `about:` & `about:about`.

Comment 11

[Dão Gottwald [:dao]]

(In reply to viv1fi from comment #10)
> Any more conservative and I may as well remove `about:` & `about:about`.

Yep, I see no reason why they should be part of this list.

Comment 12

[viv1fi]

Attachment: conservative.patch

Thanks for your feedback.

Comment 13

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8503471 [details] [diff] [review]
conservative.patch

Review of attachment 8503471 [details] [diff] [review]:
-----------------------------------------------------------------

Dao will still need to formally r+ this patch through Bugzilla. Thanks for working on this! :)

Comment 14

[Dão Gottwald [:dao]]

Comment on attachment 8503471 [details] [diff] [review]
conservative.patch

looks good, thanks

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Please attach a patch that includes commit information per the link below.
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 16

[viv1fi]

Attachment: 0001-1051847-Added-ID-blocks-for-about-license-rights.patch

Like this?

Comment 17

[Dão Gottwald [:dao]]

https://hg.mozilla.org/integration/fx-team/rev/659522f48d69

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/659522f48d69

Comment 19

[:Gavin Sharp [email: [e-mail]]]

(In reply to viv1fi from comment #10)
> buildconfig
> crashes
> memory
> plugins

I think the patch that landed is fine, but for what it's worth, these ones are exposed indirectly via about:support.

Comment 20

[Valerio]

The about:downloads page should be added to the whitelist because in private browsing it is exposed to end users through the download panel option "show all downloads"

Comment 21

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Valerio from comment #20)
> The about:downloads page should be added to the whitelist because in private
> browsing it is exposed to end users through the download panel option "show
> all downloads"

Please file a bug for this, good catch!

Comment 22

[Valerio]

Filed Bug 1094947 to add trusted identity block to about:downloads page