Allow HTTP scheme fetches to make CORS preflight for navigations

[alexpetros]

This allows navigations to make CORS preflight requests, if the navigation contains a request that is not safelisted. Navigation does not (yet) provide APIs for non-safelisted requests; this update to the fetch spec is a prerequisite for HTML spec changes that might add those APIs (i.e. PUT method support in forms).

No web platform tests or MDN updates are included, as no changes to browser functionality are required by this update alone.

This change supports the work outlined in this WHATWG Issue #3577 comment.

• At least two implementers are interested (and none opposed):
  • …
  • …
• Tests are written and can be reviewed and commented upon at:
  • N/A
• Implementation bugs are filed:
  • Chromium: …
  • Gecko: …
  • WebKit: …
  • Deno (not for CORS changes): …
• MDN issue is filed: N/A
• The top of this comment includes a clear commit message to use.

(See WHATWG Working Mode: Changes for more details.)

---

Preview | Diff

[domfarolino]

So the idea with this PR is that <var>makeCORSPreflight</var> variable is really closer to "makeCORSPreflight if needed", and can be set to true because it won't trigger preflights except for when requests otherwise demand them by using an unsafe method or request header?

But I think this PR will have side effects on other requests that would suddenly start triggering preflights, when they shouldn't. For example:

fetch("https://google.com", {method: 'GET', mode: 'no-cors', headers: {'Content-Type': 'application/json'}})

... uses an unsafe request header, which would ordinarily trigger a preflight, but for the no-cors mode which never sends preflights¹. I think this PR would change that, and preflights would be sent for these kinds of no-cors requests, at the very least.

Another way to achieve this is to set the use-CORS-preflight flag on the relevant kinds of navigation requests that we want to subject to preflights. Unfortunately, that wouldn't integrate cleanly with "Main fetch" today, given how mode=navigate fetches are handled before we consider the use-CORS-preflight flag, just a few conditions down.

So maybe the best option is to modify scheme fetch to only turn a false "makeCORSPreflight" to true if the request mode is navigate (instead of unconditionally as you're doing now)?

Footnotes

1. This is because scheme fetch is invoked from the no-cors path, which today calls HTTP fetch without the makeCORSPreflight variable set to true, so we would never even test the method or request headers. ↩

[alexpetros]

@domfarolino First of all, thank you so much for the thoughtful review!

So the idea with this PR is that makeCORSPreflight variable is really closer to "makeCORSPreflight if needed", and can be set to true because it won't trigger preflights except for when requests otherwise demand them by using an unsafe method or request header?

Yes, this is my understanding of HTTP Fetch Step 4.1, which only issues the preflight if the request has attributes (method or headers) that are not CORS-safelisted (or if the use-CORS-Preflight flag is set); otherwise, it proceeds as normal (the path navigation takes today). I'm happy to rename it, but my instinct is to touch as little as possible.

Another way to achieve this is to set the use-CORS-preflight flag on the relevant kinds of navigation requests that we want to subject to preflights. Unfortunately, that wouldn't integrate cleanly with "Main fetch" today, given how mode=navigate fetches are handled before we consider the use-CORS-preflight flag, just a few conditions down.

I agree that this is not ideal. It would also create some confusion around when exactly CORS is supposed to be checked in the request lifecycle. Since it already happens in one path that the navigation fetch will definitely reach (HTTP fetch), I'd like to re-use that logic if possible.

So maybe the best option is to modify scheme fetch to only turn a false "makeCORSPreflight" to true if the request mode is navigate (instead of unconditionally as you're doing now)?

This seems great, and more narrow. There's also precedent for inspecting the request mode after that main switch. I will go ahead and make that change!

[domfarolino]

I think the current approach is at least safe, and avoids the scary side effects pointed out further above. I think it'd be great to get a gut check from @annevk on this now.

[domfarolino]

Non-editor LGTM

[annevk]

I think these need to be separate steps as this is confusing. (Though not in the way you had before. Initialize the boolean in one step, then make the HTTP fetch call.) It also misses a bunch of markup for the various concepts.

Then, I wonder if we need to identify these preflights are for navigations in some respect to the server as they might well want to enforce different policies between subresources and navigations. Perhaps Sec-Fetch-Dest suffices for that. Ideally we find something here that is unified with Private/Local Network Access.

I would also like to research why we identify whether a request needs a preflight in two separate locations currently. I can't remember and having now read it a few times I don't understand. Any chance you want to dig into blame? If not I'll get to it in due course.

The name makeCORSPreflight also seems rather confusing as it doesn't actually mean a CORS preflight will be made.

[alexpetros]

Thanks very much @annevk for the excellent review.

I think these need to be separate steps as this is confusing. (Though not in the way you had before. Initialize the boolean in one step, then make the HTTP fetch call.)

I agree and I'll make that change.

Then, I wonder if we need to identify these preflights are for navigations in some respect to the server as they might well want to enforce different policies between subresources and navigations. Perhaps Sec-Fetch-Dest suffices for that. Ideally we find something here that is unified with Private/Local Network Access.

Strongly support this. I wonder if Sec-Fetch-Mode also helps here—since we're using the navigation request mode, I think it follows that Sec-Fetch-Modewould unambiguously be set to navigation, even for the preflight. If PLNA didn't create a new mode, it would presumably also use this (if the request did originate from a link or form). (incorrect, see below)

I would also like to research why we identify whether a request needs a preflight in two separate locations currently. I can't remember and having now read it a few times I don't understand. Any chance you want to dig into blame? If not I'll get to it in due course.

Happy to take a crack at it as well.

The name makeCORSPreflight also seems rather confusing as it doesn't actually mean a CORS preflight will be made.

I defaulted to making as few changes as possible, although I completely agree (and that name certainly contributed to a couple extra hours of work trying to parse the spec). With your invitation I am happy to rename it—how do you feel about allowCORSPreflight?

[annevk]

(Sec-Fetch-Mode will be set to "cors" for the preflight. See https://fetch.spec.whatwg.org/#cors-preflight-fetch-0 step 1.)

[alexpetros]

Oh, you're right, thanks for the link.

I think Sec-Fetch-Dest works! The definition of the document value is: "HTML’s navigate algorithm (top-level only)," which is what we want. This also makes it possible for forms to add new targets in the future without impacting servers who only want CORS for top-level navigation today.

It should also apply to PNLA links.

[domfarolino]

impacting servers who only want CORS for top-level navigation today.

But just to be clear, this PR allows CORS for non-top-level navigations too, right?

[alexpetros]

Yep. This would allow CORS for any fetch with the navigate mode. We're just saying that servers will have the opportunity to disallow CORS-enabled navigations with even more granularity (top-level, iFrame, and so on), based on the Sec-Fetch-Dest header, if they so choose.

[alexpetros]

Made Anne's suggested updates: split out "is this for navigation" into its own variable, and renamed makeCORSPreflight to allowCORS, which better captures how the variable is used.

I don't think there are any updates required for Sec-Fetch-Dest; if you feel that other headers are necessary, definitely let mee know.