feat: add OPTIONS server method

[eltigerchino]

closes #5193

Continues from the work of #6030

• Adds the OPTIONS method as valid +server.js export.
• Disables Vite config options preview.cors and server.cors which were intercepting all OPTIONS http requests.
• Passes through Vite's CORS middleware during dev and preview

This delegates the responsibility of turning off Vite's CORS middleware to the user when vite's injected headers interfere with development of OPTIONS endpoints.

// vite.config.js
import { sveltekit } from '@sveltejs/kit/vite';
import { defineConfig } from 'vite';

export default defineConfig({
  plugins: [sveltekit()],
  server: {
    cors: false // disable cors for vite dev
  },
  preview: {
    cors: false // disable cors for vite preview
  }
});

TODO

• docs

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

[changeset-bot]

🦋 Changeset detected

Latest commit: 6bdf031

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[Rich-Harris]

I sorta feel like RequestHandler is the wrong API for OPTIONS — it feels like something that should mostly be handled by the framework, which already knows which methods are allowed for which route.

Perhaps the default behaviour should be to return a 204 response with allow and (if access-control-request-method is sent, access-control-allow-methods), and we expose some way to control access-control-allow-origin and access-control-max-age? (There's also access-control-allow-headers but I don't really understand what it's for.)

I'm not 100% sure what that looks like but it feels like exporting export const OPTIONS from every API route is a lot of unnecessary manual work.

[Conduitry]

access-control-allow-headers specifies which headers the sender is allowed to send. (It corresponds with the access-control-request-headers header in the OPTIONS call.) If the browser deems that this CORS request requires a preflight request (whether it's because it's not a GET, or it include authentication, or it includes custom headers), it will include access-control-request-headers in the request with all of the headers that would be sent in the real request (including default things like content-type) and it expects the response to include them in the access-control-allow-headers if they are to be allowed in the actual request.

[Rich-Harris]

but why? why wouldn't the server just disregard any headers it wasn't interested in?

[Conduitry]

I think it's not just 'would the server be generally interested in these headers?', it's also 'would the server want to trust these headers from this origin?'. CORS wants everything that could potentially be abused to be explicitly opt-in by every server, and not assume that it will filter out things that might be harmful.

I've long since stopped trying to second guess the CORS design. But if we want to support it beyond 'idk, you need to respond to the requests in your handle hook' then we'll have to just deal with the spec as it was written. and it was written to ask 'can this origin send a request with this method including these headers to you?' and the server can say what it wants to allow and how long the client is allowed to cache this information.

For this reason, I'd be wary about doing anything fancier than saying the app needs to handle this in its handle hook, and very wary about doing anything fancier than saying the app needs an OPTIONS export from a particular +server.js file.

[eltigerchino]

Would a combination of CORS helper methods inside an OPTIONS export be preferable to using setHeaders?

Perhaps there can also be a simpler method of setting CORS options across the board in addition to the manual control of an OPTIONS export.

Or would adding a recipe to the docs for setting CORS in +hooks.server.js be sufficient?
Something like the answer from #5193 (comment) ?

[Rich-Harris]

For this reason, I'd be wary about doing anything fancier than saying the app needs to handle this in its handle hook, and very wary about doing anything fancier than saying the app needs an OPTIONS export from a particular +server.js file.

Fair enough — that's certainly the path of least resistance.

Would a combination of CORS helper methods inside an OPTIONS export be preferable to using setHeaders?

Very possibly. Though I think it'd be worth shipping this and seeing if people ask for those helpers, rather than blocking on it. I've approved this PR, though since it was marked 'draft' I haven't merged it yet in case there was anything else we wanted to do here first

[eltigerchino]

Would it be beneficial to make a note in the docs about sveltekit's CORS behaviour during vite dev and vite preview?
For example, a blockquote that mentions vite automatically appending the access-control-allow-origin and access-control-allow-methods headers during these modes?

I ask because I wonder if this default behaviour could be misleading for developers creating their own OPTIONS endpoints.

Or should a recipe also be added to the docs that explain how to configure pre-flight request in hooks? (would address this recent issue #8963 and others that found #5193 (comment) helpful)

Other than that, this PR should be good to go.