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

Allow configuration of Access-Control-Allow-Origin value #511

Merged
merged 3 commits into from
Nov 2, 2018

Conversation

flenter
Copy link
Contributor

@flenter flenter commented Jun 7, 2017

Allows responses with a specified Access-Control-Allow-Origin value instead of:
Access-Control-Allow-Origin: * or Access-Control-Allow-Origin: <req.headers.origin>

The kind of change this PR does introduce

  • a bug fix
  • a new feature
  • an update to the documentation
  • a code change that improves performance
  • other

Current behaviour

In several places the Access-Control-Allow-Origin header is set to either '*' or to the
value that was part of the request. This gives the impression that CORS is always allowed.

There are two scenarios where this happens:

  • errors that are sent via the sendErrorMessage in lib/server.js
  • responses by the XHR transport

New behaviour

It's now possible to specify an origins value (default value is '*') when initialising the engine. This value will be returned as the Access-Control-Allow-Origin value.

Note: this pr does not add support for verifying/checking the origins values, socket.io by default passes a function that performs this check already (via the allowRequest option).

Other information (e.g. related issues)

There will be an accompanying PR for socket.io that passes the origins value it has to this package (once this pr is merged or has gotten some feedback).

Related issue:

@flenter flenter force-pushed the origins branch 2 times, most recently from 79add95 to 2fcbaae Compare June 7, 2017 16:57
@darrachequesne
Copy link
Member

@flenter may I ask what use case you have in mind (which would not be covered by the allowRequest option)?

@flenter
Copy link
Contributor Author

flenter commented Jun 16, 2017

I think it helps if I explain how I came to this pr:
A vulnerability scanner found this problem it send a get request to /socket.io and got a 400 repsonse with CORS headers set to '*'. After some digging I found that this is default behaviour in several places in the code.

What allowRequest didn't cover

If you specify origins on socket.io a allowRequest function will be passed to engine.io that checks if a request is allowed based on the origins setting. The error response however ignores this and can respond with Access-Control-Allow-Origin: '*' if the origins header was not set (and otherwise returns the requests origin header value).

The new origins option is used for two things, the first one is to return the correct header for errors handled by the sendErrorMessage function. The second use is explained below.

The PR however contains another change and that came up after I noticed that the long-polling transport behaves similarly to the error handling function regarding CORS header values. This is why the opts object is added to transports constructor. It allows the origins value to be passed to the (xhr-polling) transport and the responses can contain the correct CORS header values.

@flenter
Copy link
Contributor Author

flenter commented Jul 20, 2017

Is there anything I can do to help progress this pr?

@uncinimichel
Copy link

+1

@pratheekhegde
Copy link

When will this PR get merged? Is there any blocker?

Allows responses with the correct Access-Control-Allow-Origin value
instead of:

Access-Control-Allow-Origin: *
@flenter
Copy link
Contributor Author

flenter commented Mar 29, 2018

I've rebased the pr so there are no more merge conflicts

@mooflu
Copy link

mooflu commented Apr 9, 2018

As an alternative, would it be easier to provide official api access to the headers before they are written. We ran into the same issue as @flenter where a penetration testing suite flagged socket.io as having arbitrary origins trusted. Our current approach is to register for the socket connection's transport 'headers' event as well as intercept the response's writeHead for the handshake in allowRequest (no access to socket transport at that stage) and modify the headers. Needless to say, that's icky. In our case we don't even need CORS at all.

@holm
Copy link

holm commented Jun 25, 2018

Would be really good to have this issue sorted out one way or the other.

Copy link

@Kreijstal Kreijstal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve of this

@darrachequesne darrachequesne changed the base branch from master to develop November 2, 2018 06:31
@darrachequesne darrachequesne merged commit ebf1a96 into socketio:develop Nov 2, 2018
@darrachequesne
Copy link
Member

Merged! Sorry for the delay..

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

Successfully merging this pull request may close these issues.

7 participants