New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 331571 link

Starred by 73 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Oct 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 2
Type: Bug-Security

Blocking:
issue 329973
issue 336069



Sign in to add a comment

Typing pandora.com in omnibox automatically redirects user to native app, if installed

Reported by vinodkr@chromium.org, Jan 3 2014

Issue description

Reported by rahulrc@

1. Install Pandora app
2. Type pandora.com in Chrome omnibox

Observed behavior: Pandora app opens automatically without any additional user gesture

Expected behavior: Typing in the omnibox should load the page in the browser


 
If pandora app is not installed, navigating to pandora.com opens up Play Store directly to install the app.
If pandora app is not installed, navigating to pandora.com opens up Play Store directly to install the app.
@askatte, what version are you testing on?  I tried typing pandora.com in the omnibox and it takes me to the website.
The site uses a trick to simulate click. They have an <a> link with intent:// url. In onload(), they call element.click(). We take it as user action and url hijacking logic triggered the rest. This is working as intended.

We currently use ResourceRequestInfo to tell whether it has user gesture. See
https://code.google.com/p/chromium/codesearch#chromium/src/components/navigation_interception/intercept_navigation_resource_throttle.cc&sq=package:chromium&type=cs&l=99&rcl=1388718684

If we want to change the logic to exclude the JS triggered click, we may break other cases.
And forgive my comment in #3, it turns out I was on a request desktop tab.

Comment 6 by joh...@chromium.org, Jan 10 2014

Why does an onload handler count as a user action?

Comment 7 by klo...@chromium.org, Jan 10 2014

element.click() is counted as user action.

Comment 8 by joh...@chromium.org, Jan 10 2014

Labels: Type-Bug-Security
Woah, why is element.click() counted as a user action? It should only be counted as a user action if it's called from e.g. an onclick handler (and no more than ~once per event).

There are some subtleties, like whether to allow calling it from a setTimeout called from onclick, but these issues have all been resolved for popup blocking, and we should be able to share whatever logic they use.

Comment 9 by klo...@chromium.org, Jan 10 2014

Cc: joh...@chromium.org
Blink marked it as user gesture. Want to take it over from Ted?
Cc: tedc...@chromium.org
Owner: joh...@chromium.org
Sure, I can take a look. intent:// urls and the like should be wired up to DOMWindow::allowPopUp(Frame*). It's possible we apply a weaker standard to links that are believed to be navigational, and that's what's biting us here.
Huh, so InterceptNavigationResourceThrottle::CheckIfShouldIgnoreNavigation uses the should_ignore_callback_ to validate the NavigationParams, and the callback ultimately gets plumbed through to shouldIgnoreNavigation(NavigationParams navigationParams) in Tab.java:
https://cs.corp.google.com/#clank/java/apps/chrome/src/com/google/android/apps/chrome/tab/Tab.java&l=2698

But shouldIgnoreNavigation ignores the has_user_gesture property of NavigationParams!
Project Member

Comment 12 by ClusterFuzz, Jan 10 2014

Labels: Missing_Severity-3 Missing_Impact-3
Test page: http://jsbin.com/acOcEXo/1/quiet (clicking the link should fire the intent, but the onload element.click() should be ignored).
Issue 332980 has been merged into this issue.
Labels: -Pri-2 -M-X -Missing_Severity-3 -Missing_Impact-3 Pri-1 M-33 Cr-Mobile-Intents Security_Impact-Beta Security_Impact-Stable Security_Severity-Low
First-pass patch: https://chrome-internal-review.googlesource.com/151687

With the patch, if you visit http://jsbin.com/acOcEXo/1/quiet you'll get a "This web page is not available" error, which you can expand to reveal a "ERR_UNKNOWN_URL_SCHEME" (see attached screenshots).

It's not the most informative error message, and unfortunately it becomes really hard to get to the actual web page, as when you press back from the error page, onload() runs again and it redirects you back to the error page.
Screenshot_2014-01-10-22-02-30.png
138 KB View Download
Screenshot_2014-01-10-22-02-23.png
65.5 KB View Download
Issue 332980 has been merged into this issue.
Hmm...can we treat websites trying to trigger intents exactly like we do popups (i.e. show an info bar letting them know the site was trying to launch an external application)?
Issue 332980 has been merged into this issue.
Cc: mkosiba@chromium.org
If the Pandora folks really want this kind of UX (where you type in pandora.com and it takes you to the store/app) then I'm not sure it's up to us to block them from doing that. I'm not saying we shouldn't, just saying that this makes it impossible to have that sort of UX and if we do say that we don't want that kind of UX possible in Chrome we should have some sort of story to tell the folks whose pages suddenly stop redirecting users to the native app.

What do we want these folks to do instead? What would the ideal UX be? Putting on my "app dev" hat - if I own the native app and I own the web page, I want to be able to decide that I want to redirect all of the web page users to the native app, I want that to be a choice I'm able to make.
We want the web developer having the choice. But we want the end user to make the final decision. That is why we introduce "App Install Alert". In its final form, user will see the native app in the omnibox list when they type the url and the app has been installed. If they still choose the site, we will only show the web content.

Comment 22 by f...@chromium.org, Jan 13 2014

Labels: Cr-Security-UX
klobag in #21 is correct: the priority of constituents is user > web site > browser.

I just had a conversation with a user who only wants to use Google Maps via the browser, never from a native Android app. Now, she went to the trouble of installing Cyanogenmod and not installing Google Maps, but people less technical than her may have the same preference for whatever reason. It is not ours to question that.
#21 - fully agree with 'user > web site > browser'. Based on the patch in #16 I was under the impression we were trying to go with 'browser > user > web site'.

The "App Install Alert" sounds like a good idea. Will it be possible to make it so that typing the URL will _by default_ take you to the app and the second omnibox choice would be "take me to the web page anyway"?
Cc: jcivelli@chromium.org
Actually, faking click events is a red herring - currently JS can launch intents whenever it likes just by setting window.location to an intent:// url (and the tab remains open). Test page: http://jsbin.com/elEBidEH/2

There's a slight difference between faking click events and setting window.location when used in iframes: setting window.location doesn't get intercepted in iframes (you just get the error page), and the same thing happens for click events in iframes, unless you set target="_blank" on your link in which case the popup blocker blocks it (both on Android and desktop).

tedchoc wrote:
> can we treat websites trying to trigger intents exactly like we do popups

SGTM

mkosiba wrote:
> The "App Install Alert" sounds like a good idea. Will it be possible to make it so that typing the URL will _by default_ take you to the app

As usual, the omnibox should learn which of the two options you choose more often, and that one should be suggested first. There may be additional subtleties with making it trigger when you type Go though (IIRC, that's currently only possible if the text you entered is a prefix of the final url).
Cc: pasko@google.com
I think we should still let user click go through. But treat window.location on the main frame as popup.
Blocking: chromium:336069
Issue 332980 has been merged into this issue.
Blocking: chromium:329973
Owner: ----
Status: Available
Sorry, realistically I don't think I have time to go through all the edge cases and finish off this patch. Hopefully someone else can use https://chrome-internal-review.googlesource.com/151687 as a starting point though.

Comment 33 by f...@chromium.org, Feb 5 2014

klobag@, do you know anyone else who could own this?
I don't know. felt@, are you interested?

Comment 35 by f...@chromium.org, Feb 5 2014

I don't have bandwidth for it right now, just checking in on the status of all of the cr-security-ux bugs.
Project Member

Comment 36 by ClusterFuzz, Feb 8 2014

Labels: Missing_Owner-17
Project Member

Comment 37 by ClusterFuzz, Feb 9 2014

Labels: -Missing_Owner-17 Missing_Owner-18
Project Member

Comment 38 by ClusterFuzz, Feb 11 2014

Labels: -Missing_Owner-18 Missing_Owner-19
Project Member

Comment 39 by ClusterFuzz, Feb 14 2014

Labels: -Missing_Owner-19 Missing_Owner-20
Project Member

Comment 40 by ClusterFuzz, Feb 16 2014

Labels: -Missing_Owner-20 Missing_Owner-21
Project Member

Comment 41 by ClusterFuzz, Feb 17 2014

Labels: -Missing_Owner-21 Missing_Owner-22
Project Member

Comment 42 by ClusterFuzz, Mar 31 2014

Labels: -M-33 M-34
Project Member

Comment 43 by ClusterFuzz, May 13 2014

Labels: -M-34 M-35
Issue 332980 has been merged into this issue.
Issue 332980 has been merged into this issue.

Comment 46 by aarya@google.com, May 17 2014

Labels: -Restrict-View-Google Restrict-View-SecurityTeam
Labels: -Security_Severity-Low -M-35 Security_Severity-Medium M-36
Owner: tedc...@chromium.org
Status: Assigned
tedchoc, can I pass this on to you? We really should nail this thing down. Or can you help find someone?

Also, Pri-1 doesn't match -Low; -Low is usually Pri-2. I'm not quite sure which is right; I'm going to call it Medium and 1.

I'm bumping it to M-36 since it's late in the game for M-35, and this appears to have tricky policy/compatibility implications.
Project Member

Comment 48 by ClusterFuzz, Jun 15 2014

Labels: Nag
tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 49 by ClusterFuzz, Jun 23 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
[pinged Ted on chat]
Cc: n...@chromium.org
Labels: -M-36 M-38
Sadly, we are swamped with work to get chrome ready for the Android L release.  Adding newt@ in case one of us has cycles to take a look at this in between the flurry of other changes.

At this point, 37 and maybe even 38 is going to be almost entirely devoted to Material and Hera, so I'm moving it to the first realistic milestone we might hit.  If we can pull it back to 37 some how then we will.
Project Member

Comment 52 by ClusterFuzz, Jul 6 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 53 by ClusterFuzz, Jul 14 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 54 by ClusterFuzz, Jul 23 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 55 by ClusterFuzz, Jul 28 2014

Labels: -Security_Impact-Beta
Project Member

Comment 56 by ClusterFuzz, Aug 3 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 57 by ClusterFuzz, Aug 11 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 58 by ClusterFuzz, Aug 18 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 59 by ClusterFuzz, Aug 26 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Cc: juanlang@google.com
Project Member

Comment 61 by ClusterFuzz, Sep 2 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 62 by ClusterFuzz, Sep 9 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 63 by ClusterFuzz, Sep 17 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Project Member

Comment 64 by ClusterFuzz, Sep 24 2014

tedchoc@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Labels: -M-38 M-40
Owner: jaekyun@chromium.org
jaekyun@, as you are looking at intent handling, please take over this one. Thanks.
Could anyone please clarify this issue?

Do we want to show "App Install Alert" in the omnibox, or show an info bar for a typed url which redirects to a link to an app or an intent picker?

When typing url in the omnibox, there should be no url override happens. In another word, we should not leave Chrome if user explicitly type url inside Chrome.
If we make no url override happen, we will see an error page when typing "www.pandora.com". Is this what we want?

In this case, we want to show the error page so that they won't do this trick.

High level, we should figure out what is the best solution, see some discussion in https://code.google.com/p/chromium/issues/detail?id=332980 too.

Please combine the ideas in this two bugs and write a proposal. We can discuss in the doc. Thanks.

I can't open https://code.google.com/p/chromium/issues/detail?id=332980 with http 403 error. I don't seem to have a read permission.
I added you to the bug. Try again.
Thanks, I can read now.

BTW https://code.google.com/p/chromium/issues/detail?id=332980 seems to cover not only a typed url but also any url which redirects to an intent. Should we consider it here?

Anyway, I will prepare a proposal soon.
Issue 332980 has been merged into this issue.
It seems that all the comments are resolved in my proposal doc.

As followup, I will make a change to guarantee that there should be no url override happens when the initial navigation is from a user’s typing.

And I filed http://crbug/421296 to discuss more deeply about showing an info bar.

Status: Started
I've uploaded https://chrome-internal-review.googlesource.com/#/c/179235 not to override a navigation started from user typing.

Cc: kkimlabs@chromium.org
Project Member

Comment 78 by bugdroid1@chromium.org, Oct 15 2014

The following change refers to this bug:
https://chrome-internal-review.googlesource.com/179235
Status: Fixed
Project Member

Comment 80 by ClusterFuzz, Oct 15 2014

Labels: -Restrict-View-SecurityTeam -M-40 Merge-Triage M-39 M-38 Restrict-View-SecurityNotify
Adding Merge-Triage label for tracking purposes.

Once your fix had sufficient bake time (on canary, dev as appropriate), please nominate your fix for merge by adding the Merge-Requested label.

When your merge is approved by the release manager, please start merging with higher milestone label first. Make sure to re-request merge for every milestone in the label list. You can get branch information on omahaproxy.appspot.com.

- Your friendly ClusterFuzz
Labels: -Merge-Triage -M-38 Merge-Requested
Status: Started
Grace, are you OK with merging this feature to m39 because https://chrome-internal-review.googlesource.com/179235 is needed to cherry-pick https://chrome-internal-review.googlesource.com/179897 to m39?

Comment 82 by amin...@google.com, Oct 20 2014

Labels: -Merge-Requested Merge-Approved
From a TPM perspective, merge approved for m39 branch 2171, though please wait for Grace's OK if required
Labels: -M-39 M-40
There is no rush to push to m39. Let us soak this for M40.
Labels: -Merge-Approved
Status: Fixed
I see. I will not merge this into m39.
Cc: cartland@google.com paulkinlan@chromium.org mahues@google.com a...@google.com
Labels: -Pri-1 -Security_Severity-Medium -Restrict-View-SecurityNotify Pri-2 Security_Severity-Low
Based on internal email thread, removing security view restrictions early as requested by paulkinlan@. Also dropping security-severity to low (and changing to pri-2 based on that).
Labels: -Missing_Owner-22 -Nag Release-0-M40
 Issue 454396  has been merged into this issue.
Cc: jaekyun@chromium.org
 Issue 459156  has been merged into this issue.
Cc: mariakho...@chromium.org dfalcant...@chromium.org
 Issue 477456  has been merged into this issue.
Cc: aelias@chromium.org
 Issue 484815  has been merged into this issue.
 Issue 484958  has been merged into this issue.
Cc: -kkimlabs@chromium.org
Labels: Restrict-View-Google
Adding google view restriction. Please remove if this wasn't appropriate. See issue 609744 for reference.
Labels: -Restrict-View-Google
Project Member

Comment 96 by sheriffbot@chromium.org, Oct 1 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member

Comment 97 by sheriffbot@chromium.org, Oct 2 2016

This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Components: -Security>UX
Labels: Team-Security-UX
Security>UX component is deprecated in favor of the Team-Security-UX label

Sign in to add a comment