Skip to content

Conversation

@mikewest
Copy link
Member

I don't think that GitHub supports PRs on top of PRs, so this might be a little annoying: the only relevant patch for this PR is 2e60efb. The other patch is part of #272. Do you know of a better way to do this? I can just send one big PR with each change split out into a separate patch, but I suspect that would be frustrating to review.

@mikewest mikewest changed the title Csp list Add 'CSP list' to global objects. Oct 21, 2015
@mikewest
Copy link
Member Author

@annevk: Ping? :)

@annevk
Copy link
Member

annevk commented Oct 24, 2015

I've been thinking about where to store state between Window and Document and I think Document may be more correct after all. Since Document is (optionally) kept around for quick history traversal and only Window can really get replaced (when the Document is replaced it's the result of navigation). I'm not sure if that accurately matches implementations though. @bzbarsky?

@bzbarsky
Copy link
Contributor

When the Document is kept around for quick history traversal, I believe so is the Window. Certainly in Gecko's implementation that's the case; I haven't sorted carefully through this part of the navigation spec recently.

A Window can get replaced via document.open, though. The only thing that replaces the Document while keeping the same Window is same-origin navigation from initial about:blank.

None of that tells me where we want to store this thing, because I'm not sure what it is we're storing and what it's used for. It's a list of some sort of objects, but it's not clear to me whether these are JS objects (in which case storing on the Window is likely to be the sane thing to do) or some other sort of objects. In any case, I'm 99% sure that copying it from the entry settings object is wrong, whatever it is. ;)

@mikewest
Copy link
Member Author

I've been thinking about where to store state between Window and Document and I think Document may be more correct after all.

We need to store this data for Worker as well. Given that there's no Document in those cases, it made sense to me to store it on the global in both cases.

it's not clear to me whether these are JS objects (in which case storing on the Window is likely to be the sane thing to do) or some other sort of objects

These aren't (currently) JavaScript objects, they're just representations of the policy that applies to a context. That said, I'm putting together an API that will sit on top of these lists in order to expose the state of the context, however, so they will eventually be indirectly exposed to script.

@bzbarsky
Copy link
Contributor

OK. Does the policy change when document.open() happens? Does it change when navigation from initial about:blank to a same-origin URI happens? Does it change during random DOM manipulations? Due to pushState calls?

@mikewest
Copy link
Member Author

Does the policy change when document.open() happens?

No. document.open() doesn't change the execution context, so the policy that was active for the document should remain active.

Does it change when navigation from initial about:blank to a same-origin URI happens?

Yes. It would be populated from the response from the network that contained various headers that set the policy for the resource.

Does it change during random DOM manipulations?

If the manipulation is the addition of a <meta http-equiv="Content-Security-Policy" content="..."> element to a document's <head>, a policy could be added to this list. Otherwise no.

Due to pushState calls?

No. pushState doesn't update the execution context, so shouldn't change the policy applied to that context.

@bzbarsky
Copy link
Contributor

Can you point me to the definition of "execution context" as being used here?

But in any case, given those answers hanging this off a Window is OK if it gets copied over during document.open() and gets cleared during the navigation from initial about:blank (which could have a nontrivial CSP list if someone tossed a <meta> into its DOM). Or of course you could hang it off the Document and not have to say anything special about either case... I understand that's not an option with workers, of course. It's a question of where you want the extra spec complexity: in document.open() and the navigation algorithm or in whatever the procedure is for getting this list. My personal vote would be for the latter, because document.open() and navigation have quite enough complexity already. ;)

@mikewest
Copy link
Member Author

Can you point me to the definition of "execution context" as being used here?

I don't have a good definition. It's hand-waving on my part to describe the vague feeling that nothing interesting changes about code that's executed before and after document.open(). :)

It's a question of where you want the extra spec complexity...

Happy to leave this up to @annevk. Tell me where you want stuff, and I'll give you patches. (This is somewhat related to the HTTPS state discussion we just had, and the notion that it might make sense to extract "inheritance" out into something reusable).

@bzbarsky
Copy link
Contributor

It's hand-waving on my part to describe the vague feeling that nothing interesting changes about code that's executed before and after document.open()

Well. The global object changes. All the stuff hanging off the global except .document changes. The base URI changes. The document URI changes. The conceptual source of the content changes, in some broad sense. I agree that it's probably somewhat edge-casey in terms of what should happen with CSP. And in terms of HTTPS state, perhaps. Or at least there are no compat constraints on it, unlike all that other stuff, so we can try to spec it in as simple a way as possible, as long as it makes some sense in practice.

@annevk
Copy link
Member

annevk commented Oct 28, 2015

I think we should probably continue Ian's tradition of putting state on Document until we have a better idea.

@mikewest
Copy link
Member Author

Will rework this patch after TPAC, thanks.

source Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This needs an extra newline.

@mikewest
Copy link
Member Author

mikewest commented Nov 5, 2015

Taken care of both nits. Happy to either squash, or to fix up other issues you find. :)

source Outdated
Copy link
Member

Choose a reason for hiding this comment

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

The extra newline needed to go below, not above :-)

With this addressed, and squashed/rebased, I think we're good.

Content Security Policy adds a new property to the global object that
holds the active policy objects for a context. This patch merges this
property into HTML, and initialises it whenever creating new Document
and Worker objects.

#271
@mikewest
Copy link
Member Author

mikewest commented Nov 5, 2015

Moved the newline, squished. Thanks. :)

@annevk annevk merged commit 479dfbf into whatwg:master Nov 5, 2015
@annevk
Copy link
Member

annevk commented Nov 5, 2015

Thank you!

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.

3 participants