Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cors route rule should auto response to preflight #2340

Open
MickL opened this issue Apr 5, 2024 · 18 comments
Open

cors route rule should auto response to preflight #2340

MickL opened this issue Apr 5, 2024 · 18 comments
Labels
documentation Improvements or additions to documentation enhancement New feature or request

Comments

@MickL
Copy link
Contributor

MickL commented Apr 5, 2024

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
Copy link
Contributor Author

MickL commented May 22, 2024

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 nuxt/nuxt#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 MickL changed the title Route rules with cors: true does not work Route rules are not applied if a route does not have a GET route Jun 5, 2024
@MickL
Copy link
Contributor Author

MickL commented Jun 5, 2024

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
Copy link

B-E-A-F commented Aug 24, 2024

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
Copy link

B-E-A-F commented Aug 24, 2024

Possibly related to #1806

@MickL
Copy link
Contributor Author

MickL commented Aug 24, 2024

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: nuxt/nuxt#25686

@pi0 pi0 changed the title Route rules are not applied if a route does not have a GET route cors route rule isn't effective if there is no GET handler Aug 24, 2024
@pi0
Copy link
Member

pi0 commented Aug 24, 2024

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 ""
  }
});

@pi0 pi0 added documentation Improvements or additions to documentation enhancement New feature or request and removed pending triage labels Aug 24, 2024
@pi0 pi0 changed the title cors route rule isn't effective if there is no GET handler cors route rule should auto response to preflight Aug 24, 2024
@pi0
Copy link
Member

pi0 commented Aug 24, 2024

@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
Copy link

B-E-A-F commented Aug 24, 2024

@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
Copy link

B-E-A-F commented Aug 24, 2024

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
Copy link
Member

pi0 commented Aug 24, 2024

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
Copy link
Contributor Author

MickL commented Aug 24, 2024

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: nuxt/nuxt#25686

@pi0
Copy link
Member

pi0 commented Aug 24, 2024

@MickL please locally run my reproduction from #2340 (comment). 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
Copy link
Contributor Author

MickL commented Aug 24, 2024

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
Copy link
Member

pi0 commented Aug 24, 2024

/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)
  • Route rule cors: true adds cors headers for paths it is matching (and it works)
  • If there is no route handler that matches the path + preflight method (OPTIONS), the server responds 404 (not found) or 405 (method not allowed).

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
Copy link
Contributor Author

MickL commented Aug 24, 2024

I see, thanks for all the explanations!

@MickL
Copy link
Contributor Author

MickL commented Nov 25, 2024

@pi0 Is there any update on this issue? I didnt find anything about cors in the docs yet.

Is the only workaround to add empty route handlers atm or did anything change with the underlying UnJS packages and/or Nitro v3?

Atm the/my workaround is to place this in every position where I dont have a GET route. And the file is not there the other routes wont work (high risk!):

export default defineEventHandler(async (event) => {
  if (event.method !== 'OPTIONS') {
    throw createError({
      status: 404,
    });
  }
});

@MickL
Copy link
Contributor Author

MickL commented Dec 6, 2024

Another issue with this is that non-existent routes return cors error instead of 404.

@MickL MickL mentioned this issue Dec 18, 2024
17 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants