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).

[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 https://github.com/expressjs/session/blob/1010fadc2f071ddf2add94235d72224cf65159c6/session/session.js#L92-L97

[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]

@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!