/cc @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?

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

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.

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

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.

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

I agree and I am leaning towards 1 and 3.

Asking @kele for a second opinion.

I'm leaning towards option 3 (leave as it is), because I think that the benefit of doing 1 (aligning HTMLEscaper with htmlEscape) is rather small, whereas making the change might break folks' tests.

If we had it documented somewhere that HTMLEscaper will always behave exactly the same as template.Template, I'd lean stronger towards 1.

More generally than just html/template's HTMLEscaper vs template execution escaping, there are really a bunch of different HTML escapers in the (extended) standard library.

Grouped by the same underlying implementation, I've found:

html/template template execution: https://cs.opensource.google/go/go/+/master:src/html/template/html.go;l=42;drc=2cd2ff6f564dce5be0c4fb7f06338ff7af3fc9a9

{text,html}/template.{HTMLEscape,HTMLEscaper,HTMLEscapeString}: https://cs.opensource.google/go/go/+/master:src/text/template/funcs.go;l=603;drc=2b50ab2aee75d3c361fcd1eb39e830e2e73056b6

html.EscapeString: https://cs.opensource.google/go/go/+/master:src/html/escape.go;l=178;drc=52c4488471ed52085a29e173226b3cbd2bf22b20

x/net/html.EscapeString: https://cs.opensource.google/go/x/net/+/master:html/escape.go;l=237;drc=3d87fd621ca9a824c5cff17216ce44769456cb3f

Every single one of these implementations has a slightly different set of escaped characters. As a user, particularly one unfamiliar with HTML safety, it is not at all clear which of these is "correct" or "safest" to use, or why they are different in the first place.

cc @katiehockman @empijei

This is an old issue, but I also recently ran into this. I had to construct a helper function to get a test working that would match the html/templates behavior. It would be good if the format was standardized across Go escaping functions.

I can open a separate issue if needed, but this seems to be basically the same problem of multiple inconsistent implementations that @prattmic described above.

I hit this in the context of URL escaping. This makes test code challenging because the (private) template escaping functions generate different content than the publicly available escaping functions -- namely lowercase vs uppercase.

Rendering with html/template converts = to %3d (lowercase) but using template.URLQueryEscaper results in %3D (uppercase).

Playground example: https://go.dev/play/p/jLK2bRuO8cL?v=gotip

html/template lowercase conversion: https://cs.opensource.google/go/go/+/master:src/html/template/url.go;drc=07b19bf5ab1160814ffedd448ce65c0eb6e9643a;l=134

{html,text}/template.URLQueryEscaper calls url.URLQueryEscaper: https://cs.opensource.google/go/go/+/master:src/text/template/funcs.go;l=749

url.URLQueryEscaper uppercase conversion: https://cs.opensource.google/go/go/+/master:src/net/url/url.go