-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
Conversation
@@ -17,6 +17,10 @@ import ( | |||
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 debugAllowActionJSTmpl = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jmooring this has been in Go since 1.20.3, but will be introduced in Hugo with this PR. I fear it will ... break stuff. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideal: If the choice is between backwards compatibility and security, then security wins.
Practical: It depends on how many sites will break, and to some extent, which sites.
I will need to work with this PR to understand the effects before I can provide meaningful feedback and examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth it to have this default secure, but add an option for it in Hugo's security config. I don't think either of us can know how much this will break until it ... breaks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with that approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, the above seem to be restricted to just script inside back ticks, which should be pretty rare:
var a = `{{.Title }}`;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only example usage of JS template literals that I can remember seeing is #10707.
0fb411f
to
eed4b41
Compare
The template packages are based on go1.20.5 with the patch in befec5ddbbfbd81ec84e74e15a38044d67f8785b added. This also includes a security fix that now disallows Go template actions in JS literals (inside backticks). This will throw an error saying "... appears in a JS template literal". If you're really sure this isn't a security risk in your case, you can revert to the old behaviour: ```toml [security] [security.gotemplates] allowActionJSTmpl = true ``` See golang/go#59234 Fixes gohugoio#11112
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Tested cases from #11112, and
<script>console.log(`{{ .Title }}`)</script>
with security.gotemplates.allowActionJSTmpl
undefined (default to false
), explicitly true
, and explicitly false
.
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Fix upstream Go templates bug with reversed key/value assignment
The template packages are based on go1.20.5 with the patch in befec5ddbbfbd81ec84e74e15a38044d67f8785b added.
This also includes a security fix that now disallows Go template actions in JS literals (inside backticks).
This will throw an error saying "... appears in a JS template literal".
If you're really sure this isn't a security risk in your case, you can revert to the old behaviour:
See golang/go#59234
Fixes #11112