Regression: CORS preflight request has a wrong value in the Access-Control-Allow-Origin header when issued for extensions
Reported by
co...@streak.com,
Jan 12 2017
|
||||||||||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/55.0.2883.95 Safari/537.36 Steps to reproduce the problem: 1. Install the attached extension. 2. Navigate to http://www.example.com/ 3. Check the devtools console. What is the expected behavior? The console should report that a request failed with status 400, and the extension will log the message "This is working as expected. The bug is not present here.". What went wrong? Chrome will log the error "XMLHttpRequest cannot load https://api.segment.io/v1/identify. Response to preflight request doesn't pass access control check: The 'Access-Control-Allow-Origin' header has a value 'http://www.example.com' that is not equal to the supplied origin. Origin 'chrome-extension://<extension ID>' is therefore not allowed access.", and the extension will log the message "Either the connection failed, or the bug caused the CORS request to fail." When an extension content script running in a page makes an AJAX request to a domain that does not match the current page and that the extension has no special permissions to, then the AJAX connection is treated as a cross-origin request from the domain of "chrome-extension://<extension ID>". The request's Origin header contains "chrome-extension://<extension ID>", and the response's headers must contain either "access-control-allow-origin:chrome-extension://<extension ID>" or "access-control-allow-origin:*". However, in this case in Chrome v56, Chrome incorrectly sends the page's domain in the request's Origin header, but still expects either "access-control-allow-origin:chrome-extension://<extension ID>" or "access-control-allow-origin:*" in the response's headers. The expected chrome-extension: domain is not part of the request at all. This bug is only present for cross-origin requests that need a pre-flight OPTIONS request. This entirely breaks cross-origin requests where a pre-flight OPTIONS request is necessary made by extension content scripts to domains that the extension has no special permission to and that reflect the request's Origin header value in the response's Access-Control-Allow-Origin header. Did this work before? Yes 55.0.2883.95 Does this work in other browsers? N/A Chrome version: 56.0.2924.59 Channel: beta OS Version: OS X 10.11.4 Flash Version: Shockwave Flash 24.0 r0 This is affecting extensions including "Streak CRM for Gmail" and "Clearbit Connect".
,
Jan 16 2017
Able to reproduce this issue on mac 10.12.2 using latest canary #57.0.2983.0. tyoshino@ - Could you please have a look into this issue. Thanks...!!
,
Jan 16 2017
,
Jan 17 2017
Yes, it's a bug in my patch. Thanks for reporting with the reproduction case cowan@. Thanks for identifying the commit, kavvaru@.
,
Jan 17 2017
,
Jan 17 2017
,
Jan 18 2017
As kavvaru@ tagged, the current beta M56 is affected.
,
Jan 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/78d4c0d304e60af63f0a087b8b3de73c27d489b4 commit 78d4c0d304e60af63f0a087b8b3de73c27d489b4 Author: tyoshino <tyoshino@chromium.org> Date: Wed Jan 18 10:38:55 2017 Fix a bug in origin header generation for CORS preflight in extensions In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. We should add the referrer header to a preflight to conform to the spec, but to limit the effect of this patch, it's not included. See the added TODO. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). So, just remove the meaningless code. BUG= 680320 R=yhirano@chromium.org Review-Url: https://codereview.chromium.org/2635023003 Cr-Commit-Position: refs/heads/master@{#444328} [add] https://crrev.com/78d4c0d304e60af63f0a087b8b3de73c27d489b4/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-origin-header-in-isolated-world.html [add] https://crrev.com/78d4c0d304e60af63f0a087b8b3de73c27d489b4/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/resources/access-control-origin-header-in-isolated-world.php [modify] https://crrev.com/78d4c0d304e60af63f0a087b8b3de73c27d489b4/third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp [modify] https://crrev.com/78d4c0d304e60af63f0a087b8b3de73c27d489b4/third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h [modify] https://crrev.com/78d4c0d304e60af63f0a087b8b3de73c27d489b4/third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp [modify] https://crrev.com/78d4c0d304e60af63f0a087b8b3de73c27d489b4/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
,
Jan 18 2017
Confirmed that the extension in the opening comment (cors-fail.zip) now shows: "This is working as expected. CORS was successful. The bug is not present here." I'll request merge to M56 once the patch is baked in canary. If not accepted, the fix will be included from M57.
,
Jan 18 2017
,
Jan 19 2017
The fix r444328 is included from 57.0.2986.0. No new crash seen. Crash query: REGEXP(product.name, '^Chrome') AND REGEXP(product.version, '^57\\.0\\.298') OMIT RECORD IF SUM(REGEXP(CrashedStackTrace.StackFrame.FunctionName, '^blink::DocumentThreadableLoader')) = 0
,
Jan 19 2017
The query was wrong. s/298/2986/
,
Jan 19 2017
This bug requires manual review: We are only 11 days from stable. Please contact the milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 20 2017
LGTM for merging to 56, this will likely impact a lot of extensions once it gets to stable and it would be good to get ahead of that.
,
Jan 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/3104235614e3f7bf250531bae504c706eda7d376 commit 3104235614e3f7bf250531bae504c706eda7d376 Author: Takeshi Yoshino <tyoshino@chromium.org> Date: Fri Jan 20 06:06:00 2017 Fix a bug in origin header generation for CORS preflight in extensions In the refactoring CL https://codereview.chromium.org/2420603003 by me, I forgot to set the origin header to the preflight request in DocumentThreadableLoader::makeCrossOriginAccessRequest(). When the loading happens in an isolated world, the origin header will have an incorrect value due to this bug. We should add the referrer header to a preflight to conform to the spec, but to limit the effect of this patch, it's not included. See the added TODO. This fix also cleans up the code around the main fix: - createAccessControlPreflightRequests()'s SecurityOrigin argument is unused. Remove it. - makeCrossOriginAccessRequest() has duplicated call to setHTTPOrigin() and setHTTPReferrer() (included in prepareCrossoriginRequest() call next to them). Remove them. - prepareCrossOriginRequest(crossOriginRequest) in the code path for issuing a preflight is meaningless. It should have made on m_actualRequest. That said, since loadActualRequest() has prepareCrossOriginRequest() call, we don't even have to call prepareCrossoriginRequest(m_actualRequest). The only difference https://codereview.chromium.org/2420603003 introduced is that m_actualRequest doesn't have the origin and referrer header until loadActualRequest() is called. The only concern with this change is the preflightResult->allowsCrossOriginHeaders() call in DocumentThreadableLoader::handlePreflightResponse(). However, since these headers are listed in the forbidden header names, presence of these headers doesn't affect the return value of allowsCrossOriginHeaders(). So, just remove the meaningless code. BUG= 680320 R=yhirano@chromium.org Review-Url: https://codereview.chromium.org/2635023003 Cr-Commit-Position: refs/heads/master@{#444328} (cherry picked from commit 78d4c0d304e60af63f0a087b8b3de73c27d489b4) Review-Url: https://codereview.chromium.org/2642043004 . Cr-Commit-Position: refs/branch-heads/2924@{#814} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [add] https://crrev.com/3104235614e3f7bf250531bae504c706eda7d376/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/access-control-origin-header-in-isolated-world.html [add] https://crrev.com/3104235614e3f7bf250531bae504c706eda7d376/third_party/WebKit/LayoutTests/http/tests/xmlhttprequest/resources/access-control-origin-header-in-isolated-world.php [modify] https://crrev.com/3104235614e3f7bf250531bae504c706eda7d376/third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.cpp [modify] https://crrev.com/3104235614e3f7bf250531bae504c706eda7d376/third_party/WebKit/Source/core/fetch/CrossOriginAccessControl.h [modify] https://crrev.com/3104235614e3f7bf250531bae504c706eda7d376/third_party/WebKit/Source/core/fetch/CrossOriginAccessControlTest.cpp [modify] https://crrev.com/3104235614e3f7bf250531bae504c706eda7d376/third_party/WebKit/Source/core/loader/DocumentThreadableLoader.cpp
,
Jan 20 2017
Watching betabuilders
,
Jan 20 2017
Win bot finished without any new failure.
,
Jan 20 2017
Mac done. All green.
,
Jan 23 2017
Linux too. Done
,
Jan 25 2017
Tested the issue on Windows-7, Ubuntu 14.04 and Mac OS 10.12 using chrome latest Beta M56-56.0.2924.76 by following steps mentioned in the original comment. Observed that in console display a failed with status 400, and the extension log a message "This is working as expected. The bug is not present here." as expected. Hence adding TE-Verified label. Please find the screen shot for reference. Thank you!
,
Feb 17 2017
Issue 682936 has been merged into this issue. |
||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||
Comment 1 by kavvaru@chromium.org
, Jan 12 2017Labels: -Pri-2 hasbisect-per-revision M-56 OS-Linux OS-Windows Pri-1
Owner: tyoshino@chromium.org
Status: Assigned (was: Unconfirmed)