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

with v1.8.3 there is a parseCookie in utils but with v2, what it becomes? #588

Closed
crapthings opened this issue Jun 8, 2012 · 43 comments
Closed

Comments

@crapthings
Copy link

parseCookie = parseSignedCookie ?

@gitawego
Copy link

gitawego commented Jun 8, 2012

the only way I found is:

var cookie = require('cookie');
var yourCookiesObject = connect.utils.parseSignedCookies(cookie.parse(req.headers.cookie),'your cookie secret');

@bigal488
Copy link

bigal488 commented Jun 8, 2012

connect.utils.parseCookie is no longer present in utils.js???

@gitawego
Copy link

gitawego commented Jun 8, 2012

not any more... as far as I can see

@tj
Copy link
Member

tj commented Jun 8, 2012

we're slowly moving some utils out into additional modules. For socket.io integration (assuming that's what you're doing here ;) ) you'll just require a module to unsign the cookies instead of using connect's utils

@gitawego
Copy link

gitawego commented Jun 9, 2012

it's exactly what I'm doing... I'm so glad to hear that you are trying to work with socket.io ^^

@vjpr
Copy link

vjpr commented Jun 27, 2012

So is @gitawego 's solution the way to go for now?

@gitawego
Copy link

a little notice:
in the latest version of connect, the cookie string is escaped, but parseSignedCookies trys to match unescaped string, such as "s:" instead of "s%3A". so to ensure it:

var cookie = require('cookie');
var yourCookiesObject = connect.utils.parseSignedCookies(cookie.parse(decodeURIComponent(req.headers.cookie)),'your cookie secret');

@tj tj closed this as completed Jun 27, 2012
@niftylettuce
Copy link
Contributor

shouldn't we throw an error about this or something since a lot of people are using this with socket.io? almost every tutorial out there has usage of parseCookie.

@tj
Copy link
Member

tj commented Jul 9, 2012

they were private apis in the first place

@vortec
Copy link

vortec commented Jul 29, 2012

@visionmedia : Considering how many people and tutorials were using this, don't you think it would be a good idea to revise that decision?

connect.utils.parseCookie()
vs.
connect.utils.parseSignedCookies(cookie.parse(decodeURIComponent(req.headers.cookie)),'secret');

@tj
Copy link
Member

tj commented Jul 30, 2012

they shouldn't use them though, I've seen people have this problem so often im still really surprised that no one has abstracted it out into a module so this sort of breakage can at least be taken care of at a single point

@tj
Copy link
Member

tj commented Jul 30, 2012

It'll be a bit easier once the rest of the connect related utils are npm modules

@srlowe
Copy link

srlowe commented Jul 31, 2012

Hmm, so that's why parseCookie is not working - I need something like this for websockets, but I'm not using socket.io, so the fact that it's being offloaded there is a little annoying. Pity it was taken out...

@tj
Copy link
Member

tj commented Jul 31, 2012

these are not public

@srlowe
Copy link

srlowe commented Aug 1, 2012

But the fact remains that everybody is using it like this (because there is no alternative). Look at any answer related to this on Stackoverflow for instance. Public/private aside, you say you're moving the utils out into additional modules - which is fine, but wouldn't it be less disruptive to wait until the alternative module exists before pulling them?

@vortec
Copy link

vortec commented Aug 1, 2012

I don't understand. All functions in utils are documented fully on your homepage, and they're not flagged as private: http://www.senchalabs.org/connect/utils.html

How should people know that they're private without looking at the code?

Am I missing something or are the only indicators for private functions code comments like this:
https://github.com/senchalabs/connect/blob/master/lib/utils.js#L24
?

@bigal488
Copy link

bigal488 commented Aug 1, 2012

I really think this needs to be re-instated. Given that it is in use by SO many people and is on, just about, every socket.io tutorial (and will continue to be for years to come), is it really so much of a problem to re-instate and keep everyone happy?

@tj
Copy link
Member

tj commented Aug 1, 2012

yes but if you're copy / pasting the same chunk of code around that's not the right way to have maintainable code, someone needs to make this same thing a module so it even if it does break, that it's easier to fix and everyone can benefit. We do need to move some of these into npm so things are easier

@tj
Copy link
Member

tj commented Aug 1, 2012

@vortec yes, and it's reasonably obvious since connect is not a mixed bag of utils, that's not its purpose, it's middleware. we simply need to move more of these into npm to make them more convenient, moving them all back into connect is not the right way to go

@blevine
Copy link

blevine commented Aug 7, 2012

This is really surprising. You've removed a widely-used function without any warning and without providing any alternatives. You assert that these were private functions, but there is absolutely no way for a developer to know that. This change breaks all kinds of existing code. Please reconsider this decision.

@tj
Copy link
Member

tj commented Aug 7, 2012

It's not my fault that someone choose to blog about it, using private utilities. We've made progress moving things out of Connect so that in the future more people / frameworks can use these, I dont want to go back on that progress. It's very unfortunate that so many people copy/pasted this code instead of consolidating it but that's all I can say about that, someone should be encapsulating this behaviour

@blevine
Copy link

blevine commented Aug 7, 2012

IMHO, you removed functionality that many people were using and so you should have ensured that it was encapsulated elsewhere if that's the right thing to do. The point here is that you aren't "moving" these utilities, you're simply deleting them. So that this doesn't happen again, could you explain how we should determine the difference between public and private utilities?

@blevine
Copy link

blevine commented Aug 7, 2012

And while we're on the subject of private vs public. I've implemented @gitawego 's solution (Thank you!) which works fine. I just want to make sure that it's OK to use parseSignedCookies and that this doesn't fall into the private category.

@vortec
Copy link

vortec commented Aug 7, 2012

@blevine They mention it in the code documentation:
https://github.com/senchalabs/connect/blob/master/lib/utils.js#L18

"@api private"

If there are other ways, I'd like to know too.

@vortec
Copy link

vortec commented Aug 7, 2012

@blevine it is private as well.

@blevine
Copy link

blevine commented Aug 7, 2012

@vortec. Fair enough. But as you pointed out earlier, all of these functions are in the documentation and are not marked as private. Don't get me wrong. I'm grateful that there's good documentation. However, it really seems that private functions should either not be included in the documentation or the documentation should make it clear that these functions are private.

@tj
Copy link
Member

tj commented Aug 7, 2012

yeah they should mention that in the docs, I figured "internals" kinda implied that but you're right there

@airandfingers
Copy link

I just want to thank everyone involved for this discussion, I've learned a lot, but most valuable is the solution to this issue, which gitawego posted: #588 (comment)
I've posted a link to this workaround on the blog I was following, so hopefully others will find this solution when they run into this same problem.
I'd also like to echo blevine's complaint that parseCookie wasn't marked as private in the documentation, and his/her desire to not have to refactor our use of parseSignedCookie in a future version of connect.

@Coneko
Copy link

Coneko commented Aug 20, 2012

@airandfingers parseSignedCookie is a private api too, posting it on blogs and SO is exactly how this situation started in the first place.

@airandfingers
Copy link

I understand that parseSignedCookie is private, but I think that whoever decides these things should factor in widespread usage when determining which parts of the API will be private or public.

Also, I think it's worth noting that Socket.IO's Github wiki itself points at the article in question: https://github.com/learnboost/socket.io/wiki/ (Articles and Recipes, Socket.IO Express and Sessions). That's how I found out about parseCookie, and maybe a dialog between Connect's and Socket.IO's developers needs to be taking place.

@tj
Copy link
Member

tj commented Aug 20, 2012

it's like a lot of things in node's core even, .binding() for example was never public, but many people used it. naturally when things like this are removed people complain, eventually someone does it the "correct" way and life goes on. To make them public and to revert changes would be a regression, we need to move forward and make socket.io integration easier by moving more of these utilities into npm.

@jugglinmike
Copy link

I have a similar need (my specific use case is sharing session data between socket.io and express). I'm experimenting with a solution using only public methods, and I'm wondering what the consensus is regarding this approach:

var sessionSecret = "super secret";
// Connect's session middleware expects to receive the secret, so the
// cookieParser instance used by your application likely is not aware
// of it.
var sioCookieParser = express.cookieParser(sessionSecret);

// Connect app
var app = connect()
  .use(connect.cookieParser())
  .use(connect.session({ secret: sessionSecret }))
  .use(function(req, res) {
    res.end("hello world\n");
  })
 .listen(3000);

// Socket.io app
io.sockets.on("connection", function(socket) {

  sioCookieParser(socket.handshake, {}, function(err) {
    // Parsed cookies are now in socket.handshake.cookies and
    // socket.handshake.securecookies
  });
});

@tj
Copy link
Member

tj commented Aug 31, 2012

@jugglinmike yeah if that works fine for you nothing wrong with that

@vortec
Copy link

vortec commented Aug 31, 2012

@jugglinmike and how exactly do you access the session object?

@jugglinmike
Copy link

@vortec I'll extend the above example with Express and RedisStore to show how you might do that:

var express = require("express");
var RedisStore = require("connect-redis")(express);
var sessionSecret = "super secret";
var store = new RedisStore();
var sioCookieParser = express.cookieParser(sessionSecret);

// Express app
var app = express()
  .use(connect.cookieParser())
  .use(connect.session({ store: store, secret: sessionSecret }))
  .use(function(req, res) {
    res.end("hello world\n");
  })
 .listen(3000);

// Socket.io app
io.sockets.on("connection", function(socket) {

  sioCookieParser(socket.handshake, {}, function(err) {
    store.get(socket.handshake.secureCookies["connect.sid"], function(err, sessionData) {
      // session data available here
    });
  });
});

@vortec
Copy link

vortec commented Sep 1, 2012

@jugglinmike thank you, seems to be a really neat way to connect express + socket.io + sessions. I was close to writing a library doing just that.

@tj
Copy link
Member

tj commented Sep 3, 2012

@vortec doooo it, haha, if someone abstracts this out into a declarative module we can all rest in peace and people can stop copy/pasting blog posts

@wcamarao
Copy link

hey first thing thank you all, I just abstracted this out into a module and would like to know if it works for you guys: https://github.com/functioncallback/session.socket.io

@Nick011
Copy link

Nick011 commented Oct 11, 2012

Thank you @functioncallback your module is simple and effective.

@kcaffrey
Copy link

Thanks for the code examples @functioncallback. The one problem is that it seems preferable to do the session parsing in the Socket.IO Authorizing and handshake step, so that if the session is not present or valid the connection itself is refused.

Here is the code (adapted from your code) that I used to do this:

var express = require('express');
var http = require('http');
var connect = require('express/node_modules/connect');
var secret = 'some secret';
var sessionKey = 'express.sid';
var cookieParser = express.cookieParser(secret);
var sessionStore = new connect.middleware.session.MemoryStore();
var app = express();

app.configure(function() {
  ...
  app.use(cookieParser);
  app.use(express.session({ store: sessionStore, key: sessionKey });
  ...
});

var server = http.createServer(app);
var io = require('socket.io').listen(server);

io.set('authorization', function(data, accept) {
  cookieParser(data, {}, function(err) {
    if (err) {
      accept(err, false);
    } else {
      sessionStore.get(data.signedCookies[sessionKey], function(err, session) {
        if (err || !session) {
          accept('Session error', false);
        } else {
          data.session = session;
          accept(null, true);
        }
      });
    }
  });
});

io.sockets.on('connection', function(socket) {
  socket.session = socket.handshake.session;
  // Now you can put Socket.IO code here as normal using socket.session.
});

I think a general solution could be put into a module that provided the function(data, accept) {} handler, so that the user would just have to do something like:

io.set('authorization', require('session.socket.io').authorize(cookieParser, sessionStore, sessionKey));

@wcamarao
Copy link

I see your point @kcaffrey, but I can't agree for a couple reasons: refusing a connection if the session is not found/valid and changing the socket object to add the session.

@gizmotronic
Copy link

@jugglinmike your solution was exactly what the doctor ordered - clean and effective. Thanks!

@jugglinmike
Copy link

@gizmotronic Glad to hear it :D

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