I saw this post on my feed, and caught my eye because I self-host a Mattermost instance. The fact that it has possible critical security issues is a concern for me. So I checked the github issue with the details: https://github.com/ncvgl/slawk/issues/107
Of course, it's fully LLM generated, but that's to be expected. So I opened the first issue described there, a *CRITICAL VULNERABILITY*, a possibility for XSS in email templates. Ok, sure, let's dig deeper.
The problematic code is this:
```
func prepareTextForEmail(text, siteURL string) template.HTML {
escapedText := html.EscapeString(text)
markdownText, err := utils.MarkdownToHTML(escapedText, siteURL)
if err != nil {
mlog.Warn("Encountered error while converting markdown to HTML", mlog.Err(err))
return template.HTML(text)
}
return template.HTML(markdownText)
}
```
The return inside the error checking is for the unescaped text, and ideally should use escapedText. But is it possible to even reach this situation? For that, the function utils.MarkdownToHTML() would need to error out.
And can it? No. Turns out the error checking is actually *dead code*. If you check how the goldmark package handles input, you could see that that it's not possible for this path to ever be reachable.
Now, is this a _potential_ bug? Yes, if e.g. the goldmark package ever changes the way it processes its input and creates the possibility of the conversion erroring out, then it's possible for an attacker to craft a payload that could trigger this code path and exploit this XSS vulnerability.
And should the devs just have used `return template.HTML(escapedText)` in that code path? Also yes. Why they didn't, I'm not really sure. But it's besides the point.
Lourenço