Import WPT tests to verify pings do not send a origin header.
Created attachment 389475 [details] Patch
Created attachment 389478 [details] Patch
Created attachment 389940 [details] Patch
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.
Created attachment 391639 [details] Patch
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.
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?
Created attachment 392142 [details] Patch
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.
Comment on attachment 392142 [details] Patch Clearing flags on attachment: 392142 Committed r257729: <https://trac.webkit.org/changeset/257729>
All reviewed patches have been landed. Closing bug.
<rdar://problem/59964311>