unjs / nitro

Next Generation Server Toolkit. Create web servers with everything you need and deploy them wherever you prefer.
https://nitro.unjs.io
MIT License
6.14k stars 505 forks source link

`cors` route rule should auto response to preflight #2340

Open MickL opened 6 months ago

MickL commented 6 months ago

Environment

nitropack: 2.9.6

Reproduction

https://stackblitz.com/edit/github-xaqmuq?file=nitro.config.ts

Describe the bug

Setting cors: true to does not work:

routeRules: {
    '/**': {
      cors: true,
    },
},

The browser still throws cors error:

Access to fetch at 'http://localhost:3333/' from origin 'http://localhost:3001' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: No 'Access-Control-Allow-Origin' header is present on the requested resource. If an opaque response serves your needs, set the request's mode to 'no-cors' to fetch the resource with CORS disabled.

With header, too:

routeRules: {
    '/**': {
      cors: true,
      headers: {
        'Access-Control-Allow-Origin': '*',
        'Access-Control-Allow-Headers': '*',
      },
    },
  },

Error:

Access to fetch at 'http://localhost:3333/' from origin 'http://localhost:3001' has been blocked by CORS policy: Response to preflight request doesn't pass access control check: It does not have HTTP ok status.

Additional context

I already opened an issue at Nuxt that setting corse:true does not have an effect on /api routes, only on /public folder.

Logs

No response

MickL commented 5 months ago

Hi @pi0 @danielroe , I digged deeper into this and found a critical bug with route rules not beeing applied. Maybe a problem with https://github.com/unjs/radix3?

The following rule works for all routes:

routeRules: {
    '/**': {
      cors: true,
      headers: {
        'Access-Control-Allow-Origin': '*',
        'Access-Control-Allow-Headers': '*',
      },
    },
  },

BUT if a route does not have GET route, the routeRoule is not applied. E.g.:

/products.ts
/products.patch.ts
/items.patch.ts -> Route rule not applied

Therefor the routes without a GET route throw a cors error in my case. This could also explain https://github.com/nuxt/nuxt/issues/25686

I made a reproduction repo, you can simply run bun install and then bun dev to start a Nuxt app at localhost:3000 and a Nitro app at localhost:3001

https://github.com/MickL/nitro-route-rules-bug

MickL commented 4 months ago

It gets even more weird!

The route rules are applied if there is a GET route, e.g. contact.ts and contact.post.ts BUT if the GET route just throws an error it is NOT applied:

Route rule applied for POST route:

// contact.ts
export default defineEventHandler(async (event) => {
   return '404';
});

Route rule NOT applied for POST route:

// contact.ts
export default defineEventHandler(async (event) => {
  throw createError({
    statusCode: 404,
  });
});
B-E-A-F commented 2 months ago

This is insane. I spent the last 8 hours trying to figure out what was going on with my CORS headers not being set. I added a GET route and it fixed all of my problems.

I'm genuinely regretting using nitro.

I don't quite understand how you were able to figure this out but big shout out to @MickL for saving my work project.

B-E-A-F commented 2 months ago

Possibly related to #1806

MickL commented 2 months ago

This is definitely a critical bug and an annoying one as well. Maybe it will be fixed with v3? @pi0

But in any case it needs to be fixed for v2 aswell. This also makes problems within Nuxt: https://github.com/nuxt/nuxt/issues/25686

pi0 commented 2 months ago

I have created a better sandbox to show Nitro behavior:

https://stackblitz.com/edit/github-mdtiap?file=server%2Froutes%2Fapi%2Ftest.post.ts

(update: you need to run it locally and use curl -v -X POST http://localhost:3000/api/test. stackblitz won't work)

image

Nitro actually do apply route rules to all methods regardless if there is a handler registered for them or not. (notice that there are cors headers to the post-only API route's response).

The issue is how CORS preflight works in browsers. Browsers use an OPTIONS request to check CORS availability (not even GET) and the reason adding [path].ts (without method suffix) fixes the problem is that it also handles that.

I understand it can be frustrating with CORS issues but it isn't a bug at least with how route rules work.

(...so what we can improve...)

I understand DX is not good and CORS issues, god knows how much frustration they cause every developer, and that when Nitro allows to set a magical route rule cors: true, we expect that it just works, and it should be.

We can improve our route rule handler to auto-respond to OPTIONS even if there is not a handler but the problem is that it is too soon. Route rules are applied before any of the user handlers and if a user wants to actually handle OPTIONS method of a path (to apply their security policy -- the very purpose of CORS functionality) nitro will implicitly override this. So while it might look like an easy fix/feature, it might cause security implications which are worst than a not working CORS call when it matters.

I will be thinking about how we can safely implement this as a fallback approach (nitro error handler could be one place, by checking cors route rule) but i think one first and good step is that we at least document this:

CORS functionality needs API handlers to respond to OPTIONS requests and as a userland solution, one can add server/api/[...path].ts

// server/api/[...path].ts
export default defineEventHandler((event) => {
  if (event.method === 'OPTIONS') {
    return ""
  }
});
B-E-A-F commented 2 months ago

Why do we have

Do these only work in a [...path].ts file as well?

pi0 commented 2 months ago

@ArcherScript Preflight request is not GET, it is OPTIONS.

I suggest you read MDN docs about Preflight request and CORS mechanism first.

Adding server/api/test.ts adds an event handler that handles all methods (GET, POST, OPTIONS, etc) and fixes the issue regardless of what it responds even an empty string with no headers is enough because route rule already adds * cors headers for us.

Browsers expect to have an "ok" response when they want to verify CORS (via OPTIONS fetch). If there is no event handler, Nitro (your application), responds with 404 or 405 both unacceptable by browsers.


You can alternatively use handleCors h3 utility (instead of route rule shortcut that simply adds headers) to have better control over CORS strategy but you still need to add it to a catch all handler (without .post suffix)

B-E-A-F commented 2 months ago

@ArcherScript Preflight request is not GET, it is OPTIONS.

I suggest you read MDN docs about Preflight request and CORS mechanism first.

Adding server/api/test.ts adds an event handler that handles all methods (GET, POST, OPTIONS, etc) and fixes the issue regardless of what it responds even an empty string with no headers is enough because route rule already adds * cors headers for us.

Browsers expect to have an "ok" response when they want to verify CORS (via OPTIONS fetch). If there is no event handler, Nitro (your application), responds with 404 or 405 both unacceptable by browsers.

You can alternatively use handleCors h3 utility (instead of route rule shortcut that simply adds headers) to have better control over CORS strategy but you still need to add it to a catch all handler (without .post suffix)

Yeah this is what I thought. Thanks for the explanation.

It seems like server/api/route.ts is the ONLY option if you are trying to make a request from the browser. Which is not clear from the docs.

I understand why it is that way though from the explanation.

B-E-A-F commented 2 months ago

Should we expand the documentation on handleCors and other cors helper methods so that it explains how these only function in route.ts files?

pi0 commented 2 months ago

Surely feel free to contribute to h3 docs if you could successfully use it 🙏🏼 jsdocs is here.

Mind you, nitro route rules cors handler does not use h3 utility btw. it uses simple headers (why? because it can be natively applied to deployment provider route rules).

I think we might also add a new ## cors section to the end of /guide/routing to mention different ways of handling cors but most importantly, that cors needs a catch all (or at least OPTIONS) handler.

MickL commented 2 months ago

One thing is CORS but this issue actually turned out to a critical bug. Maybe it was not clear enough in the end but it took me some time to figure it out:

Route roules are not applied if a route does not have a GET route. For example:

/products.ts
/products.patch.ts
/items.patch.ts -> Route rule not applied

Or did I miss something?

This should be visible in my repo: https://github.com/MickL/nitro-route-rules-bug Related: https://github.com/nuxt/nuxt/issues/25686

pi0 commented 2 months ago

@MickL please locally run my reproduction from https://github.com/unjs/nitro/issues/2340#issuecomment-2308264327. Route rules always apply regardless of the incoming request method or if even there is a matching router handler or not. route rule runs on its own. CORS fails in your example not because the rule cors: true does not add headers it fails because the browser expects an "ok" (like 200) response and if there is no route handler, well it will be 404/405. A catch-all handler makes sure of that 200 (and i plan to make additional behavior to route rules to be able to auto-response, considering we can be sure of security to make DX better)

MickL commented 2 months ago

But why does it work when adding the get route? Like for PATCH /products it works in my example. Sorry if I miss something.

/products.ts
/products.patch.ts
/items.patch.ts -> Route rule not applied

If I would add an /items.ts that returns just an empty string it would work for PATCH items aswell.

pi0 commented 2 months ago
/products.ts <-- This handles /products with ANY method (including OPTIONS by preflight)

/products.patch.ts <-- this handles /products with PATCH method only

/items.patch.ts <-- this handles /items with PATCH method only

Issue:

<no matching handler> <-- browser sends [OPTIONS] /items for preflight, it gets a 404/405 

Solution:

/api/[...].ts <-- respond to [OPTIONS] with 200 (route rule already added headers)

The browser makes a preflight to the same path with OPTIONS method to check cors. It expects:

  1. cors headers
  2. ok response

route rules add cors headers (1), you need to do (2) by adding any kind of catch-all handler that simply gives that "ok".

Please read about Preflight request and CORS mechanism.

Next steps:

  1. We need to better document CORS handling. route rule only adds headers, it doesn't sent ok. a catch all handler is required.
  2. Route rules can help automatically do (2) but it is not easy because first we need to make sure we don't override user logic of checking cors.
MickL commented 2 months ago

I see, thanks for all the explanations!