Disallow paste in readonly omnibox

Disallow paste in readonly omnibox

Paste should not be permitted when the Omnibox is readonly, e.g. because the
window was opened via window.open with a location=0 option set.

BUG=597382
TEST=window.open with location=0, attempt paste in omnibox

[elawrence, 2016-03-25 16:49:30]

elawrence@chromium.org changed reviewers:
+ pkasting@chromium.org

[elawrence, 2016-03-25 16:49:30]

elawrence@chromium.org changed required reviewers:
+ pkasting@chromium.org

[elawrence, 2016-03-25 16:49:31]

Howdy, Peter-- Adding you as reviewer per the OWNERS file.

[Peter Kasting, 2016-03-25 21:28:37]

Ideally this should have a test -- we should create a read-only location bar and
ensure trying to paste into it has no effect.

It might be possible to do this as a unittest.  If not, we can do it as a UI
test, but those are slower and more prone to flakiness, so prefer uinittesting
if possible.

https://codereview.chromium.org/1833133002/diff/1/chrome/browser/ui/views/omn...
File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):

https://codereview.chromium.org/1833133002/diff/1/chrome/browser/ui/views/omn...
chrome/browser/ui/views/omnibox/omnibox_view_views.cc:373: if
(Textfield::IsCommandIdEnabled(command_id)) {
Use IsCommandIdEnabled() here rather than the Textfield version, since that
explicitly handles IDS_APP_PASTE.  This way if we make changes to that in the
future, the "enabled" state of the menu item and of handling the keyboard
shortcut will change in sync.

Nit: No {}

[elawrence, 2016-03-28 19:42:20]

>
https://codereview.chromium.org/1833133002/diff/1/chrome/browser/ui/views/omn...
> File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> 
>
https://codereview.chromium.org/1833133002/diff/1/chrome/browser/ui/views/omn...
> chrome/browser/ui/views/omnibox/omnibox_view_views.cc:373: if
> (Textfield::IsCommandIdEnabled(command_id)) {
> Use IsCommandIdEnabled() here rather than the Textfield version, since that
> explicitly handles IDS_APP_PASTE.  This way if we make changes to that in the
> future, the "enabled" state of the menu item and of handling the keyboard
> shortcut will change in sync.

Done. The relationship between the Textfield and Omnibox classes and their
OnKeyPressed and HandleKeyEvents is confusing to me. For instance,
OmniboxViewViews::OnKeyPressed's handling of CTRL+VKEY_V already checked whether
read_only() is true before calling ExecuteCommand(IDS_APP_PASTE), but that check
fails to block the paste from actually taking place. In contrast, the same check
is effective when dealing with SHIFT+VKEY_INSERT a few lines later.

> Nit: No {}

Fixed. 

> Ideally this should have a test -- we should create a read-only location bar
and
> ensure trying to paste into it has no effect.
> 
> It might be possible to do this as a unittest.  If not, we can do it as a UI
> test, but those are slower and more prone to flakiness, so prefer uinittesting
> if possible.

I think I see both types of tests in omnibox_view_views_unittest and
omnibox_view_views_browsertest.cc, but unfortunately I didn't notice any
existing tests that deal with a readonly omnibox (popup_window_mode_ = true).

Unfortunately, I have not previously created entirely new unit tests and have
run out of time to work on this for a bit. Does it make sense to have the
dprenke's nominee
(https://bugs.chromium.org/p/chromium/issues/detail?id=597382#c2) take this over
instead?

[Peter Kasting, 2016-03-29 01:35:12]

On 2016/03/28 19:42:20, elawrence wrote:
> >
>
https://codereview.chromium.org/1833133002/diff/1/chrome/browser/ui/views/omn...
> > File chrome/browser/ui/views/omnibox/omnibox_view_views.cc (right):
> > 
> >
>
https://codereview.chromium.org/1833133002/diff/1/chrome/browser/ui/views/omn...
> > chrome/browser/ui/views/omnibox/omnibox_view_views.cc:373: if
> > (Textfield::IsCommandIdEnabled(command_id)) {
> > Use IsCommandIdEnabled() here rather than the Textfield version, since that
> > explicitly handles IDS_APP_PASTE.  This way if we make changes to that in
the
> > future, the "enabled" state of the menu item and of handling the keyboard
> > shortcut will change in sync.
> 
> Done. The relationship between the Textfield and Omnibox classes and their
> OnKeyPressed and HandleKeyEvents is confusing to me. For instance,
> OmniboxViewViews::OnKeyPressed's handling of CTRL+VKEY_V already checked
whether
> read_only() is true before calling ExecuteCommand(IDS_APP_PASTE), but that
check
> fails to block the paste from actually taking place. In contrast, the same
check
> is effective when dealing with SHIFT+VKEY_INSERT a few lines later.

Interesting.  This made me debug this a bit.

It looks like the Textfield's accelerator for this (
https://code.google.com/p/chromium/codesearch#chromium/src/ui/views/controls/...
) is causing the OmniboxViewViews to never see the 'v' key event.  Instead the
Textfield calls into ExecuteCommand() directly from
Textfield::AcceleratorPressed().

This codepath is OK; what this makes me wonder is if the VKEY_V clause in
OmniboxViewViews::OnKeyPressed() is ever reachable anymore.  It would be good to
test this on Linux/CrOS to make sure it's not, then remove it.

> > Ideally this should have a test -- we should create a read-only location bar
> and
> > ensure trying to paste into it has no effect.
> > 
> > It might be possible to do this as a unittest.  If not, we can do it as a UI
> > test, but those are slower and more prone to flakiness, so prefer
uinittesting
> > if possible.
> 
> I think I see both types of tests in omnibox_view_views_unittest and
> omnibox_view_views_browsertest.cc, but unfortunately I didn't notice any
> existing tests that deal with a readonly omnibox (popup_window_mode_ = true).

Shouldn't be too hard -- looks like a little bit of parametrization in
omnibox_view_views_unittest.cc would allow TestingOmniboxViewViews to either
have popup mode off or on.  I would just add that as an arg to the constructor,
move the creation out of OmniboxViewViewsTest::SetUp() into an explicitly-called
creatino func that takes this arg, update the one test in that file to use this,
and add another test that uses it with the opposite value.

> Unfortunately, I have not previously created entirely new unit tests and have
> run out of time to work on this for a bit. Does it make sense to have the
> dprenke's nominee
> (https://bugs.chromium.org/p/chromium/issues/detail?id=597382#c2) take this
over
> instead?

Oh, I thought you were said nominee.

[Peter Kasting, 2016-04-29 02:24:15]

Description was changed from

==========
Disallow paste in readonly omnibox

Paste should not be permitted when the Omnibox is readonly, e.g. because the
window was opened via window.open with a location=0 option set.

BUG=597382
TEST=window.open with location=0, attempt paste in omnibox
==========

to

==========
Disallow paste in readonly omnibox

Paste should not be permitted when the Omnibox is readonly, e.g. because the
window was opened via window.open with a location=0 option set.

BUG=597382
TEST=window.open with location=0, attempt paste in omnibox
==========

[Peter Kasting, 2016-04-29 02:24:37]

On 2016/03/29 01:35:12, Peter Kasting wrote:
> On 2016/03/28 19:42:20, elawrence wrote:
> > Unfortunately, I have not previously created entirely new unit tests and
have
> > run out of time to work on this for a bit. Does it make sense to have the
> > dprenke's nominee
> > (https://bugs.chromium.org/p/chromium/issues/detail?id=597382#c2) take this
> over
> > instead?
> 
> Oh, I thought you were said nominee.

+CC Dirk -- can one of you find the right person to move forward here?

[Dirk Pranke, 2016-04-29 02:27:16]

dpranke@chromium.org changed reviewers:
+ dpranke@chromium.org

[Dirk Pranke, 2016-04-29 02:27:16]

+thomasanderson - do you want to take a look at this?

[Tom (Use chromium acct), 2016-04-29 04:52:45]

On 2016/04/29 02:27:16, Dirk Pranke wrote:
> +thomasanderson - do you want to take a look at this?

Sure thing

[Peter Kasting, 2016-08-20 02:36:12]

pkasting@chromium.org changed reviewers:
+ thomasanderson@chromium.org
- dpranke@chromium.org, pkasting@chromium.org

[Peter Kasting, 2016-08-20 02:36:12]

pkasting@chromium.org changed required reviewers:
- pkasting@chromium.org

[Peter Kasting, 2016-08-20 02:36:41]

(Changed reviewer to thomasanderson to match previous comment)