Add Last-Event-ID to CORS-safelisted headers
#49257
Conversation
The EventSource API does _not_ run any preflight request when specifying the `Last-Event-ID` header, and so `fetch` requests should also allow this header to be set manually. See whatwg/fetch#568 for more information
rexxars
force-pushed
the
fetch-cors-safelist-last-event-id
branch
from
746a146 to
79c1fed
Compare
Makes sense - added in c80e8b3 |
rexxars
force-pushed
the
fetch-cors-safelist-last-event-id
branch
from
79c1fed to
c80e8b3
Compare
I wasn't sure if the spec changes had to be approved/merged before making this ready for review. I'll mark it as ready now, assuming based on your comment that this is acceptable. |
wpt-pr-bot
requested review from
hillbrad,
jdm,
mnot,
odinho,
Yaffle,
youennf and
yutakahirano
| // 2. Will emit either `success` or `failure` event. We expect `success`, | ||
| // which is the case if `last-event-id` is set to the same value as received above, | ||
| // and a preflight request has been sent for the unsafe `last-event-id` headers. |
There was a problem hiding this comment.
Wait shouldn't this be open and error events?
There was a problem hiding this comment.
Hmm, the way I read the spec/ask here is that if the last-event-id header contains any "unsafe bytes", AND the request is going cross-origin, then we should do a preflight request that contains access-control-request-headers: last-event-id. If the server then responds with access-control-allow-headers: last-event-id, we are okay to send the "unsafe" value to the server. If the server does not respond to the preflight request with that header, it will fail the request. Can you confirm if this is the intended behavior, and if it is not correct, outline what the correct behavior is?
As for the use of success/failure: We could theoretically send back a 400 or something if the preflight wasn't run (thus triggering the error event), but the EventSource API purposely gives very little information on errors (basically nothing at all). If we use the generic error event, it's hard/harder to then detect why the request failed - it could be because of an HTTP error code, because the stream ended (and triggered a reconnect), because the network is down, because of CORS issues etc. I prefer to handle things more explicitly - by sending named events for success/failure we'll have access to the data field and can use that in the error output, eg "expected preflight, did not get one" or "got , expected ". I find it both helps implementers figure out what is going wrong, as well as anyone reading the text/fixture, as the different cases are more explicitly called out.
| source.addEventListener('success', this.step_func_done()); | ||
| source.addEventListener('failure', this.step_func_done((evt) => { | ||
| assert_unreached(evt.data); | ||
| })); |
There was a problem hiding this comment.
You can also use this.assert_unreached though I don't think you can output event.data in that case.
There was a problem hiding this comment.
As mentioned in the above comment - I prefer to give a helpful error message when possible to help implementers understand which part of the test is failing, which is why I went for the variant where we can provide a message.
The EventSource API does not run any preflight request when specifying the
Last-Event-IDheader, and sofetchrequests should also allow this header to be set manually.See issue whatwg/fetch#568 for more information and PR whatwg/fetch#1788 for changes to the specification