Leanplum deeplinks do not open the URL in Fennec unless it is set as the default browser
Categories
(Firefox for Android Graveyard :: General, defect, P1)
Tracking
(firefox66 fixed)
Tracking | Status | |
---|---|---|
firefox66 | --- | fixed |
People
(Reporter: vlad.baicu, Assigned: vlad.baicu)
Details
Attachments
(1 file)
47 bytes,
text/x-phabricator-request
|
lizzard
:
approval-mozilla-beta+
|
Details | Review |
Lp deeplinks with an URL currently open in Fennec only if the app has been set as a default browser.
Assignee | ||
Comment 1•5 years ago
|
||
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 | ||
Comment 2•5 years ago
|
||
Created a new "open" deeplink to open pages directly in Fennec.
Dan does this need a security review?
Comment 4•5 years ago
|
||
(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.
Assignee | ||
Comment 5•5 years ago
|
||
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.
Comment 6•5 years ago
|
||
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.
Daniel you said "Both are pretty bad so maybe it's not much worse." - so does that mean we can go ahead with this?
Hi Susheel,
Can you please provide more information around timing? When do you think this can get reviewed?
Thanks you,
Miray
Comment 9•5 years ago
|
||
(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.
Assignee | ||
Comment 10•5 years ago
|
||
Thank you Daniel, I have filed a separate bug for the security review - bug 1521036.
Assignee | ||
Updated•5 years ago
|
Comment 11•5 years ago
|
||
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
Comment 12•5 years ago
|
||
Pushed by opoprus@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8d878be3b4bf
Open LP URLs specifically in Fennec. r=sdaswani
Comment 13•5 years ago
|
||
bugherder |
Comment 14•5 years ago
|
||
Vlad to confirm - this will be included with 66 on March 19th?
Comment 16•5 years ago
|
||
Definitely - NI on Andrei as he worked on LP
Comment 17•5 years ago
|
||
I can confirm that this is in beta
now!
I re-tested this issue and everything worked accordingly.
Assignee | ||
Comment 18•5 years ago
|
||
Since this is in beta, it will ride the train for release. Clearing my NI
Comment 19•5 years ago
|
||
Thanks all!
Comment 20•5 years ago
|
||
Andrel can you attach a screenshot of how you tested this (i.e., how did you configure LP to make this work)?
Comment 21•5 years ago
•
|
||
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.
Comment 22•5 years ago
|
||
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?
Assignee | ||
Comment 23•5 years ago
|
||
(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.
Comment 24•5 years ago
|
||
Vlad where is the other patch that is riding Nightly now?
Assignee | ||
Comment 25•5 years ago
|
||
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
Updated•5 years ago
|
Comment 26•5 years ago
|
||
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.
Comment 27•5 years ago
|
||
: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.
Assignee | ||
Comment 28•5 years ago
|
||
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 !
Updated•5 years ago
|
Updated•5 years ago
|
Updated•3 years ago
|
Description
•