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

Upstream navigation hooks from CSP #1619

Merged
merged 2 commits into from
Aug 18, 2016
Merged

Upstream navigation hooks from CSP #1619

merged 2 commits into from
Aug 18, 2016

Conversation

mikewest
Copy link
Member

@mikewest mikewest commented Aug 1, 2016

This will enable implementation of 'frame-ancestors' and 'form-action',
and makes a bit of progress towards #1230.

@mikewest
Copy link
Member Author

mikewest commented Aug 1, 2016

@annevk, this is probably something you need to sign off on to make sure the approach is sane.

@mikewest
Copy link
Member Author

mikewest commented Aug 2, 2016

Actually, hold off a moment. I need to make this a little bit more complicated. :)

<li><p>Let <var>navigationType</var> be "<code data-x="">form-submission</code>" if the <span
data-x="navigate">navigation algorithm</span> was invoked as a result of the <span
data-x="concept-form-submit">form submission algorithm</span>, and "<code data-x="">other</code>"
otherwise.</p></li>
Copy link
Member

Choose a reason for hiding this comment

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

At some point I'd like to get rid of this action-at-a-distance. It might be okay for now...

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah. I was surprised to see it, but since it's there... I was lazy. :(

@annevk
Copy link
Member

annevk commented Aug 16, 2016

I missed your question. We navigate to a response in <object>, <embed>, with about:srcdoc, and I think with document.open().

@mikewest
Copy link
Member Author

I missed your question. We navigate to a response in <object>, <embed>, with about:srcdoc, and I think with document.open().

null seems reasonable for about:srcdoc and document.open(). I'm surprised that it happens for <object> and <embed>, but I don't think it affects any of the current usage for this new hook. XFO and frame-ancestors apply to things loaded via <object>/<embed> but that only requires looking at the response. The only thing currently looking at the request is form-action, which doesn't come into play when there's not a request to process.

If you're OK with the rest of the patch, I think it still seems reasonable.

@annevk
Copy link
Member

annevk commented Aug 16, 2016

The way <object data> and <embed src> work is that they fetch the URL first and then navigate to the response if they end up creating a browsing context for the response. Otherwise we'd have to fetch the data twice. Once there is a browsing context you can navigate normally in it of course, but those attributes would still behave in this way.

data-x="concept-request">request</span> <var>request</var> and <span>browsing context</span>
<var>browsingContext</var>, run these steps:</p>
data-x="concept-request">request</span> <var>request</var>, <span>browsing context</span>
<var>browsingContext</var>, and string <var>type</var>, run these steps:</p>
Copy link
Member

Choose a reason for hiding this comment

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

We should pass this "source browsing context" as well. Just like you do for "process a navigate response".

Copy link
Member

Choose a reason for hiding this comment

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

Or would that get too messy further down? Could leave that as a TODO I suppose.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should probably be passing it into "navigate", honestly. But that sounds like work. :)

Passing it into "process a navigate fetch" as of b357bbb.

@mikewest
Copy link
Member Author

I see. That makes sense.

<li><p class="&#x0058;&#x0058;&#x0058;"><var>response</var>'s <span data-x="concept-response-header-list">header
list</span> contains a header named `<code data-x="">X-Frame-Options</code>` whose value
expresses constraints which are violated by <var>browsingContext</var> (TODO: Spell this out)
<ref spec="RFC7034"></p></li>
Copy link
Member

Choose a reason for hiding this comment

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

This RFC needs to be added to the references.

@mikewest
Copy link
Member Author

XFO needs more work; reduced it down to a bare TODO so that it's clear where the processing should happen in the future, but we'll need to actually spell it out here, as there's nothing in the RFC that we can easily point off to (perhaps @randomdross can take a stab at an HTMLized algorithm?).

Assigning @domenic to sign off on the final set of changes, per @annevk's comments in IRC: http://logs.glob.uno/?c=freenode%23whatwg#c1002867

@mikewest
Copy link
Member Author

Ping, @domenic?

@domenic
Copy link
Member

domenic commented Aug 17, 2016

In meetings all day today, can try to do tomorrow...

<var>browsingContext</var>, run these steps:</p>
data-x="concept-request">request</span> <var>request</var>, <span>browsing context</span>
<var>sourceBrowsingContext</var>, <span>browsing context</span> <var>browsingContext</var>, and
string <var>type</var>, run these steps:</p>

<ol>
<li><p>Let <var>response</var> be null.</p></li>

<li><p>Set <var>request</var>'s <span data-x="concept-request-client">client</span> to the
Copy link
Member

@domenic domenic Aug 17, 2016

Choose a reason for hiding this comment

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

s/the //

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropped.

@domenic
Copy link
Member

domenic commented Aug 17, 2016

Kind of sad the arguments lists grew so much but I guess there's not much you can do there. Found some editorial things worth tightening up.

@domenic
Copy link
Member

domenic commented Aug 17, 2016

Can all of the CSP algorithms handle being called with a null request? It seems like you pass that through in a few circumstances.

@mikewest
Copy link
Member Author

Rebased and addressed your comments, @domenic. Thanks!

Can all of the CSP algorithms handle being called with a null request? It seems like you pass that through in a few circumstances.

If y'all are happy with the approach in this patch, I'll update the CSP algorithms to exit early on a null request when relevant. Should be a trivial change.

This will enable implementation of 'frame-ancestors' and 'form-action',
and makes a bit of progress towards #1230.
data-x="concept-origin-opaque">opaque origin</span>, and abort these steps.</p>

<ul>
<li><var>response</var> is a network error</li>
Copy link
Member

Choose a reason for hiding this comment

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

Use <p> here as well.

Copy link
Member

Choose a reason for hiding this comment

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

Also needs a dot (or no dots anywhere in the list I suppose).

Copy link
Member Author

Choose a reason for hiding this comment

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

Done, thanks!

<ul>
<li><var>response</var> is a network error</li>
<li><p class="&#x0058;&#x0058;&#x0058;">TODO: Define <code data-x="">X-Frame-Options</code>
processing here [<a href="https://github.com/whatwg/html/issue/1230">whatwg/html#1230</a>].</p>
Copy link
Member

Choose a reason for hiding this comment

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

Closing </li> missing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@annevk annevk merged commit 2083b00 into whatwg:master Aug 18, 2016
@mikewest mikewest deleted the navigate-csp branch August 18, 2016 08:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants