Assuming the protected document is served over HTTP, the status-code field in the CSP report should be the response code for the protected document regardless of whether that document was blocked because its frame-ancestors directive was violated. Currently the status-code field in the CSP report is always 0 when a framed document is blocked due to its frame-ancestors directive being violated. For completeness here is the relevant CSP 3 text: [[ A violation represents an action or resource which goes against the set of policy objects associated with a global object. ... Each violation has a status which is a non-negative integer representing the HTTP status code of the resource for which the global object was instantiated. ]] <https://w3c.github.io/webappsec-csp/#framework-violation> (Editor's Draft, 16 February 2018) (I find it is easier to read the equivalent text from CSP 2: [[ status-code The status-code of the HTTP response that contained the protected resource, if the protected resource was obtained over HTTP. Otherwise, the number 0. ]] <https://www.w3.org/TR/CSP2/#violation-reports>)
(In reply to Daniel Bates from comment #0) > Currently the status-code field in the CSP report is > always 0 when a framed document is blocked due to its frame-ancestors > directive being violated. err, I meant to write: Currently, the status-code field in the CSP report is the status code of the document currently loaded in the frame (for an about:blank document its 0) when a new document that would be loaded into the frame is blocked due to its frame-ancestors directive being violated.
Created attachment 339691 [details] Patch
Created attachment 339692 [details] Patch
Comment on attachment 339692 [details] Patch Attachment 339692 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7589749 New failing tests: http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html
Created attachment 339699 [details] Archive of layout-test-results from ews206 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews206 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Build Bot from comment #4) > Comment on attachment 339692 [details] > Patch > > Attachment 339692 [details] did not pass win-ews (win): > Output: http://webkit-queues.webkit.org/results/7589749 > > New failing tests: > http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-video.html I am unclear how this change could cause this failure and the results.html page in the attached archive categorizes this failures as a crash, but no crash log is in the archive.
Comment on attachment 339692 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=339692&action=review Looks good! I had a few minor suggestions, but nothing you need to change if you disagree. r=me > Source/WebCore/ChangeLog:4 > + https://bugs.webkit.org/show_bug.cgi?id=185366 Is there a radar tracking this? If not please import to radar. Either way, please copy the radar # here. > Source/WebCore/dom/Document.cpp:3320 > + auto* documentLoader = frame ? frame->loader().documentLoader() : nullptr; Is the only reason we need documentLoader is for its response status code? Maybe we should just capture that here rather than embedding it twice below? > Source/WebCore/dom/Document.cpp:5515 > + auto* loader = m_frame->loader().documentLoader(); Should this be called 'documentLoader' instead? Should we just capture its httpStatusCode here instead of doing a ternary operator on line 5525?
<rdar://problem/40035116>
(In reply to Brent Fulgham from comment #7) > Comment on attachment 339692 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339692&action=review > > Looks good! I had a few minor suggestions, but nothing you need to change if > you disagree. r=me > > > Source/WebCore/ChangeLog:4 > > + https://bugs.webkit.org/show_bug.cgi?id=185366 > > Is there a radar tracking this? If not please import to radar. Either way, > please copy the radar # here. > Will do. > > Source/WebCore/dom/Document.cpp:3320 > > + auto* documentLoader = frame ? frame->loader().documentLoader() : nullptr; > > Is the only reason we need documentLoader is for its response status code? Yes. > Maybe we should just capture that here rather than embedding it twice below? > Before landing will update this to read: auto* documentLoader = frame ? frame->loader().documentLoader() : nullptr; auto httpStatusCode = documentLoader ? documentLoader->response().httpStatusCode() : 0; and reference httpStatusCode in the code below. > > Source/WebCore/dom/Document.cpp:5515 > > + auto* loader = m_frame->loader().documentLoader(); > > Should this be called 'documentLoader' instead? Will rename to documentLoader to better describe the loader it refers to. > Should we just capture its > httpStatusCode here instead of doing a ternary operator on line 5525? Notice we also use documentLoader to query its response for the taint mode. We could cache loader().documentLoader()->response() and use the null ResourceResponse to represent the case where a Document was created without a loader. This seems heavy weight since we have to stack allocate a null ResourceResponse compared to computing loader().documentLoader()->response() again.
(In reply to Daniel Bates from comment #9) > > Should we just capture its > > httpStatusCode here instead of doing a ternary operator on line 5525? > > Notice we also use documentLoader to query its response for the taint mode. > We could cache loader().documentLoader()->response() and use the null > ResourceResponse to represent the case where a Document was created without > a loader. This seems heavy weight since we have to stack allocate a null > ResourceResponse compared to computing loader().documentLoader()->response() > again. Makes sense! Keep your original design.
Committed r231464: <https://trac.webkit.org/changeset/231464>