Skip to content

Commit df260ed

Browse files
AchoArnoldCopilot
andcommitted
fix(api): harden webhook payload formatting
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 19b221a commit df260ed

4 files changed

Lines changed: 96 additions & 1 deletion

File tree

api/pkg/emails/event_payload_formatter.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ func formatEventPayload(payload string) (string, template.HTML) {
2323
content = highlightEventPayloadJSON(formattedPayload)
2424
}
2525

26+
// Every payload token is escaped before this trusted wrapper is constructed.
2627
richPayload := template.HTML(`<pre style="` + eventPayloadCodeBlockStyle + `">` + content + `</pre>`)
2728
return formattedPayload, richPayload
2829
}
@@ -52,6 +53,7 @@ func highlightEventPayloadJSON(payload string) string {
5253
index = end
5354
case payload[index] == '-' || isEventPayloadDigit(payload[index]):
5455
end := index + 1
56+
// json.Indent already validated this as JSON, so this continuation set only sees JSON number bytes.
5557
for end < len(payload) && isEventPayloadNumberCharacter(payload[end]) {
5658
end++
5759
}

api/pkg/emails/event_payload_formatter_test.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,3 +56,58 @@ func TestFormatEventPayloadPreservesInvalidJSONWithoutHighlighting(t *testing.T)
5656
assert.NotContains(t, html, `<span style="color:`)
5757
assert.Equal(t, 1, strings.Count(html, `<pre style=`))
5858
}
59+
60+
func TestFormatEventPayloadHandlesTopLevelPayloadShapes(t *testing.T) {
61+
tests := []struct {
62+
name string
63+
payload string
64+
wantPlain string
65+
wantHTMLContains []string
66+
wantHTMLNotContains []string
67+
}{
68+
{
69+
name: "empty payload falls back to unhighlighted block",
70+
payload: "",
71+
wantPlain: "",
72+
wantHTMLContains: []string{`<pre style="`, `</pre>`},
73+
wantHTMLNotContains: []string{`<span style="color:`},
74+
},
75+
{
76+
name: "top-level number stays valid JSON",
77+
payload: "42",
78+
wantPlain: "42",
79+
wantHTMLContains: []string{`<span style="color:#953800;">42</span>`},
80+
wantHTMLNotContains: []string{`color:#0550AE`},
81+
},
82+
{
83+
name: "json array stays readable and escaped",
84+
payload: `[{"message":"<b>safe</b>"},true,null,3]`,
85+
wantPlain: "[\n {\n \"message\": \"<b>safe</b>\"\n },\n true,\n null,\n 3\n]",
86+
wantHTMLContains: []string{
87+
"[\n {",
88+
`&#34;message&#34;`,
89+
`&lt;b&gt;safe&lt;/b&gt;`,
90+
`<span style="color:#953800;">3</span>`,
91+
},
92+
wantHTMLNotContains: []string{`<b>safe</b>`},
93+
},
94+
}
95+
96+
for _, tt := range tests {
97+
t.Run(tt.name, func(t *testing.T) {
98+
plain, rich := formatEventPayload(tt.payload)
99+
html := string(rich)
100+
101+
assert.Equal(t, tt.wantPlain, plain)
102+
assert.Equal(t, 1, strings.Count(html, `<pre style=`))
103+
104+
for _, want := range tt.wantHTMLContains {
105+
assert.Contains(t, html, want)
106+
}
107+
108+
for _, unwanted := range tt.wantHTMLNotContains {
109+
assert.NotContains(t, html, unwanted)
110+
}
111+
})
112+
}
113+
}

api/pkg/emails/hermes_notification_email_factory.go

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ func (factory *hermesNotificationEmailFactory) WebhookSendFailed(user *entities.
128128
return nil, stacktrace.Propagate(err, "cannot generate text email")
129129
}
130130
// Hermes/html2text collapses dictionary whitespace, so restore the payload after plain-text generation.
131-
text = strings.Replace(text, webhookSendFailedEventPayloadPlaceholder, formattedPayload, 1)
131+
text = replaceWebhookSendFailedEventPayloadPlaceholder(text, formattedPayload)
132132

133133
return &Email{
134134
ToEmail: user.Email,
@@ -138,6 +138,15 @@ func (factory *hermesNotificationEmailFactory) WebhookSendFailed(user *entities.
138138
}, nil
139139
}
140140

141+
func replaceWebhookSendFailedEventPayloadPlaceholder(text string, formattedPayload string) string {
142+
before, after, found := strings.Cut(text, webhookSendFailedEventPayloadPlaceholder)
143+
if !found {
144+
return text
145+
}
146+
147+
return before + formattedPayload + after
148+
}
149+
141150
func (factory *hermesNotificationEmailFactory) MessageExpired(user *entities.User, payload *events.MessageSendExpiredPayload) (*Email, error) {
142151
email := hermes.Email{
143152
Body: hermes.Body{

api/pkg/emails/hermes_notification_email_factory_test.go

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,7 @@ func TestWebhookSendFailedFormatsOnlyEventPayload(t *testing.T) {
4949
assert.Contains(t, email.Text, `"message": "hello"`)
5050
assert.Contains(t, email.Text, `"retry": false`)
5151
assert.NotContains(t, email.Text, "<pre")
52+
assert.NotContains(t, email.Text, webhookSendFailedEventPayloadPlaceholder)
5253
}
5354

5455
func TestWebhookSendFailedPreservesNonJSONEventPayload(t *testing.T) {
@@ -74,3 +75,31 @@ func TestWebhookSendFailedPreservesNonJSONEventPayload(t *testing.T) {
7475
assert.NotContains(t, email.HTML, `<span style="color:`)
7576
assert.Contains(t, email.Text, "line one\n line two")
7677
}
78+
79+
func TestReplaceWebhookSendFailedEventPayloadPlaceholder(t *testing.T) {
80+
tests := []struct {
81+
name string
82+
text string
83+
formattedPayload string
84+
want string
85+
}{
86+
{
87+
name: "replaces placeholder and preserves remaining text",
88+
text: "before\n" + webhookSendFailedEventPayloadPlaceholder + "\nafter",
89+
formattedPayload: "{\n \"message\": \"hello\"\n}",
90+
want: "before\n{\n \"message\": \"hello\"\n}\nafter",
91+
},
92+
{
93+
name: "returns original text when placeholder is missing",
94+
text: "before\nafter",
95+
formattedPayload: "{\n \"message\": \"hello\"\n}",
96+
want: "before\nafter",
97+
},
98+
}
99+
100+
for _, tt := range tests {
101+
t.Run(tt.name, func(t *testing.T) {
102+
assert.Equal(t, tt.want, replaceWebhookSendFailedEventPayloadPlaceholder(tt.text, tt.formattedPayload))
103+
})
104+
}
105+
}

0 commit comments

Comments
 (0)