Crash on unexpected properties

stszap commented 2 years ago

Hello, we use juice to parse incoming emails and recently we got an email that causes juice to crash without any chance to catch the error. We narrowed it down to a simple example that gives the following error:

(node:53511) UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'match' of undefined
    at addProps (/private/tmp/node_modules/juice/lib/inline.js:205:35)
    at Element.<anonymous> (/private/tmp/node_modules/juice/lib/inline.js:231:7)
    at LoadedCheerio.each (/private/tmp/node_modules/cheerio/lib/api/traversing.js:480:26)
    at handleRule (/private/tmp/node_modules/juice/lib/inline.js:120:9)
    at Array.forEach (<anonymous>)
    at inlineDocument (/private/tmp/node_modules/juice/lib/inline.js:36:9)
    at juiceDocument (/private/tmp/node_modules/juice/lib/inline.js:459:3)
    at module.exports (/private/tmp/node_modules/juice/lib/cheerio.js:61:22)
    at onInline (/private/tmp/node_modules/juice/index.js:75:7)
    at /private/tmp/node_modules/web-resource-inliner/src/html.js:281:13

The callback is never called and try/catch can't catch the error either. The code to reproduce:

const juice = require('juice')

function main() {
    const input = `<html>
    <head>
        <style type="text/css">
            .logo img {
                .snippet {
                    padding: 20px 0px 5px 0px;
                    font-size: 0;
                    line-height: 0;
                }
        </style>
    </head>
    <body>
        <div class="logo">
            <img src="https://example.com" />
        </div>
    </body>
    </html>`

    try {
        juice.juiceResources(
            input,
            {
                webResources: {
                    scripts: false,
                    images: false,
                    svgs: false,
                    links: true,
                },
            },
            async (err, html) => {
                console.log(`juice results: ${err}, ${html} `)
            },
        );
    } catch (err) {
        console.log(`trycatch error: ${err}`)
    }
}

main()

node: v14.18.1 juice: 8.0.0

hc42 commented 2 years ago

Hi,

I stumbled across the same issue. However if you look at the CSS sample you provided you see two CSS violations:

missing closing '}'
you try to nest CSS selectors, which is not valid in pure CSS.

Therefore the CSS parser "mensch" used here is producing an invalid AST (as it claims it is not validating the CSS).

This invalid AST leads then to a property access in the inline.js line 205. There ".snippet" is interpretetd as css property without value. The var value, with undefined, is accessed with '.match(...' which leads to the crash.

Solution propsals:

Juice should handle bad CSS to not crash. e.g. skipping here if undefined.
The CSS you provide should be valid.

hc42 commented 2 years ago

Bildschirmfoto vom 2022-05-23 12-52-37

stszap commented 2 years ago

Hello. You are absolutely right the email I provided has broken CSS. I should have mentioned it in the beginning but unfortunately, we can't control all the emails that we get. It's a kind of public service and anybody can send one. But we would like juice not to crash at least. I added the following hack back then

import juice from 'juice';

// monkey patch to fix juice crash when trying to use element without "value"
// https://github.com/Automattic/juice/issues/398
let parseCSS = juice.utils.parseCSS
juice.utils.parseCSS = function (css) {
  let ret = parseCSS.apply(juice.utils, [css])
  for (let i in ret) {
    let declaractions = ret[i][1]
    if (declaractions && declaractions.length) {
      declaractions = declaractions.filter(elem => elem && elem.value !== undefined)
    }
    ret[i][1] = declaractions
  }

  return ret
}

It's ugly but after 2 months we haven't found any issues with it.

lwenn commented 2 years ago

Hello. You are absolutely right the email I provided has broken CSS. I should have mentioned it in the beginning but unfortunately, we can't control all the emails that we get. It's a kind of public service and anybody can send one. But we would like juice not to crash at least. I added the following hack back then
import juice from 'juice';

// monkey patch to fix juice crash when trying to use element without "value"
// https://github.com/Automattic/juice/issues/398
let parseCSS = juice.utils.parseCSS
juice.utils.parseCSS = function (css) {
  let ret = parseCSS.apply(juice.utils, [css])
  for (i in ret) {
    let declaractions = ret[i][1]
    if (declaractions && declaractions.length) {
      declaractions = declaractions.filter(elem => elem && elem.value !== undefined)
    }
    ret[i][1] = declaractions
  }

  return ret
}
It's ugly but after 2 months we haven't found any issues with it.

Hi, I meet the same issue, juice crash but there's no error, and the callback is never called. I tried to use your solution but it not work.

stszap commented 2 years ago

Hi, I meet the same issue, juice crash but there's no error, and the callback is never called. I tried to use your solution but it not work.

I rechecked the code and found an error (i in ret instead of let i in ret in the loop), sorry for that. If that still doesn't fix the problem for you then I'm not sure I can help. It's just a very crude "solution" so it may not fix all the problems that cause unhandled exceptions.

lwenn commented 2 years ago

thanks. I found the exception is caused by the inifinite loop when the css includes :not() function, parse it will return a wrong selector. And I hack the parseCSS to skip the wrong selector refer to yours. The issue of inifinite loop: #390

Automattic / juice

Crash on unexpected properties #398