GROOVY-12275: Encode snippet attribute values and bound snippet marku… - #2812
Merged
Conversation
…p regexes
Two defects in the same {@snippet} handling, both reached from a doc comment
in the source being documented.
The class and id attributes were appended to the generated element without
encoding. The attribute parser accepts a double quote inside a value which
was single quoted or unquoted, so a value could close its attribute and the
tag around it. Encode both through a new SimpleGroovyClassDoc.encodeAttribute,
which escapes the ampersand first and then the characters that can end an
attribute or start a tag. The snippet body was already escaped; this brings
the attributes up to the same standard.
A markup directive's regex attribute was compiled and run against snippet
lines with no bound. Give each directive a deadline using RegexGuard, and
leave the line unannotated rather than half annotated if it expires.
The payload in the test is worth a note. The finding cites (a+)+$ against a
long run of characters, and on a current JDK that is not slow: the textbook
nested-quantifier patterns, (a+)+b, (a|aa)+$, (x+x+)+y and (a*)*b among
them, all complete in about a millisecond, because the engine recognises
them. A backreference still backtracks exponentially. Measured with the
guard removed, a directive carrying (a+)+\1b against a 32 character line
took 152 seconds to render one page, and grows exponentially with the line;
with the guard the same page renders in well under a second. So the finding
is right that the risk exists and wrong about how it is reached, and a test
built on its own example would have passed with or without a fix.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2812 +/- ##
==================================================
+ Coverage 70.1837% 70.1876% +0.0039%
- Complexity 35844 35849 +5
==================================================
Files 1562 1562
Lines 132528 132542 +14
Branches 24379 24379
==================================================
+ Hits 93013 93028 +15
- Misses 31106 31107 +1
+ Partials 8409 8407 -2
🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…p regexes
Two defects in the same {@snippet} handling, both reached from a doc comment in the source being documented.
The class and id attributes were appended to the generated element without encoding. The attribute parser accepts a double quote inside a value which was single quoted or unquoted, so a value could close its attribute and the tag around it. Encode both through a new SimpleGroovyClassDoc.encodeAttribute, which escapes the ampersand first and then the characters that can end an attribute or start a tag. The snippet body was already escaped; this brings the attributes up to the same standard.
A markup directive's regex attribute was compiled and run against snippet lines with no bound. Give each directive a deadline using RegexGuard, and leave the line unannotated rather than half annotated if it expires.
The payload in the test is worth a note. The finding cites (a+)+$ against a long run of characters, and on a current JDK that is not slow: the textbook nested-quantifier patterns, (a+)+b, (a|aa)+$, (x+x+)+y and (a*)*b among them, all complete in about a millisecond, because the engine recognises them. A backreference still backtracks exponentially. Measured with the guard removed, a directive carrying (a+)+\1b against a 32 character line took 152 seconds to render one page, and grows exponentially with the line; with the guard the same page renders in well under a second. So the finding is right that the risk exists and wrong about how it is reached, and a test built on its own example would have passed with or without a fix.