Errors with malformed CSS

[Synchro]

I ran into two errors when processing some malformed style tags.

The style tag contained some stray HTML injected by ckeditor:

table.columns .right-text-pad {
  padding-right: 10px;
}
<style data-cke-temp="1">html{cursor:text;*cursor:auto}
img,input,textarea{cursor:default}

Trying to process this produced two errors. The first is caused by it choosing <style data-cke-temp="1">html as a selector, which will not convert to a valid xpath query:

2015-06-19 14:02:03 DOMXPath::query(): Invalid expression (error type 2 in ./vendor/pelago/emogrifier/Classes/Emogrifier.php on line 279)

The second is a consequence of this, attempting to iterate over the failed result of the query because the return value is not checked:

2015-06-19 14:02:03 Invalid argument supplied for foreach() (error type 2 in ./vendor/pelago/emogrifier/Classes/Emogrifier.php on line 282)

Obviously it would be better if bad CSS was not passed in in the first place, but it would be better to ignore it than throw errors.

[oliverklee]

As for the cause of the error: I'm for having a whitelist of characters (i.e., a regular expression) which we can use to validate those selectors before using them.

[Synchro]

I had a little look into matching selectors - it's not simple at all! I think the only invalid char in that selector is < - everything else is valid in other contexts.

[oliverklee]

So I propose discarding any selectors that contain a "<". What do you think about this?

And would you be willing to create a PR for this?

[Synchro]

Yes, I think that's reasonable. Where would I find the code that identifies selectors?

[oliverklee]

That should be https://github.com/jjriv/emogrifier/blob/master/Classes/Emogrifier.php#L337 in the method parseSelectors.

[Synchro]

As I've been reading, it's very hard to match CSS selectors with a regex as they are not a regular grammar. I'm thinking that it would be easier to apply a simple filter to remove stray tags before trying to parse the styles, for example:

$css = preg_replace('/<[^>]*>/', '', $css);

or even a simple strip_tags(). While this is something of an edge case, it strikes me that this should be both harmless and effective as I can't think of any legitimate reason there should be HTML tags inside CSS.

[JakeQZ]

This was fixed by #400 (in 2.0.0), with some prior relevant changes in #361 and #392. The unit test EmogrifierTest::emogrifyNotInDebugModeIgnoresInvalidCssSelectors() (there's a similar one for CssInliner) should ensure that rules with invalid selectors are silently ignored, unless debug mode is enabled.

So I'm closing this now.