-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
[html] Add tests for parsing COOP values #20873
[html] Add tests for parsing COOP values #20873
Conversation
I think we need to clarify whether the plan is for COOP to be a structured header or not, since that impacts various results... |
Has that change been discussed online? |
Not to my knowledge; I just kind of thought we were not creating any new non-structured headers. (And, it'd be pretty weird if COOP was one thing and COEP another.) |
Got it. I filed whatwg/html#5172 to discuss this |
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.
As per whatwg/html#5172 (comment) I think this is the way to go, perhaps with a comment indicating we want to change this once SH is an RFC.
42287c9
to
2843a50
Compare
I've now updated this to match the current specs.
|
I also removed the A few tests time out for me in Chrome. Not sure why. |
I had misunderstood the layering between HTTP and SFV. HTTP still trims OWS around the field-value: httpwg/http-extensions#1251 (comment) Now fixed. |
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.
I have not done an extremely detailed take, but I'm assuming that the set of cases that now cause a policy to be enforced is strictly additive to what we had originally. If that's not the case let's revisit.
Thanks @annevk. Some tests here are still getting TIMEOUTs in both chrome and firefox. I tried to figure out what is causing it, without luck so far. For example, the Am I missing something obvious? |
It might be the Python header API not dealing with invalid values the way you would want for testing purposes. If that's the case you could write out the entire message instead in raw... |
The procedural test generation logic derives a name for the BroadcastChannel from the COOP value. Just like the COOP itself, the channel name is passed around via query string parameters, so we have to take care to URL-encode it as well. The latest commit fixes the timeouts. Chromium passes all 24 subtests, and Firefox fails 2:
|
Thank you @jugglinmike! The stability checks fail because they time out. I think since changing @Hexcles I can open a new PR to reintroduce the null byte test, to decouple from the rest here. Thanks! |
@annevk you have reviewed this but there were 2 commits since. |
|
||
// None of the following should be recognized as "same-origin" (hence the | ||
// "expected opener" value of `true`). | ||
[SAME_ORIGIN, "same\u2014origin", true], // non-ASCII character (em dash) |
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.
Having seen the COEP comments I guess we should address here too what bytes are going to be emitted by the server. And what shortcomings this approach has (in that 0xFF isn't feasible). Perhaps we should change approach so we can effectively specify bytes?
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, good point. I've changed it now for this PR to specify bytes.
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.
Did you check that other users of common.js not rely on encodeURIComponent? Especially reporting tests might be impacted, perhaps?
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.
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.
That seems reasonable, though I'd name it percentEncoded
.
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! I'll admin-merge, per #20873 (comment) |
No description provided.