Add domain checking to returned HTTPResponses

Add domain checking to returned HTTPResponses

I discovered today that Chrome will occasionally include a page loaded
at startup with a url like 'data:,' or 'about:blank'. This code will
remove those responses by default and shouldn't affect standard testing
use cases.

BUG=669956
R=sclittle

Committed: https://crrev.com/85e300db3062e709af455204125e42cce550f1c7
Cr-Commit-Position: refs/heads/master@{#439608}

[sclittle, 2016-12-19 19:50:09]

https://codereview.chromium.org/2572383003/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/common.py (right):

https://codereview.chromium.org/2572383003/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:362: only_include_loaded_domain=True):
I'm not sure this is the best way to solve the problem of data:// or about:blank
URLs being requested. Could you just filter out all non-http/https URLs by
default, or are there other URLs from other domains that pose problems?

[Robert Ogden, 2016-12-19 20:23:10]

https://codereview.chromium.org/2572383003/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/common.py (right):

https://codereview.chromium.org/2572383003/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:362: only_include_loaded_domain=True):
On 2016/12/19 19:50:09, sclittle wrote:
> I'm not sure this is the best way to solve the problem of data:// or
about:blank
> URLs being requested. Could you just filter out all non-http/https URLs by
> default, or are there other URLs from other domains that pose problems?

Filtering on presence of the scheme (http,https) would work. The idea behind
domains is not a concrete one, just a thought/fear of unexpected behavior for
test sites that use cross-origin requests.

[sclittle, 2016-12-19 21:17:08]

https://codereview.chromium.org/2572383003/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/common.py (right):

https://codereview.chromium.org/2572383003/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:362: only_include_loaded_domain=True):
On 2016/12/19 20:23:10, Robert Ogden wrote:
> On 2016/12/19 19:50:09, sclittle wrote:
> > I'm not sure this is the best way to solve the problem of data:// or
> about:blank
> > URLs being requested. Could you just filter out all non-http/https URLs by
> > default, or are there other URLs from other domains that pose problems?
> 
> Filtering on presence of the scheme (http,https) would work. The idea behind
> domains is not a concrete one, just a thought/fear of unexpected behavior for
> test sites that use cross-origin requests.

Hmm - I think I see what you mean, but I'm worried that ignoring the
cross-origin requests could cause more problems than it solves. E.g., suppose
there's an integration test to check if bypass works. If the request that caused
the bypass gets ignored here because it was cross origin, it'll be harder to
debug.

Plus, hopefully it shouldn't be too hard to have our test pages only make
first-party requests. If they do make cross-origin requests, then that's
probably on purpose and we likely don't want to ignore those requests anyways.

WDYT?

[Robert Ogden, 2016-12-19 22:17:46]

https://codereview.chromium.org/2572383003/diff/1/tools/chrome_proxy/webdrive...
File tools/chrome_proxy/webdriver/common.py (right):

https://codereview.chromium.org/2572383003/diff/1/tools/chrome_proxy/webdrive...
tools/chrome_proxy/webdriver/common.py:362: only_include_loaded_domain=True):
On 2016/12/19 21:17:08, sclittle wrote:
> On 2016/12/19 20:23:10, Robert Ogden wrote:
> > On 2016/12/19 19:50:09, sclittle wrote:
> > > I'm not sure this is the best way to solve the problem of data:// or
> > about:blank
> > > URLs being requested. Could you just filter out all non-http/https URLs by
> > > default, or are there other URLs from other domains that pose problems?
> > 
> > Filtering on presence of the scheme (http,https) would work. The idea behind
> > domains is not a concrete one, just a thought/fear of unexpected behavior
for
> > test sites that use cross-origin requests.
> 
> Hmm - I think I see what you mean, but I'm worried that ignoring the
> cross-origin requests could cause more problems than it solves. E.g., suppose
> there's an integration test to check if bypass works. If the request that
caused
> the bypass gets ignored here because it was cross origin, it'll be harder to
> debug.
> 
> Plus, hopefully it shouldn't be too hard to have our test pages only make
> first-party requests. If they do make cross-origin requests, then that's
> probably on purpose and we likely don't want to ignore those requests anyways.
> 
> WDYT?

SGTM. After looking through the pydoc, I ended up checking the presence of a
net_loc (or domain as I know it) instead of a scheme, since scheme='data' where
url='data:,,' which has been happening a lot recently.

[sclittle, 2016-12-19 22:38:15]

https://codereview.chromium.org/2572383003/diff/40001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/common.py (right):

https://codereview.chromium.org/2572383003/diff/40001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/common.py:265: self._logger.warn('No net_loc given
in %s. See RFC 1808', url)
nit: could you make this message a bit clearer? e.g., if an invalid URL was
passed in, you could check this and mention this specifically.

You could also only log when the URL is invalid, since it would be nice to
support calling LoadURL with an encoded "data:" URI.

[Robert Ogden, 2016-12-19 22:56:44]

https://codereview.chromium.org/2572383003/diff/40001/tools/chrome_proxy/webd...
File tools/chrome_proxy/webdriver/common.py (right):

https://codereview.chromium.org/2572383003/diff/40001/tools/chrome_proxy/webd...
tools/chrome_proxy/webdriver/common.py:265: self._logger.warn('No net_loc given
in %s. See RFC 1808', url)
On 2016/12/19 22:38:15, sclittle wrote:
> nit: could you make this message a bit clearer? e.g., if an invalid URL was
> passed in, you could check this and mention this specifically.
> 
> You could also only log when the URL is invalid, since it would be nice to
> support calling LoadURL with an encoded "data:" URI.

Done.

[sclittle, 2016-12-19 22:57:33]

lgtm

[Robert Ogden, 2016-12-19 23:01:56]

The CQ bit was checked by robertogden@chromium.org

[commit-bot: I haz the power, 2016-12-19 23:02:59]

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, 2016-12-19 23:17:53]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482188516768540,
"parent_rev": "3f69d436c5461a76f1b6f620654f475db245fe5a", "commit_rev":
"05b82a2f97b951d75a0c13d00d8ed31182a81933"}

[commit-bot: I haz the power, 2016-12-19 23:18:22]

Description was changed from

==========
Add domain checking to returned HTTPResponses

I discovered today that Chrome will occasionally include a page loaded
at startup with a url like 'data:,' or 'about:blank'. This code will
remove those responses by default and shouldn't affect standard testing
use cases.

BUG=669956
R=sclittle
==========

to

==========
Add domain checking to returned HTTPResponses

I discovered today that Chrome will occasionally include a page loaded
at startup with a url like 'data:,' or 'about:blank'. This code will
remove those responses by default and shouldn't affect standard testing
use cases.

BUG=669956
R=sclittle

Review-Url: https://codereview.chromium.org/2572383003
==========

[commit-bot: I haz the power, 2016-12-19 23:18:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-19 23:20:39]

Description was changed from

==========
Add domain checking to returned HTTPResponses

I discovered today that Chrome will occasionally include a page loaded
at startup with a url like 'data:,' or 'about:blank'. This code will
remove those responses by default and shouldn't affect standard testing
use cases.

BUG=669956
R=sclittle

Review-Url: https://codereview.chromium.org/2572383003
==========

to

==========
Add domain checking to returned HTTPResponses

I discovered today that Chrome will occasionally include a page loaded
at startup with a url like 'data:,' or 'about:blank'. This code will
remove those responses by default and shouldn't affect standard testing
use cases.

BUG=669956
R=sclittle

Committed: https://crrev.com/85e300db3062e709af455204125e42cce550f1c7
Cr-Commit-Position: refs/heads/master@{#439608}
==========

[commit-bot: I haz the power, 2016-12-19 23:20:39]

Patchset 4 (id:??) landed as
https://crrev.com/85e300db3062e709af455204125e42cce550f1c7
Cr-Commit-Position: refs/heads/master@{#439608}