[release-branch.go1.22] html/template: escape additional tokens in MarshalJSON errors

Escape "</script" and "<!--" in errors returned from MarshalJSON errors
when attempting to marshal types in script blocks. This prevents any
user controlled content from prematurely terminating the script block.

Updates #65697
Fixes #65969

Change-Id: Icf0e26c54ea7d9c1deed0bff11b6506c99ddef1b
Reviewed-on: https://go-review.googlesource.com/c/go/+/564196
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Reviewed-by: Damien Neil <dneil@google.com>
(cherry picked from commit ccbc725f2d678255df1bd326fa511a492aa3a0aa)
Reviewed-on: https://go-review.googlesource.com/c/go/+/567535
Reviewed-by: Carlos Amedee <carlos@golang.org>

diff --git a/src/html/template/js.go b/src/html/template/js.go
index b159af8e4bf26b3c466e70c91c0f53f6be57a8e7..d911ada26d3d94d2d94073ce76ee0e8394af051e 100644 (file)
--- a/src/html/template/js.go
+++ b/src/html/template/js.go
@@ -171,13 +171,31 @@ func jsValEscaper(args ...any) string {
        // cyclic data. This may be an unacceptable DoS risk.
        b, err := json.Marshal(a)
        if err != nil {
-               // Put a space before comment so that if it is flush against
+               // While the standard JSON marshaller does not include user controlled
+               // information in the error message, if a type has a MarshalJSON method,
+               // the content of the error message is not guaranteed. Since we insert
+               // the error into the template, as part of a comment, we attempt to
+               // prevent the error from either terminating the comment, or the script
+               // block itself.
+               //
+               // In particular we:
+               //   * replace "*/" comment end tokens with "* /", which does not
+               //     terminate the comment
+               //   * replace "</script" with "\x3C/script", and "<!--" with
+               //     "\x3C!--", which prevents confusing script block termination
+               //     semantics
+               //
+               // We also put a space before the comment so that if it is flush against
                // a division operator it is not turned into a line comment:
                //     x/{{y}}
                // turning into
                //     x//* error marshaling y:
                //          second line of error message */null
-               return fmt.Sprintf(" /* %s */null ", strings.ReplaceAll(err.Error(), "*/", "* /"))
+               errStr := err.Error()
+               errStr = strings.ReplaceAll(errStr, "*/", "* /")
+               errStr = strings.ReplaceAll(errStr, "</script", `\x3C/script`)
+               errStr = strings.ReplaceAll(errStr, "<!--", `\x3C!--`)
+               return fmt.Sprintf(" /* %s */null ", errStr)
        }
 
        // TODO: maybe post-process output to prevent it from containing

diff --git a/src/html/template/js_test.go b/src/html/template/js_test.go
index 259dcfbdc5bdeda48b6092d5a360d30e86afc3a6..17cedcec05f7fdf238f2259f4d227058780694d2 100644 (file)
--- a/src/html/template/js_test.go
+++ b/src/html/template/js_test.go
@@ -5,6 +5,7 @@
 package template
 
 import (
+       "errors"
        "math"
        "strings"
        "testing"
@@ -103,61 +104,72 @@ func TestNextJsCtx(t *testing.T) {
        }
 }
 
+type jsonErrType struct{}
+
+func (e *jsonErrType) MarshalJSON() ([]byte, error) {
+       return nil, errors.New("beep */ boop </script blip <!--")
+}
+
 func TestJSValEscaper(t *testing.T) {
        tests := []struct {
-               x  any
-               js string
+               x        any
+               js       string
+               skipNest bool
        }{
-               {int(42), " 42 "},
-               {uint(42), " 42 "},
-               {int16(42), " 42 "},
-               {uint16(42), " 42 "},
-               {int32(-42), " -42 "},
-               {uint32(42), " 42 "},
-               {int16(-42), " -42 "},
-               {uint16(42), " 42 "},
-               {int64(-42), " -42 "},
-               {uint64(42), " 42 "},
-               {uint64(1) << 53, " 9007199254740992 "},
+               {int(42), " 42 ", false},
+               {uint(42), " 42 ", false},
+               {int16(42), " 42 ", false},
+               {uint16(42), " 42 ", false},
+               {int32(-42), " -42 ", false},
+               {uint32(42), " 42 ", false},
+               {int16(-42), " -42 ", false},
+               {uint16(42), " 42 ", false},
+               {int64(-42), " -42 ", false},
+               {uint64(42), " 42 ", false},
+               {uint64(1) << 53, " 9007199254740992 ", false},
                // ulp(1 << 53) > 1 so this loses precision in JS
                // but it is still a representable integer literal.
-               {uint64(1)<<53 + 1, " 9007199254740993 "},
-               {float32(1.0), " 1 "},
-               {float32(-1.0), " -1 "},
-               {float32(0.5), " 0.5 "},
-               {float32(-0.5), " -0.5 "},
-               {float32(1.0) / float32(256), " 0.00390625 "},
-               {float32(0), " 0 "},
-               {math.Copysign(0, -1), " -0 "},
-               {float64(1.0), " 1 "},
-               {float64(-1.0), " -1 "},
-               {float64(0.5), " 0.5 "},
-               {float64(-0.5), " -0.5 "},
-               {float64(0), " 0 "},
-               {math.Copysign(0, -1), " -0 "},
-               {"", `""`},
-               {"foo", `"foo"`},
+               {uint64(1)<<53 + 1, " 9007199254740993 ", false},
+               {float32(1.0), " 1 ", false},
+               {float32(-1.0), " -1 ", false},
+               {float32(0.5), " 0.5 ", false},
+               {float32(-0.5), " -0.5 ", false},
+               {float32(1.0) / float32(256), " 0.00390625 ", false},
+               {float32(0), " 0 ", false},
+               {math.Copysign(0, -1), " -0 ", false},
+               {float64(1.0), " 1 ", false},
+               {float64(-1.0), " -1 ", false},
+               {float64(0.5), " 0.5 ", false},
+               {float64(-0.5), " -0.5 ", false},
+               {float64(0), " 0 ", false},
+               {math.Copysign(0, -1), " -0 ", false},
+               {"", `""`, false},
+               {"foo", `"foo"`, false},
                // Newlines.
-               {"\r\n\u2028\u2029", `"\r\n\u2028\u2029"`},
+               {"\r\n\u2028\u2029", `"\r\n\u2028\u2029"`, false},
                // "\v" == "v" on IE 6 so use "\u000b" instead.
-               {"\t\x0b", `"\t\u000b"`},
-               {struct{ X, Y int }{1, 2}, `{"X":1,"Y":2}`},
-               {[]any{}, "[]"},
-               {[]any{42, "foo", nil}, `[42,"foo",null]`},
-               {[]string{"<!--", "</script>", "-->"}, `["\u003c!--","\u003c/script\u003e","--\u003e"]`},
-               {"<!--", `"\u003c!--"`},
-               {"-->", `"--\u003e"`},
-               {"<![CDATA[", `"\u003c![CDATA["`},
-               {"]]>", `"]]\u003e"`},
-               {"</script", `"\u003c/script"`},
-               {"\U0001D11E", "\"\U0001D11E\""}, // or "\uD834\uDD1E"
-               {nil, " null "},
+               {"\t\x0b", `"\t\u000b"`, false},
+               {struct{ X, Y int }{1, 2}, `{"X":1,"Y":2}`, false},
+               {[]any{}, "[]", false},
+               {[]any{42, "foo", nil}, `[42,"foo",null]`, false},
+               {[]string{"<!--", "</script>", "-->"}, `["\u003c!--","\u003c/script\u003e","--\u003e"]`, false},
+               {"<!--", `"\u003c!--"`, false},
+               {"-->", `"--\u003e"`, false},
+               {"<![CDATA[", `"\u003c![CDATA["`, false},
+               {"]]>", `"]]\u003e"`, false},
+               {"</script", `"\u003c/script"`, false},
+               {"\U0001D11E", "\"\U0001D11E\"", false}, // or "\uD834\uDD1E"
+               {nil, " null ", false},
+               {&jsonErrType{}, " /* json: error calling MarshalJSON for type *template.jsonErrType: beep * / boop \\x3C/script blip \\x3C!-- */null ", true},
        }
 
        for _, test := range tests {
                if js := jsValEscaper(test.x); js != test.js {
                        t.Errorf("%+v: want\n\t%q\ngot\n\t%q", test.x, test.js, js)
                }
+               if test.skipNest {
+                       continue
+               }
                // Make sure that escaping corner cases are not broken
                // by nesting.
                a := []any{test.x}