-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Conversation
@annevk, this is probably something you need to sign off on to make sure the approach is sane. |
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> |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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. :(
I missed your question. We navigate to a response in |
If you're OK with the rest of the patch, I think it still seems reasonable. |
The 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> |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
I see. That makes sense. |
<li><p class="XXX"><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> |
There was a problem hiding this comment.
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.
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 |
Ping, @domenic? |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/the //
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped.
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. |
Can all of the CSP algorithms handle being called with a null request? It seems like you pass that through in a few circumstances. |
Rebased and addressed your comments, @domenic. Thanks!
If y'all are happy with the approach in this patch, I'll update the CSP algorithms to exit early on a |
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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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="XXX">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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Closing </li>
missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
This will enable implementation of 'frame-ancestors' and 'form-action',
and makes a bit of progress towards #1230.