Closed Bug 1515651 Opened 10 months ago Closed 9 months ago

Leanplum deeplinks do not open the URL in Fennec unless it is set as the default browser

Categories

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

defect

Tracking

()

RESOLVED FIXED
Firefox 66
Tracking Status
firefox66 --- fixed

People

(Reporter: vlad.baicu, Assigned: vlad.baicu)

Details

Attachments

(1 file)

Lp deeplinks with an URL currently open in Fennec only if the app has been set as a default browser.
Should we provide a new way to open the URLs in Fennec or have them all open in our app ? Are there any cases where we want LP deeplinks sent to our app with an url to be opened by other browsers ?
Assignee: nobody → vlad.baicu
Flags: needinfo?(sdaswani)
Created a new "open" deeplink to open pages directly in Fennec.
Dan does this need a security review?
Flags: needinfo?(sdaswani) → needinfo?(dveditz)

(In reply to :sdaswani only needinfo from comment #3)

Dan does this need a security review?

The entire "firefox:" pseudo-protocol scheme needs a review if it hasn't gotten one; several of the commands seemed potentially dangerous. For this new comment specifically it's not much riskier than other apps launching Firefox for Android when it is the default. From the code I assume it also suffers from bug 1357377, but that should be handled at some common choke point anyway.

This does introduce a new risk to some users: if a Firefox for Android 0-day is discovered an attack can be launched from other apps (think poor quality ad networks) even against users who use other browsers but have installed Firefox for experimentation/testing in the past. The stop-gap advice--which would come from 3rd parties!--would have to be "completely uninstall Firefox" rather than "don't use Firefox until we get a fix pushed". Both are pretty bad so maybe it's not much worse.

Flags: needinfo?(dveditz)

Hello Daniel, security wise, we are currently checking the uid of the intent if it matches the one we stored locally before doing anything - https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java#162.

Should we improve something in this area ? And if so, any ideas would be welcome.

Flags: needinfo?(dveditz)

I think your link is broken, or at least I didn't understand what you were pointing at, but I found what you are talking about elsewhere in the file. That appears to be a reasonable mitigation, yes.

Flags: needinfo?(dveditz)

Daniel you said "Both are pretty bad so maybe it's not much worse." - so does that mean we can go ahead with this?

Flags: needinfo?(dveditz)

Hi Susheel,

Can you please provide more information around timing? When do you think this can get reviewed?
Thanks you,

Miray

(In reply to :sdaswani only needinfo from comment #7)

Daniel you said "Both are pretty bad so maybe it's not much worse." - so does that mean we can go ahead with this?

The mitigation Vlad described in comment 5 prevents external abuse of the firefox: scheme, therefore he deep links this adds are no riskier than any other in-browser magic firefox: command. You can go ahead with adding this functionality.

I'm still interested in the security of these firefox: scheme links overall and think they should get a security review if they haven't already. I haven't been able to make firefox:// scheme links work from in-content pages so maybe there's additional mitigations against using these that weren't mentioned (that would be good!). Maybe that was part of a previous security review of Leanplum and the actions it can take.

Flags: needinfo?(dveditz) → needinfo?(sdaswani)

Thank you Daniel, I have filed a separate bug for the security review - bug 1521036.

Keywords: checkin-needed

Daniel, I think the firefox:// scheme links were reviewed as part of the original deep links integration here? https://bugzilla.mozilla.org/show_bug.cgi?id=1380950

Flags: needinfo?(sdaswani)

Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d878be3b4bf
Open LP URLs specifically in Fennec. r=sdaswani

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Vlad to confirm - this will be included with 66 on March 19th?

Flags: needinfo?(vlad.baicu)

Also can QA confirm this is in beta now?

Flags: needinfo?(ioana.chiorean)

Definitely - NI on Andrei as he worked on LP

Flags: needinfo?(ioana.chiorean) → needinfo?(andrei.bodea)

I can confirm that this is in beta now!
I re-tested this issue and everything worked accordingly.

Flags: needinfo?(andrei.bodea)

Since this is in beta, it will ride the train for release. Clearing my NI

Flags: needinfo?(vlad.baicu)

Thanks all!

Andrel can you attach a screenshot of how you tested this (i.e., how did you configure LP to make this work)?

Flags: needinfo?(andrei.bodea)

Hello, here is the video demonstration when opening the Push Notification and a screenshot with the configuration in LP.
It was set on the Push Notification with the action set to Open action -> Open URL.
Note that all from the above were tested while no browser was set as default, it was set to "None"
When testing with chrome set as the default browser and using the scheme provided by this patch the results are good, the Push Notification was opened every time in firefox.
Please note that the patch hasn't reached beta yet and is available only in Nightly.

Flags: needinfo?(andrei.bodea)

Vlad - something is wrong here. If you look at Andrei's configuration, he is just sending a push notification to FF beta, that is opening the user's default browser. Note in the url action he doesn't even have action: firefox://open?url=https://www.espn.com&uid={{User ID}}

Can you get together with him ASAP and get the testing configuration correct?

Flags: needinfo?(vlad.baicu)

(In reply to Andrei Bodea[:andrei]🦊 from comment #21)

When testing with chrome set as the default browser and using the scheme provided by this patch the results are good, the Push Notification was opened every time in firefox.
Please note that the patch hasn't reached beta yet and is available only in Nightly.

We've already gone over this together today, Andrei tested multiple scenarios on both Beta and Nightly. If the default browser app was set to "none" than the action would open in Fennec, therefore the initial conclusion on beta. Since the patch hasn't reached beta yet and it is currently riding on Nightly 67, I'm thinking we might have to uplift it in order for it to be included in 66.

Flags: needinfo?(vlad.baicu)

Vlad where is the other patch that is riding Nightly now?

Flags: needinfo?(vlad.baicu)

Comment on attachment 9032763 [details]
Bug 1515651 - Open LP URLs specifically in Fennec. r=sdaswani

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

LP url deeplinks without this action will open another browser if it is set as default.

Is this code covered by automated tests?

No

Has the fix been verified in Nightly?

Yes

Needs manual test from QE?

Yes

If yes, steps to reproduce

Send a push notification with the following action: firefox://open?url=https://www.espn.com&uid={{User ID}}

List of other uplifts needed

None

Risk to taking this patch

Low

Why is the change risky/not risky? (and alternatives if risky)

The patch adds a new LP action without changing anything else.

String changes made/needed

Flags: needinfo?(vlad.baicu)
Attachment #9032763 - Flags: approval-mozilla-beta?

Comment on attachment 9032763 [details]
Bug 1515651 - Open LP URLs specifically in Fennec. r=sdaswani

Additional patch verified in nightly, let's uplift for beta 7.

Attachment #9032763 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

:Vlad can you please let us know (specifically) what is needed to be uplifted? The phabricator revision that got approval-beta+ is identical to the patch that landed in comment 12, which is already part of firefox 66.

Flags: needinfo?(vlad.baicu)

Hi Cristian, we weren't sure it would make it into beta for 66 so we submitted an uplift request - at the time we tested it, the patch wasn't available on that build. If it's set to land in 66, please feel free to ignore our request, thanks !

Flags: needinfo?(vlad.baicu)
Priority: -- → P1
You need to log in before you can comment on or make changes to this bug.