1314563 - Showing green lock for manually added certificate exception

Status: VERIFIED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

According to bug 1313689 Firefox for Android is showing a green lock for manually added certificate exceptions. Desktop versions of Firefox do not. We have been trying to show the same "security" icons as desktop - so we should investigate why there's a difference here.

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Attachment: device-2016-11-08-170253.png

checking in org.mozilla.gecko.toolbar.SiteIdentityPopup#updateConnectionState

Comment 2

[Nevin Chen(Not active on Bugzilla)]

Looks like in org.mozilla.gecko.GeckoAppShell#handleGeckoMessage , EventDispatcher.getInstance().dispatchEvent() will be call twice in this scenario. I'll investigate more later.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nevin Chen [:nechen] from comment #1)
> Created attachment 8808525 [details]
> device-2016-11-08-170253.png

Yeah, this is the bug.

What icon to display is decided here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java?q=path%3AToolbarDisplayLayout&redirect_type=single#349

Can you take a look whether we can see if we can identify this state? It's likely that we do not even have an icon for this.

Comment 4

[Nevin Chen(Not active on Bugzilla)]

The icon display logic 
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java#398

is defined by siteIdentity 
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java#350

which is passed in from
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/Tabs.java#477


The pass-in var "message" from my test self-signed https website is like below
{"type":"Content:SecurityChange","tabID":1,"identity":{"origin":"https:\/\/10.247.37.73:4443","mode":{"identity":"identified","mixed_display":"unknown","mixed_active":"unknown","tracking":"unknown"},"secure":true,"host":"10.247.37.73","verifier":"You have added a security exception for this site"}}

and from yahoo.com is like
{"type":"Content:SecurityChange","tabID":1,"identity":{"origin":"https:\/\/tw.mobi.yahoo.com","mode":{"identity":"identified","mixed_display":"unknown","mixed_active":"unknown","tracking":"unknown"},"secure":true,"host":"tw.mobi.yahoo.com","verifier":"Verified by: Symantec Corporation"}}

The only difference is "verifier" , which is plain text and not suitable for business logic.

Is there any way I can tell if current url is in Permanent/Temporary exception list?

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

The message is sent from browser.js here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4297

And the identify part is constructed here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5817

With Web IDE connected to the device you can debug this code and look at the values in real time.

For comparison, this is the same code path for the desktop browser, maybe there are helpful hints in there:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#4568

Comment 6

[Nevin Chen(Not active on Bugzilla)]

I need to find a way for Android to know the site identity(security) level and if it's in the exception list.
Then show the correct icon there.
I know in js there's a way to sense that if the host is in SSLExcetion
https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#5913

But I still looking for a way for Android to call overrideService.hasMatchingOverride(.....)
Maybe I can use JavascriptInterface in Android and call js method using XPCom?
https://developer.mozilla.org/en-US/Add-ons/Code_snippets/JS_XPCOM

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Oh...JavascriptInterface is for Javascript to call Android. Maybe I should use add more info result  in #5916? so I can get more info from Gecko?

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Can't you do this call on JS side and add everything you need (Is a boolean enough?) to the 'result' object? Then it will be part of the "Content:SecurityChange" message send from JS to Java and you can extract this information in updateIdentityData() on Java side.

Comment 9

[Nevin Chen(Not active on Bugzilla)]

I've made the fix from js and Java. But I'm waiting for the correct icon to display.

Hi Anthony. Please find the screenshot in the attachment. In desktop version, url with user verified (e.g. security exception) urls will have a icon with a grey lock and yellow warning sign.... but I can't find this icon in Android. Can you help me with this icon? Thank you!

Comment 10

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

(In reply to Nevin Chen [:nechen] from comment #9)
> I've made the fix from js and Java. But I'm waiting for the correct icon to
> display.
> 
> Hi Anthony. Please find the screenshot in the attachment. In desktop
> version, url with user verified (e.g. security exception) urls will have a
> icon with a grey lock and yellow warning sign.... but I can't find this icon
> in Android. Can you help me with this icon? Thank you!

On Mobile, we don't use the same icons. In this instance, we would use just the grey lock icon. While the yellow caution sign would be displayed inside the doorhanger.

Comment 11

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

Attachment: Screen Shot 2016-11-15 at 4.41.47 PM.png

Spoke in person. I was wrong ^.

This should be what's displayed.

Comment 12

[Nevin Chen(Not active on Bugzilla)]

Per discussion with Anthony and Sebastian, URLs added as exception by users are treat as unsafe websites.

Items after diffing current version and attachment 8810750 [details]
1. Image button before URL text should be grey triangle warning icon.
2. Favicon before Image button is not needed. (Current app version is correct.)
3. Lock icon and yellow warning sign is missing in current app version

Hi Sebastian
Do you think I should created another bug for the Javascript part (item 3)?

Comment 13

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Nevin Chen [:nechen] from comment #12)
> Do you think I should created another bug for the Javascript part (item 3)?

Yeah, that's fine. However we should work in this next to avoid having inconsistent icons in url bar and doorhanger. If you file a new bug then set the "blocks" field to the ID of this bug so that they are linked.

Comment 14

[Nevin Chen(Not active on Bugzilla)]

Attachment: device-2016-11-21-162922.png

I've fixed user added exception urls as the attachment.
Patch will be sent out later.

Comment 15

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1314563 - Add info for Java to know when the url is in the exception list and show the correct icon

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

Comment 16

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1314563 - Change state icon and message when url icon is clicked

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

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8812689 [details]
Bug 1314563 - Add info for Java to know when the url is in the exception list and show the correct icon

https://reviewboard.mozilla.org/r/94350/#review94476

::: mobile/android/base/java/org/mozilla/gecko/SiteIdentity.java:127
(Diff revision 1)
>          public String toString() {
>              return mId;
>          }
>      }
>  
> +

nit: Added an empty line

::: mobile/android/base/java/org/mozilla/gecko/SiteIdentity.java:194
(Diff revision 1)
>                  mOwner = identityData.optString("owner", null);
>                  mSupplemental = identityData.optString("supplemental", null);
>                  mCountry = identityData.optString("country", null);
>                  mVerifier = identityData.optString("verifier", null);
>                  mSecure = identityData.optBoolean("secure", false);
> +                mSecurityException = identityData.getBoolean("securityException");

Maybe let's follow the pattern of the other fields an use optBoolean("securityException", false);

::: mobile/android/base/java/org/mozilla/gecko/SiteIdentity.java:252
(Diff revision 1)
> +    public boolean getSecurityException() {
> +        return mSecurityException;
> +    }

isSecurityException() might be easier to read and follows the naming of isSecure().

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java:479
(Diff revision 1)
>  
>      // GeckoEventListener implementation
>      @Override
>      public void handleMessage(String event, JSONObject message) {
>          Log.d(LOGTAG, "handleMessage: " + event);
> +

nit: This empty line change seems to be unrelated and is the only change in this file.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:358
(Diff revision 1)
>  
>          final SecurityMode securityMode;
>          final MixedMode activeMixedMode;
>          final MixedMode displayMixedMode;
>          final TrackingMode trackingMode;
> +        boolean securityException = false;

If we set this boolean only in the if-else block then we can mark it as final here.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java
(Diff revision 1)
>          // (We then continue and process the tracking / mixed content icons as usual, even for about: pages, as they
>          //  can still load external sites.)
>          if (securityMode == SecurityMode.CHROMEUI) {
>              imageLevel = LEVEL_DEFAULT_GLOBE; // == SecurityMode.UNKNOWN.ordinal()
>          }
> -

nit: Line removed?

::: mobile/android/base/java/org/mozilla/gecko/toolbar/ToolbarDisplayLayout.java:388
(Diff revision 1)
> +        } else if (securityException){
> +            imageLevel = LEVEL_WARNING_MINOR    ;

style: space before the '{' and there are a bunch of spaces before the ';' right now :)

::: mobile/android/chrome/content/browser.js:5881
(Diff revision 1)
>      if (this._lastLocation.hostname &&
>          this._overrideService.hasMatchingOverride(this._lastLocation.hostname,
>                                                    (this._lastLocation.port || 443),
>                                                    iData.cert, {}, {}))
>        result.verifier = Strings.browser.GetStringFromName("identity.identified.verified_by_you");
> +      result.securityException = true;

Looks like we set it always to true?!

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8812690 [details]
Bug 1314563 - Change state icon and  message when url icon is clicked

https://reviewboard.mozilla.org/r/94352/#review94478

::: mobile/android/chrome/content/browser.js:5879
(Diff revision 1)
> -                                                  iData.cert, {}, {}))
> +                                                  iData.cert, {}, {})){
>        result.verifier = Strings.browser.GetStringFromName("identity.identified.verified_by_you");
>        result.securityException = true;
> -
> +    }

Ah, here the issue is fixed. Maybe move this change to the first commit.

nit/style: Space before the '{'

Comment 19

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8812689 [details]
Bug 1314563 - Add info for Java to know when the url is in the exception list and show the correct icon

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

Comment 20

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8812690 [details]
Bug 1314563 - Change state icon and  message when url icon is clicked

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

Comment 21

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8812689 [details]
Bug 1314563 - Add info for Java to know when the url is in the exception list and show the correct icon

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

Comment 22

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8812690 [details]
Bug 1314563 - Change state icon and  message when url icon is clicked

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

Comment 23

[Sebastian Kaspari (:sebastian; :pocmo)]

Please mark opened "issues" on reviewboard as fixed after pushing an update :)

Comment 24

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8812689 [details]
Bug 1314563 - Add info for Java to know when the url is in the exception list and show the correct icon

https://reviewboard.mozilla.org/r/94350/#review94560

Nice!

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8812690 [details]
Bug 1314563 - Change state icon and  message when url icon is clicked

https://reviewboard.mozilla.org/r/94352/#review94562

::: mobile/android/base/java/org/mozilla/gecko/SiteIdentity.java:127
(Diff revision 3)
>          public String toString() {
>              return mId;
>          }
>      }
>  
> +

nit: There's only a whiteline change in this file.

Comment 26

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8812690 [details]
Bug 1314563 - Change state icon and  message when url icon is clicked

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94352/diff/3-4/

Comment 27

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8812690 [details]
Bug 1314563 - Change state icon and  message when url icon is clicked

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/94352/diff/4-5/

Comment 28

[Nevin Chen(Not active on Bugzilla)]

Thanks Sebastian.
Looks like there's still lots for me to learn about MozReview !
I've fixed the errors. Thanks!

Comment 29

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

Comment on attachment 8812685 [details]
device-2016-11-21-162922.png

Thanks Nevin!

Comment 30

[Sebastian Kaspari (:sebastian; :pocmo)]

Looks like we can land this :)

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36c0196cf2ef
Add info for Java to know when the url is in the exception list and show the correct icon r=sebastian
https://hg.mozilla.org/integration/autoland/rev/dfa353352f30
Change state icon and  message when url icon is clicked r=sebastian

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/36c0196cf2ef
https://hg.mozilla.org/mozilla-central/rev/dfa353352f30

Comment 33

[Sorina Florean [:sflorean]]

Verified as fixed in build 53.0a1 (2016-12-09);
Device: HTC Desire 820 (Android 6.0.1).