cleanup processing section #54
Conversation
- declare var's before setting them· - simplify logic flow Closes #53.
index.html
Outdated
| <li>If <var>mimeType</var> value is a <a>CORS-safelisted | ||
| request-header</a> value for the <code>Content-Type</code> header, | ||
| set <var>corsMode</var> to "<code>no-cors</code>". | ||
| <li>Set <var>corsMode</var> to "<code>cors</code>".</li> |
There was a problem hiding this comment.
is it intentional that you chose to set the mode to cors here and then revert at L424 than setting the mode to cors when the contentType is not CORS safelisted?
this means that the mode will be cors when contentType is null here.
There was a problem hiding this comment.
Yes, because the intent is to follow a (strict) whitelist model: #33 (comment).
There was a problem hiding this comment.
/cc @annevk
Yeah, it's important that we set the default to the stronger mode. The switch from blacklist to whitelist in 1073d61 itself really makes sense. But the concern raised by Anne was caused by:
- setting the mode separately and far away from the logic to set the Content-Type without checking what's going to be set to the Content-Type.
- switching on the type here inside the beacon spec.
The current spec doesn't do the switching on the type here but is depending on the fetch spec by invoking the extract algorithm. So, the second point is no longer a concern.
The current code is written in the form that determines both of the mode and the Content-Type to append at the same place. With this, I don't think we should be too much careful.
Setting the mode to cors when we're not adding any Content-Type header doesn't make sense and we should fix it if possible. As the concerns are gone as described above, I think we can consider fixing it. WDTY?
I'm fine with initializing the mode to cors at L417, but want to suggest that we add "Otherwise" step between L428 and L429 to set corsMode to no-cors.
There was a problem hiding this comment.
Setting the mode to cors when (we have a POST payload and) we're not adding any Content-Type header doesn't make sense and we should fix it if possible.
@toddreifsteck @annevk thoughts?
|
@tyoshino updated, ptal. I don't think we need the extra otherwise, we can just move the "cors" inside the if content-type block. Does that look good? |
|
Thank you, Ilya. After writing the last comment, I wondered if I'm still missing something from the discussion you all made previously. I'll summarize my thoughts here so that we can confirm that everyone's on the same page. As discussed in #32 (comment), to ease migration from the image ping approach, we considered relaxing the CORS requirements, and we concluded turning off the CORS check perspective of the CORS algorithm for sendBeacon() is acceptable, while turning off the CORS preflight is not. So, we need to make it perform the CORS preflight when needed. The only way for sendBeacon() to be subject to it is when to add a Content-Type with non-CORS-safelisted value. So, when we don't add any Content-Type header, it's not subject to the requirement. When fetch() is used with the no-cors mode and the POST method but without the Content-Type header (even if we try to add it, |
|
That matches my analysis of the situation. |
|
Ditto. Merging, thanks all! |
Closes #53.
Preview | Diff