html/template: predefined escaper "html" disallowed in template #19952
Comments
|
https://golang.org/cl/37880, I presume. |
|
@bep seems like this was intentional; see https://tip.golang.org/pkg/html/template/ and search for "ErrPredefinedEscaper". |
|
Yes, well, a part of me may understand and even appreciate this change, the maintainer in me would have appreciated a WARNING of sorts instead of a hard fail or a way to handle that error in a less breaking way. This is a breaking change, and with thousands of user defined templates out there, I don't see how I can fix/deprecate this behaviour without chaos and way too much support work for me and the other maintainers. |
|
CC @stjj89 |
|
I discussed this change with @rsc on https://golang.org/cl/37880, and we decided that a explicit breaking change was the best way to resolve this security issue. A warning was not a viable option, since the Go compiler only reports errors (https://golang.org/doc/faq#unused_variables_and_imports), and a non-breaking change would silently modify the output of some templates, which might be harder to track down. I understand the difficulty of fixing a large number of risky templates in a short period of time. I'm not familiar with your development process or continuous test infrastructure, though. Could you perhaps guarantee compatibility only for Go <1.8.1 until you have fixed these issues? |
|
If this is a security issue, I would expect it to be part of a security patch release, not waiting n months for a Go 1.9 release.
I'm not sure you understand the use case. I can probably fix the internal failing templates fairly fast, but there are 160 end user provided themes and many thousands of Hugo sites with their own set of templates that I don't control. But I will sure be on the receiving end of the support ticket asking what to do about:
Which, to be honest, I'm not even sure how to handle myself (but that is beside the point). I will look more closely into this issue when I get some spare time but think it is fair to say that you would not have deliberately broken thousands of "real" Go programs with a muddy upgrade path like this one. |
You're right that this is a security issue, and should probably be resolved as per the Go Security Policy. @rsc @ianlancetaylor do either of you think it makes sense to roll this change back, and release it as according to the stated procedure? This process gives you only 3 days advance notice before the fix lands in the Go repo, so I'm not sure if this would solve your problem with end-user-provided themes.
I would point your users to the ErrPredefinedEscaper documentation at https://tip.golang.org/pkg/html/template/#ErrorCode. It explains the error in greater detail than the error message, which is meant to be brief.
Sorry for the lack of clarity with this change. We can probably make this right by sending out an announcement explaining the issue and the upgrade path on the relevant mailing lists. |
|
This is a change to tip, not to a release. The security policy only applies to security issues in existing releases. As far as I am aware, nobody is arguing that we should apply this change to an existing release. So the security policy does not apply. People using tip should expect it to break at any time. That's why we make releases. The comments about a lack of warning make clear that we need to highlight this issue in the Go 1.9 release notes, so that people can be aware of it when deciding when and whether to upgrade from 1.8 to 1.9. No warning is required to break people using tip. Assuming @bep is correct that this will break thousands of real Go programs, the real issues I see here are 1) should we make this change at all? 2) if we do decide to make this change, what can we do to make it easier for users to move forward? There is clearly something missing from CL 37880: it does not change the documentation. @stjj89 Can you update the docs for both text/template and html/template to make clear that these global template functions, while still present in text/template, are no longer available in html/template? The html/template docs have explicit examples of how Would it be possible and reasonable for us to simply ignore a predefined escaper function that appears at the end of a pipeline in the expected contex?. That is, if we are going to implicitly add @bep Can you give us some examples of real user templates that are now broken? Thanks. |
A count from the templates in the Hugo themes repo (https://github.com/spf13/hugoThemes): An extract: Note that the above is the part I can control. I would have to create a bunch of Pull Requests, but that I can probably script. But then there are plenty of sites with their own set of templates, and believe me, they will cry for help. I tried to add my own no-op Looking at it now, the change here is sligthly less than I first imagined, but I still believe this will create a lot of noise that could be avoided. |
Thanks for pointing this out. I will get to this after we decide on an agreeable solution.
I like this idea. My only concern is that this "double standard" for using predefined escapers (i.e. not in the middle, but only at the end). Perhaps an announcement e-mail and updated documentation will be enough to clear this up? |
|
To be clear, part of the plan here is to see what happens during release testing and maybe adjust the restrictions or even roll them back and try again. @stjj89, allowing and ignoring |html where it has never had any effect at all sounds OK. |
|
CL https://golang.org/cl/40936 mentions this issue. |
Go 1.9 is slated to have some backwards-incompatible changes to html/template. See golang/go#19952. If I'm reading this correctly, the issue is that the context-aware auto escaper had some magic around the 'html' filter, but it would get confused if this was used in the wrong context. This does not apply to us because we never used it in an attribute, etc. Nonetheless, we can be compatible with it and tidy up markupPipeWords' type signature. It should have had type template.HTML -> template.HTML, not string -> template.HTML, because it expects the input to be pre-escaped. (The old 'html' escaper, in turn, probably should have had type string -> template.HTML, but I guess it didn't because all this existed for a text/template migration convenience of some sort?) I considered adding our own escapeHTML with type string -> template.HTML and fixing markupPipeWords to be template.HTML -> template.HTML, but markupPipeWords does not correctly handle all possible template.HTML input. If a | were in an attribute somewhere, it would mangle the text. Instead, I kept it of type string -> template.HTML and defined it to perform the HTML escaping itself. This seems to produce the same output as before in Go 1.8 and tip. Change-Id: I90618a3c5525ae54f9fe731352fcff5856b9ba60 Reviewed-on: https://boringssl-review.googlesource.com/18944 Commit-Queue: Adam Langley <agl@google.com> Reviewed-by: Adam Langley <agl@google.com> CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
html was a built-in html function for templates and this was overridden by a custom html function that is currently deprecated. See golang/go#19952 Renaming the function solves the issue.
and others
The Hugo build has started to fail for Go tip recently. Not sure exactly when, but it is green for Go 1.8.1.
https://travis-ci.org/spf13/hugo/builds/221272152
The text was updated successfully, but these errors were encountered: