Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Crash on unexpected properties #398

Open
stszap opened this issue Apr 3, 2022 · 6 comments
Open

Crash on unexpected properties #398

stszap opened this issue Apr 3, 2022 · 6 comments

Comments

@stszap
Copy link

stszap commented Apr 3, 2022

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
Copy link

hc42 commented May 23, 2022

Hi,

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

  1. missing closing '}'
  2. 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:

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

@hc42
Copy link

hc42 commented May 23, 2022

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

@stszap
Copy link
Author

stszap commented Jun 2, 2022 •
edited
Loading

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
Copy link

lwenn commented Aug 28, 2022

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
Copy link
Author

stszap commented Aug 29, 2022 •
edited
Loading

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
Copy link

lwenn commented Aug 31, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@hc42 @lwenn @stszap