226678 – Move Timing-Allow-Origin checks to the network process

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

Importance:	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)
CC List:	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
Show Obsolete (8) View All Add an attachment (proposed patch, testcase, etc.)

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>