Proof of concept for #614 resolving

[skarbovskiy]

No description provided.

[dougwilson]

Also ideally we can add a test 👍

[skarbovskiy]

@dougwilson done

[skarbovskiy]

@dougwilson

[skarbovskiy]

anybody?)

[dougwilson]

Sorry, I will take a look today.

[dougwilson]

So I threw this PR into a couple of apps this morning and one of them threw an error. I reduced it down to the following test case:

var express = require('express')
var expressSession = require('./index')

var app = express()

app.use(expressSession({
  saveUninitialized: false,
  secret: 'keyboard cat',
  resave: false
}))

app.get('/', function (req, res) {
  req.session.visitor = new String('username')
  res.end()
})

app.listen(3000)

$ node app.js &
$ curl -i http://127.0.0.1:3000/
TypeError: Data must be a string or a buffer
    at Hash.update (crypto.js:99:16)
    at write (project/node_modules/express-session/node_modules/object-hash/index.js:167:22)
    at Object._string (project/node_modules/express-session/node_modules/object-hash/index.js:301:7)
    at Object._object (project/node_modules/express-session/node_modules/object-hash/index.js:214:30)
    at Object.dispatch (project/node_modules/express-session/node_modules/object-hash/index.js:185:30)
    at project/node_modules/express-session/node_modules/object-hash/index.js:246:18
    at Array.forEach (<anonymous>)
    at Object._object (project/node_modules/express-session/node_modules/object-hash/index.js:242:21)
    at Object.dispatch (project/node_modules/express-session/node_modules/object-hash/index.js:185:30)
    at hash (project/node_modules/express-session/node_modules/object-hash/index.js:128:10)

This same app works in the current versions of express-session, so this would be breaking. I took a look and it seems to really just be a bug in the object-hash module and doesn't seem to be intended to throw like this. We could probably get a fix into object-hash. I can do this if you like, or would you like to to this?

But this now has me a bit worried. I wonder what other types will have this issue?

[skarbovskiy]

@dougwilson, finally I got my PR fixing this issue merged in object-hash repo.

[skarbovskiy]

@dougwilson ?)

[dougwilson]

Hi @skarbovskiy I apologize, I thought I responded. I used a fuzzer and found that library has false positives, as in it will falsely think two different objects are the same. Ideally it should never have a false positive, only maybe false negatives. It's always better to accidentally resave the same thing to the database than to accidentally not save an actual change that was made.

Here is a simplified version of two objects that object-hash produces identical hashes for:

> require('object-hash')({a:Buffer.from('foo,string:1:b:buffer:bar'),b:Buffer.from('bar')}, {respectType:false})
'a94b097bcb4e5a3753f871b1bdd4364e5c4b816c'
> require('object-hash')({a:Buffer.from('foo'),b:Buffer.from('bar,string:1:b:buffer:bar')}, {respectType:false})
'a94b097bcb4e5a3753f871b1bdd4364e5c4b816c'