Closed Bug 1314563 Opened 3 years ago Closed 3 years ago

Showing green lock for manually added certificate exception

Categories

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

All
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 53
Tracking Status
firefox53 --- verified

People

(Reporter: sebastian, Assigned: cnevinchen)

References

Details

(Whiteboard: [TPE-1])

Attachments

(5 files)

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.
Priority: -- → P2
Assignee: nobody → cnevinchen
checking in org.mozilla.gecko.toolbar.SiteIdentityPopup#updateConnectionState
Flags: needinfo?(s.kaspari)
Looks like in org.mozilla.gecko.GeckoAppShell#handleGeckoMessage , EventDispatcher.getInstance().dispatchEvent() will be call twice in this scenario. I'll investigate more later.
(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.
Flags: needinfo?(s.kaspari)
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?
Flags: needinfo?(s.kaspari)
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
Flags: needinfo?(s.kaspari)
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
Flags: needinfo?(s.kaspari)
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?
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.
Flags: needinfo?(s.kaspari)
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!
Flags: needinfo?(alam)
(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.
Flags: needinfo?(alam)
Spoke in person. I was wrong ^.

This should be what's displayed.
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)?
Flags: needinfo?(s.kaspari)
(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.
Flags: needinfo?(s.kaspari)
I've fixed user added exception urls as the attachment.
Patch will be sent out later.
Attachment #8812685 - Flags: review?(alam)
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?!
Attachment #8812689 - Flags: review?(s.kaspari)
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 '{'
Attachment #8812690 - Flags: review?(s.kaspari) → review+
Please mark opened "issues" on reviewboard as fixed after pushing an update :)
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!
Attachment #8812689 - Flags: review?(s.kaspari) → review+
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.
Thanks Sebastian.
Looks like there's still lots for me to learn about MozReview !
I've fixed the errors. Thanks!
Flags: needinfo?(s.kaspari)
Comment on attachment 8812685 [details]
device-2016-11-21-162922.png

Thanks Nevin!
Attachment #8812685 - Flags: review?(alam) → review+
Looks like we can land this :)
Flags: needinfo?(s.kaspari)
Pushed by cbook@mozilla.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/36c0196cf2ef
https://hg.mozilla.org/mozilla-central/rev/dfa353352f30
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Verified as fixed in build 53.0a1 (2016-12-09);
Device: HTC Desire 820 (Android 6.0.1).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.