Bug 207102 – Fix behavior of pings regarding Origin header

• Component: Page Loading
• Status: RESOLVED FIXED

[Rob Buis]

Import WPT tests to verify pings do not send a origin header.

[Rob Buis]

Created attachment 389475 [details]
Patch

[Rob Buis]

Created attachment 389478 [details]
Patch

[Rob Buis]

Created attachment 389940 [details]
Patch

[youenn fablet]

Comment on attachment 389940 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Import WPT tests to verify pings do not send a origin header

Can we change the title of the bug since we are changing behavior and not only adding tests?

> LayoutTests/imported/w3c/ChangeLog:10
> +        https://fetch.spec.whatwg.org/#append-a-request-origin-header

How about implementing this in this patch?
It seems we would regress the case where people really want to have a 'null' origin.

[Rob Buis]

Created attachment 391639 [details]
Patch

[Rob Buis]

Comment on attachment 389940 [details]
Patch

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

>> Source/WebCore/ChangeLog:3
>> +        Import WPT tests to verify pings do not send a origin header
> 
> Can we change the title of the bug since we are changing behavior and not only adding tests?

Good point, done.

>> LayoutTests/imported/w3c/ChangeLog:10
>> +        https://fetch.spec.whatwg.org/#append-a-request-origin-header
> 
> How about implementing this in this patch?
> It seems we would regress the case where people really want to have a 'null' origin.

Yes, I added it in the latest patch.

[youenn fablet]

Comment on attachment 391639 [details]
Patch

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

> Source/WebCore/loader/PingLoader.cpp:138
> +    FrameLoader::addHTTPOriginIfNeeded(request, SecurityPolicy::generateOriginHeader(document.referrerPolicy(), request.url(), sourceOrigin.toString()));

It might be nice to only create the origin value when we need to set the header.
How about adding something like
FrameLoader::addHTTPOriginIfNeeded(ResourceRequest&, ReferrerPolicy, const SecurityOrigin&)
or
FrameLoader::addHTTPOriginIfNeeded(ResourceRequest&, const Document&)

> Source/WebCore/page/SecurityPolicy.cpp:134
> +String SecurityPolicy::generateOriginHeader(ReferrerPolicy referrerPolicy, const URL& url, const String& origin)

It seems passing a SecurityOrigin would be better in the StrictOrigin/StrictOriginWhenCrossOrigin/SameOrigin cases.

> Source/WebCore/page/SecurityPolicy.cpp:138
> +        return "null";

"null"-> "null"_s here and below.

> Source/WebCore/page/SecurityPolicy.cpp:149
> +    default:

Can we enumerate all enum values instead of having default?

[Rob Buis]

Created attachment 392142 [details]
Patch

[Rob Buis]

Comment on attachment 391639 [details]
Patch

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

>> Source/WebCore/loader/PingLoader.cpp:138
>> +    FrameLoader::addHTTPOriginIfNeeded(request, SecurityPolicy::generateOriginHeader(document.referrerPolicy(), request.url(), sourceOrigin.toString()));
> 
> It might be nice to only create the origin value when we need to set the header.
> How about adding something like
> FrameLoader::addHTTPOriginIfNeeded(ResourceRequest&, ReferrerPolicy, const SecurityOrigin&)
> or
> FrameLoader::addHTTPOriginIfNeeded(ResourceRequest&, const Document&)

You have a good point. I will do this later since generateOriginHeader should indeed be called within addHTTPOriginIfNeeded and may fix some other tests.

>> Source/WebCore/page/SecurityPolicy.cpp:134
>> +String SecurityPolicy::generateOriginHeader(ReferrerPolicy referrerPolicy, const URL& url, const String& origin)
> 
> It seems passing a SecurityOrigin would be better in the StrictOrigin/StrictOriginWhenCrossOrigin/SameOrigin cases.

Done.

>> Source/WebCore/page/SecurityPolicy.cpp:138
>> +        return "null";
> 
> "null"-> "null"_s here and below.

Done.

>> Source/WebCore/page/SecurityPolicy.cpp:149
>> +    default:
> 
> Can we enumerate all enum values instead of having default?

Done.

[WebKit Commit Bot]

Comment on attachment 392142 [details]
Patch

Clearing flags on attachment: 392142

Committed r257729: <https://trac.webkit.org/changeset/257729>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59964311>