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

Add option to disable CORS headers #218

Merged
merged 6 commits into from
Oct 12, 2017
Merged

Conversation

bshamblen
Copy link
Contributor

A relatively minor change to optionally disable the CORS headers.

Testing process

The following is refactored, but similar to the block of code that Meteor uses to set up sockjs in their ddp-server package.

Copy/paste the following code into the tests/test_server/server.js file, overwriting the contents of the existing file:

var http = require('http');
var config = require('./config').config;
var sockjs = require('../../index');

var open_sockets = [];
var server = http.createServer();

var serverOptions = {
  prefix: '/sockjs',
  log: function() {},
  disable_cors: false, //change this setting to test
  heartbeat_delay: 45000,
  disconnect_delay: 60 * 1000,
  jsessionid: false
};

var meteor = sockjs.createServer(serverOptions);

meteor.on('connection', function (socket) {
  socket.send = function (data) {
    socket.write(data);
  };
  socket.on('close', function () {
    //open_sockets = _.without(open_sockets, socket);
  });
  open_sockets.push(socket);
  socket.send(JSON.stringify({server_id: "0"}));
});

meteor.installHandlers(server, {prefix: '/sockjs'});
server.listen(config.port, config.host);

console.log("[*] Listening on", config.host + ':' + config.port);

Run npm install and make test_server from the root folder of the project.

Open your browser and navigate to http://localhost:8081/sockjs/info. This will verify the old version of the info data is still returned.

Open another window and navigate to http://jsfiddle.net/bshamblen/4m118rbg/, then run the script. This will verify that you're able to load the info page from another domain.

Now, change the disable_cors option to true and restart the server.

Reload both pages above. The direct load of the page should not return the options key in the JSON body, but it should load, since it's not being called from another domain. The jsfiddle script should return an error, and the developer console should show that the CORS headers are missing.

Please let me know if you have any questions, or need me to make any adjustments.

@bshamblen
Copy link
Contributor Author

Fixes #217

@bshamblen
Copy link
Contributor Author

@brycekahle Just checking to see if there's anything you need from me before this PR can be merged. Thanks.

@leo-cheron
Copy link

Could we merge that? That would fix a huge security risk.

Copy link

@ALeschinsky ALeschinsky left a comment

Choose a reason for hiding this comment

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

Yes please! We need this for our project.

Copy link

@IncoCode IncoCode left a comment

Choose a reason for hiding this comment

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

👍

README.md Outdated
<dd>Enabling this option will prevent
<a href="https://en.wikipedia.org/wiki/Cross-origin_resource_sharing" target="_blank">CORS</a>
headers from being included in the HTTP response. Can be used when the
sockjs client is know to be connecting from the same domain as the
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change domain to origin?

Copy link

Choose a reason for hiding this comment

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

'know' ?

README.md Outdated

<dt>disable_cors (boolean)</dt>
<dd>Enabling this option will prevent
<a href="https://en.wikipedia.org/wiki/Cross-origin_resource_sharing" target="_blank">CORS</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to include the target. Let's not presume we know what the user wants.

@bshamblen
Copy link
Contributor Author

@brycekahle I've modified the documentation to include the requested changes. Thanks!

@lnader
Copy link

lnader commented Apr 7, 2017

@brycekahle - Change requests have been done. Can we please merge this PR?

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