Session: improve session hashing when detecting modified sessions

[skarbovskiy]

There is an issue in hash function
https://github.com/expressjs/session/blob/master/index.js#L582
simple JSON.stringify is not safe to use as when session contains an object with multiple properties inside (user for example with id, name, position properties). JSON.stringify can return different string (id, name, position or position, id, name). We need to do safe object keys sorting before serializing or use some kind of safe object hashing (e.g. https://github.com/puleos/object-hash)

[skarbovskiy]

simple concept https://github.com/expressjs/session/pull/615

[dougwilson]

Yea, currently the implementation does give false positives. This idea here is that it's better to just save again on accident than to accidentally not save when there was a change (i.e. false positives are ok vs false negatives are really bad). Ideally it would have no false anything, though. Efforts along those lines are welcome!

[HarshithaKP]

@skarbovskiy @dougwilson Is the PR merged into this repo or are there any pending actions ?

[skarbovskiy]

in some synthetic tests object-hash implementation gives false positive "objects are identical"
https://github.com/expressjs/session/pull/615#issuecomment-440334937
so i ended up just forking express-session and implementing this fix there.
ideally we need another object-hash implementation that wont give false positives in any cases

[gireeshpunathil]

were there reports of the existing hash function producing wrong results and causing issues in production deployments? I am yet to find one, is this one such a report? If not I would suggest we close this (and the associated PR), and revisit this later when we face issues.

[skarbovskiy]

no, only for cases when session data use the same structure as object-hash internally (that is very unlikely)

[davidmashe]

No activity for some time, and issue is debatable as an edge case. Will close for now, happy to re-open if needed.