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

Delimit function returns template.HTML, not string #10876

Closed
willfaught opened this issue Apr 5, 2023 · 8 comments · Fixed by #11618
Closed

Delimit function returns template.HTML, not string #10876

willfaught opened this issue Apr 5, 2023 · 8 comments · Fixed by #11618

Comments

@willfaught
Copy link

https://gohugo.io/functions/delimit/:

Loops through any array, slice, or map and returns a string of all the values separated by a delimiter.

This:

{{ warnf "@@@@@@@@@@@@@@ %v %T" now.UnixNano $srcset }}
{{ warnf "@@@@@@@@@@@@@@ %v %T" now.UnixNano (printf "%s %fx" $resized.RelPermalink .) }}
{{ $srcset = $srcset | append (printf "%s %fx" $resized.RelPermalink .) }}
{{ warnf "@@@@@@@@@@@@@@ %v %T" now.UnixNano $srcset }}
{{ $srcset = delimit $srcset ", " }}
{{ warnf "@@@@@@@@@@@@@@ %v %T" now.UnixNano $srcset }}

prints this:

WARN 2023/04/04 23:00:21 @@@@@@@@@@@@@@ 1680674421963491000 []interface {}
WARN 2023/04/04 23:00:21 @@@@@@@@@@@@@@ 1680674421967981000 string
WARN 2023/04/04 23:00:21 @@@@@@@@@@@@@@ 1680674421968041000 []string
WARN 2023/04/04 23:00:21 @@@@@@@@@@@@@@ 1680674421968183000 template.HTML

What version of Hugo are you using (hugo version)?

$ hugo version
hugo v0.111.3+extended darwin/amd64 BuildDate=unknown

Does this issue reproduce with the latest release?

Yes

@bep bep added this to the v0.113.0 milestone Apr 15, 2023
@bep bep modified the milestones: v0.113.0, v0.114.0, v0.115.0 Jun 8, 2023
@bep bep modified the milestones: v0.115.0, v0.116.0 Jun 30, 2023
@elkcityhazard
Copy link

Correct me if I'm wrong, but from the standard library:

type HTML string

HTML encapsulates a known safe HTML document fragment. It should not be used for HTML from a third-party, or HTML with unclosed tags or comments. The outputs of a sound HTML sanitizer and a template escaped by this package are fine for use with HTML.

Use of this type presents a security risk: the encapsulated content should come from a trusted source, as it will be included verbatim in the template output.

To summarize, template.HTML returns a santiized html string.

@willfaught
Copy link
Author

Right. This could easily be resolved by correcting the documentation, instead of changing the implementation. Although I seem to recall template.HTML causing a problem when passing it to a func that expected a string. I forget the details.

@jmooring
Copy link
Member

jmooring commented Jul 1, 2023

I seem to recall template.HTML causing a problem when passing it to a func that expected a string

The truncate function is the only string function to return template.HTML instead of a string. And it kind of makes sense given how it is typically used. I've run across one case where this was a surprise: https://discourse.gohugo.io/t/image-text-parameters/43118

If we changed delimit to return a string it could break sites:

{{ $s := slice "<i>foo</i>" }}
{{ delimit $s "," }}

I suspect that is an uncommon example, but still...

There's documentation in the works, based on source, that specifies type for both args and result.

@bep bep removed the NeedsTriage label Jul 1, 2023
@bep
Copy link
Member

bep commented Jul 1, 2023

So, template.HTML is a string with some extra features. I do think that it should be mostly compatible with a string in Hugo (unfortunately even with the Go 1.18 generics, its's not possible to take an argument that's of type stringer).

That said, I think we should fix this. Theres obvius cases where we would want to keep the template type, but this is not it.

@jmooring
Copy link
Member

jmooring commented Jul 6, 2023

I have seen this in the wild, but I can't remember where:

{{ $tags := slice }}
{{ range .GetTerms "tags" }}
  {{ $tags = $tags | append (printf "<a href=%q>%s</a>" .RelPermalink .LinkTitle) }}
{{ end }}
{{ delimit $tags ", " }}

If we return a string instead of template.HTML, we will break the site.

The construct above may have been used because this wasn't as obvious:

{{ range $k, $_ := .GetTerms "tags" -}}
  {{ if $k }}, {{ end }}
  <a href="{{ .RelPermalink }}">{{ .LinkTitle }}</a>
{{- end }}

@bep bep modified the milestones: v0.116.0, v0.117.0 Aug 1, 2023
@bep bep modified the milestones: v0.117.0, v0.118.0 Aug 30, 2023
@bep bep modified the milestones: v0.118.0, v0.119.0 Sep 15, 2023
@bep bep modified the milestones: v0.119.0, v0.120.0 Oct 5, 2023
@jmooring
Copy link
Member

I have seen this in the wild, but I can't remember where:

I just found an example of this usage in the documentation for the apply function. I will remove the example; it's of little value and quite convoluted.

@bep
Copy link
Member

bep commented Oct 27, 2023

OK, I have thought about this, and we should fix this, and I don't know how to fix it without breaking something, but there's an argument that it will most likely fix more thing than it breaks. I think we could also argue that this is a also a security concern.

Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants