search

premailer / css_parser

Ruby CSS Parser
Other
279 stars 110 forks source link

Invalid CSS raises error starting in 1.8.0 #125

Closed JasonBarnabe closed 1 year ago

JasonBarnabe commented 3 years ago

Not sure what the general strategy is in this gem for handling invalid CSS, but the behaviour changed between 1.7.1 and 1.8.0.

html = <<~HTML
<!DOCTYPE html>
<html>
  <head>
    <style type="text/css">
a {
  line-height:  !important;
}
    </style>
  </head>

  <body>

  </body>
</html>
HTML

parser = CssParser::Parser.new
parser.load_string! html
puts parser.to_h

In 1.7.1:

{"all"=>{"<!DOCTYPE html> <html> <head> <style type=\"text/css\"> a"=>{"line-height"=>"!important"}}}

In 1.8.0:

    15: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:494:in `load_string!'
    14: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:161:in `add_block!'
    13: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:329:in `parse_block_into_rule_sets!'
    12: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:329:in `scan'
    11: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:361:in `block in parse_block_into_rule_sets!'
    10: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:168:in `add_rule!'
     9: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/parser.rb:168:in `new'
     8: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:243:in `initialize'
     7: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:606:in `parse_declarations!'
     6: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:606:in `each'
     5: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:614:in `block in parse_declarations!'
     4: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/2.6.0/forwardable.rb:230:in `add_declaration!'
     3: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:99:in `[]='
     2: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:99:in `new'
     1: from /home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:33:in `initialize'
/home/jason/.rbenv/versions/2.6.6/lib/ruby/gems/2.6.0/gems/css_parser-1.8.0/lib/css_parser/rule_set.rb:41:in `value=': value is empty (ArgumentError)
grosser commented 3 years ago

kinda like the new behavior more, but it would be nice if it raised a more descriptive error ... /cc @ojab

dark-panda commented 3 years ago

We're having a similar issue where the upgrade from 1.7.1 to 1.9.0 has started raising exceptions like the following:

ArgumentError - Cannot parse calc(1em * 8 / 4) auto 0

I haven't been able to reduce this to a minimal example but I'll report back when I get one.

dark-panda commented 3 years ago

I've pushed a PR that fixes the issue I had in #126. I believe these issues aren't related so perhaps I ought to open up a new issue, but when I commented yesterday I thought it might be an overall-sort-of-parsing issue before I actually decided to try and fix the issue. At any rate, the PR fixes the calc issue I ran into.

grosser commented 3 years ago

if calc worked before then something went wrong during refactoring and we should be able to undo that ? ... have you tried a git-bisect with your testcase to pinpoint the bad commit ?

dark-panda commented 3 years ago

@grosser It never really worked, but it was often silently ignored until the commit in 8868aa69eb4369161a89765df11ab2537915c968, specifically with this code:

https://github.com/premailer/css_parser/blob/master/lib/css_parser/rule_set.rb#L373

The issue being if the value is being split on whitespace using commas as parameter separators, then the case statement can be very misleading, and can raise an error when the size of the split array is not within 1-4 values, as in the case that I was running into, where the value is calc(1em / 4 * 4), or ['calc(1em', '/', '4', '*', '4)'] when split on the whitespace. This used to silently pass by since there was no raise before, and values would not get set, and the whole thing would basically become nil and ignored. You'd get nil values for everything in that case.

However, if you did calc(1em/4*4) where there is no whitespace, the values length would be 1, and you'd end up getting:

{
  "margin-left" => "calc(1em/4*4)",
  "margin-right" => "calc(1em/4*4)",
  "margin-top" => "calc(1em/4*4)",
  "margin-bottom" => "calc(1em/4*4)"
}

Which is correct, but only because of the lack of whitespace in the calc. If you include varying amounts of whitespace, the results change. This is what happens in the current master as well as v1.7.1:

"margin: calc(1em / 4)"
{
  "margin-top" => "calc(1em", 
  "margin-right" => "/", 
  "margin-bottom" => "4)", 
  "margin-left" => "/"
}

"margin: calc(1em /4)"
{
  "margin-top" => "calc(1em", 
  "margin-right" => "/4)", 
  "margin-bottom" => "calc(1em", 
  "margin-left" => "/4)"
}

"margin: calc(1em / 4 * 4)"
# in current master
ArgumentError (Cannot parse calc(1em / 4 * 4))

"margin: calc(1em / 4 * 4)"
# in v1.7.1:
{}

# With commas:
"margin: clamp(1rem, 2.5vw, 2rem)"
{
  "margin-top" => "clamp(1rem,", 
  "margin-right" => "2.5vw,", 
  "margin-bottom" => "2rem)", 
  "margin-left" => "2.5vw,"
}

With my PR, the expected values are expanded:

"margin: calc(1em / 4)"
{
  "margin-top" => "calc(1em / 4)",
  "margin-right" => "calc(1em / 4)",
  "margin-bottom" => "calc(1em / 4)",
  "margin-left" => "calc(1em / 4)"
}

"margin: calc(1em /4)"
{
  "margin-top" => "calc(1em /4)", 
  "margin-right" => "calc(1em /4)", 
  "margin-bottom" => "calc(1em /4)", 
  "margin-left" => "calc(1em /4)"
}

"margin: calc(1em / 4 * 4)"
{
  "margin-top" => "calc(1em / 4 * 4)", 
  "margin-right" => "calc(1em / 4 * 4)", 
  "margin-bottom" => "calc(1em / 4 * 4)", 
  "margin-left" => "calc(1em / 4 * 4)"
}

"margin: clamp(1rem, 2.5vw, 2rem)"
{
  "margin-top" => "clamp(1rem, 2.5vw, 2rem)", 
  "margin-right" => "clamp(1rem, 2.5vw, 2rem)", 
  "margin-bottom" => "clamp(1rem, 2.5vw, 2rem)", 
  "margin-left" => "clamp(1rem, 2.5vw, 2rem)"
}

So basically calc and similar functions were not working even before v1.8.0 at least under certain circumstances where whitespace is involved due to the split(/\s/), basically, they just didn't raise ArgumentErrors in some situations and in others produced odd expansions.

The WHITESPACE_SEPARATOR magic-ish constant that I included is so that we can still split on whitespace as before, but this time around we temporarily change the whitespace within CSS functions to a magic string that doesn't factor in to the split(/\s/). I picked something which would never reasonably be used in actual CSS ("___SPACE___", named similarly to Ruby's constants like __FILE__, __LINE__, __CLASS__, etc.) so we could avoid any name clashes within code. It just needs to be something that's non-whitespace and also wouldn't ever really appear in any CSS.

ryanscottaudio commented 3 years ago

Can we return to the question at hand? Or is there a way to go back to pre-1.8.0 functionality (i.e. letting invalid CSS pass) using an argument or something?

ryanscottaudio commented 3 years ago

@grosser pinging on this. is there a way to pass something so that we can return to the pre-1.8.0 functionality?

grosser commented 3 years ago

removing the raise would do the trick ?

rikkipitt commented 2 years ago

Hey folks, I'm experiencing something similar where emails contain invalid margin or padding (e.g. 5 values instead of 4, 3, 2 or 1).

ArgumentError: Cannot parse 10px 10px 10px 5px 10px

In my case, it would be good to ignore these invalid styles too. Is the current workaround to use a pre-1.8.0 version of this gem?

rikkipitt commented 2 years ago

I can confirm that in my case, 1.7.1 works without the raise. @grosser would it be possible to feature flag this if people want to turn it off? Appreciate your work on this, cheers.

grosser commented 2 years ago

PR welcome ... would love a unified solution, but as long as the default is "what works for most ppl / is intuitive" 🤷

markedmondson commented 1 year ago

PR that allows option passing to mute the exception here: https://github.com/premailer/css_parser/pull/132

grosser commented 1 year ago

1.12.0

grosser commented 1 year ago

allows setting rule_set_exceptions: false to ignore these exceptions