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

auth middleware expects credentials for CORS-preflight requests #44

Closed
nfadili opened this issue Dec 2, 2021 · 4 comments
Closed

auth middleware expects credentials for CORS-preflight requests #44

nfadili opened this issue Dec 2, 2021 · 4 comments
Labels
question Further information is requested

Comments

@nfadili
Copy link

nfadili commented Dec 2, 2021

Description

I have noticed through testing and looking at the source code that the auth middleware from the express-oauth2-jwt-bearer package is checking for credentials for all requests. The potential issue here is that credentials are not meant to be included in CORS-preflight OPTIONS requests, yet the middleware still expects credentials to exist and will dutifully respond with a 401 to the preflight request.

The reason I ran into this situation is that I used your doc examples that apply the middleware with app.use(auth({ ... })). The app.use attaches the middleware to every request, and therefore this issue arises. I feel like the options here are:

  1. Leave it to the developer to decide on what requests to apply the auth middleware to and if they apply it to all requests, then cross-origin requests will not work.
    • This might benefit from a call out in the docs, otherwise the behavior can be hard to reason about.
    • Alternatively, the docs could be updated to apply the middleware to specific routes like: app.get('/', auth({ ... }), handler)
  2. The auth middleware explicitly skips OPTIONS requests because it shouldn't expect credentials to be included.

I can see why this middleware would want to remain un-opinionated about things like this, but please let me know what you think. I'd be happy to contribute a PR if that would be helpful.

References:

Reproduction

Using the example in your getting started section:

  • Start the express app server on localhost:8080 using your example as a start
  • Start some other application that serves a webpage on localhost:3000
  • Make a cross-origin request from that webpage (from localhost:3000 to localhost:8080)
  • The CORS-preflight OPTIONS request will fail

Environment

"express": "^4.17.1",
"express-oauth2-jwt-bearer": "^1.0.1",
@adamjmcgrath
Copy link
Contributor

Hi @nfadili - thanks for raising this

I see what you mean and I know we put some logic in express-jwt to handle this, but I'd rather just expect the user to put their cors handling middleware before their auth handling, this seems more logical to me than making it possible to put them out of order.

app.use(cors());
app.use(auth());

I also feel a little uncomfortable about letting the request decide if it should bypass authentication checks (although this is probably overkill)

@adamjmcgrath adamjmcgrath added the question Further information is requested label Dec 3, 2021
@nfadili
Copy link
Author

nfadili commented Dec 3, 2021

Hey @adamjmcgrath thanks for the quick reply!

It is my understanding that the cors middleware doesn't affect this. Whether it comes before or after the auth middleware, it is only ever attaching CORS related headers to responses, nothing more. Please let me know if I am incorrect there! In my mind, the root of the issue is that the auth middleware checks for credentials (in headers, body, and query) for every request it is ran for. Requests that are CORS-preflight OPTIONS requests will not contain credentials because the CORS spec advises against it, so the example usages that show app.use(auth({...}) will not work with cross-origin requests.

I do agree that it makes sense for this lib to not be opinionated on what requests to bypass auth on 😄 so I think the existing behavior makes sense! It is an important behavioral difference from the last lib Auth0 maintained (express-jwt) so hopefully this will help someone out if they are going through the migration!

@adamjmcgrath
Copy link
Contributor

Hi @nfadili

It is my understanding that the cors middleware doesn't affect this. Whether it comes before or after the auth middleware, it is only ever attaching CORS related headers to responses, nothing more. Please let me know if I am incorrect there!

The default value for preflightContinue is false (see https://expressjs.com/en/resources/middleware/cors.html#configuration-options) which means that preflight requests will terminate at the cors middleware by default.

Also, the cors middleware docs recommend you put the cors middleware before other routes (see https://expressjs.com/en/resources/middleware/cors.html#enabling-cors-pre-flight). Which is what we do in the example app https://github.com/auth0/node-oauth2-jwt-bearer/blob/main/packages/examples/express-api.ts#L20

@nfadili
Copy link
Author

nfadili commented Dec 8, 2021

Hi @nfadili

It is my understanding that the cors middleware doesn't affect this. Whether it comes before or after the auth middleware, it is only ever attaching CORS related headers to responses, nothing more. Please let me know if I am incorrect there!

The default value for preflightContinue is false (see https://expressjs.com/en/resources/middleware/cors.html#configuration-options) which means that preflight requests will terminate at the cors middleware by default.

Also, the cors middleware docs recommend you put the cors middleware before other routes (see https://expressjs.com/en/resources/middleware/cors.html#enabling-cors-pre-flight). Which is what we do in the example app https://github.com/auth0/node-oauth2-jwt-bearer/blob/main/packages/examples/express-api.ts#L20

Ahhhh I see now. Thanks for the explanation and links, the current behavior makes total sense 👍 please feel free to close this out! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants