Check for equality of the URL's origin in replaceState/pushState

Check for equality of the URL's origin in replaceState/pushState

history.pushState/replaceState should not change the origin part of the
URL (the whole URL minus the path, query and reference fragment). The
previous check failed to do this because canAccess respects whitelists
(such as those for extensions) and allows same-origin URLs (such as
blob:).

BUG=447414
Committed: https://crrev.com/587195f6886817f7b634bf58eafa7f900a9b6532
Cr-Commit-Position: refs/heads/master@{#364140}

[robwu, 2015-12-06 17:54:28]

rob@robwu.nl changed reviewers:
+ brettw@chromium.org

[brettw, 2015-12-08 05:41:32]

brettw@chromium.org changed reviewers:
+ mkwst@chromium.org

[brettw, 2015-12-08 05:41:32]

+mkwst who looks like he's done the most work on SecurityOrigin. I'm not
actually super familiar with this code.

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/KURL.cpp (right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:734: // FIXME: Abstraction
this into a function in WTFString.h.
I would remove this FIXME, I don't think this should be in WTFString. If you
want to re-use code, make a StringView with the prefix and check equality.

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:377: if
(!equalIgnoringPathQueryAndFragment(a, b))
Personally, I would prefer manually checking scheme/host/port for equality
rather than adding this new function on KURL that's only called once. Would be
interested in Mike's opinion.

[robwu, 2015-12-08 08:45:17]

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/KURL.cpp (right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/KURL.cpp:734: // FIXME: Abstraction
this into a function in WTFString.h.
On 2015/12/08 05:41:32, brettw wrote:
> I would remove this FIXME, I don't think this should be in WTFString. If you
> want to re-use code, make a StringView with the prefix and check equality.

Then it also applies to the function above. In either case, I try to be
consistent.

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:377: if
(!equalIgnoringPathQueryAndFragment(a, b))
On 2015/12/08 05:41:32, brettw wrote:
> Personally, I would prefer manually checking scheme/host/port for equality
> rather than adding this new function on KURL that's only called once. Would be
> interested in Mike's opinion.

Checking for scheme/host/port equality is not sufficient, username/password
should also be ignored. To avoid hard-coding the meaning "URL minus path, query
and fragment" = "protocol, username, password, host and port", I've put the
helper method in KURL (so that others can also rely on it if wanted).

[Mike West, 2015-12-08 13:45:19]

Thanks for taking a stab at this! I have a few comments inline.

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.html
(right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.html:2:
<script>
Please write new tests with `testharness.js` (and consider upstreaming the
non-Chrome-specific bits to
https://github.com/w3c/web-platform-tests/tree/master/html/browsers/history/t...
if the tests there aren't sufficient).

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-in-blob-denied.html
(right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-in-blob-denied.html:1:
<script>
Please write new tests with `testharness.js` (and consider upstreaming them to
https://github.com/w3c/web-platform-tests/tree/master/html/browsers/history/t...
if the tests there aren't sufficient).

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/History.cpp (right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/History.cpp:172: if (!fullURL.isValid() ||
!document->securityOrigin()->areSamePageUrls(fullURL, document->url())) {
If we need a special case for `pushState`/`replaceState`, I'd prefer to put it
here, rather than special-casing things in `SecurityOrigin` and `KURL`.

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:377: if
(!equalIgnoringPathQueryAndFragment(a, b))
On 2015/12/08 at 08:45:17, robwu wrote:
> On 2015/12/08 05:41:32, brettw wrote:
> > Personally, I would prefer manually checking scheme/host/port for equality
> > rather than adding this new function on KURL that's only called once. Would
be
> > interested in Mike's opinion.
> 
> Checking for scheme/host/port equality is not sufficient, username/password
should also be ignored. To avoid hard-coding the meaning "URL minus path, query
and fragment" = "protocol, username, password, host and port", I've put the
helper method in KURL (so that others can also rely on it if wanted).

This is defined somewhat strangely in HTML as "If any component of these two
URLs differ other than the path, query, and fragment components, then throw a
SecurityError exception and abort these steps."

This is fairly explicitly not an origin check; it's checking properties of the
page's URLs. To that end, I'd suggest that we should avoid putting the code in
'SecurityOrigin'. I spot-checked Firefox, and it agrees with the
username/password bits.

Given that this is the only place in the codebase that we need this kind of
check (and, given it's weirdness, I'd prefer not to encourage others to reuse
it), I'd suggest packing up the check in an anon. namepsace of `History.cpp`.

[robwu, 2015-12-08 14:06:39]

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.html
(right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.html:2:
<script>
On 2015/12/08 13:45:18, Mike West wrote:
> Please write new tests with `testharness.js` (and consider upstreaming the
> non-Chrome-specific bits to
>
https://github.com/w3c/web-platform-tests/tree/master/html/browsers/history/t...
> if the tests there aren't sufficient).

I believe that the test is not suited for the test harness, because the imported
tests run at file:// with universal access. This is also the reason why this
expectation failed:
https://chromium.googlesource.com/chromium/src/+/1eb37c3ac47c16d2b463879a2402...

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-in-blob-denied.html
(right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-in-blob-denied.html:1:
<script>
On 2015/12/08 13:45:19, Mike West wrote:
> Please write new tests with `testharness.js` (and consider upstreaming them to
>
https://github.com/w3c/web-platform-tests/tree/master/html/browsers/history/t...
> if the tests there aren't sufficient).

All -whitelisted- tests that I introduced here interface closely with Blink's
implementation, and assert that Chromium/Blink behaves properly even with
specific whitelists (that's why I'm using testRunner). Is the current test fine,
or do I still need to change to testharness.js?

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/History.cpp (right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/History.cpp:172: if (!fullURL.isValid() ||
!document->securityOrigin()->areSamePageUrls(fullURL, document->url())) {
On 2015/12/08 13:45:19, Mike West wrote:
> If we need a special case for `pushState`/`replaceState`, I'd prefer to put it
> here, rather than special-casing things in `SecurityOrigin` and `KURL`.

Will do.

[robwu, 2015-12-08 14:10:39]

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:377: if
(!equalIgnoringPathQueryAndFragment(a, b))
On 2015/12/08 13:45:19, Mike West wrote:
> On 2015/12/08 at 08:45:17, robwu wrote:
> > On 2015/12/08 05:41:32, brettw wrote:
> > > Personally, I would prefer manually checking scheme/host/port for equality
> > > rather than adding this new function on KURL that's only called once.
Would
> be
> > > interested in Mike's opinion.
> > 
> > Checking for scheme/host/port equality is not sufficient, username/password
> should also be ignored. To avoid hard-coding the meaning "URL minus path,
query
> and fragment" = "protocol, username, password, host and port", I've put the
> helper method in KURL (so that others can also rely on it if wanted).
> 
> This is defined somewhat strangely in HTML as "If any component of these two
> URLs differ other than the path, query, and fragment components, then throw a
> SecurityError exception and abort these steps."
> 
> This is fairly explicitly not an origin check; it's checking properties of the
> page's URLs. To that end, I'd suggest that we should avoid putting the code in
> 'SecurityOrigin'. I spot-checked Firefox, and it agrees with the
> username/password bits.
> 
> Given that this is the only place in the codebase that we need this kind of
> check (and, given it's weirdness, I'd prefer not to encourage others to reuse
> it), I'd suggest packing up the check in an anon. namepsace of `History.cpp`.

I presume that you're fine with exposing the value of m_universalAccess through
a getter on SecurityOrigin, because this was one of the reasons why I put this
implementation in SecurityOrigin.cpp?

[Mike West, 2015-12-08 14:18:41]

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.html
(right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-at-unique-origin-denied.html:2:
<script>
On 2015/12/08 at 14:06:39, robwu wrote:
> I believe that the test is not suited for the test harness, because the
imported tests run at file:// with universal access. This is also the reason why
this expectation failed:
https://chromium.googlesource.com/chromium/src/+/1eb37c3ac47c16d2b463879a2402...

That's a bug in the way that we're running those tests. We ought to be running
them through `wptserve`, but that isn't wired up to the test runner yet. If you
execute the tests from http://w3c-test.org/tools/runner/index.html, you'll see
more reasonable results. There's value to the ecosystem in upstreaming these
tests, even if we're doing a bad job as a project at running them in an
automated fashion.

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-in-blob-denied.html
(right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/navigation/pushstate-whitelisted-in-blob-denied.html:1:
<script>
On 2015/12/08 at 14:06:39, robwu wrote:
> On 2015/12/08 13:45:19, Mike West wrote:
> > Please write new tests with `testharness.js` (and consider upstreaming them
to
> >
https://github.com/w3c/web-platform-tests/tree/master/html/browsers/history/t...
> > if the tests there aren't sufficient).
> 
> All -whitelisted- tests that I introduced here interface closely with Blink's
implementation, and assert that Chromium/Blink behaves properly even with
specific whitelists (that's why I'm using testRunner). Is the current test fine,
or do I still need to change to testharness.js?

`testharness.js` tests have the lovely property of not requiring `-expected.txt`
files. Even if that was the only difference, I think it would still be worth
migrating to that system. :)

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp (right):

https://codereview.chromium.org/1495013002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SecurityOrigin.cpp:377: if
(!equalIgnoringPathQueryAndFragment(a, b))
On 2015/12/08 at 14:10:39, robwu wrote:
> I presume that you're fine with exposing the value of m_universalAccess
through a getter on SecurityOrigin, because this was one of the reasons why I
put this implementation in SecurityOrigin.cpp?

I would dearly love to murder that property entirely. Until that happens,
exposing it as a getter isn't a problem. :)

[robwu, 2015-12-08 16:33:48]

Mike: I've moved all logic to History.cpp and changed all but one test to
testharness. PTAL.

[Mike West, 2015-12-08 17:34:59]

LGTM. Thanks for taking another pass. :)

https://codereview.chromium.org/1495013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/History.cpp (right):

https://codereview.chromium.org/1495013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/History.cpp:188: if (!url.isValid())
Nit: Can you assert that you have `m_frame` and `m_frame->document()`?

[robwu, 2015-12-08 20:31:48]

https://codereview.chromium.org/1495013002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/History.cpp (right):

https://codereview.chromium.org/1495013002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/History.cpp:188: if (!url.isValid())
On 2015/12/08 17:34:59, Mike West wrote:
> Nit: Can you assert that you have `m_frame` and `m_frame->document()`?

This is already implied by the existing code. `m_frame` is checked at the start
of History::stateObjectAdded, and `m_frame->document()` is used in
History::urlFor. The assert would be a bit too paranoid / pointless.

[robwu, 2015-12-09 16:12:41]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2015-12-09 16:14:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1495013002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1495013002/40001

[commit-bot: I haz the power, 2015-12-09 18:19:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 18:19:01]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[robwu, 2015-12-09 18:33:20]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2015-12-09 18:36:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1495013002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1495013002/40001

[commit-bot: I haz the power, 2015-12-09 20:11:05]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-12-09 20:12:16]

Description was changed from

==========
Check for equality of the URL's origin in replaceState/pushState

history.pushState/replaceState should not change the origin part of the
URL (the whole URL minus the path, query and reference fragment). The
previous check failed to do this because canAccess respects whitelists
(such as those for extensions) and allows same-origin URLs (such as
blob:).

BUG=447414
==========

to

==========
Check for equality of the URL's origin in replaceState/pushState

history.pushState/replaceState should not change the origin part of the
URL (the whole URL minus the path, query and reference fragment). The
previous check failed to do this because canAccess respects whitelists
(such as those for extensions) and allows same-origin URLs (such as
blob:).

BUG=447414

Committed: https://crrev.com/587195f6886817f7b634bf58eafa7f900a9b6532
Cr-Commit-Position: refs/heads/master@{#364140}
==========

[commit-bot: I haz the power, 2015-12-09 20:12:17]

Patchset 3 (id:??) landed as
https://crrev.com/587195f6886817f7b634bf58eafa7f900a9b6532
Cr-Commit-Position: refs/heads/master@{#364140}