Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix upstream Go templates bug with reversed key/value assignment #11114

Merged
merged 1 commit into from
Jun 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions config/allconfig/load.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
hglob "github.com/gohugoio/hugo/hugofs/glob"
"github.com/gohugoio/hugo/modules"
"github.com/gohugoio/hugo/parser/metadecoders"
"github.com/gohugoio/hugo/tpl"
"github.com/spf13/afero"
)

Expand Down Expand Up @@ -89,6 +90,9 @@ func LoadConfig(d ConfigSourceDescriptor) (*Configs, error) {
return nil, fmt.Errorf("failed to init config: %w", err)
}

// This is unfortunate, but this is a global setting.
tpl.SetSecurityAllowActionJSTmpl(configs.Base.Security.GoTemplates.AllowActionJSTmpl)

return configs, nil

}
Expand Down
12 changes: 12 additions & 0 deletions config/security/securityConfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ type Config struct {

// Allow inline shortcodes
EnableInlineShortcodes bool `json:"enableInlineShortcodes"`

// Go templates related security config.
GoTemplates GoTemplates `json:"goTemplates"`
}

// Exec holds os/exec policies.
Expand All @@ -93,6 +96,15 @@ type HTTP struct {
MediaTypes Whitelist `json:"mediaTypes"`
}

type GoTemplates struct {

// Enable to allow template actions inside bakcticks in ES6 template literals.
// This was blocked in Hugo 0.114.0 for security reasons and you now get errors on the form
// "... appears in a JS template literal" if you have this in your templates.
// See https://github.com/golang/go/issues/59234
AllowActionJSTmpl bool
}

// ToTOML converts c to TOML with [security] as the root.
func (c Config) ToTOML() string {
sec := c.ToSecurityMap()
Expand Down
2 changes: 1 addition & 1 deletion config/security/securityConfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ func TestToTOML(t *testing.T) {
got := DefaultConfig.ToTOML()

c.Assert(got, qt.Equals,
"[security]\n enableInlineShortcodes = false\n\n [security.exec]\n allow = ['^(dart-)?sass(-embedded)?$', '^go$', '^npx$', '^postcss$']\n osEnv = ['(?i)^((HTTPS?|NO)_PROXY|PATH(EXT)?|APPDATA|TE?MP|TERM|GO\\w+)$']\n\n [security.funcs]\n getenv = ['^HUGO_', '^CI$']\n\n [security.http]\n methods = ['(?i)GET|POST']\n urls = ['.*']",
"[security]\n enableInlineShortcodes = false\n\n [security.exec]\n allow = ['^(dart-)?sass(-embedded)?$', '^go$', '^npx$', '^postcss$']\n osEnv = ['(?i)^((HTTPS?|NO)_PROXY|PATH(EXT)?|APPDATA|TE?MP|TERM|GO\\w+)$']\n\n [security.funcs]\n getenv = ['^HUGO_', '^CI$']\n\n [security.goTemplates]\n AllowActionJSTmpl = false\n\n [security.http]\n methods = ['(?i)GET|POST']\n urls = ['.*']",
)
}

Expand Down
2 changes: 2 additions & 0 deletions tpl/internal/go_templates/htmltemplate/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ const (
stateJSDqStr
// stateJSSqStr occurs inside a JavaScript single quoted string.
stateJSSqStr
// stateJSBqStr occurs inside a JavaScript back quoted string.
stateJSBqStr
// stateJSRegexp occurs inside a JavaScript regexp literal.
stateJSRegexp
// stateJSBlockCmt occurs inside a JavaScript /* block comment */.
Expand Down
2 changes: 1 addition & 1 deletion tpl/internal/go_templates/htmltemplate/css.go
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,7 @@ func cssValueFilter(args ...any) string {
// inside a string that might embed JavaScript source.
for i, c := range b {
switch c {
case 0, '"', '\'', '(', ')', '/', ';', '@', '[', '\\', ']', '`', '{', '}':
case 0, '"', '\'', '(', ')', '/', ';', '@', '[', '\\', ']', '`', '{', '}', '<', '>':
return filterFailsafe
case '-':
// Disallow <!-- or -->.
Expand Down
2 changes: 2 additions & 0 deletions tpl/internal/go_templates/htmltemplate/css_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,8 @@ func TestCSSValueFilter(t *testing.T) {
{`-exp\000052 ession(alert(1337))`, "ZgotmplZ"},
{`-expre\0000073sion`, "-expre\x073sion"},
{`@import url evil.css`, "ZgotmplZ"},
{"<", "ZgotmplZ"},
{">", "ZgotmplZ"},
}
for _, test := range tests {
got := cssValueFilter(test.css)
Expand Down
7 changes: 7 additions & 0 deletions tpl/internal/go_templates/htmltemplate/doc.go
Original file line number Diff line number Diff line change
Expand Up @@ -231,5 +231,12 @@ Least Surprise Property:
"A developer (or code reviewer) familiar with HTML, CSS, and JavaScript, who
knows that contextual autoescaping happens should be able to look at a {{.}}
and correctly infer what sanitization happens."
As a consequence of the Least Surprise Property, template actions within an
ECMAScript 6 template literal are disabled by default.
Handling string interpolation within these literals is rather complex resulting
in no clear safe way to support it.
To re-enable template actions within ECMAScript 6 template literals, use the
GODEBUG=jstmpllitinterp=1 environment variable.
*/
package template
13 changes: 13 additions & 0 deletions tpl/internal/go_templates/htmltemplate/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,19 @@ const (
// pipeline occurs in an unquoted attribute value context, "html" is
// disallowed. Avoid using "html" and "urlquery" entirely in new templates.
ErrPredefinedEscaper

// errJSTmplLit: "... appears in a JS template literal"
// Example:
// <script>var tmpl = `{{.Interp}`</script>
// Discussion:
// Package html/template does not support actions inside of JS template
// literals.
//
// TODO(rolandshoemaker): we cannot add this as an exported error in a minor
// release, since it is backwards incompatible with the other minor
// releases. As such we need to leave it unexported, and then we'll add it
// in the next major release.
errJSTmplLit
)

func (e *Error) Error() string {
Expand Down
16 changes: 13 additions & 3 deletions tpl/internal/go_templates/htmltemplate/escape.go
Original file line number Diff line number Diff line change
Expand Up @@ -161,6 +161,8 @@ func (e *escaper) escape(c context, n parse.Node) context {
panic("escaping " + n.String() + " is unimplemented")
}

// var debugAllowActionJSTmpl = godebug.New("jstmpllitinterp")

// escapeAction escapes an action template node.
func (e *escaper) escapeAction(c context, n *parse.ActionNode) context {
if len(n.Pipe.Decl) != 0 {
Expand Down Expand Up @@ -224,6 +226,15 @@ func (e *escaper) escapeAction(c context, n *parse.ActionNode) context {
c.jsCtx = jsCtxDivOp
case stateJSDqStr, stateJSSqStr:
s = append(s, "_html_template_jsstrescaper")
case stateJSBqStr:
if SecurityAllowActionJSTmpl.Load() { // .Value() == "1" {
s = append(s, "_html_template_jsstrescaper")
} else {
return context{
state: stateError,
err: errorf(errJSTmplLit, n, n.Line, "%s appears in a JS template literal", n),
}
}
case stateJSRegexp:
s = append(s, "_html_template_jsregexpescaper")
case stateCSS:
Expand Down Expand Up @@ -370,9 +381,8 @@ func normalizeEscFn(e string) string {
// for all x.
var redundantFuncs = map[string]map[string]bool{
"_html_template_commentescaper": {
"_html_template_attrescaper": true,
"_html_template_nospaceescaper": true,
"_html_template_htmlescaper": true,
"_html_template_attrescaper": true,
"_html_template_htmlescaper": true,
},
"_html_template_cssescaper": {
"_html_template_attrescaper": true,
Expand Down
81 changes: 52 additions & 29 deletions tpl/internal/go_templates/htmltemplate/escape_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -683,38 +683,49 @@ func TestEscape(t *testing.T) {
`<img srcset={{",,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,"}}>`,
`<img srcset=,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,>`,
},
{
"unquoted empty attribute value (plaintext)",
"<p name={{.U}}>",
"<p name=ZgotmplZ>",
},
{
"unquoted empty attribute value (url)",
"<p href={{.U}}>",
"<p href=ZgotmplZ>",
},
{
"quoted empty attribute value",
"<p name=\"{{.U}}\">",
"<p name=\"\">",
},
}

for _, test := range tests {
tmpl := New(test.name)
tmpl = Must(tmpl.Parse(test.input))
// Check for bug 6459: Tree field was not set in Parse.
if tmpl.Tree != tmpl.text.Tree {
t.Errorf("%s: tree not set properly", test.name)
continue
}
b := new(strings.Builder)
if err := tmpl.Execute(b, data); err != nil {
t.Errorf("%s: template execution failed: %s", test.name, err)
continue
}
if w, g := test.output, b.String(); w != g {
t.Errorf("%s: escaped output: want\n\t%q\ngot\n\t%q", test.name, w, g)
continue
}
b.Reset()
if err := tmpl.Execute(b, pdata); err != nil {
t.Errorf("%s: template execution failed for pointer: %s", test.name, err)
continue
}
if w, g := test.output, b.String(); w != g {
t.Errorf("%s: escaped output for pointer: want\n\t%q\ngot\n\t%q", test.name, w, g)
continue
}
if tmpl.Tree != tmpl.text.Tree {
t.Errorf("%s: tree mismatch", test.name)
continue
}
t.Run(test.name, func(t *testing.T) {
tmpl := New(test.name)
tmpl = Must(tmpl.Parse(test.input))
// Check for bug 6459: Tree field was not set in Parse.
if tmpl.Tree != tmpl.text.Tree {
t.Fatalf("%s: tree not set properly", test.name)
}
b := new(strings.Builder)
if err := tmpl.Execute(b, data); err != nil {
t.Fatalf("%s: template execution failed: %s", test.name, err)
}
if w, g := test.output, b.String(); w != g {
t.Fatalf("%s: escaped output: want\n\t%q\ngot\n\t%q", test.name, w, g)
}
b.Reset()
if err := tmpl.Execute(b, pdata); err != nil {
t.Fatalf("%s: template execution failed for pointer: %s", test.name, err)
}
if w, g := test.output, b.String(); w != g {
t.Fatalf("%s: escaped output for pointer: want\n\t%q\ngot\n\t%q", test.name, w, g)
}
if tmpl.Tree != tmpl.text.Tree {
t.Fatalf("%s: tree mismatch", test.name)
}
})
}
}

Expand Down Expand Up @@ -941,6 +952,10 @@ func TestErrors(t *testing.T) {
"{{range .Items}}<a{{if .X}}{{end}}>{{if .X}}{{break}}{{end}}{{end}}",
"",
},
{
"<script>var a = `${a+b}`</script>`",
"",
},
// Error cases.
{
"{{if .Cond}}<a{{end}}",
Expand Down Expand Up @@ -1087,6 +1102,10 @@ func TestErrors(t *testing.T) {
// html is allowed since it is the last command in the pipeline, but urlquery is not.
`predefined escaper "urlquery" disallowed in template`,
},
{
"<script>var tmpl = `asd {{.}}`;</script>",
`{{.}} appears in a JS template literal`,
},
}
for _, test := range tests {
buf := new(bytes.Buffer)
Expand Down Expand Up @@ -1308,6 +1327,10 @@ func TestEscapeText(t *testing.T) {
`<a onclick="'foo&quot;`,
context{state: stateJSSqStr, delim: delimDoubleQuote, attr: attrScript},
},
{
"<a onclick=\"`foo",
context{state: stateJSBqStr, delim: delimDoubleQuote, attr: attrScript},
},
{
`<A ONCLICK="'`,
context{state: stateJSSqStr, delim: delimDoubleQuote, attr: attrScript},
Expand Down
3 changes: 3 additions & 0 deletions tpl/internal/go_templates/htmltemplate/html.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ import (
// htmlNospaceEscaper escapes for inclusion in unquoted attribute values.
func htmlNospaceEscaper(args ...any) string {
s, t := stringify(args...)
if s == "" {
return filterFailsafe
}
if t == contentTypeHTML {
return htmlReplacer(stripTags(s), htmlNospaceNormReplacementTable, false)
}
Expand Down
6 changes: 6 additions & 0 deletions tpl/internal/go_templates/htmltemplate/hugo_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,15 @@
package template

import (
"sync/atomic"

template "github.com/gohugoio/hugo/tpl/internal/go_templates/texttemplate"
)

// See https://github.com/golang/go/issues/59234
// Moved here to avoid dependency on Go's internal/debug package.
var SecurityAllowActionJSTmpl atomic.Bool

/*

This files contains the Hugo related addons. All the other files in this
Expand Down
10 changes: 9 additions & 1 deletion tpl/internal/go_templates/htmltemplate/js.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,11 @@ import (
"unicode/utf8"
)

// jsWhitespace contains all of the JS whitespace characters, as defined
// by the \s character class.
// See https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_expressions/Character_classes.
const jsWhitespace = "\f\n\r\t\v\u0020\u00a0\u1680\u2000\u2001\u2002\u2003\u2004\u2005\u2006\u2007\u2008\u2009\u200a\u2028\u2029\u202f\u205f\u3000\ufeff"

// nextJSCtx returns the context that determines whether a slash after the
// given run of tokens starts a regular expression instead of a division
// operator: / or /=.
Expand All @@ -27,7 +32,8 @@ import (
// JavaScript 2.0 lexical grammar and requires one token of lookbehind:
// https://www.mozilla.org/js/language/js20-2000-07/rationale/syntax.html
func nextJSCtx(s []byte, preceding jsCtx) jsCtx {
s = bytes.TrimRight(s, "\t\n\f\r \u2028\u2029")
// Trim all JS whitespace characters
s = bytes.TrimRight(s, jsWhitespace)
if len(s) == 0 {
return preceding
}
Expand Down Expand Up @@ -309,6 +315,7 @@ var jsStrReplacementTable = []string{
// Encode HTML specials as hex so the output can be embedded
// in HTML attributes without further encoding.
'"': `\u0022`,
'`': `\u0060`,
'&': `\u0026`,
'\'': `\u0027`,
'+': `\u002b`,
Expand All @@ -332,6 +339,7 @@ var jsStrNormReplacementTable = []string{
'"': `\u0022`,
'&': `\u0026`,
'\'': `\u0027`,
'`': `\u0060`,
'+': `\u002b`,
'/': `\/`,
'<': `\u003c`,
Expand Down
13 changes: 8 additions & 5 deletions tpl/internal/go_templates/htmltemplate/js_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,14 +83,17 @@ func TestNextJsCtx(t *testing.T) {
{jsCtxDivOp, "0"},
// Dots that are part of a number are div preceders.
{jsCtxDivOp, "0."},
// Some JS interpreters treat NBSP as a normal space, so
// we must too in order to properly escape things.
{jsCtxRegexp, "=\u00A0"},
}

for _, test := range tests {
if nextJSCtx([]byte(test.s), jsCtxRegexp) != test.jsCtx {
t.Errorf("want %s got %q", test.jsCtx, test.s)
if ctx := nextJSCtx([]byte(test.s), jsCtxRegexp); ctx != test.jsCtx {
t.Errorf("%q: want %s got %s", test.s, test.jsCtx, ctx)
}
if nextJSCtx([]byte(test.s), jsCtxDivOp) != test.jsCtx {
t.Errorf("want %s got %q", test.jsCtx, test.s)
if ctx := nextJSCtx([]byte(test.s), jsCtxDivOp); ctx != test.jsCtx {
t.Errorf("%q: want %s got %s", test.s, test.jsCtx, ctx)
}
}

Expand Down Expand Up @@ -294,7 +297,7 @@ func TestEscapersOnLower7AndSelectHighCodepoints(t *testing.T) {
`0123456789:;\u003c=\u003e?` +
`@ABCDEFGHIJKLMNO` +
`PQRSTUVWXYZ[\\]^_` +
"`abcdefghijklmno" +
"\\u0060abcdefghijklmno" +
"pqrstuvwxyz{|}~\u007f" +
"\u00A0\u0100\\u2028\\u2029\ufeff\U0001D11E",
},
Expand Down
9 changes: 9 additions & 0 deletions tpl/internal/go_templates/htmltemplate/jsctx_string.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading
Loading