Last Comment Bug 1051847 - Add trusted identity block to remaining end-user facing about: pages
: Add trusted identity block to remaining end-user facing about: pages
Status: RESOLVED FIXED
[lang=js][good first bug]
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: x86 All
-- normal (vote)
: Firefox 36
Assigned To: viv1fi
:
:
Mentors: Nathan Froyd [:froydnj]
: 977752 (view as bug list)
Depends on: 988959 1094947
Blocks: 935753
  Show dependency treegraph
 
Reported: 2014-08-11 07:49 PDT by Sevaan Franks [:sevaan]
Modified: 2015-02-02 07:07 PST (History)
13 users (show)
sfranks: firefox‑backlog+
florin.mezei: qe‑verify-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: 36.1
Points: 3
Has Regression Range: ---
Has STR: ---


Attachments
Example of Identity Block (128.54 KB, image/png)
2014-08-11 07:49 PDT, Sevaan Franks [:sevaan]
no flags Details
extended about: whitelist (1.03 KB, patch)
2014-10-08 14:55 PDT, viv1fi
dao+bmo: review-
nfroyd: feedback+
Details | Diff | Splinter Review
revised.patch (928 bytes, patch)
2014-10-10 15:04 PDT, viv1fi
no flags Details | Diff | Splinter Review
conservative.patch (920 bytes, patch)
2014-10-10 15:39 PDT, viv1fi
dao+bmo: review+
Details | Diff | Splinter Review
0001-1051847-Added-ID-blocks-for-about-license-rights.patch (1.21 KB, patch)
2014-10-15 16:19 PDT, viv1fi
viv1fi: review+
Details | Diff | Splinter Review

Description User image Sevaan Franks [:sevaan] 2014-08-11 07:49:24 PDT
Created attachment 8470862 [details]
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 1 User image :Gijs 2014-08-11 08:00:55 PDT
*** Bug 977752 has been marked as a duplicate of this bug. ***
Comment 2 User image Jared Wein [:jaws] (please needinfo? me) 2014-08-18 07:21:02 PDT
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 User image Nathan Froyd [:froydnj] 2014-10-04 08:49:06 PDT
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 User image jaramat 2014-10-05 23:57:42 PDT
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 User image Lukas Blakk [:lsblakk] use ?needinfo 2014-10-06 13:40:18 PDT
Assigning this to Sofie as part of her Ascend project work.
Comment 6 User image Jared Wein [:jaws] (please needinfo? me) 2014-10-06 13:43:57 PDT
(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 User image viv1fi 2014-10-08 14:55:29 PDT
Created attachment 8502077 [details] [diff] [review]
extended about: whitelist

Kept it a regex literal...
Comment 8 User image Nathan Froyd [:froydnj] 2014-10-10 08:35:22 PDT
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 User image Dão Gottwald [:dao] 2014-10-10 08:52:15 PDT
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 User image viv1fi 2014-10-10 15:04:00 PDT
Created attachment 8503449 [details] [diff] [review]
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 User image Dão Gottwald [:dao] 2014-10-10 15:10:21 PDT
(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 User image viv1fi 2014-10-10 15:39:19 PDT
Created attachment 8503471 [details] [diff] [review]
conservative.patch

Thanks for your feedback.
Comment 13 User image Jared Wein [:jaws] (please needinfo? me) 2014-10-10 20:29:53 PDT
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 User image Dão Gottwald [:dao] 2014-10-10 21:02:41 PDT
Comment on attachment 8503471 [details] [diff] [review]
conservative.patch

looks good, thanks
Comment 15 User image Ryan VanderMeulen [:RyanVM] 2014-10-11 18:19:55 PDT
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 User image viv1fi 2014-10-15 16:19:20 PDT
Created attachment 8505839 [details] [diff] [review]
0001-1051847-Added-ID-blocks-for-about-license-rights.patch

Like this?
Comment 17 User image Dão Gottwald [:dao] 2014-10-16 06:45:51 PDT
https://hg.mozilla.org/integration/fx-team/rev/659522f48d69
Comment 18 User image Carsten Book [:Tomcat] 2014-10-17 00:48:34 PDT
https://hg.mozilla.org/mozilla-central/rev/659522f48d69
Comment 19 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2014-10-23 17:40:08 PDT
(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 User image Valerio 2014-11-06 08:41:51 PST
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 User image Jared Wein [:jaws] (please needinfo? me) 2014-11-06 08:45:29 PST
(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 User image Valerio 2014-11-06 09:12:45 PST
Filed Bug 1094947 to add trusted identity block to about:downloads page

Note You need to log in before you can comment on or make changes to this bug.