Closed Bug 1051847 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox :: General, defect)

x86
All
defect
Not set
normal
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 36
Iteration:
36.1

People

(Reporter: sevaan, Assigned: viv1fi, Mentored)

References

Details

(Whiteboard: [lang=js][good first bug])

Attachments

(2 files, 3 obsolete files)

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
Flags: firefox-backlog+
Blocks: 935753
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.
Points: --- → 3
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.
Mentor: nfroyd
Whiteboard: [lang=js][good first bug]
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).
Assignee: nobody → viv1fi
Assigning this to Sofie as part of her Ascend project work.
(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.
Attached patch extended about: whitelist (obsolete) — Splinter Review
Kept it a regex literal...
Attachment #8502077 - Flags: review?(nfroyd)
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.
Attachment #8502077 - Flags: review?(nfroyd)
Attachment #8502077 - Flags: review?(jaws)
Attachment #8502077 - Flags: feedback+
Flags: qe-verify?
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.
Attachment #8502077 - Flags: review?(jaws) → review-
Attached patch revised.patch (obsolete) — Splinter Review
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`.
(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.
Attached patch conservative.patch (obsolete) — Splinter Review
Thanks for your feedback.
Attachment #8502077 - Attachment is obsolete: true
Attachment #8503449 - Attachment is obsolete: true
Attachment #8503471 - Flags: review+
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! :)
Attachment #8503471 - Flags: review+ → review?(dao)
Comment on attachment 8503471 [details] [diff] [review]
conservative.patch

looks good, thanks
Attachment #8503471 - Flags: review?(dao) → review+
Like this?
Attachment #8503471 - Attachment is obsolete: true
Attachment #8505839 - Flags: review+
https://hg.mozilla.org/integration/fx-team/rev/659522f48d69
Summary: Add identity block to about: pages that do not have an identity block → Add trusted identity block to remaining end-user facing about: pages
https://hg.mozilla.org/mozilla-central/rev/659522f48d69
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 36
Iteration: --- → 36.1
(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.
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"
(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!
Flags: needinfo?(valeriopetroni)
Depends on: 1094947
Filed Bug 1094947 to add trusted identity block to about:downloads page
Flags: needinfo?(valeriopetroni)
Flags: qe-verify? → qe-verify-
Depends on: 1365552
No longer depends on: 1365552
You need to log in before you can comment on or make changes to this bug.