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 middleware headers get overwritten for error responses #605

Closed
1 of 2 tasks
FabianElsmer opened this issue Oct 20, 2020 · 3 comments
Closed
1 of 2 tasks

CORS middleware headers get overwritten for error responses #605

FabianElsmer opened this issue Oct 20, 2020 · 3 comments

Comments

@FabianElsmer
Copy link

FabianElsmer commented Oct 20, 2020

You want to:

  • report a bug
  • request a feature

Current behaviour

If an error occurs (e.g. Bad Request) while using the CORS middleware, CORS headers previously set via the middleware get overwritten in sendErrorMessage in the following lines

  if (req.headers.origin) {
    headers["Access-Control-Allow-Credentials"] = "true";
    headers["Access-Control-Allow-Origin"] = req.headers.origin;
  } else {
    headers["Access-Control-Allow-Origin"] = "*";
  }

Steps to reproduce (if the current behaviour is a bug)

  1. Start SocketIO Server
  2. Trigger a Bad Request (400), e.g. POST without body to the SocketIO Endpoint
  3. The response has the wrong CORS headers
curl 'https://domain/socket.io/?EIO=4&transport=polling&t=NL645og&sid=BMdQ8VDOxS8pJa55AAAC' -v
 ...
 < server: nginx/1.17.7
 < date: Tue, 20 Oct 2020 14:56:22 GMT
 < vary: Origin
 < access-control-allow-origin: *

Expected behaviour

CORS headers should only be added by the middleware,
if the middleware is not used, CORS headers should not be set at all.

Setup

  • OS: Fedora 32
  • browser: Chrome 85
  • engine.io version: 4.0.0

Other information (e.g. stacktraces, related issues, suggestions how to fix)

I'm not sure if the CORS headers are overwritten in the code for any particular reason, I would personally remove them (can make a PR).
If error responses have separate fixed CORS header settings for a reason, I would like to comment the function to make it clear this is not just legacy CORS code.

darrachequesne added a commit that referenced this issue Oct 21, 2020
The Access-Control-Allow-xxx headers added by the cors middleware were
overwritten when sending an error response.

Those lines should have been removed in [1].

[1]: 61b9492

Related: #605
@darrachequesne
Copy link
Member

You are right, it's legacy CORS code which should have been removed.

It should be fixed by fe093ba.

@darrachequesne
Copy link
Member

Released in 4.0.1

@FabianElsmer
Copy link
Author

Awesome 👍
Thanks for the update!

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

No branches or pull requests

2 participants