Closed Bug 1419245 Opened 2 years ago Closed 2 years ago

A2HS badge is shown on site with broken HTTPS

Categories

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

Firefox 59
defect

Tracking

()

VERIFIED FIXED
Firefox 59
Tracking Status
firefox58 --- verified
firefox59 --- verified

People

(Reporter: abovens, Assigned: cnevinchen)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [pwa-front-end][FNC][SPT59.2][MVP])

Attachments

(2 files, 1 obsolete file)

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.
Whiteboard: [pwa-front-end]
good P1 candidate for 58 Beta cycle.
Priority: -- → P1
The pages in comment 0 is safe now.
we should find another sample website.
Assignee: nobody → cnevinchen
Whiteboard: [pwa-front-end] → [pwa-front-end][FNC][SPT59.2][MVP]
Please use this temp website to test.
https://s3-us-west-2.amazonaws.com/pwa-nevin/index.html
Flags: needinfo?(oana.horvath)
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.
Attachment #8931270 - Flags: review?(walkingice0204) → review+
Attachment #8934077 - Attachment is obsolete: true
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)
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4a009e20e912
A2HS badge is shown on site with broken HTTPS. r=walkingice
https://hg.mozilla.org/mozilla-central/rev/4a009e20e912
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 58 → Firefox 59
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?
Flags: needinfo?(oana.horvath) → needinfo?(cnevinchen)
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.
Flags: needinfo?(cnevinchen) → needinfo?(oana.horvath)
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.
Flags: needinfo?(oana.horvath)
QA Contact: oana.horvath
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
Attachment #8931270 - Flags: approval-mozilla-beta?
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".
Flags: needinfo?(cnevinchen)
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
Flags: needinfo?(cnevinchen) → needinfo?(oana.horvath)
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
Flags: needinfo?(oana.horvath)
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.
Keywords: checkin-needed
Attachment #8931270 - Flags: approval-mozilla-beta?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
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)
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
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
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
Attachment #8931270 - Flags: approval-mozilla-beta?
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+.
Attachment #8931270 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified the fix on Beta 58.0b15.
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.