CORS header Origin allowing any domain

[bshamblen]

During a recent security scan of our production environment a potential CORS vulnerability was raised. It seems that sockjs automatically sets the Access-Control-Allow-Origin header to whatever the requesting domain is, if any sockjs path is referenced from another domain.

For example, if I request the URL https://www.meteor.com/sockjs/info from a jsfiddle script, the Access-Control-Allow-Origin value is set to https://fiddle.jshell.net, by sockjs.

After looking at your source code, I noticed a hard coded reference where the requesting origin header is being used to set the response Access-Control-Allow-Origin header :

https://github.com/sockjs/sockjs-node/blob/master/src/trans-xhr.coffee#L63

For security purposes our application should not allow cross origin access to any paths. Would it be possible to add an option to disable this default behavior, or even the option to disable CORS completely, for the sockjs paths?

[brycekahle]

I'd happily accept a PR that disables CORS completely or allows restricting the value.

[bshamblen]

@brycekahle I'll have something for you to review early next week. Thanks.

[bshamblen]

@brycekahle Do you know if the "origins": ["*:*"] key/value that's returned in the JSON response of the info endpoint is used by any sockjs clients, or is it purely for informational purposes? It would make sense to me to remove this key from the response if CORS were disabled, but I want to make sure it's not going to break anything downstream if I remove it. I searched the sockjs-client source code and didn't find any references to origins.

I don't see any integration tests, other than the sample test_server app. Is there some process you use to verify PRs, to check for errors? If not, I have a sample script that'll test my use case, which I can share when I submit the PR.

[brycekahle]

Looks like it is ignored: https://github.com/sockjs/sockjs-protocol/blob/8816e93c695d92d6a16f744a6a8a08f22df64d7e/sockjs-protocol.py#L278

The tests are in the sockjs-protocol repo.

[bshamblen]

@brycekahle I've run the tests in the sockjs-protocol repo against the changes I made in PR #218. All of the tests pass with the default configuration (in tests/test-server/config.js). If I add the disable_cors: true option to server_opts and retest, 8 tests fail, which is expected since many of the tests are checking for the existence of CORS headers.

[brycekahle]

Fixed in #218

[fwhatley]

I'd happily accept a PR that disables CORS completely or allows restricting the value.

Thanks for allowing us to disable CORS. It works beautifully.

Currently, we only have 2 options: enable CORS to all origins or disable it. I must enable CORS and limit it to a set of origins (URLs). Would you still happily accept a PR for this addition? I know the statement above was made 7 years ago hence asking. I'd be happy to make a PR to limit CORS to a set of origins.

[auvipy]

I would like to know more about the implementation and implication of that approach, care to share more? or you can come with a draft proof of concept PR