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

proposal: html/template: Better handling of HTML comments #54380

Open
kbolino opened this issue Aug 10, 2022 · 4 comments
Open

proposal: html/template: Better handling of HTML comments #54380

kbolino opened this issue Aug 10, 2022 · 4 comments
Labels
Milestone

Comments

@kbolino
Copy link

kbolino commented Aug 10, 2022

Background

The html/template package completely strips HTML comments from the rendered output. This is apparently intentional (#14256) but is not documented (#28628). The best summation of the rationale I could find comes from the mailing list:

That would not be safe. html/template needs to know the proper context at each point of the template evaluation, but these types of comments cause their contents to be in a browser-dependent context, so the autoescaping can not be done reliably.

To wit, "these types of comments" seems to refer to old-school conditional comments like <!--[if lt IE 9]>...<![endif]-->. I don't know if these comments even do anything anymore on modern browsers, and a quick Google search wasn't very revealing.

The canonical solution to this problem seems to be to use the template.HTML type to wrap "trusted" strings. For example, there's this StackOverflow answer recommending this approach. However, this means that only way to use comments literally in the template is via function-call indirection, e.g.:

{{ safe "<!-- I just wanted to put a comment in my template -->" }}

This also imposes the limitation that comments cannot have injected values within them.

Rationale for Change

I contend that based upon my own and a number of others' experiences, this behavior is unexpected. First of all, there are uses for comments besides affecting the browser's behavior (e.g. including debugging info useful to the developer). Second, many will have never seen or used IE "conditional comments" in no small part because IE is (almost) dead. Third, if the rationale for this behavior is to address the specific concern of conditional context confounding the auto-escaping mechanism, then I would think that only those comments which raise that issue would be stripped, not all comments. Fourth, of course, is the total lack of documentation of this behavior in the package itself.

There is another proposed "solution" already which is to fall back to using text/template but that is obviously a dangerous regression as then one gives up on all the benefits of html/template.

Though server-side manipulation of HTML is falling by the wayside in favor of script-heavy web apps doing extensive DOM manipulation client-side, the html/template package is not deprecated, and all browsers still (mostly) support "Web 1.0" sites that have minimal or no scripts.

Finally, the package documentation does say the following:

The security model used by this package assumes that template authors are trusted, while Execute's data parameter is not.

Whereas the policy of stripping comments seems to contradict this maxim.

Thus I believe this unintuitive behavior is ripe for change, albeit ideally in a backwards-compatible way.

Possible Paths

  1. Add a new built-in function to the template language: Basically take the SO answer and bless it as canonical, eliminating the need for the user to define a function on their own. This is probably the most conservative approach, especially if the user is allowed to override the built-in function with one of their own (since otherwise there could be a naming conflict from old code), however I also consider this the least beneficial solution, since you still can't safely inject values into comments.
    • An improved approach would be to add a comment function that takes a string argument, escapes it properly for the body of a comment, surrounds it with comment markup, and finally wraps the result in template.HTML, this way you could at least inject values safely but indirectly with printf
  2. Just stop stripping comments altogether. If there's a literal comment in the source, pass it through, injecting properly escaped values if there are any. Put a tombstone on IE while you're at it. This is probably the most aggressive approach and could have thorny repercussions if turned on by default.
  3. Fail to compile any template containing HTML comments. The author probably put the comments there on purpose, and may not even be aware that they're being stripped. The template language has its own comment syntax which can be used for local-only comments. This replaces one unexpected outcome (comments just disappear) with another (some templates that used to compile now don't) but IMO reduces the amount of surprise involved.
  4. Narrow the scope of the comment stripping to only those comments known to be problematic. I don't know if there are other kinds of problematic comments besides the aforementioned conditional comments from IE.
  5. Introduce a new block to the template language inside of which comments are allowed. This improves on idea 1 by allowing safely escaped value injection.

Any of the above suggestions could also be gated, e.g. with template.Option, to improve backwards compatibility.

@gopherbot gopherbot added this to the Proposal milestone Aug 10, 2022
@kbolino
Copy link
Author

kbolino commented Aug 10, 2022

Well, I just discovered an interesting wrinkle that could undermine some of the suggestions: there doesn't seem to be a good rule to escape the things you put inside of comments

@ianlancetaylor
Copy link
Contributor

@kbolino I'm not clear: are you retracting the proposal?

@kbolino
Copy link
Author

kbolino commented Aug 11, 2022

No, at least not without some kind of resolution (even if it is merely to finally document this behavior). I thought a bit on this last night, and putting some of the other ideas along with that unfortunate discovery together, I'd propose a middle-ground solution:

Define a template option htmlcomments which takes one of the following values:

  • remove (= default) which corresponds to the current behavior
  • error which causes an error to be returned (ideally by the Parse* functions, i.e. as early as possible) if an HTML comment is present in the template
  • allow which enables HTML comments to pass through to rendered output, but Execute will still fail with an error if a value substitution inserts -- or > into a comment block

I have no idea how feasible this solution is, but could do some code research into whether it would be possible.

@luflow
Copy link

luflow commented Mar 15, 2024

We have interest in resolution of this issue - can we help with resolving this? Currently we stumbled upon this issue because we are using html/template for mail templates where <!--[if IE 8]> html comments are used - unfortunately they are breaking without that fix currently.

So we are interested in the implementation of the proposal and could even help with it if necessary :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

4 participants