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

OPTIONS request handler missing Allow header #305

Open
Lordfirespeed opened this issue Sep 20, 2023 · 13 comments
Open

OPTIONS request handler missing Allow header #305

Lordfirespeed opened this issue Sep 20, 2023 · 13 comments

Comments

@Lordfirespeed
Copy link

As per the MDN Docs, servers' responses to OPTIONS requests should include an Allow: header that contains the allowed methods.

@dougwilson
Copy link
Contributor

You are correct, but that is even if you are not using CORS. The Express.js server will add one for you, if you set up method for your path. This module is just for adding on CORS; your server should include Allow to OPTIONS requests without CORS at all too, which is all this module adds.

@Lordfirespeed
Copy link
Author

Can you show me a minimal reproducible example of an Express server that provides a GET endpoint, such that an OPTIONS request to that endpoint will yield a response with the Allow header?

@dougwilson
Copy link
Contributor

dougwilson commented Sep 20, 2023

$ node -e 'require("express")().get("/foo",(req,res)=>res.send("Hello")).listen(3000)' &
[1] 622

$ curl -i -XOPTIONS http://localhost:3000/foo
HTTP/1.1 200 OK
X-Powered-By: Express
Allow: GET,HEAD
Content-Type: text/html; charset=utf-8
Content-Length: 8
ETag: W/"8-ZRAf8oNBS3Bjb/SU2GYZCmbtmXg"
Date: Wed, 20 Sep 2023 19:46:40 GMT
Connection: keep-alive
Keep-Alive: timeout=5

GET,HEAD

@Lordfirespeed
Copy link
Author

Lordfirespeed commented Sep 20, 2023

When I use the cors() middleware with minimal options, according to 'Simple Usage' on this repo's readme:

$ node -e 'require("express")().use(require("cors")()).get("/foo",(req,res)=>res.send("Hello")).listen(3000)' &
[3] 2226

$ curl -i -XOPTIONS http://localhost:3000/foo
HTTP/1.1 204 No Content
X-Powered-By: Express
Vary: Origin, Access-Control-Request-Headers
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Content-Length: 0
Date: Wed, 20 Sep 2023 20:09:21 GMT
Connection: keep-alive
Keep-Alive: timeout=5

no Allow header is present on the response.

@dougwilson
Copy link
Contributor

Oh, interesting. So seems like there is some kind of bug in here that is suppression the Allow header or something 🤔

@Lordfirespeed

This comment was marked as off-topic.

@dougwilson

This comment was marked as off-topic.

@Lordfirespeed
Copy link
Author

Lordfirespeed commented Sep 21, 2023

That's all well and good, but I actually can't post issues on the expressjs/express issue board as that action is limited to contributors.

Similarly: Lordfirespeed does not have the appropriate permissions to CreateDiscussion or something of that ilk for the discussion board.

Ironically, I would like to contribute to expressjs/express#2414 but am unable to initiate discussion on that, as I am not yet a contributor 😛

Edit (26/09/2023): I hid this message previously because issues on expressjs/express were opened, but having checked again today, they are closed once more.

@siddhartasr10
Copy link

Enabling cors for a single route, as cors npm doc shows doesn't mess the headers up.

node -e 'require("express")().get("/foo", require("cors")(), (req,res) =>res.send("Hello")).listen(3000)' &
[1] 10062

curl -i -X OPTIONS localhost:3000/foo
HTTP/1.1 200 OK
X-Powered-By: Express
Allow: GET,HEAD
Content-Type: text/html; charset=utf-8
Content-Length: 8
ETag: W/"8-ZRAf8oNBS3Bjb/SU2GYZCmbtmXg"
Date: Mon, 25 Sep 2023 22:19:03 GMT
Connection: keep-alive
Keep-Alive: timeout=5

GET,HEAD% 

Enabling cors on a single route but with pre-flight enabled messes the headers too though.

node -e 'require("express")().options("/foo", require("cors")()).get("/foo", require("cors")(), (req,res) => res.send("Hello")).listen(3000)' &
[1] 10632

curl -i localhost:3000/foo -X OPTIONS
HTTP/1.1 204 No Content
X-Powered-By: Express
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Vary: Access-Control-Request-Headers
Content-Length: 0
Date: Mon, 25 Sep 2023 22:38:39 GMT
Connection: keep-alive
Keep-Alive: timeout=5

Also checked changing the status code from the cors options and express response just in case, but that isn't the cause either.

I know nothing about the express framework inner architecture but might be something the cors module is overwriting, given that cors has also request and response objects and similar functionality for some things like headers.

@Lordfirespeed
Copy link
Author

Lordfirespeed commented Sep 26, 2023

This line leads to this block executing by default (with preflight enabled disabled (thankyou @siddhartasr10)) which always end()s the response and does not call next().

Express' default OPTIONS request handling implementation is here, in the pillarjs/router repository. It is documented accurately:

for options requests, respond with a default if nothing else responds

the cors() middleware has already responded, so the default OPTIONS handling which generates the Allow header never executes.

@siddhartasr10
Copy link

Typo: preflight enabled makes cors call next and not end the res, preflight disabled (the default config) is what always ends the response, making it impossible to add nothing to It.

Based in what you explained, in the 'Simple Usage' example that you showed (cors globally enabled), if we change the default preflightContinue (false) to true, the response gets passed to the next handler and doesn't end, getting the wanted behaviour.

node -e 'require("express")().use(require("cors")({preflightContinue:true})).get("/foo", (req, res) => res.send("bar")).listen(3000)' &
[1] 5378

curl -i localhost:3000/foo -X OPTIONS
HTTP/1.1 200 OK
X-Powered-By: Express
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET,HEAD,PUT,PATCH,POST,DELETE
Vary: Access-Control-Request-Headers
Allow: GET,HEAD
Content-Type: text/html; charset=utf-8
Content-Length: 8
ETag: W/"8-ZRAf8oNBS3Bjb/SU2GYZCmbtmXg"
Date: Tue, 26 Sep 2023 21:27:05 GMT
Connection: keep-alive
Keep-Alive: timeout=5

GET,HEAD%

If I recall correctly, allowed methods were showing in the single route cors example because after cors its called, the (req, res) handler gets called "on top", setting the allow header in the function sendOptionsResponse that you noted.

Im not very sure how, but possibly the cors middleware doesn't execute the same way or even correctly, because if you check the single route test I did, it didn't had the common access control origin and access control methods headers that cors sets.

node -e 'require("express")().get("/foo", require("cors")(), (req,res) =>res.send("Hello")).listen(3000)' &
[1] 10062

curl -i -X OPTIONS localhost:3000/foo
HTTP/1.1 200 OK
X-Powered-By: Express
Allow: GET,HEAD
Content-Type: text/html; charset=utf-8
Content-Length: 8
ETag: W/"8-ZRAf8oNBS3Bjb/SU2GYZCmbtmXg"
Date: Mon, 25 Sep 2023 22:19:03 GMT
Connection: keep-alive
Keep-Alive: timeout=5

GET,HEAD% 

So maybe is a bigger issue than it looks because the single route example I did is from the common-usage in the npm page.

@jub0bs
Copy link

jub0bs commented Oct 13, 2023

As far as I understand, this issue entirely stems from the middleware's misclassification of all OPTIONS requests as preflight requests. The Fetch standard (the de facto standard for CORS) makes it clear that not all OPTIONS requests are preflight requests.

In fact, evidence (see the first comment of PR #40) suggests that the preflightContinue option was introduced solely as a way to compensate for such misclassification. I've written about this topic in a recent blog post.

One way to fix things would be to correctly categorise OPTIONS requests and render preflightContinue inert. Then, non-preflight OPTIONS requests would pass through the middleware, as they should. Whether doing so would break existing programs that depend on preflightContinue is unclear to me, though; but it's worth a shot.

@yss14
Copy link

yss14 commented Jan 8, 2024

Also had the issue that the Access-Control-Allow-Origin header was not present on the request headers.
After reading up a lot on the topic and also having a careful look at the README I finally figured it out.

This is the final code which works for me:

const allowedOrigins: string[] | string = "*"
expressApp.use(
    cors({
      origin: allowedOrigins,
      preflightContinue: true,
    })
  )
  expressApp.options(
    '*',
    cors({
      origin: allowedOrigins,
    })
  )

Three important things I learned on this topic:

  1. If you wanna apply a wildcard, you have to pass "*" as a string, and not as an array like ["*"]
  2. If you wanna apply specific domains/hosts as origin property, when testing and e.g. sending an OPTIONS request via Postman you have to make sure to pass a matching Origin header to the request, otherwise the Access-Control-Allow-Origin header won't show up.
  3. Make sure you mount/run the cors middleware before every other middleware. Otherwise e.g. auth middlewares could exit the request before cors could add some headers to the request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants