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 sending of cookie header in XHR handshake. #304

Closed
chill117 opened this issue Jun 2, 2014 · 10 comments
Closed

Allow sending of cookie header in XHR handshake. #304

chill117 opened this issue Jun 2, 2014 · 10 comments

Comments

@chill117
Copy link

chill117 commented Jun 2, 2014

It would be beneficial to be able to send a cookie in the headers of the initial XHR handshake. This would allow persisting of user sessions between socket connections for integration tests of user-driven socket.io applications. I had this working in my own, modified version of socket.io-client and submitted a pull request to socket.io-client [1]. It appears that these changes will need to be made to engine.io-client now.

See related issues from socket.io-client repo:

socketio/socket.io-client#587
socketio/socket.io-client#439

[1] https://github.com/chill117/socket.io-client/commit/14a99041521f5c03ccf7ee3a71c3b56a3131c9ab

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Jun 2, 2014

Doesn't it do this by default?

@chill117
Copy link
Author

chill117 commented Jun 2, 2014

I don't believe so. The use-case I am describing involves running socket.io-client from within integration tests in nodejs (not the browser). Previously, I had to modify socket.io-client so that I could send a cookie in the initial XHR handshake. I am currently looking at how to do this with engine.io-client. If you know of a way without changing any source, that'd be fantastic.

@mokesmokes
Copy link
Contributor

Why not use the query string for session information? Works across XHR and websockets.

@chill117
Copy link
Author

chill117 commented Jun 2, 2014

My application uses cookie-based session authentication. I don't want to change the app just to make it more testable. Plus, it would unnecessarily increase the attack surface of the user authentication.

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Jun 2, 2014

It's just as easy for attackers to send a request with a fake cookie headers as it is to send a request with a fake query string. I don't see how that would unnecessarily increase the attack surface.

Also you don't want to make your change your app to make it more testable but you do want to add extra code to the client that only runs on node and not by your actual users in order to make your app more testable. This makes little sense to me

@chill117
Copy link
Author

chill117 commented Jun 2, 2014

Because, I'd have to support both methods of authentication.

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Jun 2, 2014

Which is what? 1 line of code? req.headers.cookie = req.headers.cookie || req.query.cookie?

@chill117
Copy link
Author

chill117 commented Jun 2, 2014

Well, I suppose that doesn't seem so terrible after all.. I'll give that a try. Thanks.

@mokesmokes
Copy link
Contributor

Query string will be more robust, since (off the top of my head, but you can verify) you cannot persist a cookie in a CORS websocket scenario, for example.

@chill117
Copy link
Author

chill117 commented Jun 2, 2014

Looks like the query string solution is going to work. Thanks again both of you for the helpful information!

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

3 participants