html/template: panic if predefined escapers are found in pipelines during rewriting

Report an error if a predefined escaper (i.e. "html", "urlquery", or "js")
is found in a pipeline that will be rewritten by the contextual auto-escaper,
instead of trying to merge the escaper-inserted escaping directives
with these predefined escapers. This merging behavior is a source
of several security and correctness bugs (eee #19336, #19345, #19352,
and #19353.)

This merging logic was originally intended to ease migration of text/template
templates with user-defined escapers to html/template. Now that
migration is no longer an issue, this logic can be safely removed.

NOTE: this is a backward-incompatible change that fixes known security
bugs (see linked issues for more details). It will explicitly break users
that attempt to execute templates with pipelines containing predefined
escapers.

Fixes #19336, #19345, #19352, #19353

Change-Id: I46b0ca8a2809d179c13c0d4f42b63126ed1c3b49
Reviewed-on: https://go-review.googlesource.com/37880
Run-TryBot: Russ Cox <rsc@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Russ Cox <rsc@golang.org>

1 files changed, 21 insertions, 0 deletions

diff --git a/src/html/template/error.go b/src/html/template/error.go
index cbcaf92e4a..3b70ba1ec8 100644
--- a/src/html/template/error.go
+++ b/src/html/template/error.go
@@ -183,6 +183,27 @@ const (
 	//   Look for missing semicolons inside branches, and maybe add
 	//   parentheses to make it clear which interpretation you intend.
 	ErrSlashAmbig
+
+	// ErrPredefinedEscaper: "predefined escaper ... disallowed in template"
+	// Example:
+	//   <a href="{{.X | urlquery}}">
+	// Discussion:
+	//   Package html/template already contextually escapes all pipelines to
+	//   produce HTML output safe against code injection. Manually escaping
+	//   pipeline output using the predefined escapers "html", "urlquery", or "js"
+	//   is unnecessary, and might affect the correctness or safety of the escaped
+	//   pipeline output. In the above example, "urlquery" should simply be
+	//   removed from the pipeline so that escaping is performed solely by the
+	//   contextual autoescaper.
+	//   If the predefined escaper occurs in the middle of a pipeline where
+	//   subsequent commands expect escaped input, e.g.
+	//     {{.X | html | makeALink}}
+	//   where makeALink does
+	//     return "<a href='+input+'>link</a>"
+	//   consider refactoring the surrounding template to make use of the
+	//   contextual autoescaper, i.e.
+	//     <a href='{{.X}}'>link</a>
+	ErrPredefinedEscaper
 )
 
 func (e *Error) Error() string {