Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2873843002:
Support autopresenting WebVr content. (Closed)
DescriptionSupport autopresenting WebVr content.
We support autopresenting if the intent
- launches a ChromeTabbedActivity
- has a VR specific extra
- originates from a first-party app
There's no perfect way to know the sender of an intent. We use PendingIntents
since it has a getCreatorPackage function that cannot be spoofed. That is, a
trusted sender adds a PendingIntent as an extra which tags the intent with
the sender credentials. The obvious risk here is that if something in between
can get a hold of the PendingIntent, it can pretend to be the send indefinitely.
That means that we must really trust the sender.
This CL takes over bshe@'s https://codereview.chromium.org/2829193003/
Follow-up work will be to respond to this intent more elegantly so that we can
get rid of the browser UI that will show up before we start autopresenting (see
TODO).
BUG=713369
TBR=dominickn@chromium.org
Review-Url: https://codereview.chromium.org/2873843002
Cr-Commit-Position: refs/heads/master@{#473303}
Committed: https://chromium.googlesource.com/chromium/src/+/e78a83bef3d557cd2f375caa189e7fe256b7b1ba
Patch Set 1 #
Total comments: 15
Patch Set 2 : Address review comments #
Total comments: 4
Patch Set 3 : address more review comments #
Total comments: 3
Patch Set 4 : move VrShellDelegate.onNewIntent call to CTA #
Total comments: 2
Patch Set 5 : tedchoc nit #Patch Set 6 : nit #Patch Set 7 : guard autopresentation behind a flag #Patch Set 8 : add inVr assert #Patch Set 9 : rebase #Messages
Total messages: 45 (22 generated)
Description was changed from ========== Support autopresenting WebVr content. The primary usecase for now is being able to deep-link content from Daydream home. We support autopresenting if the intent - launches CTA - has a VR specific extra - originates is from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds an a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That is, we must really trust the sender. This CL is fine because Daydream home is a first-party app and uses the DaydreamAPI to send the intent which is also google3 code that we can trust. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 ========== to ========== Support autopresenting WebVr content. The primary usecase for now is being able to deep-link content from Daydream home. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL is fine because Daydream home is a first-party app and uses the DaydreamAPI to send the intent which is also google3 code that we can trust. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 ==========
ymalik@chromium.org changed reviewers: + mthiesse@chromium.org
Mike, PTAL :)
ymalik@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc, please sign-off on the first-party verification part as we talked offline.
Description was changed from ========== Support autopresenting WebVr content. The primary usecase for now is being able to deep-link content from Daydream home. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL is fine because Daydream home is a first-party app and uses the DaydreamAPI to send the intent which is also google3 code that we can trust. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 ========== to ========== Support autopresenting WebVr content. The primary usecase for now is being able to deep-link content from Daydream home. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 ==========
https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:650: public boolean canAutopresentVr() { I don't know what Ted's opinion on this is, but so far we've been avoiding making these activities aware of VR where possible. See VrShellDelegate#activitySupportsVrBrowsing for how I've been doing similar things so far. You could add a VrShellDelegate#activitySupportsWebVrAutopresent. https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:804: VrShellDelegate.onNewIntent(this, getIntent()); What happens when you use the task switcher to switch away from chrome, then back to chrome? Won't this read the same intent and try to autopresent again when you resume Chrome? https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:134: private boolean mAutopresentWebVr = false; To be on the safe side, you probably also want to reset this on top-level page navs? https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:523: if (mVrDaydreamApi == null || mActivity == null) return false; mActivity should never be null. If you're worried, add an assert? As long as mVrSupportLevel > VR_NOT_AVAILABLE, mVrDaydreamApi will also not be null. So maybe just assert mVrSupportLevel != VR_NOT_AVAILABLE as well. https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:543: Log.i(TAG, String.format("received intent to autopresent WebVr")); No need for String.format. Probably no need for this logging at all? For binary size reasons we should probably only log errors.
https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:650: public boolean canAutopresentVr() { On 2017/05/10 14:52:14, mthiesse wrote: > I don't know what Ted's opinion on this is, but so far we've been avoiding > making these activities aware of VR where possible. > > See VrShellDelegate#activitySupportsVrBrowsing for how I've been doing similar > things so far. You could add a VrShellDelegate#activitySupportsWebVrAutopresent. Adding this to VrShellDelegate sgtm and seems consistent. https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:804: VrShellDelegate.onNewIntent(this, getIntent()); On 2017/05/10 14:52:14, mthiesse wrote: > What happens when you use the task switcher to switch away from chrome, then > back to chrome? Won't this read the same intent and try to autopresent again > when you resume Chrome? Yes it would, why not have this in onNewIntentWithNative? The gotcha is that isn't called for the initial run, so you would need to add any additional logic in this "general" area. https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:134: private boolean mAutopresentWebVr = false; On 2017/05/10 14:52:14, mthiesse wrote: > To be on the safe side, you probably also want to reset this on top-level page > navs? false/null/0 are defaults in java and including them actually slightly increases binary size. So drop = false here. https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:538: public static void onNewIntent(ChromeActivity activity, Intent intent) { you should probably return a boolean here to give a signal it was consumed and we shouldn't continue processing it? Or would we expect this to include a URL as well that we would then enter VR for?
mthiesse@, tedchoc@ PTAL :) https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:650: public boolean canAutopresentVr() { On 2017/05/11 00:05:03, Ted C wrote: > On 2017/05/10 14:52:14, mthiesse wrote: > > I don't know what Ted's opinion on this is, but so far we've been avoiding > > making these activities aware of VR where possible. > > > > See VrShellDelegate#activitySupportsVrBrowsing for how I've been doing similar > > things so far. You could add a > VrShellDelegate#activitySupportsWebVrAutopresent. > > Adding this to VrShellDelegate sgtm and seems consistent. Done. https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:804: VrShellDelegate.onNewIntent(this, getIntent()); On 2017/05/11 00:05:03, Ted C wrote: > On 2017/05/10 14:52:14, mthiesse wrote: > > What happens when you use the task switcher to switch away from chrome, then > > back to chrome? Won't this read the same intent and try to autopresent again > > when you resume Chrome? > > Yes it would, why not have this in onNewIntentWithNative? The gotcha is that > isn't > called for the initial run, so you would need to add any additional logic in > this "general" > area. > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Thanks for the tip. Done. https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:134: private boolean mAutopresentWebVr = false; On 2017/05/11 00:05:03, Ted C wrote: > On 2017/05/10 14:52:14, mthiesse wrote: > > To be on the safe side, you probably also want to reset this on top-level page > > navs? > Are you referring to WebVr to WebVr? I thought that would happen still go through shutdownVr. > false/null/0 are defaults in java and including them actually slightly increases > binary size. So drop = false here. Done https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:523: if (mVrDaydreamApi == null || mActivity == null) return false; On 2017/05/10 14:52:14, mthiesse wrote: > mActivity should never be null. If you're worried, add an assert? > > As long as mVrSupportLevel > VR_NOT_AVAILABLE, mVrDaydreamApi will also not be > null. So maybe just assert mVrSupportLevel != VR_NOT_AVAILABLE as well. Done. https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:538: public static void onNewIntent(ChromeActivity activity, Intent intent) { On 2017/05/11 00:05:03, Ted C wrote: > you should probably return a boolean here to give a signal it was consumed and > we shouldn't continue processing it? Or would we expect this to include a URL > as well that we would then enter VR for? That's right. This intent will have the url as well. https://codereview.chromium.org/2873843002/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:543: Log.i(TAG, String.format("received intent to autopresent WebVr")); On 2017/05/10 14:52:14, mthiesse wrote: > No need for String.format. Probably no need for this logging at all? > > For binary size reasons we should probably only log errors. Done.
https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:919: VrShellDelegate.onNewIntent(this, getIntent()); s/getIntent()/intent https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:870: // We send this intent so that we can enter WebVr presentation mode if needed. This should probably live in ChromeActivity in case we want to support autopresent from other activities in the future as well.
Description was changed from ========== Support autopresenting WebVr content. The primary usecase for now is being able to deep-link content from Daydream home. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 ========== to ========== Support autopresenting WebVr content. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 ==========
mthiesse, PTAL :) I also added a safety check when VrShellDelegate handles the intent to ensure that the intent is not from Chrome (in case a malicious site is triggers an intent from Chrome). This is not really an issue because Chrome ensures that we only sign intents with a PendingIntent in certain cases and not from intents from the web, but doesn't hurt to have that check anyway. https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:919: VrShellDelegate.onNewIntent(this, getIntent()); On 2017/05/11 14:25:17, mthiesse wrote: > s/getIntent()/intent Ah thanks. This is why you shouldn't address review feedback late at night :P. https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java (right): https://codereview.chromium.org/2873843002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeTabbedActivity.java:870: // We send this intent so that we can enter WebVr presentation mode if needed. On 2017/05/11 14:25:17, mthiesse wrote: > This should probably live in ChromeActivity in case we want to support > autopresent from other activities in the future as well. Totally. At first I was worried because the first run bit only exists in ChromeTabbedActivity, but we probably don't want to autopresent after a first run anyway, so this is easy. Done.
lgtm https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:745: // We send this intent so that we can enter WebVr presentation mode if needed. Add comment on why you don't mark the intent as handled here (and below)?
https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:745: // We send this intent so that we can enter WebVr presentation mode if needed. On 2017/05/11 17:29:15, mthiesse wrote: > Add comment on why you don't mark the intent as handled here (and below)? The problem is ChromeTabbedActivity creates the tabs after this point, so if you try to do anything with the current tab you might get some weird behavior. For now until we start doing this for other activity types (which I don't see a path where that would work), I would actually just add this in ChromeTabbedActivity here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... processUrlViewIntent is done when we are creating a new tab or doing something on them. It handles the saved instance state stuff for you, so I think that is actually cleaner.
@tedchoc PTAL :) https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java (right): https://codereview.chromium.org/2873843002/diff/40001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/ChromeActivity.java:745: // We send this intent so that we can enter WebVr presentation mode if needed. On 2017/05/11 18:10:38, Ted C wrote: > On 2017/05/11 17:29:15, mthiesse wrote: > > Add comment on why you don't mark the intent as handled here (and below)? > > The problem is ChromeTabbedActivity creates the tabs after this point, so if you > try to do anything with the current tab you might get some weird behavior. > > For now until we start doing this for other activity types (which I don't see a > path where that would work), I would actually just add this in > ChromeTabbedActivity here: > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > processUrlViewIntent is done when we are creating a new tab or doing something > on them. It handles the saved instance state stuff for you, so I think that is > actually cleaner. Ah I see. Okay moved it to processUrlViewIntent. This function is also called from onNewNativeIntent (via mIntentHandler.onNewIntent), so we would only need it in one place which is much cleaner (with the caveat that we're CCT won't get this, but we're not supporting CCT anyway).
lgtm https://codereview.chromium.org/2873843002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2873843002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:543: if (intent.getBooleanExtra(DAYDREAM_VR_EXTRA, false) nit, I would use IntentUtils.safeGetBooleanExtra
https://codereview.chromium.org/2873843002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java (right): https://codereview.chromium.org/2873843002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/vr_shell/VrShellDelegate.java:543: if (intent.getBooleanExtra(DAYDREAM_VR_EXTRA, false) On 2017/05/11 21:51:17, Ted C wrote: > nit, I would use IntentUtils.safeGetBooleanExtra Done.
ymalik@chromium.org changed reviewers: + palmer@chromium.org
ymalik@chromium.org changed reviewers: + estark@chromium.org
ymalik@chromium.org changed reviewers: - estark@chromium.org
ymalik@chromium.org changed reviewers: + estark@chromium.org
Conceptually, we're okay with this for security. However, can you please add dominickn@ as TBR, or cc him and ask him to take a look when he gets back from vacation? He did an Enamel review for this feature so he should probably take a look at some point, even if this lands before he gets back. What I recall from the review is the following concerns. Could you please verify these before I stamp the CL? - Does the origin and security state show up for some amount of time after autopresenting? - It looks like this intent is allowed from any first-party app, whereas we had hoped to limit it to the Daydream app only. Is that possible? - We had asked for a special kind of intent for autopresenting, such that any old intent from Daydream doesn't autopresent, just special ones that specifically ask to bypass the user gesture requirement. Is that what the DAYDREAM_VR_EXTRA flag is? Also, it doesn't look like this CL contains any tests. Would it be possible to add tests for the above security requirements (e.g. that autopresent is not allowed if the intent comes from a non-Daydream app)?
On 2017/05/15 18:59:59, estark (temporarily slow) wrote: > Conceptually, we're okay with this for security. However, can you please add > dominickn@ as TBR, or cc him and ask him to take a look when he gets back from > vacation? He did an Enamel review for this feature so he should probably take a > look at some point, even if this lands before he gets back. > Done > What I recall from the review is the following concerns. Could you please verify > these before I stamp the CL? > > - Does the origin and security state show up for some amount of time after > autopresenting? Currently, if the the WebVr page is insecure, we show a transient warning for some time. Spoke to you offline about this, and you mentioned that we will need this before launch. Added a bug for this (crbug.com/724361) which blocks shipping this feature. Also guarded the feature behind a flag (off by default) as discussed. > - It looks like this intent is allowed from any first-party app, whereas we had > hoped to limit it to the Daydream app only. Is that possible? Yes, Done. > - We had asked for a special kind of intent for autopresenting, such that any > old intent from Daydream doesn't autopresent, just special ones that > specifically ask to bypass the user gesture requirement. Is that what the > DAYDREAM_VR_EXTRA flag is? Yes. > > Also, it doesn't look like this CL contains any tests. Would it be possible to > add tests for the above security requirements (e.g. that autopresent is not > allowed if the intent comes from a non-Daydream app)? We go through the onNewIntent logic in all of our Vr tests. The entry point for these tests is to start Chrome with a blank-page intent (see this https://cs.chromium.org/chromium/src/chrome/test/android/javatests/src/org/ch...). I added an assert in WebVrTest checking that we're not in VR as a result of any of these. I'm not sure how to test the positive case though (that daydream home intents actually auto present). We can consider this when we're ready to turn this feature on.
Description was changed from ========== Support autopresenting WebVr content. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 ========== to ========== Support autopresenting WebVr content. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 TBR=dominickn@chromium.org ==========
ymalik@chromium.org changed reviewers: + mariakhomenko@chromium.org
+mariakhomenko to confirm the changes in IntentHandler re whitelisting
The CQ bit was checked by ymalik@chromium.org to run a CQ dry run
The CQ bit was unchecked by ymalik@chromium.org
flag changes lgtm
On 2017/05/19 15:44:07, mthiesse wrote: > flag changes lgtm IntentHandler code lgtm
Thanks for the changes! lgtm
estark@chromium.org changed reviewers: + dominickn@chromium.org
The CQ bit was checked by ymalik@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ymalik@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org, mariakhomenko@chromium.org, mthiesse@chromium.org, estark@chromium.org Link to the patchset: https://codereview.chromium.org/2873843002/#ps160001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1495226044628170,
"parent_rev": "9010e14d8fc816219fe22ca57b63a9d1eefef45b", "commit_rev":
"e78a83bef3d557cd2f375caa189e7fe256b7b1ba"}
Message was sent while issue was closed.
Description was changed from ========== Support autopresenting WebVr content. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 TBR=dominickn@chromium.org ========== to ========== Support autopresenting WebVr content. We support autopresenting if the intent - launches a ChromeTabbedActivity - has a VR specific extra - originates from a first-party app There's no perfect way to know the sender of an intent. We use PendingIntents since it has a getCreatorPackage function that cannot be spoofed. That is, a trusted sender adds a PendingIntent as an extra which tags the intent with the sender credentials. The obvious risk here is that if something in between can get a hold of the PendingIntent, it can pretend to be the send indefinitely. That means that we must really trust the sender. This CL takes over bshe@'s https://codereview.chromium.org/2829193003/ Follow-up work will be to respond to this intent more elegantly so that we can get rid of the browser UI that will show up before we start autopresenting (see TODO). BUG=713369 TBR=dominickn@chromium.org Review-Url: https://codereview.chromium.org/2873843002 Cr-Commit-Position: refs/heads/master@{#473303} Committed: https://chromium.googlesource.com/chromium/src/+/e78a83bef3d557cd2f375caa189e... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/e78a83bef3d557cd2f375caa189e...
Message was sent while issue was closed.
Thanks for the TBR here. This lgtm from a Security UX perspective, assuming that crbug.com/724361 is addressed prior to launch. |
