Bug 226678 - Move Timing-Allow-Origin checks to the network process
Summary: Move Timing-Allow-Origin checks to the network process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-06-04 22:44 PDT by Alex Christensen
Modified: 2021-06-10 16:31 PDT (History)
14 users (show)

See Also:


Attachments
Patch (257.98 KB, patch)
2021-06-04 22:51 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (240.16 KB, patch)
2021-06-07 15:06 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (241.79 KB, patch)
2021-06-07 15:15 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (242.50 KB, patch)
2021-06-08 11:20 PDT, Alex Christensen
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (244.66 KB, patch)
2021-06-08 12:38 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (245.55 KB, patch)
2021-06-08 14:43 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (246.72 KB, patch)
2021-06-08 17:57 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (250.63 KB, patch)
2021-06-09 10:18 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (251.38 KB, patch)
2021-06-10 10:27 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2021-06-04 22:44:54 PDT
Move Timing-Allow-Origin checks to the network process
Comment 1 Alex Christensen 2021-06-04 22:51:23 PDT
Created attachment 430645 [details]
Patch
Comment 2 EWS Watchlist 2021-06-04 22:52:29 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 3 Alex Christensen 2021-06-07 15:06:47 PDT
Created attachment 430785 [details]
Patch
Comment 4 Alex Christensen 2021-06-07 15:15:14 PDT
Created attachment 430787 [details]
Patch
Comment 5 Alex Christensen 2021-06-08 11:20:24 PDT
Created attachment 430868 [details]
Patch
Comment 6 Alex Christensen 2021-06-08 12:38:32 PDT
Created attachment 430876 [details]
Patch
Comment 7 Alex Christensen 2021-06-08 14:43:41 PDT
Created attachment 430893 [details]
Patch
Comment 8 Alex Christensen 2021-06-08 17:57:35 PDT
Created attachment 430927 [details]
Patch
Comment 9 Alex Christensen 2021-06-09 10:18:26 PDT
Created attachment 430974 [details]
Patch
Comment 10 Alex Christensen 2021-06-10 09:29:41 PDT
I need to mark http/wpt/resource-timing/rt-revalidate-requests-2.html as failing on Windows, but otherwise this should be ready for review
Comment 11 Chris Dumez 2021-06-10 09:41:54 PDT
Comment on attachment 430974 [details]
Patch

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

> Source/WebCore/platform/network/TimingAllowOrigin.cpp:42
> +    const String& timingAllowOriginString = response.httpHeaderField(HTTPHeaderName::TimingAllowOrigin);

auto& ?

> Source/WebCore/platform/network/TimingAllowOrigin.cpp:43
> +    const String& securityOrigin = initiatorSecurityOrigin.toString();

ditto.

> Source/WebCore/platform/network/TimingAllowOrigin.cpp:44
> +    for (auto& originWithSpace : timingAllowOriginString.split(',')) {

Wouldn't it be more efficient to iterate over StringView(timingAllowOriginString).split(',') ?

> Source/WebCore/platform/network/TimingAllowOrigin.cpp:45
> +        auto origin = stripLeadingAndTrailingHTTPSpaces(StringView(originWithSpace));

Since you want StringViews anyway?

> Source/WebCore/platform/network/TimingAllowOrigin.h:33
> +WEBCORE_EXPORT bool passesTimingAllowOriginCheck(const ResourceResponse&, const WebCore::SecurityOrigin& initiatorSecurityOrigin);

WebCore:: is unnecessary.

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:525
> +void ResourceHandle::platformLoadResourceSynchronously(NetworkingContext* context, const ResourceRequest& request,  StoredCredentialsPolicy storedCredentialsPolicy, SecurityOrigin* sourceOrigin, ResourceError& error, ResourceResponse& response, Vector<uint8_t>& data)

extra space before StoredCredentialsPolicy

> Source/WebCore/platform/network/cf/ResourceHandleCFNet.cpp:540
> +    RefPtr<ResourceHandle> handle = adoptRef(new ResourceHandle(context, request, &client, defersLoading, shouldContentSniff, shouldContentEncodingSniff, sourceOrigin, false));

A comment by the 'false' to clarify what it means or an enum class would be nice

> LayoutTests/imported/w3c/web-platform-tests/resource-timing/buffer-full-inspect-buffer-during-callback-expected.txt:2
> +Harness Error (TIMEOUT), message = null

Please skip in TestExpectations.

> LayoutTests/imported/w3c/web-platform-tests/resource-timing/buffer-full-set-to-current-buffer-expected.txt:2
> +Harness Error (TIMEOUT), message = null

Please skip in TestExpectations.

> LayoutTests/imported/w3c/web-platform-tests/resource-timing/document-domain-no-impact-opener-expected.txt:2
> +Harness Error (TIMEOUT), message = null

Please skip test in TestExpectations to avoid slowing runs.
Comment 12 Alex Christensen 2021-06-10 10:27:33 PDT
Created attachment 431092 [details]
Patch
Comment 13 Alex Christensen 2021-06-10 16:30:49 PDT
Marked EWS failing tests as flaky and landed in r278738
Comment 14 Radar WebKit Bug Importer 2021-06-10 16:31:21 PDT
<rdar://problem/79166791>