Form submission with blank target is broken.

Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the
form post request. However, this is no longer needed with WKWebView's
callbacks. This removes the early |registerLoadRequest:referrer:transition:|
call which was added for UIWebView and is no longer needed.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
Review-Url: https://codereview.chromium.org/2763173002
Cr-Commit-Position: refs/heads/master@{#460688}
Committed: https://chromium.googlesource.com/chromium/src/+/9736d7d0d0c9999c239e43486a901b1f4a61cbdd

[michaeldo, 2017-03-21 22:51:00]

michaeldo@chromium.org changed reviewers:
+ eugenebut@chromium.org, kkhorimoto@chromium.org

[michaeldo, 2017-03-21 22:51:00]

Note that I tried to add a test to cover this case, but it is not trivial as
EarlGrey doesn't simulate the touch needed to allow a window to open. Also, if
the WebView is tapped separately, the currently broken case still passes the
test which means it wouldn't be beneficial to catch this regression the future.

[kkhorimoto, 2017-03-21 23:28:20]

On 2017/03/21 22:51:00, michaeldo wrote:
> Note that I tried to add a test to cover this case, but it is not trivial as
> EarlGrey doesn't simulate the touch needed to allow a window to open. Also, if
> the WebView is tapped separately, the currently broken case still passes the
> test which means it wouldn't be beneficial to catch this regression the
future.

Your fix seems reasonable, but I'm wondering if it might be easier to test using
unittests instead of EG.  We could add a function to the CRWWebController
UsedOnlyForTesting category that can reset |_lastUserInteraction|, then execute
window.open via JavaScript, using a TestWebStateDelegate to ensure that the
correct flag is passed.

[Eugene But (OOO till 7-30), 2017-03-22 00:46:46]

On 2017/03/21 23:28:20, kkhorimoto_ wrote:
> On 2017/03/21 22:51:00, michaeldo wrote:
> > Note that I tried to add a test to cover this case, but it is not trivial as
> > EarlGrey doesn't simulate the touch needed to allow a window to open. Also,
if
> > the WebView is tapped separately, the currently broken case still passes the
> > test which means it wouldn't be beneficial to catch this regression the
> future.
> 
> Your fix seems reasonable, but I'm wondering if it might be easier to test
using
> unittests instead of EG.  We could add a function to the CRWWebController
> UsedOnlyForTesting category that can reset |_lastUserInteraction|, then
execute
> window.open via JavaScript, using a TestWebStateDelegate to ensure that the
> correct flag is passed.
From "Unit Testing Best Practices": Test code using real APIs - don’t define
back doors only for tests.
go/unit-test-practices/?cl=head#public-apis

Tests that use private API fail during refactoring, which makes them not very
useful for catching regressions. Also they add technical debt because they make
refactoring harder. If the choice is between writing an integration test or
violating encapsulation, I think we should choose the former.

[Eugene But (OOO till 7-30), 2017-03-22 00:59:39]

https://codereview.chromium.org/2763173002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2763173002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:4218: requestURL, openerURL, [self
userClickedRecently]);
This will allow more popups from malicious pages, so I would be very cautious
about this change. CL descriptions says that "window.open code expects that user
is still interacting with the page", but I don't think that's true.
|userIsInteracting| name is just misleading, it's not doing what it says :(

Current code seems to work correctly with window.open and target="_blank" links.
It does not work correctly with forms. Do you know what is the difference
between "form" case vs. "_blank" links f.e?

[michaeldo, 2017-03-22 23:26:26]

On 2017/03/22 00:59:39, Eugene But wrote:
>
https://codereview.chromium.org/2763173002/diff/1/ios/web/web_state/ui/crw_we...
> File ios/web/web_state/ui/crw_web_controller.mm (right):
> 
>
https://codereview.chromium.org/2763173002/diff/1/ios/web/web_state/ui/crw_we...
> ios/web/web_state/ui/crw_web_controller.mm:4218: requestURL, openerURL, [self
> userClickedRecently]);
> This will allow more popups from malicious pages, so I would be very cautious
> about this change. CL descriptions says that "window.open code expects that
user
> is still interacting with the page", but I don't think that's true.
> |userIsInteracting| name is just misleading, it's not doing what it says :(
> 
> Current code seems to work correctly with window.open and target="_blank"
links.
> It does not work correctly with forms. Do you know what is the difference
> between "form" case vs. "_blank" links f.e?

I see, you're right this is just the form case. The code paths are in fact
different, the native code receives the form submission action from javascript
that the form was submitted. However, for the link _blank navigation, the
WKWebView delegate notifies the application to create a new child window.

In the form submit case, we know the user interacted, but loose that information
by the time the new window is created

Out of curiosity, would the additional abuse just be that the page has a
slightly longer time frame after the user interacted? In practice, would the
time increase make a difference?

[Eugene But (OOO till 7-30), 2017-03-22 23:34:31]

On 2017/03/22 23:26:26, michaeldo wrote:
> On 2017/03/22 00:59:39, Eugene But wrote:
> >
>
https://codereview.chromium.org/2763173002/diff/1/ios/web/web_state/ui/crw_we...
> > File ios/web/web_state/ui/crw_web_controller.mm (right):
> > 
> >
>
https://codereview.chromium.org/2763173002/diff/1/ios/web/web_state/ui/crw_we...
> > ios/web/web_state/ui/crw_web_controller.mm:4218: requestURL, openerURL,
[self
> > userClickedRecently]);
> > This will allow more popups from malicious pages, so I would be very
cautious
> > about this change. CL descriptions says that "window.open code expects that
> user
> > is still interacting with the page", but I don't think that's true.
> > |userIsInteracting| name is just misleading, it's not doing what it says :(
> > 
> > Current code seems to work correctly with window.open and target="_blank"
> links.
> > It does not work correctly with forms. Do you know what is the difference
> > between "form" case vs. "_blank" links f.e?
> 
> I see, you're right this is just the form case. The code paths are in fact
> different, the native code receives the form submission action from javascript
> that the form was submitted. However, for the link _blank navigation, the
> WKWebView delegate notifies the application to create a new child window.
>
> In the form submit case, we know the user interacted, but loose that
information
> by the time the new window is created
I thought |createWebViewWithConfiguration| is called for both form submission
and a link click. Otherwise I don't quite understand how changing
|createWebViewWithConfiguration| could fix the form bug.


> Out of curiosity, would the additional abuse just be that the page has a
> slightly longer time frame after the user interacted? In practice, would the
> time increase make a difference?
With this change WebController will count touches from the previous page. e.g.
user touches a link on foo.com, foo.com navigates to bar.com and now bar.com can
open child windows w/o user interaction.

[michaeldo, 2017-03-23 05:18:31]

On 2017/03/22 23:34:31, Eugene But wrote:
> On 2017/03/22 23:26:26, michaeldo wrote:
> > On 2017/03/22 00:59:39, Eugene But wrote:
> > >
> >
>
https://codereview.chromium.org/2763173002/diff/1/ios/web/web_state/ui/crw_we...
> > > File ios/web/web_state/ui/crw_web_controller.mm (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/2763173002/diff/1/ios/web/web_state/ui/crw_we...
> > > ios/web/web_state/ui/crw_web_controller.mm:4218: requestURL, openerURL,
> [self
> > > userClickedRecently]);
> > > This will allow more popups from malicious pages, so I would be very
> cautious
> > > about this change. CL descriptions says that "window.open code expects
that
> > user
> > > is still interacting with the page", but I don't think that's true.
> > > |userIsInteracting| name is just misleading, it's not doing what it says
:(
> > > 
> > > Current code seems to work correctly with window.open and target="_blank"
> > links.
> > > It does not work correctly with forms. Do you know what is the difference
> > > between "form" case vs. "_blank" links f.e?
> > 
> > I see, you're right this is just the form case. The code paths are in fact
> > different, the native code receives the form submission action from
javascript
> > that the form was submitted. However, for the link _blank navigation, the
> > WKWebView delegate notifies the application to create a new child window.
> >
> > In the form submit case, we know the user interacted, but loose that
> information
> > by the time the new window is created
> I thought |createWebViewWithConfiguration| is called for both form submission
> and a link click. Otherwise I don't quite understand how changing
> |createWebViewWithConfiguration| could fix the form bug.
> 

It is called for both, but from what I recall, it is called almost right away
when clicking the link. However, with the form submission, the form navigation
is registered first, before the new window is created. This is why
|userIsInteracting| is different, because the form submitted load is registered
before the new window is created. I suppose it could be possibly checked if we
are in a form submit state, but I don't know if that logic would be correct. 

> 
> > Out of curiosity, would the additional abuse just be that the page has a
> > slightly longer time frame after the user interacted? In practice, would the
> > time increase make a difference?
> With this change WebController will count touches from the previous page. e.g.
> user touches a link on http://foo.com, http://foo.com navigates to
http://bar.com and now http://bar.com can
> open child windows w/o user interaction.

Got it, thanks for the explanation, that does in fact sound pretty bad, WDYT
about above comment specifically checking for form submit state. That sounds
messier, but it would likely not have as many side effects? I would need to look
into it more to see if it feasible though.

[michaeldo, 2017-03-23 23:52:31]

Description was changed from

==========
Form submission with blank target is broken.

The current window open code expects the user to still be interacting
with the page when a user-triggered popup is displayed. This is usually
not the case when a link is clicked or a form is submitted. Ensure that
the user recently clicked instead of is still interacting with the page
when creating a new tab.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

to

==========
Form submission with blank target is broken.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

[michaeldo, 2017-03-28 18:40:17]

PTAL at the latest patch.

[Eugene But (OOO till 7-30), 2017-03-28 18:47:42]

Could you please update CL description and explain what this change does and why
we don't need this code anymore.

https://codereview.chromium.org/2763173002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (left):

https://codereview.chromium.org/2763173002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:2564:
message->GetBoolean("targetsFrame", &targetsFrame);
Do we also need to remove targetsFrame from JavaScript?

[michaeldo, 2017-03-29 22:45:07]

Description was changed from

==========
Form submission with blank target is broken.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

to

==========
Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the form
post request. However, this is no longer needed with WKWebView's callbacks.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

[michaeldo, 2017-03-29 22:46:44]

Description was changed from

==========
Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the form
post request. However, this is no longer needed with WKWebView's callbacks.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

to

==========
Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the
form post request. However, this is no longer needed with WKWebView's
callbacks.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

[michaeldo, 2017-03-29 22:52:49]

Description was changed from

==========
Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the
form post request. However, this is no longer needed with WKWebView's
callbacks.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

to

==========
Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the
form post request. However, this is no longer needed with WKWebView's
callbacks. This removes the early |registerLoadRequest:referrer:transition:|
call which was added for UIWebView and is no longer needed.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

[michaeldo, 2017-03-29 22:53:32]

Description was changed from

==========
Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the
form post request. However, this is no longer needed with WKWebView's
callbacks. This removes the early |registerLoadRequest:referrer:transition:|
call which was added for UIWebView and is no longer needed.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

to

==========
Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the
form post request. However, this is no longer needed with WKWebView's
callbacks. This removes the early |registerLoadRequest:referrer:transition:|
call which was added for UIWebView and is no longer needed.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

[michaeldo, 2017-03-29 22:53:37]

https://codereview.chromium.org/2763173002/diff/20001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (left):

https://codereview.chromium.org/2763173002/diff/20001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:2564:
message->GetBoolean("targetsFrame", &targetsFrame);
On 2017/03/28 18:47:42, Eugene But wrote:
> Do we also need to remove targetsFrame from JavaScript?

Good call, I removed it and updated description. PTAL.

[Eugene But (OOO till 7-30), 2017-03-30 00:59:03]

lgtm!

https://codereview.chromium.org/2763173002/diff/40001/ios/web/web_state/js/re...
File ios/web/web_state/js/resources/core.js (right):

https://codereview.chromium.org/2763173002/diff/40001/ios/web/web_state/js/re...
ios/web/web_state/js/resources/core.js:558: var action =
evt.target.getAttribute('action');
Optional nit:

JavaScript Style Guide encourages to have all ivars in the top of the function
(because hoisting moves them to the top anyways). So if you don't mind cleaning
up then, this code could be like this:

document.addEventListener('submit', function(evt) {
  var action;
  if (evt['defaultPrevented'])
    return;
  action = evt.target.getAttribute('action');

[michaeldo, 2017-03-30 06:43:28]

https://codereview.chromium.org/2763173002/diff/40001/ios/web/web_state/js/re...
File ios/web/web_state/js/resources/core.js (right):

https://codereview.chromium.org/2763173002/diff/40001/ios/web/web_state/js/re...
ios/web/web_state/js/resources/core.js:558: var action =
evt.target.getAttribute('action');
On 2017/03/30 00:59:03, Eugene But wrote:
> Optional nit:
> 
> JavaScript Style Guide encourages to have all ivars in the top of the function
> (because hoisting moves them to the top anyways). So if you don't mind
cleaning
> up then, this code could be like this:
> 
> document.addEventListener('submit', function(evt) {
>   var action;
>   if (evt['defaultPrevented'])
>     return;
>   action = evt.target.getAttribute('action');

Done.

[michaeldo, 2017-03-30 06:44:52]

The CQ bit was checked by michaeldo@chromium.org

[michaeldo, 2017-03-30 06:44:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2763173002/#ps60001
(title: "Fix JS formatting.")

[commit-bot: I haz the power, 2017-03-30 06:45:08]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 07:36:27]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490856292437830,
"parent_rev": "1b742b13fa036f310eb294d0b3a3f5889bd78787", "commit_rev":
"9736d7d0d0c9999c239e43486a901b1f4a61cbdd"}

[commit-bot: I haz the power, 2017-03-30 07:37:18]

Description was changed from

==========
Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the
form post request. However, this is no longer needed with WKWebView's
callbacks. This removes the early |registerLoadRequest:referrer:transition:|
call which was added for UIWebView and is no longer needed.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755
==========

to

==========
Form submission with blank target is broken.

The UIWebView's implementation required a call to register the load of the
form post request. However, this is no longer needed with WKWebView's
callbacks. This removes the early |registerLoadRequest:referrer:transition:|
call which was added for UIWebView and is no longer needed.

TEST=Test that popups are blocked when expected, and shown when the user
initiates them. I used popuptest.com to validate and there is also a
link at the bottom of the page to “good” user-initiated popups that
should not be blocked. The results looked good to me after this change.
BUG=702755

Review-Url: https://codereview.chromium.org/2763173002
Cr-Commit-Position: refs/heads/master@{#460688}
Committed:
https://chromium.googlesource.com/chromium/src/+/9736d7d0d0c9999c239e43486a90...
==========

[commit-bot: I haz the power, 2017-03-30 07:37:19]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/9736d7d0d0c9999c239e43486a90...