Crash: TypeError: Cannot read property 'cid' of undefined

[djensen47]

The following not only manifests as an error but actually takes down the server.

This bug occurs when guest posts are turned off, email replies (via the plugin) are enabled, and an unrecognized email replies. (There might be a separate issue whether or not it's correct lookup uid based on email).

error: TypeError: Cannot read property 'cid' of undefined
    at Object.SocketHelpers.notifyOnlineUsers (/srv/nodebb/src/socket.io/helpers.js:19:33)
    at Emailer.notifyUsers (/srv/nodebb/node_modules/nodebb-plugin-emailer-mandrill/index.js:235:19)
    at fn (/srv/nodebb/node_modules/async/lib/async.js:741:34)
    at /srv/nodebb/node_modules/async/lib/async.js:1208:16
    at /srv/nodebb/node_modules/async/lib/async.js:166:37
    at /srv/nodebb/node_modules/async/lib/async.js:701:43
    at /srv/nodebb/node_modules/async/lib/async.js:167:37
    at Immediate._onImmediate (/srv/nodebb/node_modules/async/lib/async.js:1201:34)
    at processImmediate [as _immediateCallback] (timers.js:383:17)
TypeError: Cannot read property 'cid' of undefined
    at Object.SocketHelpers.notifyOnlineUsers (/srv/nodebb/src/socket.io/helpers.js:19:33)
    at Emailer.notifyUsers (/srv/nodebb/node_modules/nodebb-plugin-emailer-mandrill/index.js:235:19)
    at fn (/srv/nodebb/node_modules/async/lib/async.js:741:34)
    at /srv/nodebb/node_modules/async/lib/async.js:1208:16
    at /srv/nodebb/node_modules/async/lib/async.js:166:37
    at /srv/nodebb/node_modules/async/lib/async.js:701:43
    at /srv/nodebb/node_modules/async/lib/async.js:167:37
    at Immediate._onImmediate (/srv/nodebb/node_modules/async/lib/async.js:1201:34)
    at processImmediate [as _immediateCallback] (timers.js:383:17)
``

[djensen47]

A few of the functions in this async.waterfall are causing the issue.

async.waterfall([
    async.apply(Emailer.verifyEvent, eventObj),
    Emailer.resolveUserOrGuest,
    Emailer.processEvent,
    Emailer.notifyUsers
], function(err) {
    Emailer.handleError(err, eventObj);
    next(); // Don't block the continued handling of received events!
});

Specifically in processEvent:

if (!eventObj) {
    return callback();
}

In async.waterfall whenever we invoke callback without an error (always the first parameter), it always goes to the next step. This include invoking callback with no parameters, like in the code above. This might be fine except that code in notifyUsers is expecting postData to be present, instead it is undefined.

Option 1: Put some defensive code in notifyUsers to check for an "empty" postData. Downside: you have to continue to do checks like these all the way down the waterfall making it error prone.

Option 2: Use the async.waterfall error handling mechanism. In resolveUserOrGuest Instead of invoking callback(null, false) invoke the callback with an error condition and handle that condition in the handleError.

Option 3: Switch from async to rxjs because this really seems like a situation where reactive extensions would come in handy. Downside: it's a lot more refactoring work and if you're not familiar with Rx, there is a learning curve.

I think Option 2 is the best solution and if you don't mind, I'll submit a pull request today or tomorrow.

[akhoury]

related to this right? https://github.com/akhoury/nodebb-plugin-emailer-mandrill/issues/18 I'm gonna ping @barisusakli on this one.

[djensen47]

No, they are independent issues. This issue causes a crash but #18 just doesn't do anything if the reply cannot be posted via email.

[julianlam]

I'll take a look @akhoury, thanks for the analysis @djensen47, helpful as always :smile:

[julianlam]

Looks like I (or the original author) used a callback(null, false); to ensure eventObj was always populated, but yes, it shouldn't have fired off the callback.

Error handler handles it now, but does nothing at the moment. To be fixed with #18.

[djensen47]

Excellent, thanks for the fix! :+1: On Jan 4, 2016 8:48 AM, "Julian Lam" notifications@github.com wrote:

Looks like I (or the original author) used a callback(null, false); to ensure eventObj was always populated, but yes, it shouldn't have fired off the callback.

Error handler handles it now, but does nothing at the moment. To be fixed with #18 https://github.com/akhoury/nodebb-plugin-emailer-mandrill/issues/18.

— Reply to this email directly or view it on GitHub https://github.com/akhoury/nodebb-plugin-emailer-mandrill/issues/17#issuecomment-168730633 .