Summary: | [Beacon] Add support for CORS-preflighting for WK2 / NETWORK_SESSION | ||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, beidson, buildbot, commit-queue, ggaren, rniwa, webkit-bug-importer, youennf | ||||||||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||
URL: | https://w3c.github.io/beacon/#privacy | ||||||||||||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=175330 | ||||||||||||||||||||||||||||||||||||||||
Bug Depends on: | 175280 | ||||||||||||||||||||||||||||||||||||||||
Bug Blocks: | 147885 | ||||||||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2017-08-07 09:36:43 PDT
Created attachment 317456 [details]
WIP Patch
Created attachment 317457 [details]
WIP Patch
Attachment 317457 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/PingLoad.cpp:41: One line control clauses should not use braces. [whitespace/braces] [4]
Total errors found: 1 in 16 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317477 [details]
WIP Patch
Attachment 317477 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:104: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/NetworkConnectionToWebProcess.cpp". [list/order] [5]
ERROR: Source/WebKit/NetworkProcess/PingLoad.cpp:146: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Total errors found: 2 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317478 [details]
WIP Patch
Attachment 317478 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 1 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317484 [details]
WIP Patch
Attachment 317484 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 1 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317499 [details]
WIP Patch
Attachment 317499 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 1 in 22 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317501 [details]
WIP Patch
Attachment 317501 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 1 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317539 [details]
WIP Patch
Attachment 317539 [details] did not pass style-queue:
ERROR: Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:47: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:48: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:49: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.h:50: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 5 in 25 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 317539 [details] WIP Patch Attachment 317539 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4274844 New failing tests: http/wpt/beacon/headers/cors-preflight-success.html http/wpt/beacon/headers/cors-preflight-failure.html Created attachment 317544 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 317580 [details]
WIP Patch
Attachment 317580 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 1 in 28 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 317580 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317580&action=review > LayoutTests/imported/w3c/web-platform-tests/beacon/resources/beacon-preflight.py:24 > +def main(request, response): I could not figure out how to keep this script in http/wpt, I was getting 404 errors. @Youenn: please let me know if you know how I can do this. Comment on attachment 317580 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317580&action=review > LayoutTests/http/wpt/beacon/headers/cors-preflight-failure.html:13 > +var RESOURCES_DIR = "/beacon/resources/"; If you want to have beacon-preflight.py in LayoutTests/http/wpt/beacon/resources, you need to set RESOURCES_DIR to an absolute path like /WebKit/beacon/resources or a relative one like ../resources/ The latter would ease the migration to web-platform-tests. Created attachment 317595 [details]
Patch
(In reply to youenn fablet from comment #22) > Comment on attachment 317580 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=317580&action=review > > > LayoutTests/http/wpt/beacon/headers/cors-preflight-failure.html:13 > > +var RESOURCES_DIR = "/beacon/resources/"; > > If you want to have beacon-preflight.py in > LayoutTests/http/wpt/beacon/resources, you need to set RESOURCES_DIR to an > absolute path like /WebKit/beacon/resources or a relative one like > ../resources/ > The latter would ease the migration to web-platform-tests. I have to use an absolute URL because this is testing CORS. I need a cross-origin URL. Attachment 317595 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 1 in 36 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 317607 [details]
Patch
Attachment 317607 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 1 in 36 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 317607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317607&action=review > Source/WebCore/loader/LoaderStrategy.h:65 > + virtual void createPingHandle(NetworkingContext*, ResourceRequest&, Ref<SecurityOrigin>&& sourceOrigin, const FetchOptions&) = 0; This ResourceRequest+origin+fetch options API is looking good to me, since this is what a fetch request is. > Source/WebCore/loader/PingLoader.h:59 > + static void startPingLoad(Frame&, ResourceRequest&, SecurityOrigin& sourceOrigin, ShouldFollowRedirects); I guess the styler might not like sourceOrigin parameter name. > Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:71 > + loadParameters.request = createAccessControlPreflightRequest(m_parameters.originalRequest, m_parameters.sourceOrigin, m_parameters.originalRequest.httpReferrer()); We had issues in the past with preflight requests using credentials or using the session that has credentials. Code here looks good to me with that respect. It would be great though if we could add a test to ensure that since this is a new code path where we could do mistake. > Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:74 > + m_task = NetworkDataTask::create(*networkSession, *this, loadParameters); It seems strange that we are not moving loadParameters. It contains a request which is probably stored somewhere in NetworkDataTask and we would gain if we could move it instead of copying it. > Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:83 > + completionHandler(ResourceRequest { }); Can we write completionHandler({ })? > Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:106 > +void NetworkCORSPreflightChecker::didCompleteWithError(const WebCore::ResourceError& error, const WebCore::NetworkLoadMetrics&) This code is really similar to CrossOriginPreflightChecker::validatePreflightResponse. I guess it is fine although some additional code sharing would be good too. > Source/WebKit/NetworkProcess/PingLoad.cpp:43 > + : m_parameters(parameters) We should take a NetworkResourceLoadParameters&& here. Probably came for past code, which probably comes from the fact that IPC is calling with const& parameters. Maybe in the future IPC APIs could use &&, which might make the change to NetworkResourceLoadParameters&& useful. > Source/WebKit/NetworkProcess/PingLoad.cpp:78 > + // FIXME: Do a CORS preflight if necessary. FIXME: Ensure max redirection count is lower than 20. Comment on attachment 317607 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317607&action=review >> Source/WebCore/loader/PingLoader.h:59 >> + static void startPingLoad(Frame&, ResourceRequest&, SecurityOrigin& sourceOrigin, ShouldFollowRedirects); > > I guess the styler might not like sourceOrigin parameter name. Style script does not complain because of the 'source' prefix. I think the parameter name is useful here it clarifies that it is the SecurityOrigin of the source (e.g. not the destination). >> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:71 >> + loadParameters.request = createAccessControlPreflightRequest(m_parameters.originalRequest, m_parameters.sourceOrigin, m_parameters.originalRequest.httpReferrer()); > > We had issues in the past with preflight requests using credentials or using the session that has credentials. > Code here looks good to me with that respect. > It would be great though if we could add a test to ensure that since this is a new code path where we could do mistake. Will clarify with your offline how to test this. >> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:74 >> + m_task = NetworkDataTask::create(*networkSession, *this, loadParameters); > > It seems strange that we are not moving loadParameters. > It contains a request which is probably stored somewhere in NetworkDataTask and we would gain if we could move it instead of copying it. OK. >> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:83 >> + completionHandler(ResourceRequest { }); > > Can we write completionHandler({ })? Yes, OK. >> Source/WebKit/NetworkProcess/NetworkCORSPreflightChecker.cpp:106 >> +void NetworkCORSPreflightChecker::didCompleteWithError(const WebCore::ResourceError& error, const WebCore::NetworkLoadMetrics&) > > This code is really similar to CrossOriginPreflightChecker::validatePreflightResponse. > I guess it is fine although some additional code sharing would be good too. Ok, Will see if I can share a little bit more code here. >> Source/WebKit/NetworkProcess/PingLoad.cpp:43 >> + : m_parameters(parameters) > > We should take a NetworkResourceLoadParameters&& here. > Probably came for past code, which probably comes from the fact that IPC is calling with const& parameters. > Maybe in the future IPC APIs could use &&, which might make the change to NetworkResourceLoadParameters&& useful. Actually, the IPC code seems to correctly move. I'll fix in this patch. >> Source/WebKit/NetworkProcess/PingLoad.cpp:78 >> + // FIXME: Do a CORS preflight if necessary. > > FIXME: Ensure max redirection count is lower than 20. Will add FIXME and fix in a follow up. Patch looks good overall. We could foresee a far future where https://fetch.spec.whatwg.org/#concept-http-network-or-cache-fetch would more or less be implemented in network process. As a side note, we do no see preflights in the web inspector. I guess we would need to retrofit that information to the web process if we want to do that. Created attachment 317645 [details]
Patch
Still working on the additional testing but this new iteration should address other review comments. Attachment 317645 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 1 in 40 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 317645 [details] Patch Attachment 317645 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4280918 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html http/tests/xmlhttprequest/access-control-preflight-async-method-denied.html http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html http/tests/xmlhttprequest/access-control-preflight-sync-header-denied.html http/tests/xmlhttprequest/access-control-basic-non-simple-deny-cached.html http/tests/xmlhttprequest/access-control-preflight-sync-method-denied.html http/tests/xmlhttprequest/access-control-and-redirects-async.html Created attachment 317655 [details]
Archive of layout-test-results from ews101 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 317645 [details] Patch Attachment 317645 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4280980 New failing tests: imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html http/tests/xmlhttprequest/access-control-preflight-async-method-denied.html http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html http/tests/xmlhttprequest/access-control-preflight-sync-header-denied.html http/tests/xmlhttprequest/access-control-basic-non-simple-deny-cached.html http/tests/xmlhttprequest/access-control-preflight-sync-method-denied.html http/tests/xmlhttprequest/access-control-and-redirects-async.html Created attachment 317659 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 317645 [details] Patch Attachment 317645 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4280964 New failing tests: http/tests/xmlhttprequest/access-control-preflight-sync-method-denied.html imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight-star.any.html http/tests/xmlhttprequest/access-control-preflight-async-method-denied.html http/tests/xmlhttprequest/access-control-preflight-async-header-denied.html http/tests/xmlhttprequest/cors-non-standard-safelisted-headers-should-trigger-preflight.html http/tests/xmlhttprequest/access-control-basic-non-simple-deny-cached.html http/tests/xmlhttprequest/access-control-preflight-sync-header-denied.html http/tests/xmlhttprequest/access-control-and-redirects-async.html Created attachment 317660 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 317668 [details]
Patch
Created attachment 317670 [details]
Patch
Comment on attachment 317670 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=317670&action=review > LayoutTests/http/wpt/beacon/cors/cors-preflight-cookie.html:16 > + testRunner.setAlwaysAcceptCookies(true); Added cookie test as suggested. Sadly, it looks like I need rely on this testRunner API or the cookie does not get set cross origin. Attachment 317670 [details] did not pass style-queue:
ERROR: Source/WebKit/CMakeLists.txt:103: Alphabetical sorting problem. "NetworkProcess/NetworkCORSPreflightChecker.cpp" should be before "NetworkProcess/FileAPI/NetworkBlobRegistry.cpp". [list/order] [5]
Total errors found: 1 in 43 files
If any of these errors are false positives, please file a bug against check-webkit-style.
What about an iframe? (In reply to youenn fablet from comment #44) > What about an iframe? What do you mean? My test is using an iframe. Comment on attachment 317670 [details] Patch Clearing flags on attachment: 317670 Committed r220442: <http://trac.webkit.org/changeset/220442> All reviewed patches have been landed. Closing bug. |