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

don't allow invalid syntax inside comment tag #1770

Merged
merged 2 commits into from
Jan 2, 2024
Merged

don't allow invalid syntax inside comment tag #1770

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c5ad7d
don't allow invalid syntax inside comment tag
ggmichaelgo Dec 14, 2023
fc9c338
refactor: rename comment tag unit test
ggmichaelgo Dec 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions lib/liquid/tags/comment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,15 +49,9 @@ def parse_body(body, tokenizer)
next if tag_name_match.nil?

tag_name_match[1]
elsif token =~ BlockBody::FullToken && Regexp.last_match(2) == "comment" || Regexp.last_match(2) == "endcomment"
# aggressively match comment tag or comment tag delimiter
Regexp.last_match(2)
else
tag_name_match = BlockBody::FullTokenPossiblyInvalid.match(token)

next if tag_name_match.nil?

tag_name_match[2]
token =~ BlockBody::FullToken
Regexp.last_match(2)
end

case tag_name
Expand Down
42 changes: 31 additions & 11 deletions test/unit/tags/comment_tag_unit_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,25 +31,45 @@ def test_does_not_parse_nodes_inside_a_comment
LIQUID
end

def test_allows_incomplete_tags_inside_a_comment
assert_template_result("", <<~LIQUID.chomp)
def test_allows_unclosed_tags
assert_template_result('', <<~LIQUID.chomp)
{% comment %}
{% assign foo = "1"
{% if true %}
{% endcomment %}
LIQUID
end

assert_template_result("", <<~LIQUID.chomp)
def test_open_tags_in_comment
assert_template_result('', <<~LIQUID.chomp)
{% comment %}
{% comment %}
{% invalid
{% endcomment %}
{% assign a = 123 {% comment %}
{% endcomment %}
LIQUID

assert_template_result("", <<~LIQUID.chomp)
{% comment %}
{% {{ {%- endcomment %}
LIQUID
assert_raises(Liquid::SyntaxError) do
assert_template_result("", <<~LIQUID.chomp)
{% comment %}
{% assign foo = "1"
{% endcomment %}
LIQUID
end

assert_raises(Liquid::SyntaxError) do
assert_template_result("", <<~LIQUID.chomp)
{% comment %}
{% comment %}
{% invalid
{% endcomment %}
{% endcomment %}
LIQUID
end

assert_raises(Liquid::SyntaxError) do
assert_template_result("", <<~LIQUID.chomp)
{% comment %}
{% {{ {%- endcomment %}
LIQUID
end
Comment on lines +49 to +72
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be valid templates right? Incomplete tags should be allowed inside of a comment block.

Copy link
Contributor Author

@ggmichaelgo ggmichaelgo Dec 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, however, we can't introduce a change that will break existing Liquid templates.

For an example,

{% comment %}
  {% assign a = 123
  {% endcomment %}
{% endcomment %}

This Liquid template is a valid with version 5.4.0 because the assign tag is captured as {% assign a = 123 {% endcomment %}. (Liquid allows extra string inside a tag)

So, if we parse comment tag like the raw tag, we would capture, we would ignore the {% assign a = 123 and capture {% endcomment %} from line 3 as the comment tag's delimiter. Since the template has an extra {% endcomment %} tag delimiter, the template is no longer valid.

end

def test_child_comment_tags_need_to_be_closed
Expand Down