Don't apply HTML escape to .Attributes passed to render hooks

[bep]

See https://github.com/gohugoio/hugo/blob/master/markup/goldmark/render_hooks.go#L63

a.attributes[string(attr.Name)] = string(util.EscapeHTML(attr.Value.([]byte)))

These are the .Attributes passed to the render hooks. When doing this whole render hooks for code blocks and diagram thing, I stumble upon this "not so fantastic" decision of mine from the past: This assumes that 1) What gets rendered is always HTML (which is kind of true for the Markdown bit today) and 2) That all attributes are meant to be rendered. I can see many use cases for passing arbitrary data down to the templates, and having it escaped is unfortunate.

I think it escapes only <, >, &, ' and ", so the side effects of fixing this I would say should be very small, and it's better to do it know than later. If you need to escape you can do it in the template, e.g:

<h{{ .Level }}
  {{- range $k, $v := .Attributes -}}
    {{- printf " %s=%q" $k ($v | htmlEscape) | safeHTMLAttr -}}
  {{- end -}}
>{{ .Text | safeHTML }}</h{{ .Level }}>

/cc @jmooring @kaushalmodi @onedrawingperday @moorereason etc.

[kaushalmodi]

I am not against this proposal, but I am curious how the current auto-escaping in render_hooks.go causes a problem. Can you share an example?

[bep]

I am not against this proposal, but I am curious how the current auto-escaping in render_hooks.go causes a problem

I'm having hard times coming up with great current problems; this is more about the information loss and potential future issues ... when it may be too late to fix. In the current situation we have been used to think about these CommonMark attributes as HTML attributes, but what if they are used to store arbitrary information? E.g.:

 { companyID="Smith & Wesson" }

Not the best example, I'm sure -- but if I were to use companyID to look some information in, say, site.Data, I cannot do that today unless I consider the possible escaping (which would be a little guesswork).

I think this is analogous to the attributes passed in via shortcodes -- no escaping there.

[kaushalmodi]

Understood. So these "Attributes" may not necessarily be HTML attributes in future. Does it mean that the Attributes could contain HTML attributes for some render hooks and not for others?

Is this correct as of today?

render hook type	Attributes	Change after this proposal gets applied
heading	HTML Attributes only	User will need to manually escape the attribute values in render-heading.html using something like {{- printf " %s=%q" $k ($v | htmlEscape) | safeHTMLAttr -}}
image	no Attributes	not applicable as there are no Attributes here
link	no Attributes	not applicable as there are no Attributes here
(future xyz)	non-HTML Attributes	not applicable as the Attributes won't be HTML attributes

[bep]

Is this correct as of today?

Yes, this currently only applies to headings, so I suspect 99.99% of all current use of this is to pass CSS class-names which should be safe in this regard.

If we include the upcoming code blocks, then headings and code blocks can pass attributes. I think the main take here is that the template that renders this knows if it needs to be escaped or not. Hugo does knot know that, so it just escapes everything, just in case ... And you get potential loss of information.

[kaushalmodi]

Cool! I am OK with this proposal then.

Only the render-heading docs will need to be updated with your code snippet at the top then, highlighting that the Attributes need to be HTML escaped and passed through safeHTMLAttr.

[bep]

OK, testing this a little further, I would say that the existing behaviour is a bug that needs to be fixed.

• Go's HTML templates performs escaping (if not marked as safeHTML etc.)
• So passing Smith & Wesson escaped to the templates would (unless you mark it as safeHTML) double-escape it into Smith & Wesson.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.