nodemailer/mailparser

Cannot read properties of undefined (reading 'forEach') in processNode()

0x557269656C opened this issue · 5 comments

0x557269656C commented

Hi community!

I'm hitting a condition with some messages where, at the very end of processNode the recursive call throws complaining about children, being undefined.

mailparser/lib/mail-parser.js

Line 752 in 88c26ca

node.children.forEach(subNode => {

Not being familiar with the code base I tried to understand the edge-case condition that could lead to this. My guess (which may be incorrect, sorry 😞) is that this is a condition trigger by cleanup being called early after readData was invoked without actually triggering the processChunk call, leading to the this.tree node not being updated with a newNode and keeping the construct assigned value of false.

I might be off though... The data triggering this might not be RFC822 compliant at all, heck it might not be a valid email at all. I have not issue with the actual parsing of adequate messages, the report condition is just about an unhandled error condition.

Thank you all for your work and contribution!

andris9 commented

Can you provide a message that triggers this? Also, can you catch this error? If you provide random input to mailparser and can't catch the error, then it is a bug. If, on the other hand, you can catch it, then it is expected behavior.

0x557269656C commented

Can you provide a message that triggers this? Also, can you catch this error? If you provide random input to mailparser and can't catch the error, then it is a bug. If, on the other hand, you can catch it, then it is expected behaviour.

Thank you @andris9 I'm trying to control the error but its not practical to identify the actual perpetrator that is generating the issue, but I'll try to get some sample code in place.

But I would kindly ask for your consideration on handling the invoking of .forEach on a potentially undefined value. I am unfamiliar with the library version compatibility targets but perhaps this is one of those things that could just be fixed by dropping an optional chaining operator ?. here and there and control the unhandled exception.

Thank you for sharing your time and your work!

0x557269656C commented

Hi @andris9 here's the test code to reproduce the issue:

// Base imports
const fs = require('fs');
const mailparser = require('mailparser');

try{
  // Create test fixture file
  fs.writeFileSync('./dummy', Buffer.from(Array(1e7).fill(0x0)));

  // Instantiate the parser and create a read stream 
  const parser = new mailparser.MailParser();
  const stream = fs.createReadStream('./dummy');

  // Hookup parser listners
  parser.once('end', () => { console.log('END') });
  parser.once('headers', headers => { console.log('HEADERS', headers); });

  parser.on('error', error => { console.log('ERROR', error); });

  parser.on('data' , data => {
    if (data.type === 'attachment') {
        console.log(data.filename);
        data.content.pipe(process.stdout);
        data.content.on('end', () => data.release());
    }
  });

  // Program a stream unexpected failure condition (remove the file mid-parsing)
  setTimeout(() => fs.unlink('./dummy', () =>{}), 200);

  // Leverage the parser's stream facilities
  stream.pipe(parser);

}
catch(e) { console.log('CAPTURED', e); } // Attempt to capture the error in try/catch block

As described the code, when raising an error in the stream that will go back to the mentioned forEach call that will create an uncatchable error.

In node v18.12.1 using mailparser 3.6.2 running under MacOs Monterey this yields the following exception:

/test-case/node_modules/mailparser/lib/mail-parser.js:752
            node.children.forEach(subNode => {
                          ^

TypeError: Cannot read properties of undefined (reading 'forEach')
    at processNode (/test-case/node_modules/mailparser/lib/mail-parser.js:752:27)
    at MailParser.getTextContent (/test-case/node_modules/mailparser/lib/mail-parser.js:757:9)
    at finish (/test-case/node_modules/mailparser/lib/mail-parser.js:276:26)
    at Immediate.<anonymous> (/test-case/node_modules/mailparser/lib/mail-parser.js:288:17)
    at process.processImmediate (node:internal/timers:471:21)

Node.js v18.12.1
0x557269656C commented

I can confirm that adding the optional chaining operator as follows provides mitigation to the issue:

node?.children?.forEach(subNode => {
  processNode(alternative, level + 1, subNode);
});

Although this would fix the underlaying bug it will not address the limitations on the library error control flows, meaning that the code, under these circumstances, will emit the end event and silently exit without emitting an error event.

Running the same sample code after patching, yields the following output:

END
andris9 commented

You're right about it. I published a fix as mailparser@3.6.3 b1d6a25

In general, if you feed nonsense to mailparser then you should not expect anything reasonable as a response. So returning an empty response for such an input is OK. The module should never generate uncatchable errors, though.

  • 🎉1