CREDENTIAL: Credentials should be submitted within a registrable domain.

CREDENTIAL: Credentials should be submitted within a registrable domain.

The current code checks for an exact origin match when creating a
Request. That doesn't match the specification; see step 3.1 of
https://w3c.github.io/webappsec-credential-management/#body-extraction.

BUG=606788
Committed: https://crrev.com/6036e62ac8638c3cdfd76db5dfe44ca05c62f682
Cr-Commit-Position: refs/heads/master@{#389858}

[Mike West, 2016-04-26 15:05:25]

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

[Mike West, 2016-04-26 15:05:25]

Vaclav, mind taking a look at this?

[vabr (Chromium), 2016-04-26 16:49:19]

Thanks, Mike, this LGTM with comments.

To be honest, I'm not sure I understand the tests completely, e.g., the initial
change of the host, or that all the URLs are HTTP, while the CM API is supposed
to only work on HTTPS (the check is done higher than in Blink I suppose?). But
you are the expert there so I trust you, and I'll grab you this week to chat
about CM API anyway, so you can explain to me there. :)

Cheers,
Vaclav

https://codereview.chromium.org/1918253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1918253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Request.cpp:330: const
OriginAccessEntry& accessEntry = OriginAccessEntry(r->url().protocol(),
r->url().host(), OriginAccessEntry::AllowRegisterableDomains);
This should be just const OriginAccessEntry, not a reference.

https://codereview.chromium.org/1918253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Request.cpp:332:
exceptionState.throwTypeError("Credentials may only be submitted to endpoints on
the same registrable domain.");
nit: The spelling in the spec is with "e" between "t" and "r": registerable.

https://w3c.github.io/webappsec-credential-management/#body-extraction

[Mike West, 2016-04-26 18:14:15]

On 2016/04/26 at 16:49:19, vabr wrote:
> Thanks, Mike, this LGTM with comments.
> 
> To be honest, I'm not sure I understand the tests
> completely, e.g., the initial change of the host

Blink's layout tests are loaded from `http://127.0.0.1:8000`. IP addresses are
not registrable domains; we resolve `*.test` to 127.0.0.1 for this purpose.
Navigating to `subdomain.example.test` loads the same test file from that
origin, which means we can check variants of that origin to see if we're allowed
to make the request. 

> or that all the URLs are HTTP

I'm being sneaky here. The `navigator.credentials.*` bits of the API are locked
to HTTPS. `PasswordCredential` objects are not. I'm not sure there's a good
security reason to hide the object on nonsecure origins, as without
`navigator.credentials.*`, you can't actually do much of anything with them
(except create them, and submit a form in a much more complicated way than you
could do by just submitting the form...).

> But you are the expert there so I trust you

AND THAT WAS YOUR FIRST MISTAKE MUWAHAHAHA. *cough*

> I'll grab you this week to chat about CM API anyway, so you can explain to me
there. :)

My calendar is your calendar.

https://codereview.chromium.org/1918253002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/modules/fetch/Request.cpp (right):

https://codereview.chromium.org/1918253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Request.cpp:330: const
OriginAccessEntry& accessEntry = OriginAccessEntry(r->url().protocol(),
r->url().host(), OriginAccessEntry::AllowRegisterableDomains);
On 2016/04/26 at 16:49:18, vabr (Chromium) wrote:
> This should be just const OriginAccessEntry, not a reference.

Indeed, thanks!

https://codereview.chromium.org/1918253002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/modules/fetch/Request.cpp:332:
exceptionState.throwTypeError("Credentials may only be submitted to endpoints on
the same registrable domain.");
On 2016/04/26 at 16:49:19, vabr (Chromium) wrote:
> nit: The spelling in the spec is with "e" between "t" and "r": registerable.
> 
> https://w3c.github.io/webappsec-credential-management/#body-extraction

Ha! That's a typo in the spec, thanks (see https://publicsuffix.org/list/ (and
http://www.merriam-webster.com/dictionary/registrable)).

[vabr (Chromium), 2016-04-26 18:19:46]

Thanks, Mike, for the explanation of the tests, it's much more clearer now!

Also, happy to repeat that first mistake of mine. :)


Still LGTM (provided you will upload the patch removing the "&" before landing).


See you soon,
Vaclav

[Mike West, 2016-04-26 18:22:46]

The CQ bit was checked by mkwst@chromium.org

[Mike West, 2016-04-26 18:22:47]

The patchset sent to the CQ was uploaded after l-g-t-m from vabr@chromium.org
Link to the patchset: https://codereview.chromium.org/1918253002/#ps40001
(title: "vabr@")

[commit-bot: I haz the power, 2016-04-26 18:23:27]

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

[commit-bot: I haz the power, 2016-04-26 19:52:02]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-26 19:53:59]

Description was changed from

==========
CREDENTIAL: Credentials should be submitted within a registrable domain.

The current code checks for an exact origin match when creating a
Request. That doesn't match the specification; see step 3.1 of
https://w3c.github.io/webappsec-credential-management/#body-extraction.

BUG=606788
==========

to

==========
CREDENTIAL: Credentials should be submitted within a registrable domain.

The current code checks for an exact origin match when creating a
Request. That doesn't match the specification; see step 3.1 of
https://w3c.github.io/webappsec-credential-management/#body-extraction.

BUG=606788

Committed: https://crrev.com/6036e62ac8638c3cdfd76db5dfe44ca05c62f682
Cr-Commit-Position: refs/heads/master@{#389858}
==========

[commit-bot: I haz the power, 2016-04-26 19:54:00]

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