Go: html/template: escapes + to +

[Dynom]

As far as I know, the + character has no special meaning when used as: <p> This is a plus "+" sign</p>. However html/template's escaping analysis thinks otherwise. What strikes me as odd is that template.HTMLEscaper("+") doesn't provide the same behaviour, even when skipping the allocation optimisation check.

What version of Go are you using (go version)?

$ go version
go1.15.4 darwin/amd64

Does this issue reproduce with the latest release?

It reproduces with 1.15.4, haven't tried _tip_.

What did you do?

package main

import (
    "html/template"
    "fmt"
    "strings"
)

func main() {
    var buf strings.Builder

    tpl := template.Must(template.New("foo").Parse(`{{ "+" }}`))
    tpl.Execute(&buf, nil)

    fmt.Printf("HTML Escaper's result %q\n", template.HTMLEscaper("+"))
    fmt.Printf("html/template         %q\n", buf.String())
}

Playground with the problem I'm having: https://play.golang.org/p/6rbaYLi9_Bt

(_I've also tried variations of html elements to create a different escaping context (e.g.: surround the action with e.g. <li> tags), which I thought would be the problem initially. That produced the same result however._)

What did you expect to see?

The same output as if the value ran through template.HTMLEscaper()

HTML Escaper's result "+"
html/template         "+"

What did you see instead?

HTML Escaper's result "+"
html/template         "+"

[cagedmantis]

/cc @empijei

[empijei]

Hi, do you mind expanding on what is the issue? The browser should unescape the "+" in all HTML contexts so it everything should still work, right?

Is this an issue on the efficiency of the escaper?

[empijei]

The reason that gets escaped is that it is in the following map and the two defined below it:

https://github.com/golang/go/blob/75e16f5127eb6affb4b473c93565a8d29a802e51/src/html/template/html.go#L50-L67

According to the doc the escape set should be a composition of special chars in these states:

• before attribute
• quoted attribute
• single quoted attribute
• unquoted attribute.
• after attribute

None of these includes a "+" sign.

I don't honestly know why it was there in the first place and, to my knowledge, it should not be.

But before we stop escaping it and we introduce XSS and break tests in the community I would like to do some additional checks on this.

[tmthrgd]

Digging through git blame shows it was first introduced 9 years ago in CL 4968058. This is the justification given at the time:

https://github.com/golang/go/blob/4670d9e6349119914c9a5a1b40ce32045f3c0061/src/pkg/exp/template/html/html.go#L16-L29

[empijei]

This looks like it is an extra-cautious escaping to take into account URLs and such in html attributes and other contexts.

I would be inclined to leave it as it is unless you think this is causing some misbehavior.

[Dynom]

Thanks for your time!

Given a <script> context, it makes sense. I would expect the JS escape analysis to do something with it. But with risk of stating the obvious, JS escaping probably shouldn't apply to HTML.

I ran into an issue when generating HTML with timestamps and where this character got escaped.

• html/template.HTMLEscaper() does escape < to <, but not +
• html/template.JSEscaper() does escape < to \u003C, but not +

I'm not entirely sure about the behaviour similarities between the public and private variants, but it feels like these should at least be aligned. I understand that risking up XSS is undesirable. My work-around and expectations were that, if my test data would be wrapped by HTMLEscaper() that it would result in exactly the same result, so that I can rely on that in my tests, e.g.:

patternsToMatch := []string{
    data.DateCreated.String(),
}
for _, p := range patternsToMatch {
    if !strings.Contains(tplResult, template.HTMLEscaper(p)) {
        t.Errorf("expected %q to occur in the template, but it didn't", p)
    }
}

The work-around now is to

if !strings.Contains(tplResult, func(p string) string {
    return strings.ReplaceAll(template.HTMLEscaper(p), "+", "+")
}(p)) {
    t.Errorf("expected %q to occur in the template, but it didn't", p)
}

So I suppose it's one of:

1. Bring HTMLEscaper() in line with htmlEscaper()
2. Improve the escape analysis so that + is only escaped within a JS context (e.g.: <script />), although I suspect this is nearly impossible to do perfectly if obfuscation is considered as well.
3. Leave as is

[empijei]

I agree and I am leaning towards 1 and 3.

Asking @kele for a second opinion.