Bug 160028 - CrossOrigin preflight checker should compute the right Access-Control-Request-Headers value
Summary: CrossOrigin preflight checker should compute the right Access-Control-Request...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on: 160052
Blocks: 151937
  Show dependency treegraph
 
Reported: 2016-07-21 10:00 PDT by youenn fablet
Modified: 2016-07-29 04:09 PDT (History)
4 users (show)

See Also:


Attachments
Patch (16.11 KB, patch)
2016-07-27 07:02 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (578.73 KB, application/zip)
2016-07-27 08:00 PDT, Build Bot
no flags Details
Patch (18.88 KB, patch)
2016-07-27 09:16 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing case comparison and beefing up tests (29.45 KB, patch)
2016-07-28 01:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Fixing case comparison and beefing up tests (41.39 KB, patch)
2016-07-28 02:40 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-07-21 10:00:53 PDT
As per https://fetch.spec.whatwg.org/#cors-preflight-fetch, computation of this value should be done before Origin header is set.
It should also remove safe request headers from the list and sort the unsafe headers.
Comment 1 youenn fablet 2016-07-27 07:02:55 PDT
Created attachment 284692 [details]
Patch
Comment 2 Build Bot 2016-07-27 08:00:09 PDT
Comment on attachment 284692 [details]
Patch

Attachment 284692 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/1761974

New failing tests:
imported/w3c/web-platform-tests/fetch/api/cors/cors-preflight.html
Comment 3 Build Bot 2016-07-27 08:00:13 PDT
Created attachment 284696 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.5
Comment 4 youenn fablet 2016-07-27 09:16:44 PDT
Created attachment 284702 [details]
Patch
Comment 5 WebKit Commit Bot 2016-07-27 10:00:57 PDT
Comment on attachment 284702 [details]
Patch

Rejecting attachment 284702 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 284702, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/1762526
Comment 6 Alex Christensen 2016-07-28 00:14:59 PDT
Comment on attachment 284702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284702&action=review

> Source/WebCore/loader/DocumentThreadableLoader.cpp:121
> +            preflightSuccess(ResourceRequest(request));

Is it really impossible to make request a ResourceRequest&&?  This copy shouldn't be necessary.

> Source/WebCore/platform/network/HTTPParsers.cpp:770
> +        return value == "application/x-www-form-urlencoded" || value == "multipart/form-data" || value == "text/plain";

These should probably be case-insensitive comparisons and tested as such. tExT/pLaIn should be safe, right?
Comment 7 youenn fablet 2016-07-28 00:27:14 PDT
Comment on attachment 284702 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=284702&action=review

>> Source/WebCore/loader/DocumentThreadableLoader.cpp:121
>> +            preflightSuccess(ResourceRequest(request));
> 
> Is it really impossible to make request a ResourceRequest&&?  This copy shouldn't be necessary.

request is const and preflightSuccess will add the origin in the headers, so we need to make a copy.
Or we should make DocumentThreadableLoader constructor take a ResourceRequest&&.

>> Source/WebCore/platform/network/HTTPParsers.cpp:770
>> +        return value == "application/x-www-form-urlencoded" || value == "multipart/form-data" || value == "text/plain";
> 
> These should probably be case-insensitive comparisons and tested as such. tExT/pLaIn should be safe, right?

I need to fix that obviously, thanks!
Comment 8 youenn fablet 2016-07-28 01:35:13 PDT
Created attachment 284761 [details]
Fixing case comparison and beefing up tests
Comment 9 youenn fablet 2016-07-28 02:40:06 PDT
Created attachment 284764 [details]
Fixing case comparison and beefing up tests
Comment 10 youenn fablet 2016-07-28 02:42:25 PDT
I added rebasing of some worker tests (currently skipped).
As can be seen, referrer is set for workers before preflighting, in WorkerThreadableLoader probably.
Like we did for Origin, setting referrer should be done at a lower loader code level.
Comment 11 Alex Christensen 2016-07-28 10:16:39 PDT
> Or we should make DocumentThreadableLoader constructor take a
> ResourceRequest&&.
Yes.  Let's do that.  That's better than making a copy.
Comment 12 Alex Christensen 2016-07-28 10:31:30 PDT
Comment on attachment 284764 [details]
Fixing case comparison and beefing up tests

r=me other than that.
Comment 13 youenn fablet 2016-07-29 03:50:14 PDT
(In reply to comment #11)
> > Or we should make DocumentThreadableLoader constructor take a
> > ResourceRequest&&.
> Yes.  Let's do that.  That's better than making a copy.

OK, I'll prepare patches to optimize ResourceResponse and ResourceRequest transfer around DocumentThreadableLoader.

As for ResourceRequest, note that DocumentThreadableLoader is already creating a request copy when creating its CachedResourceRequest.
We could probably optimize this already without changing its clients.
That should go back to the previous number of ResourceRequest copies.
Comment 14 WebKit Commit Bot 2016-07-29 04:09:19 PDT
Comment on attachment 284764 [details]
Fixing case comparison and beefing up tests

Clearing flags on attachment: 284764

Committed r203899: <http://trac.webkit.org/changeset/203899>
Comment 15 WebKit Commit Bot 2016-07-29 04:09:25 PDT
All reviewed patches have been landed.  Closing bug.