1099088 - Add a "This is a secure Firefox page" site identity popup on about: pages, as per desktop

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Depends on bug 1078508 for doorhangers on unidentified SiteIdentity pages.

Comment 1

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Got a screen shot? This sounds like a simple string of info though unless I'm mistaken?

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: about:home popup

Note that this looks a bit different on the release channel too.

Comment 3

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Sounds like a good, straight forward idea to me.

Let's add these for our about: pages then.

"This is a secure Firefox page" works I think. We can look to tell me as we refine the UX around our door hangers.

Comment 4

[Andrzej Hunt :ahunt]

Attachment: Draft patch - whitelist not yet complete.

I've started working on this - a screenshot and draft patch are attached.

I still need to double check the whitelist of about: pages, since there seem to be some about: pages that are mobile only (and probably vice-versa), e.g. about:firefox doesn't seem to exist.

Comment 5

[Andrzej Hunt :ahunt]

Attachment: Screenshot of current appearance

Here's the screenshot of the current appearance.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Thanks Andrzej! :)

I only skimmed the implementation details, but I'd say a whitelist is probably unnecessary – in Java we use [1]:
  `url.startsWith("about:");`

[1]: http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AboutPages.java#30

Comment 7

[Andrzej Hunt :ahunt]

(In reply to Michael Comella (:mcomella) from comment #6)
> I only skimmed the implementation details, but I'd say a whitelist is
> probably unnecessary – in Java we use [1]:
>   `url.startsWith("about:");`


Yup - I did notice those checks - I was just worried about whitelisting all about: urls since apparently extensions can also add their own about pages [1] (or is that not possible on Android?). And presumably we only want to confirm that it's a secure about: page for those pages?

I guess it would be possible to add a different doorhanger string for such addon/unknown about: pages, though, maybe something like "This is an about page provided by a Firefox extension"? (I've got no idea what desktop Firefox does for though, looking at the code I'd assume it's treated like any other unverified page though.)

[1]: https://developer.mozilla.org/en-US/docs/Custom_about:_URLs

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

To be explicit, when you think your patch is ready for review, you can click "Details" and add a review flag for myself (:mcomella) or :liuche to review the code.

(In reply to Andrzej Hunt :ahunt from comment #7)
> Yup - I did notice those checks - I was just worried about whitelisting all
> about: urls since apparently extensions can also add their own about pages
> [1] (or is that not possible on Android?). And presumably we only want to
> confirm that it's a secure about: page for those pages?

Good call – from my limited knowledge, I'd assume addons can still add `about:` pages. As you recommend, I'd prefer to only show "secure Firefox page" on about: pages we ship.

> I guess it would be possible to add a different doorhanger string for such
> addon/unknown about: pages, though, maybe something like "This is an about
> page provided by a Firefox extension"? (I've got no idea what desktop
> Firefox does for though, looking at the code I'd assume it's treated like
> any other unverified page though.)

I'd be fine saying those addon about: pages are unverified.

It actually appears desktop firefox is not comprehensive with their, "This is a secure Firefox page" (e.g. "about:" and "about:about" though addons can probably add content to the latter) so perhaps we shouldn't be too worried if we aren't comprehensive.

Comment 9

[:Margaret Leibovic]

(In reply to Michael Comella (:mcomella) from comment #8)
> To be explicit, when you think your patch is ready for review, you can click
> "Details" and add a review flag for myself (:mcomella) or :liuche to review
> the code.
> 
> (In reply to Andrzej Hunt :ahunt from comment #7)
> > Yup - I did notice those checks - I was just worried about whitelisting all
> > about: urls since apparently extensions can also add their own about pages
> > [1] (or is that not possible on Android?). And presumably we only want to
> > confirm that it's a secure about: page for those pages?
> 
> Good call – from my limited knowledge, I'd assume addons can still add
> `about:` pages. As you recommend, I'd prefer to only show "secure Firefox
> page" on about: pages we ship.

Yes, good call, add-ons can indeed add about: pages.

Comment 10

[Andrzej Hunt :ahunt]

Attachment: Add secure site hanger for internal about: pages

Sorry, this has taken so long. Here's an updated patch ready for review (identical to the first one, except I've expanded the whitelist to cover more mobile-only pages, and also "about:").

(It would probably be nicer to have some sort of automated way of preparing the whitelist, but I'm not sure that that's worth doing right now?)

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8648981 [details] [diff] [review]
Add secure site hanger for internal about: pages

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

I'm going to deflect this review to liuche – she recently rewrote all of our Doorhanger code.

(In reply to Andrzej Hunt :ahunt from comment #10)
> Sorry, this has taken so long.

No worries – we're all busy! :)

> (It would probably be nicer to have some sort of automated way of preparing
> the whitelist, but I'm not sure that that's worth doing right now?)

There are plenty of things we'd love to have but just don't have time to do. I imagine this isn't going to change frequently so I'm not concerned about it not being automated.

If you really want to do it and the fix is simple, I doubt we'd stop you though. :)

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8648981 [details] [diff] [review]
Add secure site hanger for internal about: pages

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

This looks great! A few changes needed, but I'm really happy with the work on this patch. Send it back when you've addressed comments! I promise I won't take as long this time :P

I do notice that Desktop differentiates between a chrome page and a file:
http://mxr.mozilla.org/mozilla-central/source/browser/components/controlcenter/content/panel.inc.xul#31

I'll file that as another bug (displaying a doorhanger type for Files). At first I was confused about why on Desktop, something like about:config would be a secure page, but about:telemetry is just listed as "a file on your computer", but I think that that is just this whitelist getting out of sync with new about: pages (maybe).

::: mobile/android/base/SiteIdentity.java
@@ +28,5 @@
>      public enum SecurityMode {
>          UNKNOWN("unknown"),
>          IDENTIFIED("identified"),
> +        VERIFIED("verified"),
> +        CHROMEUI("chromeui");

Mmmm, I'm not super excited that Desktop named it chromeUI, which is an implementation description rather than a usage description (like systempage or something), but we should stay consistent. Let's match case though - "chromeUI".

::: mobile/android/base/toolbar/SiteIdentityPopup.java
@@ +308,5 @@
>       * @param siteIdentity SiteIdentity information about the connection.
>       */
>      private void updateConnectionState(final SiteIdentity siteIdentity) {
> +        if (siteIdentity.getSecurityMode() == SecurityMode.CHROMEUI) {
> +                mSecurityState.setText(R.string.identity_connection_chromeui);

You probably need to set text color here too.

@@ +462,5 @@
>              return;
>          }
>  
> +        // Verified about: pages have the CHROMEUI SiteIdentity, however there can also
> +        // be unverfied about: pages for which  "This site's identity is unknown" or

Nit: typo "unverfied"

@@ +488,5 @@
> +        if (mSiteIdentity.getSecurityMode() == SecurityMode.CHROMEUI) {
> +            // For about: pages we display the product icon in place of the verified/globe
> +            // image, hence we don't also set the favicon (for most about pages the
> +            // favicon is the product icon, hence we'd be showing the same icon twice).
> +            mTitle.setText(R.string.moz_app_displayname);

I'd like to see the url here as the title, but you don't have to do it in this bug - you can file a follow-up.

::: mobile/android/base/toolbar/ToolbarDisplayLayout.java
@@ +444,5 @@
> +        // about: pages should default to having no icon too, however
> +        // the relevant SecurityMode has a different ordinal from
> +        // an insecure connection (which also shows no icon).
> +        if (securityMode == SecurityMode.CHROMEUI) {
> +            imageLevel = SecurityMode.UNKNOWN.ordinal();

Ok, this will get a little confusing because it's going to pull the reader out to SecurityMode which isn't too useful; and I've found myself looking up where these security levels are defined every time I look at this file anyways.

So instead of using SecurityMode.UNKNOWN.ordinal, just define a LEVEL_DEFAULT_GLOBE = 0 with the other LEVELS at the beginning of this file, and then add a comment that points to site_security_level.xml as the place where these levels and icons are linked.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/drawable/site_security_level.xml

::: mobile/android/chrome/content/browser.js
@@ +7174,5 @@
>        return this.IDENTITY_MODE_IDENTIFIED;
>      }
>  
> +    // We also allow "about:" by allowing the selector to be empty (i.e. '(|.....|...|...)'
> +    let whitelist = /^about:(|addons|buildconfig|config|crashes|customizing|devices|downloads|firefox|feedback|healthreport|home|license|logins|logo|memory|mozilla|newaddon|permissions|preferences|privatebrowsing|rights|sessionrestore|serviceworkers|support|telemetry|webrtc|welcomeback)/i;

Oh man, whitelist :P

I see that's what Desktop does:
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#6857

A few things though:
* I notice that your regex is too permissive - I tried it out and it allows anything with an ^about: through, which we shouldn't do. See screenshot. (There are also security problems with this, if an add-on makes an about:myEvilAddon page, this would also show up as a "secure Firefox page".)

* Firefox for Android also uses pages that are different from Desktop. So we don't have all of these pages (such as about:permissions). Take a look at AboutRedirector.js, which handles our about: page redirecting.
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js

* One thing that Desktop does is only whitelist chrome-privileged pages. The rest are listed as "this file is stored on your computer", which is a little confusing, maybe? I think that we could whitelist chrome pages, as well as any pages that a user might go to where they might want some reassurance that the page is a secure Firefox page.

@@ +7276,5 @@
>  
>      // Don't show identity data for pages with an unknown identity or if any
>      // mixed content is loaded (mixed display content is loaded by default).
> +    if (identityMode == this.IDENTITY_MODE_UNKNOWN ||
> +        identityMode == this.IDENTITY_MODE_CHROMEUI |

You want a || (logic OR) here, instead of a | (bitwise OR).

Comment 13

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Non-existent page

Needs a setTextColor, and also a less permissive regex.

Comment 14

[Andrzej Hunt :ahunt]

Attachment: patch_doorhanger_v2.diff

(In reply to Chenxia Liu [:liuche] from comment #12)
> This looks great! A few changes needed, but I'm really happy with the work
> on this patch. Send it back when you've addressed comments! I promise I
> won't take as long this time :P

Thank you for the review : ) (It's taken me a while to get back to this too...)

I've attached an updated patch which I think addresses everything you've brought up, there however are a few things I'm still unsure about (see below).

(I'd also like to try and see if I can write some tests for this -- I don't have much of a clue about the frameworks/etc yet, so no idea how long it'll take me to figure that out.)

> ::: mobile/android/base/SiteIdentity.java
> @@ +28,5 @@
> >      public enum SecurityMode {
> >          UNKNOWN("unknown"),
> >          IDENTIFIED("identified"),
> > +        VERIFIED("verified"),
> > +        CHROMEUI("chromeui");
> 
> Mmmm, I'm not super excited that Desktop named it chromeUI, which is an
> implementation description rather than a usage description (like systempage
> or something), but we should stay consistent. Let's match case though -
> "chromeUI".

It turns out that the SiteIdentity.SecurityMode constructor does a semi-case-insensitive comparison (it transforms the id that we receive from browser.js with toLowerCase(), and compares that to the untransformed String defined in our java code) - which would fail for chromeUI. I think it's best to just compare the original input strings (i.e. no toLowerCase() for either side), hence that's what I've done in my patch (as far as I can tell we don't have any wrongly-cased versions of these strings defined anywhere, so this shouldn't break anything) - but I'm not entirely happy since I don't have any idea why this was semi-case-insensitive in the first place.

The relevant constructor is at:
https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/SiteIdentity.java#45

> ::: mobile/android/chrome/content/browser.js
> @@ +7174,5 @@
> >        return this.IDENTITY_MODE_IDENTIFIED;
> >      }
> >  
> > +    // We also allow "about:" by allowing the selector to be empty (i.e. '(|.....|...|...)'
> > +    let whitelist = /^about:(|addons|buildconfig|config|crashes|customizing|devices|downloads|firefox|feedback|healthreport|home|license|logins|logo|memory|mozilla|newaddon|permissions|preferences|privatebrowsing|rights|sessionrestore|serviceworkers|support|telemetry|webrtc|welcomeback)/i;
> 
> Oh man, whitelist :P
> 

> A few things though:
> * I notice that your regex is too permissive - I tried it out and it allows
> anything with an ^about: through, which we shouldn't do. See screenshot.
> (There are also security problems with this, if an add-on makes an
> about:myEvilAddon page, this would also show up as a "secure Firefox page".)

Urggh, sorry - that should be fixed now (by adding the line-ending $ to the ($|....) selector). (I didn't test properly after I tried to add support for the "about:" page.)

> * One thing that Desktop does is only whitelist chrome-privileged pages. The
> rest are listed as "this file is stored on your computer", which is a little
> confusing, maybe? I think that we could whitelist chrome pages, as well as
> any pages that a user might go to where they might want some reassurance
> that the page is a secure Firefox page.

Hmm, I don't think I get this part. Do we have other bundled/built-in pages that aren't about: pages that should get the same treatment? (I've been testing a bit, and have discovered it's possible to load some of the about pages via chrome://...... - are there further pages there that are by default reached via a chrome://... url rather than as an about: page?)

Comment 15

[Chenxia Liu [:liuche]]

Comment on attachment 8666914 [details] [diff] [review]
patch_doorhanger_v2.diff

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

Okay! This looks good, it's pretty much completely done, just a couple of last things mainly about the list of about: pages.

> Hmm, I don't think I get this part. Do we have other bundled/built-in pages
> that aren't about: pages that should get the same treatment? (I've been
> testing a bit, and have discovered it's possible to load some of the about
> pages via chrome://...... - are there further pages there that are by
> default reached via a chrome://... url rather than as an about: page?)

You don't need to worry about chrome:// urls - these are page resources for us, and users shouldn't be loading them anyways.

What I mean by this is, there are some about: pages that Fennec provides that should also be on this whitelist, but aren't - I went through the about:about page, which lists (most of) our about: pages, and there are a few that don't show a "This is a secure [Firefox] page" doorhanger. Of the ones I saw, the following should also be listed as secure Fennec pages: about:cache, :accounts, :networking, :crashes (crashreporter is disabled, so you won't actually see a page), :performance, and :plugins.
For a complete list, you can take a look at AboutRedirector.js.

...and again, I'm really sorry this took so long :( No more conferences coming up, just released, and I've learned that leaving a patch too long means it more time to refamiliarize myself with the context.

::: mobile/android/base/SiteIdentity.java
@@ +179,5 @@
>              }
>  
>              try {
>                  mOrigin = identityData.getString("origin");
> +                mHost = identityData.optString("host", null);

I was thinking about these changes (using optional JSON fields instead of throwing when they're not present) - we use these "JSON key not found" exceptions to reset the SiteIdentity info when one of these throws, but it doesn't look like it's strictly necessary, from some page loading that I was looking at (switching between http/https).

So I think this is fine, but I'll keep it in mind and maybe look at some code too.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
@@ +310,5 @@
>       */
>      private void updateConnectionState(final SiteIdentity siteIdentity) {
> +        if (siteIdentity.getSecurityMode() == SecurityMode.CHROMEUI) {
> +                mSecurityState.setText(R.string.identity_connection_chromeui);
> +                mSecurityState.setTextColor(ColorUtils.getColor(mContext, R.color.affirmative_green));

This color should actually be the normal text color, not green :)

::: mobile/android/chrome/content/browser.js
@@ +6421,5 @@
>        return this.IDENTITY_MODE_IDENTIFIED;
>      }
>  
> +    // We also allow "about:" by allowing the selector to be empty (i.e. '(|.....|...|...)'
> +    let whitelist = /^about:($|about|addons|buildconfig|config|crashes|devices|downloads|fennec|firefox|feedback|healthreport|home|license|logins|logo|memory|mozilla|newaddon|privatebrowsing|rights|serviceworkers|support|telemetry|webrtc)/i;

Actually, I think we should remove about:home! (and by extension, about: ) We no longer have the url highlighted when you click on the urlbar, and there is no longer a favicon, so there's now no indication that this is a "real" page with site identity information.

Comment 16

[:Margaret Leibovic]

(In reply to Chenxia Liu [:liuche] from comment #15)
> Comment on attachment 8666914 [details] [diff] [review]
> patch_doorhanger_v2.diff
> 
> Review of attachment 8666914 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Okay! This looks good, it's pretty much completely done, just a couple of
> last things mainly about the list of about: pages.
> 
> > Hmm, I don't think I get this part. Do we have other bundled/built-in pages
> > that aren't about: pages that should get the same treatment? (I've been
> > testing a bit, and have discovered it's possible to load some of the about
> > pages via chrome://...... - are there further pages there that are by
> > default reached via a chrome://... url rather than as an about: page?)
> 
> You don't need to worry about chrome:// urls - these are page resources for
> us, and users shouldn't be loading them anyways.
> 
> What I mean by this is, there are some about: pages that Fennec provides
> that should also be on this whitelist, but aren't - I went through the
> about:about page, which lists (most of) our about: pages, and there are a
> few that don't show a "This is a secure [Firefox] page" doorhanger. Of the
> ones I saw, the following should also be listed as secure Fennec pages:
> about:cache, :accounts, :networking, :crashes (crashreporter is disabled, so
> you won't actually see a page), :performance, and :plugins.
> For a complete list, you can take a look at AboutRedirector.js.

You should also look at nsAboutRedirector.cpp in docshell:
http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsAboutRedirector.cpp

about:about isn't actually a comprehensive list of all the about: pages, since there's a HIDE_FROM_ABOUTABOUT flag.

> ::: mobile/android/chrome/content/browser.js
> @@ +6421,5 @@
> >        return this.IDENTITY_MODE_IDENTIFIED;
> >      }
> >  
> > +    // We also allow "about:" by allowing the selector to be empty (i.e. '(|.....|...|...)'
> > +    let whitelist = /^about:($|about|addons|buildconfig|config|crashes|devices|downloads|fennec|firefox|feedback|healthreport|home|license|logins|logo|memory|mozilla|newaddon|privatebrowsing|rights|serviceworkers|support|telemetry|webrtc)/i;
> 
> Actually, I think we should remove about:home! (and by extension, about: )
> We no longer have the url highlighted when you click on the urlbar, and
> there is no longer a favicon, so there's now no indication that this is a
> "real" page with site identity information.

Is this whitelist really the best solution here? Is that what desktop does? This seems a bit fragile to maintain. I suppose we wouldn't want this showing for add-on about: pages, but then again, an add-on could theoretically add itself to this list anyway.

Comment 17

[Chenxia Liu [:liuche]]

> Is this whitelist really the best solution here? Is that what desktop does?
> This seems a bit fragile to maintain. I suppose we wouldn't want this
> showing for add-on about: pages, but then again, an add-on could
> theoretically add itself to this list anyway.

Yeah...that's exactly what desktop does.

http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#7126

Comment 18

[Chenxia Liu [:liuche]]

Comment on attachment 8666914 [details] [diff] [review]
patch_doorhanger_v2.diff

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

::: mobile/android/chrome/content/browser.js
@@ +6523,5 @@
>  
>      // Don't show identity data for pages with an unknown identity or if any
>      // mixed content is loaded (mixed display content is loaded by default).
> +    if (identityMode == this.IDENTITY_MODE_UNKNOWN ||
> +        identityMode == this.IDENTITY_MODE_CHROMEUI ||

Also, we shouldn't be checking CHROMEUI here - I need to fix this in a different bug, where we don't use a whitelist to determine secure/insecure, but let Gecko make that check. (And probably we'd want CHROMEUI to be secure, not insecure.)

Comment 19

[:Margaret Leibovic]

ahunt, can you work on landing this? This would fix bug 1219819.

Comment 20

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1099088 - Part 1: Add CHROMEUI SiteIdentity for about: pages r=liuche

This replicates desktop behaviour.

Review commit: https://reviewboard.mozilla.org/r/29973/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29973/

Comment 21

[Andrzej Hunt :ahunt]

Attachment: MozReview Request: Bug 1099088 - Part 2: Show "This is a secure Firefox page" site identity popup on about: pages r=liuche

Review commit: https://reviewboard.mozilla.org/r/29975/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29975/

Comment 22

[Andrzej Hunt :ahunt]

Comment on attachment 8666914 [details] [diff] [review]
patch_doorhanger_v2.diff

(New patches are on reviewboard - this patch is obsolete.)

Comment 23

[Chenxia Liu [:liuche]]

Comment on attachment 8705414 [details]
MozReview Request: Bug 1099088 - Part 1: Add CHROMEUI SiteIdentity for about: pages r=liuche

https://reviewboard.mozilla.org/r/29973/#review27159

just a few comments on the regex.

::: mobile/android/chrome/content/browser.js:6589
(Diff revision 1)
> +    let whitelist = /^about:($|about|accounts|addons|buildconfig|cache|crashes|config|crashes|devices|downloads|fennec|firefox|feedback|healthreport|license|logins|logo|memory|mozilla|networking|newaddon|performance|plugins|privatebrowsing|rights|serviceworkers|support|telemetry|webrtc)/i;

ty for alphabetizing this. "crashes" is in here twice though.

You're missing a $ terminator - e.g., about:addonss passes the whitelist regex even if it could be provided by a malicious addon. Although I guess that's tricky if we want to support things like about:accounts?action...

Is there someway to check the base url, stripping off any parameters? If it gets tricky, we can land the bulk of this with the $ terminator, and handle the about:accounts in a followup bug.

Of this list, a few questions:
- heh, about:fennec? We should remove that as a page. Nothing to do with your patch, but making a note for myself.
- what is about:newaddon, should that be in here?
- I wonder if about:performance is only present on Nightly or was only just recently added, because I don't see it on aurora. Can you verify? And if it is Nightly only, we should handle that (or just leave it off).

Comment 24

[Chenxia Liu [:liuche]]

Comment on attachment 8705415 [details]
MozReview Request: Bug 1099088 - Part 2: Show "This is a secure Firefox page" site identity popup on about: pages r=liuche

https://reviewboard.mozilla.org/r/29975/#review27177

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:450
(Diff revision 1)
> +        if (securityMode == SecurityMode.CHROMEUI) {

Should this be part of the if...else if chain, or did you want to return? We shouldn't have a lone if... statement followed by a chain of if...else ifs.

Comment 25

[Andrzej Hunt :ahunt]

Just a few notes (I'll try and do the fixup tomorrow):

(In reply to Chenxia Liu [:liuche] from comment #23)
> just a few comments on the regex.
> 
> You're missing a $ terminator - e.g., about:addonss passes the whitelist
> regex even if it could be provided by a malicious addon. Although I guess
> that's tricky if we want to support things like about:accounts?action...
> 
> Is there someway to check the base url, stripping off any parameters? If it
> gets tricky, we can land the bulk of this with the $ terminator, and handle
> the about:accounts in a followup bug.

I think it's enough if we add ($|"?") at the end - this would allow parameters for any of the listed about: pages, but disallow pages such as about:addonss (I'll expand the tests to make sure we cover these cases).

> 
> Of this list, a few questions:
> - heh, about:fennec? We should remove that as a page. Nothing to do with
> your patch, but making a note for myself.
> - what is about:newaddon, should that be in here?

I'm not actually sure when it's used, it seems to be a page that's shown when a new addon has been installed in a non-standard way - I suspect it's not relevant for mobile, but I haven't been able to find any obvious documentation.


> - I wonder if about:performance is only present on Nightly or was only just
> recently added, because I don't see it on aurora. Can you verify? And if it
> is Nightly only, we should handle that (or just leave it off).

This does seem to be nightly only ( http://mxr.mozilla.org/mozilla-central/source/docshell/base/nsAboutRedirector.cpp#93 ), I'll remove it.

Comment 26

[Andrzej Hunt :ahunt]

https://reviewboard.mozilla.org/r/29975/#review27177

> Should this be part of the if...else if chain, or did you want to return? We shouldn't have a lone if... statement followed by a chain of if...else ifs.

This is deliberately separate - I've updated the comment, does that make it any clearer?

Comment 27

[Andrzej Hunt :ahunt]

Comment on attachment 8705414 [details]
MozReview Request: Bug 1099088 - Part 1: Add CHROMEUI SiteIdentity for about: pages r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29973/diff/1-2/

Comment 28

[Andrzej Hunt :ahunt]

Comment on attachment 8705415 [details]
MozReview Request: Bug 1099088 - Part 2: Show "This is a secure Firefox page" site identity popup on about: pages r=liuche

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29975/diff/1-2/

Comment 29

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #26)
> https://reviewboard.mozilla.org/r/29975/#review27177
> 
> > Should this be part of the if...else if chain, or did you want to return? We shouldn't have a lone if... statement followed by a chain of if...else ifs.
> 
> This is deliberately separate - I've updated the comment, does that make it
> any clearer?

Here's the updated comment - is that any clearer?

https://reviewboard.mozilla.org/r/29975/diff/2#1.6

Comment 30

[Chenxia Liu [:liuche]]

Nice - thanks for the comment!

Comment 31

[Andrzej Hunt :ahunt]

https://hg.mozilla.org/integration/fx-team/rev/0042c780d090f0209b64e6af10b26d63a67088d3
Bug 1099088 - Part 1: Add CHROMEUI SiteIdentity for about: pages r=liuche

https://hg.mozilla.org/integration/fx-team/rev/c60cb3b3bf83ea6fc8faccddf56965b1e279422d
Bug 1099088 - Part 2: Show "This is a secure Firefox page" site identity popup on about: pages r=liuche

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0042c780d090
https://hg.mozilla.org/mozilla-central/rev/c60cb3b3bf83

Comment 33

[Teodora Vermesan (:TeoVermesan)]

Attachment: Screenshot_20160118-183604.png

Verified as fixed using:
Device: Nexus 6 (Android 6.0)
Build: Firefox for Android 46.0a1 (2016-01-17)

Comment 34

[Nick Alexander :nalexander [he/him]]

Two comments on this.

First, the whitelist is incredibly fragile.  Let's find a way to use https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/AboutRedirector.js#11 or -- better -- a dynamic list of about: pages that could include addon-provided about: pages.

Second, will this have addressed Bug 1221711?  (I think it will have.)

ahunt, can you respond?

Comment 35

[Andrzej Hunt :ahunt]

(In reply to Nick Alexander :nalexander from comment #34)
> Two comments on this.
> 
> First, the whitelist is incredibly fragile.  Let's find a way to use
> https://dxr.mozilla.org/mozilla-central/source/mobile/android/components/
> AboutRedirector.js#11 or -- better -- a dynamic list of about: pages that
> could include addon-provided about: pages.

Filed as Bug 1244278 !

> 
> Second, will this have addressed Bug 1221711?  (I think it will have.)
> 
> ahunt, can you respond?

I think 1221711 is unrelated, I've answered in https://bugzilla.mozilla.org/show_bug.cgi?id=1221711#c6