Bug 175410

<rdar://problem/33815470>

Created attachment 317821 [details] Patch

Created attachment 317824 [details] Patch

Comment on attachment 317824 [details] 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?

Comment on attachment 317824 [details] 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. Actually, I can get a contentSecurityPolicy from any ScriptExecutionContext, not just the document. I'll look at making this clearer. >> 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? Yes, this is the point. m_parameters.cspResponseHeaders is null when document->shouldBypassMainWorldContentSecurityPolicy() is true. >> 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? Ok.

Comment on attachment 317824 [details] 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. > > Actually, I can get a contentSecurityPolicy from any ScriptExecutionContext, not just the document. I'll look at making this clearer. Hmm... This method takes in a CachedResourceLoader which seems to be tied to a document, isn't it?

Created attachment 317837 [details] Patch

> > 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? > > Yes, this is the point. m_parameters.cspResponseHeaders is null when > document->shouldBypassMainWorldContentSecurityPolicy() is true. When document->shouldBypassMainWorldContentSecurityPolicy() is true, we are currently creating an empty m_contentSecurityPolicy and calling allowConnectToSource on it. What is done in WebCore is that we are not calling at all allowConnectToSource if document->shouldBypassMainWorldContentSecurityPolicy() is true. It seems to me we should return a null m_contentSecurityPolicy if m_parameters.cspResponseHeaders is null. Or probably better add a checkCSP routine that would return true if m_parameters.cspResponseHeaders is null. > Hmm... This method takes in a CachedResourceLoader which seems to be tied to > a document, isn't it? Yes

Created attachment 317841 [details] Patch

Comment on attachment 317841 [details] Patch Clearing flags on attachment: 317841 Committed r220549: <http://trac.webkit.org/changeset/220549>

All reviewed patches have been landed. Closing bug.