[HarriFent]

Fixes

Fixes #1038

Purpose

In an .html template, ${name} is HTML-escaped but &{'key', name} is not. The arguments go into Messages.get(key, args) and the result is printed raw. So any user-set value passed as a message argument is a stored XSS:

# conf/messages
welcome=Welcome %s

&{'welcome', user.name}   <!-- user.name = <script>alert(1)</script> runs -->

This PR escapes the arguments in ExecutableTemplate.__getMessage(), using the same rule as __safe():

• Only in .html templates, and not inside #{verbatim}.
• Arguments are escaped with HTML.htmlEscape(arg.toString()), the same as ${arg}.
• The message text is not escaped. Messages files often hold markup (Please <a href="...">register</a>), and they are written by the developer.
• These pass through untouched: RawData (so arg.raw() opts out), null, Number, Boolean, Character, Date, Calendar and java.time.temporal.TemporalAccessor. This keeps %d, %f, %c and %t formats working.
• .txt, .xml, .json and other templates are unchanged.

The docs section on &{…} in templates.textile now says this.

Background Context

• HTML escape in internationalized message #1038 (2016): reported again. In 2019 asolntsev and tomparle agreed escaping should apply to arguments but not to the message itself. They asked for a clean PR that only does this, plus a short migration note.
• HEALTH-198 prevent XSS through localized message parameters #1318 (2019): an earlier fix with the same idea, but it bundled a lot of unrelated changes and was closed. This PR is the curated version: one method and its tests.

Choices:

• Any other object is escaped through toString(), not only String/CharSequence. That matches ${obj} and also covers GStrings and entities whose toString() returns user data.
• Only HTML. __safeFaster() also has XML and CSV formatters, but the CSV one quotes the whole value, which would be wrong for a fragment inside a message.
• A side effect: a user-supplied argument that contains &{other.key} is no longer expanded as a message reference, because the & is escaped. Before, user input could trigger message lookups.

Migration note

If a template passes HTML on purpose as a message argument, wrap it in raw():

&{'welcome', user.name, link.raw()}

Arguments used as nested message keys (key=%d &{%s}) are escaped before lookup. So a key containing <, >, &, " or ' would no longer match. Wrap that argument in raw() if that applies.

Release notes

Security: &{'key', args} in HTML templates now HTML-escapes its arguments like ${}; the message text is unchanged. Wrap arguments that hold intentional HTML in .raw().

Tests

New GroovyTemplateMessageArgsTest covers: String and GString args escaped, other objects escaped like ${}, raw() and RawData untouched, message text not escaped, %d/%tY with numbers, numeric strings, Calendar, Date and null, &{…} inside an argument not expanded, .txt not escaped, #{verbatim} not escaped.

Ran on JDK 21 (macOS):

• ant clean jar unittest -Dbuild-release=21: all 257 unit tests pass. On master the new test fails 6 of 9; the three "not escaped" cases pass on both.
• ant test -Dbuild-release=21: passes, including the sample apps whose CRUD views use &{'crud.…', type.modelName}.

References

• HTML escape in internationalized message #1038
• HEALTH-198 prevent XSS through localized message parameters #1318

[mergify]

Tick the box to add this pull request to the merge queue (same as @mergifyio queue).

• Queue this pull request