Skip to content

Commit 77253bf

Browse files
f0sselsreya
f0ssel
and
sreya
authored
fix(site): escape appearance values in HTML output (#25804) (#26321)
Backport of #25804 to `release/2.29` (SEC-86 / ENG-2745). Conflict resolution: applied the `html.EscapeString` calls to the pre-refactor `renderHTMLWithState` (main has since extracted `populateHTMLState`). Adapted `site.New` call in the test (returns one value on 2.29) and recreated `smtp_internal_test.go` with only the escaping test, since the file does not exist on this branch. > 🤖 Backport prepared by Coder Agents on behalf of @f0ssel. Co-authored-by: Jon Ayers <jon@coder.com>
1 parent 18ded82 commit 77253bf

4 files changed

Lines changed: 126 additions & 5 deletions

File tree

‎coderd/notifications/dispatch/smtp/html.gotmpl‎

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 }}

‎coderd/notifications/dispatch/smtp_internal_test.go‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
package dispatch
2+
3+
import (
4+
"html"
5+
"strings"
6+
"testing"
7+
8+
"github.com/stretchr/testify/require"
9+
10+
"github.com/coder/coder/v2/coderd/notifications/render"
11+
"github.com/coder/coder/v2/coderd/notifications/types"
12+
)
13+
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+
}

‎site/site.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -437,8 +437,8 @@ func (h *Handler) renderHTMLWithState(r *http.Request, filePath string, state ht
437437
// nolint:gocritic // User is not expected to be signed in.
438438
ctx := dbauthz.AsSystemRestricted(r.Context())
439439
cfg, _ = af.Fetch(ctx)
440-
state.ApplicationName = applicationNameOrDefault(cfg)
441-
state.LogoURL = cfg.LogoURL
440+
state.ApplicationName = html.EscapeString(applicationNameOrDefault(cfg))
441+
state.LogoURL = html.EscapeString(cfg.LogoURL)
442442
return execTmpl(tmpl, state)
443443
}
444444

@@ -523,8 +523,8 @@ func (h *Handler) renderHTMLWithState(r *http.Request, filePath string, state ht
523523
appr, err := json.Marshal(cfg)
524524
if err == nil {
525525
state.Appearance = html.EscapeString(string(appr))
526-
state.ApplicationName = applicationNameOrDefault(cfg)
527-
state.LogoURL = cfg.LogoURL
526+
state.ApplicationName = html.EscapeString(applicationNameOrDefault(cfg))
527+
state.LogoURL = html.EscapeString(cfg.LogoURL)
528528
}
529529
}
530530
}()

‎site/site_test.go‎

Lines changed: 77 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"
@@ -24,6 +25,7 @@ import (
2425
"github.com/stretchr/testify/assert"
2526
"github.com/stretchr/testify/require"
2627

28+
"github.com/coder/coder/v2/coderd/appearance"
2729
"github.com/coder/coder/v2/coderd/database"
2830
"github.com/coder/coder/v2/coderd/database/db2sdk"
2931
"github.com/coder/coder/v2/coderd/database/dbgen"
@@ -36,6 +38,81 @@ import (
3638
"github.com/coder/coder/v2/testutil"
3739
)
3840

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

0 commit comments

Comments
 (0)