Adding verification that sess.cookie is set

[brismuth]

In the environment where I'm using express-session, sometimes the session is set to {}. That causes this library to crash, because upon session load/creation it is currently only checking if sess is truthy, and is not checking that sess.cookie exists, prior to accessing properties on sess.cookie.

The library currently throws an error, and I have not found an elegant way to catch that error. This change should resolve the issue, by no longer calling self.createSession in the event that sess.cookie is unset.

[dougwilson]

Thanks! So to better understand: you are saying that the store module is returning an improper object when this module asks for the session? If so, it does sound like that is a bug in your store. But as for handing it better here, it seems like that shouldn't be a silent like your change, as there is something in the store with that id, but this change makes it think there is nothing. I would think in a case like this, this module should raise an error and of course make sure it gets propogated correctly (vs uncatchable).

[dougwilson]

See comment and also needs tests.

[brismuth]

You are saying that the store module is returning an improper object when this module asks for the session?

Yes, exactly.

But as for handing it better here, it seems like that shouldn't be a silent like your change, as there is something in the store with that id, but this change makes it think there is nothing.

Agreed, that makes sense. I'll update it and add tests when I get the chance.

[dougwilson]

Cool. After I typed that in the car, I was thinking about it looking at the code, and it could potentially be as simple as this:

  this.get(sid, function(err, sess){
    if (err) return fn(err);
    if (!sess) return fn();
    var err = null
    var req = { sessionID: sid, sessionStore: self };
    try {
      sess = self.createSession(req, sess)
    } catch (e) {
      err = e
      sess = null
    }
    fn(err, sess)
  });

Sorry if I typo'ed anything above, was just typing this out in the car still :) I figure the try will make it so any issues that createSession has it will get propagated up, as I suppose (though didn't check yet) that there could be other conditions it can throw. And of course those conditions could change over time, so not having a second place to track them is probably nicer.

[brismuth]

@dougwilson I've updated the error handling to be broader as you suggested and added a test that demonstrates the issue.

[brismuth]

Apologies, I should have checked the docs on assert.match, it was added in later node versions. I think that it should pass on all nodejs versions now.

[dougwilson]

Nice, sorry about that!

I was just looking this over, and it seems we probably need to add the same guard at

session/session/session.js

Lines 92 to 97 in 1010fad

	store.get(this.id, function(err, sess){
	if (err) return fn(err);
	if (!sess) return fn(new Error('failed to load session'));
	store.createSession(req, sess);
	fn();
	});

[brismuth]

@dougwilson here I am 7 months later closing the loop on this. 😆

I went ahead and wrote a unit test that demonstrates the error you found in .reload() and added the fix in .reload() for it.

[brismuth]

store.get needs to be overwritten here instead of on line 1719 because otherwise the error is caught (and surfaced gracefully) in the wrong spot and .reload() never executes.

[brismuth]

@dougwilson hey, just wanted to check if there's anything you needed from me for this to get across the line, thanks!

[brismuth]

Hi @dougwilson, any additional feedback on this? We're still seeing this issue crop up on our end and would love if we could get this merged.

Thanks!