[webkit-reviews] review granted: [Bug 175410] [Beacon] Do connect-src CSP check on redirects as well : [Attachment 317824] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Chris Dumez <cdumez at apple.com>'s
request for review:
Bug 175410: [Beacon] Do connect-src CSP check on redirects as well
https://bugs.webkit.org/show_bug.cgi?id=175410

Attachment 317824: Patch

https://bugs.webkit.org/attachment.cgi?id=317824&action=review




--- Comment #4 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 317824
  --> https://bugs.webkit.org/attachment.cgi?id=317824
Patch

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

> Source/WebCore/loader/cache/CachedResource.cpp:266
> +	   auto* contentSecurityPolicy = document &&
!document->shouldBypassMainWorldContentSecurityPolicy() ?
document->contentSecurityPolicy() : nullptr;

I guess this code is fine since beacon is only exposed in document.
But it goes somehow against the idea of CachedResource being something that
relates to a loading context, which is not always a document.
Maybe worth a comment.

> Source/WebKit/NetworkProcess/PingLoad.cpp:191
> +	      
m_contentSecurityPolicy->didReceiveHeaders(*m_parameters.cspResponseHeaders,
ContentSecurityPolicy::ReportParsingErrors::No);

Do we always need to create m_contentSecurityPolicy?
If m_parameters.cspResponseHeaders is null, does not it mean we do not need to
do CSP checks?

> Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.h:62
> +    void createPingHandle(WebCore::NetworkingContext*,
WebCore::ResourceRequest&, Ref<WebCore::SecurityOrigin>&& sourceOrigin,
WebCore::ContentSecurityPolicy*, const WebCore::FetchOptions&) override;

Can we make WebResourceLoadScheduler final and change override to final here?