New issue
Advanced search Search tips
Starred by 5 users
Status: Fixed
Owner:
Closed: Jun 29
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 594639
issue 710837



Sign in to add a comment
Module scripts should not be requested with credentials
Project Member Reported by jakearchibald@chromium.org, May 2 2017 Back to list
https://module-script-tests-zoelmqooyv.now.sh/cookie-page

By default, module scripts should not be requested with credentials. The output of the above page should be:

No random number cookie found.
Random number cookie is: 0.30567383219441924
Random number cookie is: 0.30567383219441924

(Obviously your random number will be different).
 
Comment 1 by addyo@chromium.org, May 2 2017
Cc: adamk@chromium.org kouhei@chromium.org addyo@chromium.org
Comment 2 by adamk@chromium.org, May 2 2017
Blocking: 594639
Labels: -Pri-3 Pri-2
Status: Available
Cc: module-dev@chromium.org
Owner: hirosh...@chromium.org
Tentatively assigning this to hiroshige@, as I'm technically currently on vacation.
Status: Started
Cc: tyoshino@chromium.org
Components: Blink>Loader
Step 16 of prepare script sets module script credentials to "omit"
(correct in ScriptLoader) but in ModuleScriptLoader::Fetch() we set
CrossOriginAttributeValue cross_origin to kCrossOriginAttributeAnonymous,
for which "their credentials mode set to "same-origin"."
https://html.spec.whatwg.org/#attr-crossorigin-anonymous

I think we should call SetCrossOriginAccessControl() or something to enable
CORS while omitting credentials, but currently SetCrossOriginAccessControl() does not support credentials mode "omit":
According to spec
https://fetch.spec.whatwg.org/#http-network-or-cache-fetch
Step 4:
> Let credentials flag be set if one of
> request’s credentials mode is "include"
> request’s credentials mode is "same-origin" and request’s response tainting is "basic"
Implementation:
>  if (is_same_origin_request || use_credentials) {
>    options_.allow_credentials = kAllowStoredCredentials;
>    resource_request_.SetAllowStoredCredentials(true);

tyoshino@, what do you think?

(Next update will be next week as Loader/Network API teams on Tokyo are on public holidays this week)
Due to this bug, currently <script type=module> issues two network requests for every same-origin module script. First HTMLPreloadScanner requests it without credentials, and then ScriptLoader requests it again with credentials.

If this bug isn't easy to fix, we might want to disable preload for module scripts for now.

Comment 8 by kouhei@chromium.org, May 10 2017
disabling preload for now sgtm
Comment 9 by kouhei@chromium.org, May 10 2017
Components: -Blink>HTML>Modules
Blocking: 710837
Project Member Comment 11 by bugdroid1@chromium.org, May 11 2017
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/32a2d84e17b0b63b284f259260accc8dd765d26b

commit 32a2d84e17b0b63b284f259260accc8dd765d26b
Author: ksakamoto <ksakamoto@chromium.org>
Date: Thu May 11 06:54:02 2017

Disable preload for module scripts

This patch disables preload for <script type="module">, to prevent
duplicated requests due to credentials mode mismatch. In the long term,
we should teach Preload Scanner about the module script credentials mode
and re-enable preload.

BUG= 717525 

Review-Url: https://codereview.chromium.org/2871153002
Cr-Commit-Position: refs/heads/master@{#470853}

[modify] https://crrev.com/32a2d84e17b0b63b284f259260accc8dd765d26b/third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp

#5: Yeah, SetCrossOriginAccessControl() doesn't support "omit" now. But I guess it can easily extended to support it. I'll be ooo next week but can help after that.
BTW, is there any document/mail by which I can understand why it's specified to use "omit". I think I checked it before but cannot remember. Just in case some of you know.
Comment 14 by adamk@chromium.org, May 24 2017
In https://github.com/whatwg/html/issues/1888#issuecomment-253011325, Domenic suggests that the "omit" default matches Fetch (not sure if that was the reasoning, though). There's also some more recent discussion of this issue on the spec at https://github.com/whatwg/html/issues/2557
Yes, that was basically the reasoning. I think we should follow fetch; https://github.com/whatwg/html/issues/2557 discusses changing both together, which I would be OK with.
Labels: -Pri-2 Pri-1
Bumping to P1 as this will block modules ship.
I'm working on changing SetCrossOriginAccessControl() to allow omit.
Project Member Comment 19 by bugdroid1@chromium.org, Jun 21
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2e231cf052ca5e68e22baf0008ac9e5e29121707

commit 2e231cf052ca5e68e22baf0008ac9e5e29121707
Author: Takeshi Yoshino <tyoshino@chromium.org>
Date: Wed Jun 21 09:00:30 2017

Replace the credentials related parameters in ResourceLoaderOptions with FetchCredentialsMode

This has been causing lots of pain in working on the loading code.

This patch is for:
- code health
- architecture improvement
- to enable ES6 modules to use the omit credentials mode

ResourceLoaderOptions::cors_enabled will be replaced with
- cors_handling_by_resource_fetcher
- ResourceRequest::FetchRequestMode()
as it's been confusing.

DocumentThreadableLoader will be refactored to have cors_flag_ which
better corresponds to the spec term.

Same-origin-ness checking logics are unified from FetchManager,
EventSource, XMLHttpRequest into DocumentThreadableLoader.

One in FetchParameters::SetCrossOriginAccessControl() will be also
unified into ResourceFetcher/ResourceLoader.

The logic to handle the unsafe-credentials policy of the Suborigin will
be also unified similarly.

This patch fixes the issue that Service Workers get incorrect
FetchCredentialsMode. DocumentThreadableLoader has been passing the
credentials mode based on the determined credentials flag
(ResourceRequest::AllowStoredCredentials()). After this patch, the
right credentials mode will be passed. Due to this fix, several tests
are rebaselined.

WebAssociatedURLLoader users are changed to use the credentials mode
while trying to fix them if possible based on the corresponding specs,
and keep the existing behavior as possible as we can where we cannot
fix them.

The UMA kXMLHttpRequestCrossOriginWithCredentials in XMLHttpRequest is
changed not to take into account the Suborigin policy effect, since to
keep it we'll need to keep the complicated useless logic there.

FileReaderLoader is changed to just use the default FetchCredentialsMode
as it does nothing for Blobs.

PingLoader is fixed to use CanRequestNoSuborigin().

WorkerScriptLoader is kept using the same credentials mode while the
wrong same-origin-ness check will be fixed by just depending on the
DocumentThreadableLoader.

NotificationImageLoader is fixed similarly.

CORS RFC 1918 logic in DocumentThreadableLoader forces it to issue
a preflight for external requests. This was causing same-origin
requests to fail due to the unnecessarily performed CORS check. As a
side effect of this patch, the issue will be fixed. This is why
request-from-popup.html layout test is rebaselined.

ModuleScriptLoader now can use the omit credentials mode.

See the design doc at
https://docs.google.com/document/d/1Iso_WSAUY_KKXWr9X3niEf1eu9Z_o5Gesp_P4xmJRDw/edit#

Bug:  717525 , 427429, 727596,  695808 
Change-Id: I663e40c8385868b6e64fe71248f85f8aff1ae176
Reviewed-on: https://chromium-review.googlesource.com/516925
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Tsuyoshi Horo <horo@chromium.org>
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Reviewed-by: Dale Curtis <dalecurtis@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Bill Budge <bbudge@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Commit-Queue: Takeshi Yoshino <tyoshino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#481152}
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/chrome/browser/chrome_service_worker_browsertest.cc
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/components/nacl/renderer/ppb_nacl_private_impl.cc
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/content/renderer/fetchers/manifest_fetcher.cc
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/content/renderer/fetchers/multi_resolution_image_resource_fetcher.cc
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/content/renderer/media/android/media_info_loader.cc
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/content/renderer/pepper/pepper_url_loader_host.cc
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/extensions/renderer/guest_view/mime_handler_view/mime_handler_view_container.cc
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/media/blink/resource_multibuffer_data_provider.cc
[delete] https://crrev.com/acf467f15ce17fcfcd710b663fb86da82fc54140/third_party/WebKit/LayoutTests/external/wpt/service-workers/service-worker/fetch-cors-xhr.https-expected.txt
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/LayoutTests/http/tests/serviceworker/resources/fetch-request-xhr-iframe.html
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/request-from-popup-expected.txt
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/css/CSSFontFaceSrcValue.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/css/CSSImageSetValue.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/css/CSSImageValue.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/css/StyleRuleImport.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/dom/ProcessingInstruction.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/dom/ScriptLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/exported/WebAssociatedURLLoaderImpl.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/exported/WebAssociatedURLLoaderImplTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/exported/WebSharedWorkerImpl.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/fileapi/FileReaderLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/html/LinkStyle.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/html/imports/LinkImport.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/html/parser/PreloadRequest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/inspector/InspectorResourceContentLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/BaseFetchContextTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.h
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/FrameFetchContextTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/LinkLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/PingLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/TextTrackLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/ThreadableLoaderTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/WorkletScriptLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/modulescript/ModuleScriptLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/private/CrossOriginPreflightResultCache.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/private/CrossOriginPreflightResultCache.h
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/resource/CSSStyleSheetResource.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/resource/ImageResource.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/resource/ImageResourceTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/loader/resource/ScriptResource.h
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/svg/SVGElementProxy.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/svg/SVGFEImageElement.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/svg/SVGUseElement.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/workers/InProcessWorkerBase.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/workers/WorkerGlobalScope.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/workers/WorkerScriptLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/workers/WorkerScriptLoader.h
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/xml/XSLImportRule.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/xml/XSLTProcessorLibxslt.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/xml/parser/XMLDocumentParser.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/modules/eventsource/EventSource.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/modules/exported/WebEmbeddedWorkerImpl.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/modules/fetch/BlobBytesConsumer.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/modules/fetch/FetchManager.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/modules/notifications/NotificationImageLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/CrossOriginAccessControl.h
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/FetchParameters.h
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/FetchUtils.h
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/MemoryCacheCorrectnessTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/MemoryCacheTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/RawResource.h
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/Resource.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/ResourceFetcherTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/ResourceLoader.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/ResourceLoaderOptions.h
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/fetch/ResourceLoaderOptionsTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/platform/loader/testing/MockResource.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/2e231cf052ca5e68e22baf0008ac9e5e29121707/third_party/WebKit/public/web/WebAssociatedURLLoaderOptions.h

I think it's fixed. Need to add some tests to verify this for ES6 modules. Could you please help, kouhei@?
Cc: hirosh...@chromium.org
Owner: kouhei@chromium.org
Thanks for fixing this! Let's chat tomorrow re:test
I verified that the example in the OP by Jake displays the same numbers. 

No random number cookie found.
Random number cookie is: 0.8773221591613491
Random number cookie is: 0.8773221591613491
Owner: tyoshino@chromium.org
Status: Fixed
The tests are in.

Seems the task ksakamoto@ suggested in the description in the CL in https://bugs.chromium.org/p/chromium/issues/detail?id=717525#c11 has been put in the code as TODO().

So, I'm just closing this issue as fixed.
Components: Blink>SecurityFeature>CORS
Labels: M-61
Sign in to add a comment