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

Adding verification that sess.cookie is set #904

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

brismuth
Copy link

@brismuth brismuth commented Jul 28, 2022

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
Copy link
Contributor

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

Copy link
Contributor

@dougwilson dougwilson left a comment

Choose a reason for hiding this comment

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

See comment and also needs tests.

@brismuth
Copy link
Author

brismuth commented Jul 28, 2022

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
Copy link
Contributor

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
Copy link
Author

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

@brismuth
Copy link
Author

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.

@brismuth brismuth requested a review from dougwilson July 29, 2022 01:52
@dougwilson
Copy link
Contributor

dougwilson commented Jul 29, 2022

Nice, sorry about that!

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

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
Copy link
Author

brismuth commented Mar 3, 2023

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

store.clear(function (err) {
if (err) return done(err)

store.get = function returnCorruptSession(sid, callback) {
Copy link
Author

Choose a reason for hiding this comment

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

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
Copy link
Author

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

@brismuth
Copy link
Author

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants