Bug 185366 – CSP status-code incorrect for document blocked due to violation of its frame-ancestors directive

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Daniel Bates]

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>)

[Daniel Bates]

(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.

[Daniel Bates]

Created attachment 339691 [details]
Patch

[Daniel Bates]

Created attachment 339692 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Daniel Bates]

(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.

[Brent Fulgham]

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?

[Radar WebKit Bug Importer]

<rdar://problem/40035116>

[Daniel Bates]

(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.

[Brent Fulgham]

(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.

[Daniel Bates]

Committed r231464: <https://trac.webkit.org/changeset/231464>