Qute - escape expressions in HTML by default #6194

mkouba · 2019-12-16T11:30:50Z

resolves Handle escaping in Qute #6155
API changes:
introduce TemplateLocator
add Template#getVariant()

What this PR does?

For HTML templates ',",<, > and & are escaped by default
It's possilble to use raw and safe extension methods for String to "unescape" the value; ie. {foo.myString.raw}
RawString is never escaped
Template now has an optional Variant

gsmet · 2019-12-16T11:37:33Z

I would lean towards including that one into 1.1. I'm not very happy to release a templating engine without default escaping. WDYT?

Any chance you could add some documentation about this behavior?

mkouba · 2019-12-16T11:41:48Z

I would lean towards including that one into 1.1

I thought that 1.1 is released already ;-). If it's easy to backport (and I believe it should be because we did not change in Qute since 1.1 code freeze) then +1.

Any chance you could add some documentation about this behavior?

+1, I'll mention this in the docs.

extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/VariantTemplateProducer.java

independent-projects/qute/core/src/main/java/io/quarkus/qute/Escaper.java

mkouba · 2019-12-16T13:31:15Z

Hm, I've been thinking about the implementation and it's very likely not correct. The ResultMapper that performs escaping is only applied to expression values that resolve to a String. However, the escaping should be performed after the value is converted to String.

I'll need to change the default ResultMapper to convert everything to String first (if needed) and then escape the value. Custom ResultMapper can be still implemented if it's using higher getPriority().

independent-projects/qute/core/src/main/java/io/quarkus/qute/Escaper.java

- resolves quarkusio#6155 - API changes: - introduce TemplateLocator - add Template#getVariant()

FroMage

LGTM

FroMage · 2019-12-16T16:38:26Z

extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/EngineProducer.java

+        builder.addValueResolver(ValueResolvers.rawResolver());
+
+        // Escape some characters for HTML templates
+        Escaper htmlEscaper = Escaper.builder().add('"', "&quot;").add('\'', "&#39;")


https://en.wikipedia.org/wiki/List_of_XML_and_HTML_character_entity_references#Predefined_entities_in_XML says ' is available for both xml and html.

FroMage · 2019-12-16T16:40:22Z

extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/EngineProducer.java

+            @Override
+            public boolean appliesTo(Origin origin, Object result) {
+                return !(result instanceof RawString)
+                        && origin.getVariant().filter(EngineProducer::requiresDefaultEscaping).isPresent();


I'm guessing in the future we'll need to turn this into an SPI for custom escaping of other variant content types. But it can wait.

+1 for proper SPI... in the furure ;-).

FroMage · 2019-12-16T16:41:33Z

extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/EngineProducer.java

+        // Escape some characters for HTML templates
+        Escaper htmlEscaper = Escaper.builder().add('"', "&quot;").add('\'', "&#39;")
+                .add('&', "&amp;").add('<', "&lt;").add('>', "&gt;").build();
+        builder.addResultMapper(new ResultMapper() {


Are we sure it's the last mapper? I don't think users should be able to register mappers after this one, no?

Mappers with higher priority are tried first. So if a user wants to map some object then the priority should be > io.quarkus.qute.WithPriority.DEFAULT_PRIORITY.

FroMage · 2019-12-16T16:43:11Z

extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/VariantTemplateProducer.java

        } else if (suffix.equalsIgnoreCase(".json")) {
-            return "application/json";
+            return Variant.APPLICATION_JSON;


We support JSON but I didn't see any escaper for JSON.

Well, actually we don't support JSON ATM ;-). TBD

FroMage · 2019-12-16T16:46:17Z

independent-projects/qute/core/src/main/java/io/quarkus/qute/Variant.java

+
+    public final static String TEXT_HTML = "text/html";
+    public final static String TEXT_PLAIN = "text/plain";
+    public final static String TEXT_XML = "text/xml";


Note that XML can be application/xml in many cases, but I don't think it matters here. https://www.ietf.org/rfc/rfc2376.txt

Yes, I know. But in our case we guess the content type from a file suffix and set always text/xml for *.xml templates.

gastaldi · 2019-12-16T19:29:07Z

independent-projects/qute/core/src/test/java/io/quarkus/qute/EscaperTest.java

+    }
+
+    @Test
+    public void testRawStringRevolver() {


Suggested change

public void testRawStringRevolver() {

public void testRawStringResolver() {

I may be blind but what's the suggested change?

Revolver != resolver 😄

Ok, it's confirmed. I'm blind...

Most beautiful typo I've seen so far :)

I'm very good at typos!

independent-projects/qute/core/src/main/java/io/quarkus/qute/TemplateLocator.java

gsmet · 2019-12-16T23:10:24Z

Let's get moving, we can adjust later but we need at least some escaping in 1.1.

mkouba added the area/qute The template engine label Dec 16, 2019

mkouba added this to the 1.2.0 milestone Dec 16, 2019

mkouba requested review from FroMage and gsmet December 16, 2019 11:30

gsmet added the backport? label Dec 16, 2019

gsmet reviewed Dec 16, 2019

View reviewed changes

extensions/qute/runtime/src/main/java/io/quarkus/qute/runtime/VariantTemplateProducer.java Outdated Show resolved Hide resolved

gastaldi reviewed Dec 16, 2019

View reviewed changes

independent-projects/qute/core/src/main/java/io/quarkus/qute/Escaper.java Show resolved Hide resolved

FroMage reviewed Dec 16, 2019

View reviewed changes

independent-projects/qute/core/src/main/java/io/quarkus/qute/Escaper.java Show resolved Hide resolved

Qute - escape expressions in html by default

6321fc8

- resolves quarkusio#6155 - API changes: - introduce TemplateLocator - add Template#getVariant()

mkouba force-pushed the issue-6155 branch from bb272db to 6321fc8 Compare December 16, 2019 15:50

FroMage approved these changes Dec 16, 2019

View reviewed changes

gastaldi reviewed Dec 16, 2019

View reviewed changes

independent-projects/qute/core/src/main/java/io/quarkus/qute/TemplateLocator.java Show resolved Hide resolved

gsmet approved these changes Dec 16, 2019

View reviewed changes

gsmet merged commit 3f30c8d into quarkusio:master Dec 16, 2019

gsmet modified the milestones: 1.2.0, 1.1.0.Final Dec 16, 2019

gsmet removed the backport? label Dec 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Qute - escape expressions in HTML by default #6194

Qute - escape expressions in HTML by default #6194

mkouba commented Dec 16, 2019

gsmet commented Dec 16, 2019

mkouba commented Dec 16, 2019

mkouba commented Dec 16, 2019

FroMage left a comment

FroMage Dec 16, 2019

FroMage Dec 16, 2019

mkouba Dec 16, 2019

FroMage Dec 16, 2019

mkouba Dec 16, 2019

FroMage Dec 16, 2019

mkouba Dec 16, 2019

FroMage Dec 16, 2019

mkouba Dec 16, 2019

gastaldi Dec 16, 2019

mkouba Dec 17, 2019

gastaldi Dec 17, 2019

mkouba Dec 17, 2019

FroMage Dec 17, 2019

mkouba Dec 17, 2019

gsmet commented Dec 16, 2019

	public void testRawStringRevolver() {
	public void testRawStringResolver() {

Qute - escape expressions in HTML by default #6194

Qute - escape expressions in HTML by default #6194

Conversation

mkouba commented Dec 16, 2019

gsmet commented Dec 16, 2019

mkouba commented Dec 16, 2019

mkouba commented Dec 16, 2019

FroMage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Dec 16, 2019