Sanitize comment text in email notifications (GHSA-74pc-3r2m-ppx3)
Email notification templates rendered the comment HTML via text/template, so the store-level UGC sanitizer's permitted <a> and <img> tags reached the email body verbatim. An authenticated user could plant phishing links and remote tracking pixels in notification emails sent from the legitimate remark42 address. Switch notify to html/template (auto-escaping every non-HTML field) and add a stricter email-only bluemonday policy that drops <a> and <img> while keeping basic text formatting; the sanitized comment HTML is passed as template.HTML. Add regression tests asserting links and images are stripped while anchor text and formatting survive.
This commit is contained in:
Dmitry Verkhoturov
committed by
Umputun
Umputun
parent
11d8a978a2
commit
3e18681ca7
@@ -4,14 +4,15 @@ import (
|
||||
"bytes"
|
||||
"context"
|
||||
"fmt"
|
||||
"html/template"
|
||||
"net/url"
|
||||
"text/template"
|
||||
"time"
|
||||
|
||||
log "github.com/go-pkgz/lgr"
|
||||
ntf "github.com/go-pkgz/notify"
|
||||
"github.com/go-pkgz/repeater/v2"
|
||||
"github.com/hashicorp/go-multierror"
|
||||
"github.com/microcosm-cc/bluemonday"
|
||||
|
||||
"github.com/umputun/remark42/backend/app/templates"
|
||||
)
|
||||
@@ -42,12 +43,12 @@ type Email struct {
|
||||
type msgTmplData struct {
|
||||
UserName string
|
||||
UserPicture string
|
||||
CommentText string
|
||||
CommentText template.HTML
|
||||
CommentLink string
|
||||
CommentDate time.Time
|
||||
ParentUserName string
|
||||
ParentUserPicture string
|
||||
ParentCommentText string
|
||||
ParentCommentText template.HTML
|
||||
ParentCommentLink string
|
||||
ParentCommentDate time.Time
|
||||
PostTitle string
|
||||
@@ -56,6 +57,30 @@ type msgTmplData struct {
|
||||
ForAdmin bool
|
||||
}
|
||||
|
||||
// emailCommentPolicy sanitizes comment HTML for inclusion in notification emails.
|
||||
// It is intentionally stricter than the store-level UGC policy used for web rendering:
|
||||
// links (<a>) and images (<img>) are dropped so a comment can't smuggle phishing links
|
||||
// or remote tracking pixels into an email sent from the legitimate remark42 address,
|
||||
// while basic inline and block text formatting is preserved.
|
||||
var emailCommentPolicy = func() *bluemonday.Policy {
|
||||
p := bluemonday.NewPolicy()
|
||||
p.AllowElements(
|
||||
"p", "br", "hr", "div", "span",
|
||||
"b", "strong", "i", "em", "u", "s", "strike", "del", "ins", "sub", "sup", "mark", "small",
|
||||
"blockquote", "q", "cite",
|
||||
"code", "pre", "kbd", "samp", "var",
|
||||
"ul", "ol", "li", "dl", "dt", "dd",
|
||||
"h1", "h2", "h3", "h4", "h5", "h6",
|
||||
)
|
||||
return p
|
||||
}()
|
||||
|
||||
// emailSafeHTML strips links and images from pre-rendered comment HTML and returns
|
||||
// it as template.HTML so html/template renders the remaining safe formatting as-is.
|
||||
func emailSafeHTML(commentHTML string) template.HTML {
|
||||
return template.HTML(emailCommentPolicy.Sanitize(commentHTML)) //nolint:gosec // sanitized above: <a>/<img> dropped, only formatting tags survive
|
||||
}
|
||||
|
||||
// verifyTmplData store data for verification message template execution
|
||||
type verifyTmplData struct {
|
||||
User string
|
||||
@@ -257,7 +282,7 @@ func (e *Email) buildMessageFromRequest(req Request, email string, forAdmin bool
|
||||
tmplData := msgTmplData{
|
||||
UserName: req.Comment.User.Name,
|
||||
UserPicture: req.Comment.User.Picture,
|
||||
CommentText: req.Comment.Text,
|
||||
CommentText: emailSafeHTML(req.Comment.Text),
|
||||
CommentLink: commentURLPrefix + req.Comment.ID,
|
||||
CommentDate: req.Comment.Timestamp,
|
||||
PostTitle: req.Comment.PostTitle,
|
||||
@@ -269,7 +294,7 @@ func (e *Email) buildMessageFromRequest(req Request, email string, forAdmin bool
|
||||
if req.Comment.ParentID != "" {
|
||||
tmplData.ParentUserName = req.parent.User.Name
|
||||
tmplData.ParentUserPicture = req.parent.User.Picture
|
||||
tmplData.ParentCommentText = req.parent.Text
|
||||
tmplData.ParentCommentText = emailSafeHTML(req.parent.Text)
|
||||
tmplData.ParentCommentLink = commentURLPrefix + req.parent.ID
|
||||
tmplData.ParentCommentDate = req.parent.Timestamp
|
||||
}
|
||||
|
||||
@@ -3,8 +3,8 @@ package notify
|
||||
import (
|
||||
"context"
|
||||
"fmt"
|
||||
"html/template"
|
||||
"testing"
|
||||
"text/template"
|
||||
|
||||
ntf "github.com/go-pkgz/notify"
|
||||
"github.com/stretchr/testify/assert"
|
||||
@@ -164,7 +164,7 @@ User: test_user
|
||||
01.01.0001 at 00:00
|
||||
Comment:
|
||||
test@example.org for parent_user
|
||||
Unsubscribe link: https://remark42.com/api/v1/email/unsubscribe?site=&tkn=token
|
||||
Unsubscribe link: https://remark42.com/api/v1/email/unsubscribe?site=&tkn=token
|
||||
`, msg.body)
|
||||
assert.Equal(t, "https://remark42.com/api/v1/email/unsubscribe?site=&tkn=token", msg.unsubscribeLink)
|
||||
assert.Equal(t, `New reply to your comment for "test_title"`, msg.subject)
|
||||
@@ -190,6 +190,50 @@ admin@example.org
|
||||
assert.Empty(t, msg.unsubscribeLink)
|
||||
}
|
||||
|
||||
func TestEmail_CommentTextSanitizedForEmail(t *testing.T) {
|
||||
// comment HTML reaching the email path is sanitized by the store-level UGC policy,
|
||||
// which permits <a> and <img>. The email must drop both so a comment can't inject
|
||||
// phishing links or remote tracking pixels into a notification (GHSA-74pc-3r2m-ppx3).
|
||||
email, err := NewEmail(EmailParams{
|
||||
From: "from@example.org",
|
||||
MsgTemplatePath: "testdata/msg.html.tmpl",
|
||||
}, ntf.SMTPParams{})
|
||||
require.NoError(t, err)
|
||||
email.TokenGenFn = TokenGenFn
|
||||
|
||||
malicious := `hello <a href="https://phishing.example/verify">click to verify</a>` +
|
||||
` <img src="https://attacker.example/track.png" width="1" height="1"> <b>kept</b>`
|
||||
req := Request{
|
||||
Comment: store.Comment{ID: "999", User: store.User{ID: "1", Name: "test_user"}, PostTitle: "test_title", Text: malicious},
|
||||
Emails: []string{"test@example.org"},
|
||||
}
|
||||
msg, err := email.buildMessageFromRequest(req, req.Emails[0], false)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.NotContains(t, msg.body, "phishing.example", "phishing link must be stripped")
|
||||
assert.NotContains(t, msg.body, "attacker.example", "tracking pixel must be stripped")
|
||||
assert.NotContains(t, msg.body, "<img", "no image tags in email body")
|
||||
assert.NotContains(t, msg.body, "<a ", "no anchor tags in email body")
|
||||
assert.Contains(t, msg.body, "click to verify", "anchor text is preserved, only the link is dropped")
|
||||
assert.Contains(t, msg.body, "<b>kept</b>", "basic formatting is preserved")
|
||||
}
|
||||
|
||||
// emailSafeHTML drops links/images while keeping inline/block formatting and escaping nothing extra.
|
||||
func TestEmailSafeHTML(t *testing.T) {
|
||||
tbl := []struct{ name, in, want string }{
|
||||
{"strips anchor keeps text", `<a href="http://evil">x</a>`, "x"},
|
||||
{"strips image entirely", `a<img src="http://evil/t.png">b`, "ab"},
|
||||
{"keeps bold/italic/code", `<b>b</b><i>i</i><code>c</code>`, `<b>b</b><i>i</i><code>c</code>`},
|
||||
{"keeps blockquote and lists", `<blockquote>q</blockquote><ul><li>x</li></ul>`, `<blockquote>q</blockquote><ul><li>x</li></ul>`},
|
||||
{"drops onclick handlers", `<span onclick="alert(1)">s</span>`, `<span>s</span>`},
|
||||
}
|
||||
for _, tt := range tbl {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
assert.Equal(t, tt.want, string(emailSafeHTML(tt.in)))
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestEmail_SendVerification(t *testing.T) {
|
||||
email, err := NewEmail(EmailParams{
|
||||
From: "from@example.org",
|
||||
|
||||
Reference in New Issue
Block a user