Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modified initialized document's CSP to match changes in CSP spec #4190

Merged
merged 3 commits into from Nov 26, 2018

Conversation

andypaicu
Copy link
Contributor

@andypaicu andypaicu commented Nov 22, 2018

Slight changes in the initialize a document's CSP call parameters have been made in the CSP spec
w3c/webappsec-csp#358
and later
w3c/webappsec-csp#365

This PR mirrors those modifications.

Tracking issue w3c/webappsec-csp#364

data-x="concept-response">response</span> used to generate the document. <ref spec="CSP"></p>
algorithm on the <code>Document</code> object, the <span data-x="concept-response">
response</span> used to generate the document, and the <span data-x="concept-request">
request</span> used to generate the document. <ref spec="CSP"></p>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we simply pass response and request here? It's not clear to me why the original text uses concept-response. Also, does CSP anticipate request sometimes being null?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've modified it.

CSP does not anticipate the request being null. I've tried but failed to follow in which situations the request is null. From what I can tell, step 13 here could call process a navigation response with a null request but it's not clear to me in which situation this would occur. When is navigation called with a response-type resource?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

srcdoc uses this. There was also a document.open() situation, but that got removed. I thought javascript: used it too, but that seems to pass along the request as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is concerning because srcdoc is one of the scenarios where we want to inherit from the navigation initiator. What serves as the source browsing context for srcdoc navigations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a distinct argument to the navigation algorithm not carried by request. Though note #1130. It should probably be "source document" or some such.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I meant that https://html.spec.whatwg.org/#process-the-iframe-attributes does not make it clear what the source browsing context (or the navigation initiator, as it makes more sense in my head) for a srcdoc navigation is. And https://html.spec.whatwg.org/#navigate does not explain anything other than that it involves a source browsing context.

Looking at it again it seems like the node document's CSP list is cloned onto the response's CSP list which seems to imply that the navigation initiator is always the embedding document. I'm not entirely sure this is correct. What if a different context sets the srcdoc attribute on a frame? It's possible that this was relying on the old parent/opener inheritance model that I'm trying to change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure, you'd have to test I'm afraid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will update the CSP spec to be able to handle a null request. For now the existing srcdoc behavior is fine.

@annevk
Copy link
Member

annevk commented Nov 23, 2018

Also, should this really be something like "run XXX with AAA, BBB, and CCC"? We're not executing it upon the passed arguments, are we?

@andypaicu
Copy link
Contributor Author

Also, should this really be something like "run XXX with AAA, BBB, and CCC"? We're not executing it upon the passed arguments, are we?

I'm happy to use whatever terminology seems most appropriate. The CSP spec says:
§4.2.1 Initialize a Document's CSP list is called during the [bla bla bla]

What should it be then, something like call XXX using a,b, and c as parameters?

@annevk
Copy link
Member

annevk commented Nov 23, 2018

Yeah, but we use "run" rather than "call" normally.

@andypaicu
Copy link
Contributor Author

I've changed it to run. Let me know if you think the patch looks good now.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this seems fine, thanks. You'll take care of the tests as part of updating CSP? Is there a follow-up issue already?

@andypaicu
Copy link
Contributor Author

Yeah I'm in the process of adding general inheritance tests as part of https://chromium-review.googlesource.com/c/chromium/src/+/1314633

Specific tests for the srcdoc case discussed above are also in progress: https://chromium-review.googlesource.com/c/chromium/src/+/1350889

The tracking issue is w3c/webappsec-csp#364 where I've added a comment about what needs to be updated in CSP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants