◐ Shell
reader mode source ↗
Skip to content

Commit aba0853

Browse files
jdomeracki-codersreya
jdomeracki-coder
and
sreya
authored
fix: escape appearance values in HTML output (#25804) (#26260)
Cherry-pick backport to `release/2.34`. Co-authored-by: Jon Ayers <jon@coder.com>
1 parent bf5a220 commit aba0853

4 files changed

Lines changed: 120 additions & 5 deletions

File tree

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88
<body style="margin: 0; padding: 0; font-family: -apple-system, system-ui, BlinkMacSystemFont, 'Segoe UI', 'Roboto', 'Oxygen', 'Ubuntu', 'Cantarell', 'Fira Sans', 'Droid Sans', 'Helvetica Neue', sans-serif; color: #020617; background: #f8fafc;">
99
<div style="max-width: 600px; margin: 20px auto; padding: 60px; border: 1px solid #e2e8f0; border-radius: 8px; background-color: #fff; text-align: left; font-size: 14px; line-height: 1.5;">
1010
<div style="text-align: center;">
11-
<img src="{{ logo_url }}" alt="{{ app_name }} Logo" style="height: 40px;" />
11+
<img src="{{ logo_url | html }}" alt="{{ app_name | html }} Logo" style="height: 40px;" />
1212
</div>
1313
<h1 style="text-align: center; font-size: 24px; font-weight: 400; margin: 8px 0 32px; line-height: 1.5;">
1414
{{ .Labels._subject }}
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,48 @@
11
package dispatch
22

33
import (
4+
"html"
5+
"strings"
46
"testing"
57

68
"github.com/stretchr/testify/require"
9+
10+
"github.com/coder/coder/v2/coderd/notifications/render"
11+
"github.com/coder/coder/v2/coderd/notifications/types"
712
)
813

14+
func TestSMTPHTMLTemplateEscapesAppearanceHelpers(t *testing.T) {
15+
t.Parallel()
16+
17+
const (
18+
appName = `Coder"><script>alert(1)</script>`
19+
logoURL = `https://example.com/logo.png"><img src=x onerror=alert(1)>`
20+
)
21+
22+
payload := types.MessagePayload{
23+
NotificationTemplateID: "00000000-0000-0000-0000-000000000000",
24+
UserName: "Test User",
25+
Labels: map[string]string{
26+
"_subject": "Test notification",
27+
"_body": "<p>Test body</p>",
28+
},
29+
}
30+
helpers := map[string]any{
31+
"base_url": func() string { return "https://coder.example.com" },
32+
"current_year": func() string { return "2026" },
33+
"logo_url": func() string { return logoURL },
34+
"app_name": func() string { return appName },
35+
}
36+
37+
got, err := render.GoTemplate(htmlTemplate, payload, helpers)
38+
require.NoError(t, err)
39+
40+
require.True(t, strings.Contains(got, html.EscapeString(appName)), "application name must be HTML escaped")
41+
require.True(t, strings.Contains(got, html.EscapeString(logoURL)), "logo URL must be HTML escaped")
42+
require.False(t, strings.Contains(got, appName), "raw application name must not be rendered")
43+
require.False(t, strings.Contains(got, logoURL), "raw logo URL must not be rendered")
44+
}
45+
946
func TestValidateFromAddr(t *testing.T) {
1047
t.Parallel()
1148

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -397,8 +397,8 @@ func (h *Handler) renderHTMLWithState(r *http.Request, filePath string, state ht
397397
// nolint:gocritic // User is not expected to be signed in.
398398
ctx := dbauthz.AsSystemRestricted(r.Context())
399399
cfg, _ = af.Fetch(ctx)
400-
state.ApplicationName = applicationNameOrDefault(cfg)
401-
state.LogoURL = cfg.LogoURL
400+
state.ApplicationName = html.EscapeString(applicationNameOrDefault(cfg))
401+
state.LogoURL = html.EscapeString(cfg.LogoURL)
402402
return execTmpl(tmpl, state)
403403
}
404404

@@ -488,8 +488,8 @@ func (h *Handler) populateHTMLState(
488488
appr, err := json.Marshal(cfg)
489489
if err == nil {
490490
state.Appearance = html.EscapeString(string(appr))
491-
state.ApplicationName = applicationNameOrDefault(cfg)
492-
state.LogoURL = cfg.LogoURL
491+
state.ApplicationName = html.EscapeString(applicationNameOrDefault(cfg))
492+
state.LogoURL = html.EscapeString(cfg.LogoURL)
493493
}
494494
}
495495
})
Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import (
1414
"path/filepath"
1515
"strconv"
1616
"strings"
17+
"sync/atomic"
1718
"testing"
1819
"testing/fstest"
1920
"time"
@@ -26,6 +27,7 @@ import (
2627
"github.com/stretchr/testify/require"
2728
"golang.org/x/exp/maps"
2829

30+
"github.com/coder/coder/v2/coderd/appearance"
2931
"github.com/coder/coder/v2/coderd/database"
3032
"github.com/coder/coder/v2/coderd/database/db2sdk"
3133
"github.com/coder/coder/v2/coderd/database/dbgen"
@@ -39,6 +41,82 @@ import (
3941
"github.com/coder/coder/v2/testutil"
4042
)
4143

44+
type staticAppearanceFetcher struct {
45+
cfg codersdk.AppearanceConfig
46+
}
47+
48+
func (f staticAppearanceFetcher) Fetch(context.Context) (codersdk.AppearanceConfig, error) {
49+
return f.cfg, nil
50+
}
51+
52+
func TestInjectionAppearanceEscapesMetaAttributes(t *testing.T) {
53+
t.Parallel()
54+
55+
const (
56+
applicationName = `Coder"><script>alert(1)</script>`
57+
logoURL = `https://example.com/logo.png"><img src=x onerror=alert(1)>`
58+
)
59+
60+
tests := []struct {
61+
name string
62+
authenticated bool
63+
}{
64+
{
65+
name: "unauthenticated",
66+
},
67+
{
68+
name: "authenticated",
69+
authenticated: true,
70+
},
71+
}
72+
73+
for _, tt := range tests {
74+
t.Run(tt.name, func(t *testing.T) {
75+
t.Parallel()
76+
77+
siteFS := fstest.MapFS{
78+
"index.html": &fstest.MapFile{
79+
Data: []byte(`<meta name="application-name" content="{{ .ApplicationName }}" /><meta property="logo-url" content="{{ .LogoURL }}" />`),
80+
},
81+
}
82+
db, _ := dbtestutil.NewDB(t)
83+
var appearanceFetcher atomic.Pointer[appearance.Fetcher]
84+
fetcher := appearance.Fetcher(staticAppearanceFetcher{cfg: codersdk.AppearanceConfig{
85+
ApplicationName: applicationName,
86+
LogoURL: logoURL,
87+
}})
88+
appearanceFetcher.Store(&fetcher)
89+
handler, err := site.New(&site.Options{
90+
Telemetry: telemetry.NewNoop(),
91+
Database: db,
92+
SiteFS: siteFS,
93+
AppearanceFetcher: &appearanceFetcher,
94+
})
95+
require.NoError(t, err)
96+
97+
r := httptest.NewRequest("GET", "/", nil)
98+
if tt.authenticated {
99+
user := dbgen.User(t, db, database.User{})
100+
_, token := dbgen.APIKey(t, db, database.APIKey{
101+
UserID: user.ID,
102+
ExpiresAt: time.Now().Add(time.Hour),
103+
})
104+
r.Header.Set(codersdk.SessionTokenHeader, token)
105+
}
106+
rw := httptest.NewRecorder()
107+
108+
handler.ServeHTTP(rw, r)
109+
require.Equal(t, http.StatusOK, rw.Code)
110+
body := rw.Body.String()
111+
112+
require.True(t, strings.Contains(body, html.EscapeString(applicationName)), "application name must be HTML escaped")
113+
require.True(t, strings.Contains(body, html.EscapeString(logoURL)), "logo URL must be HTML escaped")
114+
require.False(t, strings.Contains(body, applicationName), "raw application name must not be rendered")
115+
require.False(t, strings.Contains(body, logoURL), "raw logo URL must not be rendered")
116+
})
117+
}
118+
}
119+
42120
func TestInjection(t *testing.T) {
43121
t.Parallel()
44122

0 commit comments

Comments
 (0)