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

Issue metadata

Status: Fixed
Owner:
Leaves the project on 2018/03/02
Closed: Jun 2014
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Windows
Pri: 2
Type: Bug



Sign in to add a comment

Chrome sends user credentials with preflighed CORS request

Reported by collinsa...@gmail.com, May 26 2014

Issue description

UserAgent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36

Example URL:

Steps to reproduce the problem:
1. Send a CORS request that requires a preflight, to a domain which has cookies
2. Note that the preflight request includes cookies
3. Note that this violates https://dvcs.w3.org/hg/cors/raw-file/tip/Overview.html#preflight-request

What is the expected behavior?
Preflight requests should not include cookies

What went wrong?
Preflight request included cookies

Did this work before? N/A 

Chrome version: 35.0.1916.114  Channel: stable
OS Version: 6.1 (Windows 7, Windows Server 2008 R2)
Flash Version: Shockwave Flash 13.0 r0

This was originally filed here: https://code.google.com/p/chromium/issues/detail?id=121904

But was closed as bug is in WebKit.  (filed against WebKit here: https://bugs.webkit.org/show_bug.cgi?id=83333 and here https://bugs.webkit.org/show_bug.cgi?id=37676)

Since the fork of WebKit to Blink, I assume you are no longer tracking WebKit bugs.  This bug should be fixed in Blink.
 

Comment 1 by a...@chromium.org, May 27 2014

Cc: bbudge@chromium.org a...@chromium.org
Labels: Needs-Feedback
Cc'ing bbudge@ to have a look at this.
Labels: Cr-Blink-XHR Cr-Privacy
Owner: tyoshino@chromium.org

Comment 4 by asanka@chromium.org, May 27 2014

Labels: -Cr-Internals-Network Cr-Internals-Network-Auth
Do you have an example site or code to reproduce this?

DocumentThreadableLoader uses createAccessControlPreflightRequest() to build a preflight request. It calls setAllowStoredCredentials() with false to prevent Chromium net stack from sending cookies.

This change by bbudge (bbudge reported https://bugs.webkit.org/show_bug.cgi?id=83333) fixed it.
http://src.chromium.org/viewvc/blink/trunk/Source/core/fetch/CrossOriginAccessControl.cpp?r1=113588&r2=113589&

I checked that a preflight OPTIONS request without any cookie is sent using a small script.
See http://jsfiddle.net/z5ZM5/

It appears that you are using the withCredentials properly on the XHR to determine whether or not to send cookies with the preflight request.  

The withCredentials property is used to control whether or not to send cookies on the actual resource request - not the preflight (OPTIONS) request.   Cookies should never be included in preflights, regardless of withCredentials.

Is the above change by bbudge included in my Chrome version (listed above)?


The jsfiddle above sends the following request in my Chrome:

OPTIONS https://www.google.com/ HTTP/1.1
Host: www.google.com
Connection: keep-alive
Access-Control-Request-Method: PUT
Origin: http://fiddle.jshell.net
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36
Access-Control-Request-Headers: accept, content-type
Accept: */*
X-Client-Data: COa1yQEIhLbJAQiitskBCKm2yQEIwbbJAQiehsoBCLmIygEI7YjKAQiplMoB
Referer: http://fiddle.jshell.net/_display/
Accept-Encoding: gzip,deflate,sdch
Accept-Language: en-US,en;q=0.8
Cookie: PREF=ID=b8...redacted..BzZ


Comment 7 by Deleted ...@, May 31 2014

I found the same bug relating to basic authentication rather than cookies. I was just about to submit this when I found your bug.

Chrome Version 35.0.1916.114 m
Windows 8.1

Chrome sends credentials on CORS preflight requests. The credentials should be omitted until the preflight response headers have been verified. Once verified the credentials can be sent on the real request.

## Reproduce steps
1. Open a new Chrome tab
2. Open the developer console
3. Run this JavaScript to send a request with basic authentication

  var xhr = new XMLHttpRequest();
  xhr.open('POST', 'http://user:passwd@httpbin.org/basic-auth/user/passwd', true);
  xhr.withCredentials = true;
  xhr.setRequestHeader('Content-Type', 'application/json');
  xhr.send();

## Request
OPTIONS http://httpbin.org/basic-auth/user/passwd HTTP/1.1
Host: httpbin.org
Connection: keep-alive
Cache-Control: no-cache
Authorization: Basic dXNlcjpwYXNzd2Q=
Access-Control-Request-Method: POST
Pragma: no-cache
Origin: https://www.google.com
User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/35.0.1916.114 Safari/537.36
Access-Control-Request-Headers: content-type
Accept: */*
Accept-Encoding: gzip,deflate,sdch
Accept-Language: en-US,en;q=0.8
Cookie: _gauges_unique_hour=1; _gauges_unique_day=1; _gauges_unique_month=1; _gauges_unique_year=1; _gauges_unique=1

## Response
HTTP/1.1 200 OK
Access-Control-Allow-Credentials: true
Access-Control-Allow-Methods: GET, POST, PUT, DELETE, PATCH, OPTIONS
Access-Control-Allow-Origin: https://www.google.com
Access-Control-Max-Age: 3600
Allow: GET, HEAD, OPTIONS
Content-Type: text/html; charset=utf-8
Date: Sat, 31 May 2014 15:40:12 GMT
Server: gunicorn/18.0
Content-Length: 0
Connection: keep-alive

## Bug
The Authorization header `Authorization: Basic dXNlcjpwYXNzd2Q=` should not be included on the CORS preflight request. This should only be included once it has verified the `Access-Control-Allow-Origin` and `Access-Control-Allow-Credentials` response headers.

## W3C Spec
http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0

Section 7.1.5 discusses "Cross-Origin Request with Preflight" and states "Exclude user credentials".

Section 7.2 discusses the "Resource Sharing Check" that defines how to check the `Access-Control-Allow-Origin` and `Access-Control-Allow-Credentials` response headers.
Thanks for your help. I just identified the cause. createAccessControlPreflightRequest() does call setAllowStoredCredentials() on the request object for a preflight request, but options passed together to FetchRequest's constructor in loadRequest is overriding the parameter. I'll fix it.
Project Member

Comment 9 by bugdroid1@chromium.org, Jun 2 2014

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=175283

------------------------------------------------------------------
r175283 | tyoshino@chromium.org | 2014-06-02T14:19:36.284265Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/DocumentThreadableLoader.h?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/ThreadableLoader.h?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/page/EventSource.cpp?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/web/AssociatedURLLoader.cpp?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/workers/WorkerScriptLoader.cpp?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/fileapi/FileReaderLoader.cpp?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/WorkerThreadableLoader.cpp?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/inspector/InspectorResourceAgent.cpp?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/platform/CrossThreadCopier.h?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/DocumentThreadableLoader.cpp?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/WorkerThreadableLoader.h?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/ThreadableLoader.cpp?r1=175283&r2=175282&pathrev=175283
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/xml/XMLHttpRequest.cpp?r1=175283&r2=175282&pathrev=175283

Refactor ThreadableLoaderOptions for readability

ThreadableLoaderOptions shouldn't derive from ResourceLoaderOptions.
ResourceLoaderOptions members are basically just passed through to
FetchRequest while items added by ThreadableLoaderOptions definition
are configuring how CORS, etc. are handled in ThreadableLoader.
Inheritance looks making things less readable to me.

Also, I'd like to make DocumentThreadbleLoader more readable by making
constant variables held by const members.

Items in ThreadableLoaderOptions that determines ThreadableLoader's
behavior could be held by DocumentThreadableLoader as a const member.
Items defined in ResourceLoaderOptions can be altered inside
DocumentThreadableLoader, but can still be held as a const member by
having members to hold overridden items.

BUG= 377541 

Review URL: https://codereview.chromium.org/301243015
-----------------------------------------------------------------
Project Member

Comment 11 by bugdroid1@chromium.org, Jun 5 2014

The following revision refers to this bug:
  http://src.chromium.org/viewvc/blink?view=rev&rev=175527

------------------------------------------------------------------
r175527 | tyoshino@chromium.org | 2014-06-05T04:46:14.438539Z

Changed paths:
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/fetch/FetchRequest.cpp?r1=175527&r2=175526&pathrev=175527
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/xmlhttprequest/resources/access-control-preflight-request-must-not-contain-cookie.php?r1=175527&r2=175526&pathrev=175527
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/DocumentThreadableLoader.h?r1=175527&r2=175526&pathrev=175527
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/xmlhttprequest/access-control-preflight-request-must-not-contain-cookie.html?r1=175527&r2=175526&pathrev=175527
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/xmlhttprequest/access-control-preflight-credential-sync-expected.txt?r1=175527&r2=175526&pathrev=175527
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/xmlhttprequest/access-control-preflight-credential-async-expected.txt?r1=175527&r2=175526&pathrev=175527
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/cookies/resources/third-party-cookie-relaxing-iframe.html?r1=175527&r2=175526&pathrev=175527
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/xmlhttprequest/access-control-preflight-credential-sync.html?r1=175527&r2=175526&pathrev=175527
   M http://src.chromium.org/viewvc/blink/trunk/Source/core/loader/DocumentThreadableLoader.cpp?r1=175527&r2=175526&pathrev=175527
   M http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/xmlhttprequest/access-control-preflight-credential-async.html?r1=175527&r2=175526&pathrev=175527
   A http://src.chromium.org/viewvc/blink/trunk/LayoutTests/http/tests/xmlhttprequest/access-control-preflight-request-must-not-contain-cookie-expected.txt?r1=175527&r2=175526&pathrev=175527

ResourceLoaderOptions also must be updated by updateRequestForAccessControl()

updateRequestForAccessControl() updates ResourceRequest for access
control, but ResourceLoaderOptions passed to FetchRequest also contains
configuration items that must be updated for access control.

This CL removes cookie handling code and iframe from the following
layout tests since they're testing user:pass but not cookie.
- access-control-preflight-credential-async.html
- access-control-preflight-credential-sync.html

A new test is added to check that no cookie is set in a preflight
request. This new test uses third-party-cookie-relaxing-iframe.html
which was loaded by the two tests but was not actually used.

Missing testRunner.setAlwaysAcceptCookies() calls are added to
third-party-cookie-relaxing-iframe.html.

BUG= 377541 
R=japhet

Review URL: https://codereview.chromium.org/312653002
-----------------------------------------------------------------
Status: Fixed
Checked that
- #6 is fixed with this patch
- #7 is confirmed by logging in to httpbin first, and then use the code. It's also confirmed to be fixed with this patch

Comment 13 by tkent@chromium.org, Nov 27 2015

Labels: -Cr-Blink-XHR Cr-Blink-Network-XHR

Sign in to add a comment