Closed Bug 1424179 Opened 2 years ago Closed 2 years ago

A2HS badge is shown on site with broken HTTPS (Follow Up)

Categories

(Firefox for Android :: Toolbar, defect, P1)

56 Branch
defect

Tracking

()

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

People

(Reporter: cnevinchen, Assigned: cnevinchen)

Details

(Whiteboard: [FNC][SPT59.3][INT])

Attachments

(1 file)

per https://bugzilla.mozilla.org/show_bug.cgi?id=1419245#c22
"
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

"

After consulting cosmin_sabou, since bug 1419245 is landed, I'll land the extra fix here.

After this bug is fixed, I'll also want to uplift this bug and bug 1419245
Priority: -- → P1
Target Milestone: --- → Firefox 58
Comment on attachment 8935676 [details]
Bug 1424179 - Hide PWA badge when site identity updates to mix content.

https://reviewboard.mozilla.org/r/206574/#review213636
Attachment #8935676 - Flags: review?(walkingice0204) → review+
Pushed by nechen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c97612e2aa6e
Hide PWA badge when site identity updates to mix content. r=walkingice
https://hg.mozilla.org/mozilla-central/rev/c97612e2aa6e
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
HI Oana
Could you please help if this is fixed on Night?
I'll like to request beta uplift if it pases.
see https://bugzilla.mozilla.org/show_bug.cgi?id=1419245#c23
Flags: needinfo?(oana.horvath)
I'll request for uplift if it pass QA
Flags: needinfo?(jcristau)
I'm not sure why I'm being needinfo'ed here, but I still don't understand what the intent in this and 1419245 is, and the state of these bugs looks very much confused.
Flags: needinfo?(jcristau)
Target Milestone: Firefox 58 → Firefox 59
Hi Nevin,
Unfortunately, I can't get to the same behavior on aliexpress or other sites to check the fix. I think the solution would be to have again a test page that covers mixed & insecure content. Also see https://bugzilla.mozilla.org/show_bug.cgi?id=1423587#c1 to make sure we are covered.
Thank you!
Flags: needinfo?(oana.horvath)
I've finally managed to verify the fix today, on aliexpress.com again. So the changes are verified on Nightly 59 (2017-12-20).
Comment on attachment 8935676 [details]
Bug 1424179 - Hide PWA badge when site identity updates to mix content.

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 #8935676 - Flags: approval-mozilla-beta?
Whiteboard: [FNC][SPT59.3][INT]
Hi Andreas

There are "Site identity" related scenarios needs to be considered:
1. Before the user adds PWA website to shortcut:
  A. The website doesn't have a valid certificate (e.g. expired or non-https)
  B. The website has mixed content

2. After the user adds PWA website to shortcut, he clicks the PWA shortcut but:
  C. The target url doesn't have a valid certificate (e.g. expired)
  D. The target url has mixed content

Bug 1419245 fixed scenario A
Bug 1424179 fixed scenario B
Bug 1423587 is for C & D, but I think they should be handled by GeckoView (e.g. display a prompt, or fallback to use the main browser app)

As far as I know, Chrome still let you add PWA to shortcut in scenario A,B (tested with https://s3-us-west-2.amazonaws.com/pwa-nevin/mix.html). My question is
1. Do you want to align Chrome's behavior? (That way I'll need to backout Bug 1419245 and Bug 1424179 in Nightly.
or
2. Do you want the fix for A & B (Bug 1419245, Bug 1424179) landed in 58 or 59?
Flags: needinfo?(abovens)
Whiteboard: [FNC][SPT59.3][INT]
Whiteboard: [FNC][SPT59.3][INT]
Answers to your questions:
1. I think we're doing the right thing here, so it's fine to not align with Chrome's behavior.

2. I'd like to have it in 58.

@snorp, as for bug 1423587 (C & D), when do you think this can be fixed? Will we make it for 58?
Flags: needinfo?(abovens) → needinfo?(snorp)
Comment on attachment 8935676 [details]
Bug 1424179 - Hide PWA badge when site identity updates to mix content.

We start to support PWA on 58. Take this to hide the PWA badge when the website has mixed content. Beta58+.
Attachment #8935676 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Andreas Bovens [:abovens] from comment #12)
I commented in bug 1423587, but I think GeckoView (via GeckoSession) already exposes enough information for the frontend folks to fix this.
Flags: needinfo?(snorp)
(In reply to Nevin Chen [:nechen] from comment #11)
> Bug 1423587 is for C & D, but I think they should be handled by GeckoView
> (e.g. display a prompt, or fallback to use the main browser app)

GeckoView in general does not display prompts, but delegates these tasks to the app. In the same vein, it does not launch other apps for fallback scenarios, etc.
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.