Protocol handler dialogs should only be enabled by browser or page actions [Not committed]

Protocol handler dialogs should only be enabled by browser or page actions
and not by arbitrary extension functions.

BUG=173557

[meacer, 2014-01-28 00:16:25]

Jeffrey, can you take a look? Not sure how many extensions rely on the fact that
any method call could enable protocol handlers, so there is a chance that this
might break existing extensions.

[Jeffrey Yasskin, 2014-01-28 00:54:18]

We already have user gestures wired up in a bunch of places, so it's possible
that this has started Just Working since 2010. If not, I have a couple other
places to double-check below. There are a couple more places that call
ActiveTabPermissionGranter::GrantIfRequested, which it'd also be good to cover:
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...

https://codereview.chromium.org/148153008/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/148153008/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:360:
ExternalProtocolHandler::PermitLaunchUrl();
The other place I'd expect people to be relying on a user gesture being
available is in postMessage()s to their extensions. The bottleneck for that in
the browser process looks like MessageService::DispatchMessage() at
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...,
where you'd check for message.user_gesture. If you can enable this
renderer-side, you can also use  MessagingBindings::DeliverMessage
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/renderer/e...).
It's also possible that postMessage already enables external protocol handlers,
since it does get a Blink-side WebScopedUserGesture.

Context menus might also need this: MenuManager::ExecuteCommand at
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...

[meacer, 2014-01-30 00:20:44]

https://codereview.chromium.org/148153008/diff/1/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/148153008/diff/1/chrome/browser/extensions/ap...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:360:
ExternalProtocolHandler::PermitLaunchUrl();
On 2014/01/28 00:54:19, Jeffrey Yasskin wrote:
> The other place I'd expect people to be relying on a user gesture being
> available is in postMessage()s to their extensions. The bottleneck for that in
> the browser process looks like MessageService::DispatchMessage() at
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...,
> where you'd check for message.user_gesture. If you can enable this
> renderer-side, you can also use  MessagingBindings::DeliverMessage
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/renderer/e...).

Just to clarify: If postMessage wasn't called from within a user gesture, it
shouldn't enable protocol handlers, right? Or do you mean developers would
expect postMessage to enable handlers even without a user gesture?

> It's also possible that postMessage already enables external protocol
handlers,
> since it does get a Blink-side WebScopedUserGesture.

Looks like it doesn't already enable, the WebScopedUserGesture is created in
that method if message.user_gesture is true.

> Context menus might also need this: MenuManager::ExecuteCommand at
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...

I'll add the enable code into ActiveTabPermissionGranter::GrantIfRequested as it
looks like it's only invoked by user actions. Should cover
MenuManager::ExecuteCommand as well.

[Jeffrey Yasskin, 2014-01-30 02:40:23]

On Wed, Jan 29, 2014 at 4:20 PM,  <meacer@chromium.org> wrote:
>
>
https://codereview.chromium.org/148153008/diff/1/chrome/browser/extensions/ap...
> File
> chrome/browser/extensions/api/extension_action/extension_action_api.cc
> (right):
>
>
https://codereview.chromium.org/148153008/diff/1/chrome/browser/extensions/ap...
> chrome/browser/extensions/api/extension_action/extension_action_api.cc:360:
> ExternalProtocolHandler::PermitLaunchUrl();
> On 2014/01/28 00:54:19, Jeffrey Yasskin wrote:
>>
>> The other place I'd expect people to be relying on a user gesture
>
> being
>>
>> available is in postMessage()s to their extensions. The bottleneck for
>
> that in
>>
>> the browser process looks like MessageService::DispatchMessage() at
>
>
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...,
>>
>> where you'd check for message.user_gesture. If you can enable this
>> renderer-side, you can also use  MessagingBindings::DeliverMessage
>
>
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/renderer/e...).
>
> Just to clarify: If postMessage wasn't called from within a user
> gesture, it shouldn't enable protocol handlers, right? Or do you mean
> developers would expect postMessage to enable handlers even without a
> user gesture?

Oh, yes, I meant that if a user gesture causes a postMessage() to an
extension, the extension might expect to be able to use a protocol
handler.

>> It's also possible that postMessage already enables external protocol
>
> handlers,
>>
>> since it does get a Blink-side WebScopedUserGesture.
>
>
> Looks like it doesn't already enable, the WebScopedUserGesture is
> created in that method if message.user_gesture is true.
>
>
>> Context menus might also need this: MenuManager::ExecuteCommand at
>
>
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...
>
> I'll add the enable code into
> ActiveTabPermissionGranter::GrantIfRequested as it looks like it's only
> invoked by user actions. Should cover MenuManager::ExecuteCommand as
> well.

SG.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[meacer, 2014-01-30 19:11:55]

On 2014/01/30 02:40:23, Jeffrey Yasskin wrote:
> On Wed, Jan 29, 2014 at 4:20 PM,  <mailto:meacer@chromium.org> wrote:
> >
> >
>
https://codereview.chromium.org/148153008/diff/1/chrome/browser/extensions/ap...
> > File
> > chrome/browser/extensions/api/extension_action/extension_action_api.cc
> > (right):
> >
> >
>
https://codereview.chromium.org/148153008/diff/1/chrome/browser/extensions/ap...
> > chrome/browser/extensions/api/extension_action/extension_action_api.cc:360:
> > ExternalProtocolHandler::PermitLaunchUrl();
> > On 2014/01/28 00:54:19, Jeffrey Yasskin wrote:
> >>
> >> The other place I'd expect people to be relying on a user gesture
> >
> > being
> >>
> >> available is in postMessage()s to their extensions. The bottleneck for
> >
> > that in
> >>
> >> the browser process looks like MessageService::DispatchMessage() at
> >
> >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...,
> >>
> >> where you'd check for message.user_gesture. If you can enable this
> >> renderer-side, you can also use  MessagingBindings::DeliverMessage
> >
> >
> >
>
(https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/renderer/e...).
> >
> > Just to clarify: If postMessage wasn't called from within a user
> > gesture, it shouldn't enable protocol handlers, right? Or do you mean
> > developers would expect postMessage to enable handlers even without a
> > user gesture?
> 
> Oh, yes, I meant that if a user gesture causes a postMessage() to an
> extension, the extension might expect to be able to use a protocol
> handler.
> 
> >> It's also possible that postMessage already enables external protocol
> >
> > handlers,
> >>
> >> since it does get a Blink-side WebScopedUserGesture.
> >
> >
> > Looks like it doesn't already enable, the WebScopedUserGesture is
> > created in that method if message.user_gesture is true.
> >
> >
> >> Context menus might also need this: MenuManager::ExecuteCommand at
> >
> >
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/chrome/browser/ex...
> >
> > I'll add the enable code into
> > ActiveTabPermissionGranter::GrantIfRequested as it looks like it's only
> > invoked by user actions. Should cover MenuManager::ExecuteCommand as
> > well.
> 
> SG.
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

PTAL. Hopefully this covers all user gesture related paths.

[Jeffrey Yasskin, 2014-01-31 21:59:07]

This LGTM, but kalman has some concerns. In particular, he suspects that the
PermitLaunchUrl() call may not be needed at all anymore because this is already
covered by window.open() restrictions.

[not at google - send to devlin, 2014-01-31 22:01:40]

Yeah this browser side state is suspicious, I don't know why external URL
handling is any different and the not-really-scoped nature of the current (old)
code seems bogus.

The original bug this was all based on didn't come with any tests, but I
wouldn't be surprised if just reverting it fixed this problem.

[meacer, 2014-01-31 22:31:06]

On 2014/01/31 22:01:40, kalman wrote:
> Yeah this browser side state is suspicious, I don't know why external URL
> handling is any different and the not-really-scoped nature of the current
(old)
> code seems bogus.

Just tested, reverting it reintroduces bug 39178. Do you mean why external URL
handling is different from any other dialog that requires user gestures?
 
> The original bug this was all based on didn't come with any tests, but I
> wouldn't be surprised if just reverting it fixed this problem.

[not at google - send to devlin, 2014-01-31 22:35:39]

Yeah pretty much. Clicking on the browser action button does set the user action
bit all the way down into the renderer. Why is that any different for external
protocol handlers?

Your patch is obviously an improvement over the current situation and I wouldn't
want to send you down a rabbit hold of figuring out what is going on here.

[meacer, 2014-02-01 00:30:28]

On 2014/01/31 22:35:39, kalman wrote:
> Yeah pretty much. Clicking on the browser action button does set the user
action
> bit all the way down into the renderer. Why is that any different for external
> protocol handlers?
> 
> Your patch is obviously an improvement over the current situation and I
wouldn't
> want to send you down a rabbit hold of figuring out what is going on here.

Right. The way the handler works is that it's checked on the browser side right
after a url resource is requested. Renderers don't know about protocol handlers.
I guess that's why the initial patch introduced the browser state.

I have another patch that gets rid of the state and checks the user gesture
during resource load. I'll upload it once tests pass.

[meacer, 2014-02-01 01:47:11]

> I have another patch that gets rid of the state and checks the user gesture
> during resource load. I'll upload it once tests pass.

Ok looks like it needs some more work. "Send To Gmail" extension does an
executeScript on the page, and per
https://code.google.com/p/chromium/issues/detail?id=155056#c4 executeScript
doesn't pass a user_gesture bit. Thus there won't be a gesture and the protocol
handler (mailto:) won't work, meaning the extension will break.

I'm now working on passing the user_gesture to executeScript on the renderer
side if the browser action is clicked. The patch has grown significantly but I
hope it's going to be the right fix. This should also fix bug 155056. Let me
know if you object to the plan.

[not at google - send to devlin, 2014-02-01 02:02:54]

Passing the user gesture bit along with executeScript sounds reasonable. I don't
think it would be that hard really.

https://codereview.chromium.org/148153008/diff/60001/chrome/browser/extension...
File chrome/browser/extensions/extension_function_dispatcher.cc (left):

https://codereview.chromium.org/148153008/diff/60001/chrome/browser/extension...
chrome/browser/extensions/extension_function_dispatcher.cc:387:
ExternalProtocolHandler::PermitLaunchUrl();
I wonder whether a simpler fix to this would be to just guard this on
params.user_gesture, but I don't fully understand the interactions going on
here.