diff options
author | Roland Shoemaker <roland@golang.org> | 2024-02-14 17:18:36 -0800 |
---|---|---|
committer | Carlos Amedee <carlos@golang.org> | 2024-02-28 19:53:38 +0000 |
commit | 056b0edcb8c152152021eebf4cf42adbfbe77992 (patch) | |
tree | b1262b5f4d34f0051327da4cd8fdfad8d463b5d3 | |
parent | f73eba76a04f5a4c8a1ba1382c6c4ade7b21e720 (diff) | |
download | go-056b0edcb8c152152021eebf4cf42adbfbe77992.tar.gz |
[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>
-rw-r--r-- | src/html/template/js.go | 22 | ||||
-rw-r--r-- | src/html/template/js_test.go | 96 |
2 files changed, 74 insertions, 44 deletions
diff --git a/src/html/template/js.go b/src/html/template/js.go index b159af8e4b..d911ada26d 100644 --- 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 259dcfbdc5..17cedcec05 100644 --- 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} |