1419245 - A2HS badge is shown on site with broken HTTPS

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Web Apps (PWAs), defect, P1)

Description

[Andreas Bovens [:abovens]]

Attachment: Screenshot_20171118-162645.jpg

https://m.gsmarena.com/ gets A2HS badge although HTTPS warning is shown in the address bar.

If HTTPS is broken, no badge should be shown.

Comment 1

[Wesly Huang (EPM)]

good P1 candidate for 58 Beta cycle.

Comment 2

[Nevin Chen(Not active on Bugzilla)]

The pages in comment 0 is safe now.
we should find another sample website.

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

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

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Please use this temp website to test.
https://s3-us-west-2.amazonaws.com/pwa-nevin/index.html

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8931270 [details]
Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

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

Comment 6

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8931270 [details]
Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

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

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8931270 [details]
Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

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

Comment 8

[Julian Chu [:walkingice]]

Comment on attachment 8931270 [details]
Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

https://reviewboard.mozilla.org/r/202418/#review210446

::: mobile/android/base/java/org/mozilla/gecko/pwa/PwaUtils.java:11
(Diff revision 4)
> +
> +import org.mozilla.gecko.Tab;
> +
> +
> +public class PwaUtils {
> +    public static boolean safeForPwa(Tab tab) {

Maybe we could have another naming for this method so people could understand its intention. As far as I know, we want to verify whether a Web-page could be added as a PWA shortcut or not.

The below comment could also rewrite as Javadoc for this function.

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Attachment: Add multidex

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

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8931270 [details]
Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

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

Comment 11

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 926d90481781 -d c09b561dd202: rebasing 437173:926d90481781 "Bug 1419245 - A2HS badge is shown on site with broken HTTPS. r=walkingice" (tip)
merging mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
merging mobile/android/base/java/org/mozilla/gecko/Tab.java
merging mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java
merging mobile/android/base/moz.build
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 12

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8931270 [details]
Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4a009e20e912
A2HS badge is shown on site with broken HTTPS. r=walkingice

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/4a009e20e912

Comment 15

[Oana Horvath [:ohorvath]]

I've verified the test app, with the following STR:
1. Add the page to the homescreen as a PWA. Close the PWA.
2. Set the device date to 2020.
3. Launch the PWA from the homescreen.

Results:
#1. Opening the PWA after the certificate expires, a blank PWA is opened in the background and an unsecure connection error page is opened in Nightly. This will redirect to the test page if you allow it.
*Chrome only opens the error page in the browser without launching the web app.

#2. The badge is still displayed on the test page if you add a security exception. - this I think would be expected?
*Can't verify if it's displayed on a working http page, without adding the exception. If I understand correctly, this was the actual fix you did, but I can't verify it if the  page will give me the "untrusted connection" error page. Do you have any ideas here?

Comment 16

[Nevin Chen(Not active on Bugzilla)]

Hi Oana
For your STR: the shortcut already added, and then become insecure, I don't have a fix about it. I think snorp will help.


About my fix, my test step:
1. Set the device date to 2020.
2. Go to the test page
3. Add to exception
4. The badge shouldn't apear.


But I don't think the code is on Nightly yet.
I can't find my change on m-c yet.

Comment 17

[Oana Horvath [:ohorvath]]

Ok, I've verified that the badge is no longer displayed on insecure pages, on Nightly 59 (2017-12-06).
Samsung Galaxy Note 4 (Android 5.0.1)
Google Pixel (Android 8.0)

And I've created Bug 1423587 for the remaining issue, described in Comment 15.

Comment 18

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8931270 [details]
Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

Approval Request Comment
[Feature/Bug causing the regression]: We show PWA badge for sites that have manifest.json, but didn't check if the connection is secure or not
[User impact if declined]: Users may add a PWA shortcut via a non-https website. Or a website with an expired certificate.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: no
[Is the change risky?]: no
[Why is the change risky/not risky?]: We add one more check so it's a simple change
[String changes made/needed]: no

Comment 19

[Julien Cristau [:jcristau]]

I'm confused here.  The site from comment 0 has some mixed content (fonts and images), but that's not "non-https" or "expired certificate".

Comment 20

[Nevin Chen(Not active on Bugzilla)]

Hi Juliuen 
Thanks for point this out!
Since mixed content, non-https, expired certificate all uses "SiteIdentity.isSecure()" to determine which site identity icon to show[1]. So my patch here is able to fix this issue.


Hi Oana
I've created another test page for mixed content[2]. Could you please help verify in Nightly? ( no need to land new code)
Thanks!

[1] https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/mobile/android/base/java/org/mozilla/gecko/toolbar/SiteIdentityPopup.java#322

[2] https://googlesamples.github.io/web-fundamentals/fundamentals/security/prevent-mixed-content/active-mixed-content.html

Comment 21

[Nevin Chen(Not active on Bugzilla)]

> [2]
> https://googlesamples.github.io/web-fundamentals/fundamentals/security/
> prevent-mixed-content/active-mixed-content.html

Oops..sorry.
[2] should be this:

https://s3-us-west-2.amazonaws.com/pwa-nevin/mix.htmlhttps://s3-us-west-2.amazonaws.com/pwa-nevin/mix.htmlhttps://s3-us-west-2.amazonaws.com/pwa-nevin/mix.html

Comment 22

[Oana Horvath [:ohorvath]]

Hi, I've verified your mixed content test page and looks good, the badge doesn't appear.
But, I've noticed that m.aliexpress.com today had the Insecure content warning so I checked and the badge appears on this site and it doesn't do anything when you tap the add to home screen button.
Please have a look at this screen recording: https://drive.google.com/open?id=16vWSaEG-OtV0pK1uSTYGzgaR2h7_xbMu

Comment 23

[Nevin Chen(Not active on Bugzilla)]

Nice catch!
For m.aliexpress.com, the site itentity changes after the badge apears. So the badge need to be updated too.

I'll cancel the uplift request. 

Please help backout my patch in Nightly. I'll submit another patch.

Comment 24

[Nevin Chen(Not active on Bugzilla)]

After consulting cosmin_sabou, since this bug is landed, I'll land the extra fix in bug 1424179.

After bug 1424179 is fixed, I'll also want to uplift this bug and bug 1424179.

see https://bugzilla.mozilla.org/show_bug.cgi?id=1424179#c0

Comment 25

[Mozilla Autoland]

We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s d891304d2e48 -d 1ee72d1a9430: rebasing 438319:d891304d2e48 "Bug 1419245 - A2HS badge is shown on site with broken HTTPS. r=walkingice" (tip)
merging mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java
merging mobile/android/base/java/org/mozilla/gecko/Tab.java
merging mobile/android/base/moz.build
warning: conflicts while merging mobile/android/base/java/org/mozilla/gecko/Tab.java! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 26

[Oana Horvath [:ohorvath]]

Since the patch on this bug was already fixed and verified I'm marking this bug back as Resolved fixed. Follow up bug: Bug 1424179

Comment 27

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8931270 [details]
Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Approval Request Comment
[Feature/Bug causing the regression]: We had a new feature to let users add PWA websites to users' mobile launcher. We want to add some restrictions here.
[User impact if declined]: Users will be able to add PWA icons with unsafe sites.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: It's done.
[List of other uplifts needed for the feature/fix]:
Bug 1419245 fixed:
If a website is PWA-ready (has manifest) but its certificated is expired, we should hide the PWA badge.
Bug 1424179 fixed:
If a website is PWA-ready (has manifest) but has mixed content, we should hide PWA badge.

[Is the change risky?]: I think it's not risky. 
[Why is the change risky/not risky?]: The worst case is we'll be too strict to the new feature. But this won't break other current features.
[String changes made/needed]: no

Comment 28

[Gerry Chang [:gchang]]

Comment on attachment 8931270 [details]
Bug 1419245 - A2HS badge is shown on site with broken HTTPS.

We start to support PWA on 58. Take this to hide the PWA badge when the website's certificate is expired. Beta58+.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/55b95085ee3b

Comment 30

[Oana Horvath [:ohorvath]]

Verified the fix on Beta 58.0b15.
Device: HTC Desire 820 (Android 6.0.1)