New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 599597 link

Starred by 2 users

Issue metadata

Status: Started
Owner:
Buried. Ping if important.
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 400674



Sign in to add a comment

CREDENTIAL: Refactor the Fetch integration to match the spec.

Project Member Reported by mkwst@chromium.org, Mar 31 2016

Issue description

Project Member

Comment 1 by bugdroid1@chromium.org, Apr 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5dc4a153d1ddc9b7083427fe2705cd1199795038

commit 5dc4a153d1ddc9b7083427fe2705cd1199795038
Author: mkwst <mkwst@chromium.org>
Date: Wed Apr 06 07:07:45 2016

CREDENTIAL: Introduce an "attached credential" to requests.

The spec has been updated after some discussion with Mozilla about the
interaction between PasswordCredential objects and Service Workers.
Details are at [1], the TL;DR is that we'll now pass PasswordCredential
objects in as the RequestInit dictionary 'credentials' member. This will
produce a Request object whose visible 'credentials' attribute is
'password', and which carries a hidden PasswordCredential around for
serialization when we hit the network.

The current patch does the first part of the work, refactoring the
integration between modules/credentialmanager and modules/fetch. It
stops when the WebURLRequest is handed over to //content for processing.
Currently, this means that the credentials are visible in the Request
body that's presented to Service Workers. This isn't where we want to
end up, but it maintains the status quo while presenting developers with
the new interface, so it seems like a good place for this first patch to
stop.

[1]: https://w3c.github.io/webappsec-credential-management/#fetch-integration

BUG=599597

Review URL: https://codereview.chromium.org/1844053003

Cr-Commit-Position: refs/heads/master@{#385390}

[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/content/child/web_url_request_util.cc
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/content/common/service_worker/service_worker_types.h
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
[add] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-redirect.html
[add] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/LayoutTests/http/tests/credentialmanager/resources/redirect-to-echo-post.php
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/bindings/modules/v8/DictionaryHelperForModules.cpp
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/modules/fetch/Body.cpp
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/modules/fetch/Body.h
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/modules/fetch/FetchManager.cpp
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/modules/fetch/FetchRequestData.cpp
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/modules/fetch/FetchRequestData.h
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/modules/fetch/Request.cpp
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/modules/fetch/Request.h
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/modules/fetch/RequestInit.cpp
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/modules/fetch/RequestInit.h
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/platform/exported/WebURLRequest.cpp
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/platform/network/ResourceRequest.cpp
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/Source/platform/network/ResourceRequest.h
[modify] https://crrev.com/5dc4a153d1ddc9b7083427fe2705cd1199795038/third_party/WebKit/public/platform/WebURLRequest.h

Project Member

Comment 2 by bugdroid1@chromium.org, Apr 7 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/93a08a4b973b9cca6f7f3d182d9a13102755da2b

commit 93a08a4b973b9cca6f7f3d182d9a13102755da2b
Author: mkwst <mkwst@chromium.org>
Date: Thu Apr 07 11:57:12 2016

Fetch: Fix 'body' processing in RequestInit.

https://codereview.chromium.org/1844053003 broke 'RequestInit's ability
to process a body on a request with 'credentials' set because it was
bypassing 'body' processing if any 'credentials' property was set.

It _should_ have been bypassing 'body' processing if and only if the
'credentials' attribute contained a 'PasswordCredential' object.

This patch fixes the broken comparison.

BUG=599597

Review URL: https://codereview.chromium.org/1862293003

Cr-Commit-Position: refs/heads/master@{#385716}

[modify] https://crrev.com/93a08a4b973b9cca6f7f3d182d9a13102755da2b/third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
[modify] https://crrev.com/93a08a4b973b9cca6f7f3d182d9a13102755da2b/third_party/WebKit/LayoutTests/http/tests/fetch/script-tests/request.js
[modify] https://crrev.com/93a08a4b973b9cca6f7f3d182d9a13102755da2b/third_party/WebKit/Source/modules/fetch/RequestInit.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Apr 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a40efde9a68ead4d71421854fda2409e1cc7b053

commit a40efde9a68ead4d71421854fda2409e1cc7b053
Author: mkwst <mkwst@chromium.org>
Date: Fri Apr 08 21:51:55 2016

Fetch: 'password' credentials mode should include credentials.

I am not going to admit how long it took me to find this bug. Because it
was a long time and involved me installing Fiddler to verify that
chrome://net-internals wasn't lying to me because OBVIOUSLY the data
wasn't being posted to the server even though net-internals said it was
and ugh.

'password' should act like 'include' (see #2 in [1]). We should include
cookies in either mode. *sigh*

[1]: https://w3c.github.io/webappsec-credential-management/#monkey-patching

BUG=599597, 601923 
R=horo@chromium.org, estark@chromium.org

Review URL: https://codereview.chromium.org/1868253002

Cr-Commit-Position: refs/heads/master@{#386212}

[modify] https://crrev.com/a40efde9a68ead4d71421854fda2409e1cc7b053/third_party/WebKit/LayoutTests/http/tests/credentialmanager/passwordcredential-fetch.html
[add] https://crrev.com/a40efde9a68ead4d71421854fda2409e1cc7b053/third_party/WebKit/LayoutTests/http/tests/credentialmanager/resources/echo-cookies.php
[modify] https://crrev.com/a40efde9a68ead4d71421854fda2409e1cc7b053/third_party/WebKit/Source/modules/fetch/FetchManager.cpp

Comment 4 by rbyers@chromium.org, Nov 18 2016

Components: Blink>SecurityFeature

Comment 5 by stillers@google.com, Feb 15 2017

Demo of retrieving the credentials client-side (because request body is visible to the SW):

https://addicted-perfume.gomix.me/

(Probably need to be signed into Chrome for this to work.)

Comment 6 by est...@chromium.org, Nov 10 2017

Labels: Hotlist-EnamelAndFriendsFixIt

Comment 7 by est...@chromium.org, Feb 18 2018

Labels: -Hotlist-EnamelAndFriendsFixIt

Sign in to add a comment