Existing encrypted v3.2.0 sessions are not decrypted correctly by v4.4.1 (Cannot read property 'expires' of undefined)

[pauldwaite]

I'm submitting a ...

[X] bug report
[ ] feature request
[ ] question about the decisions made in the repository
[ ] question about how to use this project

Summary

Existing encrypted session objects from connect-mongo v3.2.0 remain over-stringified when decrypted by v4.4.1, as described in #393.

Other information

Steps to reproduce:

(Minimal docker-compose project to reproduce the issue: https://github.com/pauldwaite/connect-mongo-issue-420)

1. Start an Express web app that creates a session using connect-mongo 3.2.0.
2. Visit the web app in a browser.
3. Stop the web app (but persist the session data)
4. Update the web app to use connect-mongo 4.4.1
5. Restart the web app
6. Refresh the browser

Expected behaviour

The session still exists, and works correctly.

Actual behaviour

connect-mongo throws an error:

Error: Unable to parse ciphertext object!
    at /workdir/node_modules/connect-mongo/build/main/lib/MongoStore.js:187:23
    at processTicksAndRejections (internal/process/task_queues.js:93:5)
    at async MongoStore.decryptSession (/workdir/node_modules/connect-mongo/build/main/lib/MongoStore.js:186:31)
    at async /workdir/node_modules/connect-mongo/build/main/lib/MongoStore.js:211:21
TypeError: Cannot read property 'expires' of undefined
    at MongoStore.Store.createSession (/workdir/node_modules/express-session/session/store.js:87:29)
    at inflate (/workdir/node_modules/express-session/index.js:368:13)
    at /workdir/node_modules/express-session/index.js:495:11
    at /workdir/node_modules/connect-mongo/build/main/lib/MongoStore.js:218:17
    at processTicksAndRejections (internal/process/task_queues.js:93:5)

Additional information

I added a breakpoint in the get method in kruptein, and the cipher text variable started like this:

"\"{\\\"hmac\\\":\\\"97d0b61cd19623a...

Which I thought looked a bit like the over-stringified session object referred to in issue #393.

[mingchuno]

@pauldwaite I think we have few options here

1. Stick with 3.2.0 here since V4 is really a breaking change upgrade in a lot of aspects.
2. Behaviour in 3.2.0 is not expected as stated in TypeError: Cannot read property 'expires' of undefined when using the crypto option #393 so I think I will not workaround V4 to make it compatible for both correct and wrong implementation.
3. Can we find a way to make some script to strip away the over stringify part in the DB so that it will work under V4.
4. Maybe you can add some extra logic to catch the error it throws and strip away the over-stringified session object in DB and retry again. It should work.
5. Perhaps I need to add this issue in Known issue and migration guide.

[jas-]

@pauldwaite if you normalize the encrypted sessions then convert the session manually to ASN.1 using the private function available from the kruptein module... kruptein.schema.encode(session).toString(‘kruptein._encodeas) then that should be all the conversion you need to keep sessions intact between the breaking change from 3x to 4x

[pauldwaite]

Cool cool — so, as I understand it, the issue is caused by 3.2.0's session encryption, which only causes a problem after updating to 4.x, if there are existing sessions encrypted by 3.2.0.

On the app I'm working on, I think our other security measures are robust enough, and our user base small enough, that we could disable session encryption for 3.2.0, wait for all encrypted sessions to expire, update to 4.x, then re-enable session encryption; and thus avoid having to write any code to deal with the issue.

I totally understand not wanting to add code to 4.x to handle this migration issue. I'm not sure how many people encrypt sessions, and would also need to preserve existing sessions across an update to connect-mongo, maybe it's hardly anyone. It would be great to note this as a known issue in the v4 migration guide though.

[jas-]

@pauldwaite You could always convert them all... (pseudo code) prior to a go live event.

let secret = "squirrel", opts = {};
let mongoose = require('mongoose');
let kruptein = require('kruptein')(opts);

const uri = 'mongodb://localhost';

mongoose.connect(uri);
mongoose.connection.once('open', () => {
    console.log(`Connected to ${uri}`);
});

let Schema = mongoose.Schema;
let sessions = new Schema({
    id: String,
    session: String
});

sessions.aggregate({
  $group:{
    id: $id,
    session: $session
  }, function(err, obj) {
    if (err) throw err;
    
    // Test obj to ensure we don't mangle valid ASN.1 encoded sessions
    if (obj.session.match(/hmac/)) {

      // simple fix, hopefully it doesn't need more than this
      try {
        obj.session = JSON.parse(obj.session);
      } catch(e) {
        console.log("Unable to parse");
      }

       // Generate a new value 
       // this one will be base64 encoded ASN.1 value if using v3.0.0 of the kruptein module
       kruptein.set(secret, obj.session, function(e, ct) {
         if (e) return e;
         obj.session = ct;
       });

       // Make sure obj.session is ok then save
       console.log(obj);
    }
  }
})

[mingchuno]

Added doc in commit 9c1d0b5