Giter Site home page Giter Site logo

Comments (13)

jub0bs avatar jub0bs commented on April 27, 2024 3

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.

from cors.

Lordfirespeed avatar Lordfirespeed commented on April 27, 2024 2

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.

from cors.

siddhartasr10 avatar siddhartasr10 commented on April 27, 2024 2

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.

from cors.

dougwilson avatar dougwilson commented on April 27, 2024

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.

from cors.

Lordfirespeed avatar Lordfirespeed commented on April 27, 2024

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?

from cors.

dougwilson avatar dougwilson commented on April 27, 2024
$ 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

from cors.

Lordfirespeed avatar Lordfirespeed commented on April 27, 2024

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.

from cors.

dougwilson avatar dougwilson commented on April 27, 2024

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

from cors.

Lordfirespeed avatar Lordfirespeed commented on April 27, 2024

On a separate note; how would you suggest to register a 'catch-all' middleware such that it doesn't interfere with the built-in options request handling? For example:

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

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

unrecognised

from cors.

dougwilson avatar dougwilson commented on April 27, 2024

The auto options response in Express only takes effect if your code does not write a response. Since you wrote a response that said unrecognized, Express considers it as if you overrode the default OPTIONS handling with your own custom one, in which you would need to also send an Allow response header to whatever you want it set to.

General questions for how to use Express should ideally be in the Express.js issue tracker or discussion board, instead of the issue tracker of the cors middleware module, as there are more folks watching it to help instead of just myself and maybe a handful of others here.

from cors.

Lordfirespeed avatar Lordfirespeed commented on April 27, 2024

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.

from cors.

siddhartasr10 avatar siddhartasr10 commented on April 27, 2024

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.

from cors.

yss14 avatar yss14 commented on April 27, 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.

from cors.

Related Issues (20)

Recommend Projects

  • React photo React

    A declarative, efficient, and flexible JavaScript library for building user interfaces.

  • Vue.js photo Vue.js

    🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.

  • Typescript photo Typescript

    TypeScript is a superset of JavaScript that compiles to clean JavaScript output.

  • TensorFlow photo TensorFlow

    An Open Source Machine Learning Framework for Everyone

  • Django photo Django

    The Web framework for perfectionists with deadlines.

  • D3 photo D3

    Bring data to life with SVG, Canvas and HTML. 📊📈🎉

Recommend Topics

  • javascript

    JavaScript (JS) is a lightweight interpreted programming language with first-class functions.

  • web

    Some thing interesting about web. New door for the world.

  • server

    A server is a program made to process requests and deliver data to clients.

  • Machine learning

    Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.

  • Game

    Some thing interesting about game, make everyone happy.

Recommend Org

  • Facebook photo Facebook

    We are working to build community through open source technology. NB: members must have two-factor auth.

  • Microsoft photo Microsoft

    Open source projects and samples from Microsoft.

  • Google photo Google

    Google ❤️ Open Source for everyone.

  • D3 photo D3

    Data-Driven Documents codes.