fix: split html sanitization from plaintext handling (#5403)
Some checks failed
Pipeline: Test, Lint, Build / Get version info (push) Has been cancelled
Pipeline: Test, Lint, Build / Lint Go code (push) Has been cancelled
Pipeline: Test, Lint, Build / Test Go code (push) Has been cancelled
Pipeline: Test, Lint, Build / Test Go code (Windows) (push) Has been cancelled
Pipeline: Test, Lint, Build / Test JS code (push) Has been cancelled
Pipeline: Test, Lint, Build / Lint i18n files (push) Has been cancelled
Pipeline: Test, Lint, Build / Check Docker configuration (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-4 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build Windows installers (push) Has been cancelled
Pipeline: Test, Lint, Build / Package/Release (push) Has been cancelled
Pipeline: Test, Lint, Build / Upload Linux PKG (push) Has been cancelled
Pipeline: Test, Lint, Build / Build (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-1 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-2 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-3 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-5 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-6 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-7 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-8 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-9 (push) Has been cancelled
Pipeline: Test, Lint, Build / Build-10 (push) Has been cancelled
Pipeline: Test, Lint, Build / Push to GHCR (push) Has been cancelled
Pipeline: Test, Lint, Build / Push to Docker Hub (push) Has been cancelled
Pipeline: Test, Lint, Build / Cleanup digest artifacts (push) Has been cancelled

* fix: split html sanitization from plaintext handling

Add a dedicated SanitizeHTML helper for HTML-rendered values so entity-encoded markup is decoded before bluemonday sanitization. Use the new helper for the login welcome message and artist biographies while preserving SanitizeText semantics for lyrics and other plaintext callers. Add regression coverage for both helpers and the serveIndex welcomeMessage path.

* docs: add SanitizeText and SanitizeHTML godoc

Signed-off-by: Deluan <deluan@navidrome.org>

* fix: preserve plain text in artist biographies

Revert artist biography storage to SanitizeText so entity-encoded plain text remains decoded for Subsonic consumers. This avoids double-escaping values like R&B in XML responses while keeping the new welcomeMessage HTML sanitization in place, and adds a regression test covering the biography storage behavior.

---------

Signed-off-by: Deluan <deluan@navidrome.org>
This commit is contained in:
Deluan Quintão 2026-04-23 17:53:28 -04:00 committed by GitHub
parent 4488349a3a
commit 7e083e0795
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
5 changed files with 74 additions and 1 deletions

View file

@ -105,6 +105,29 @@ var _ = Describe("Provider - UpdateArtistInfo", func() {
ag.AssertExpectations(GinkgoT())
})
It("preserves decoded plain text in biography storage", func() {
originalArtist := &model.Artist{
ID: "ar-encoded-bio",
Name: "Encoded Bio Artist",
}
mockArtistRepo.SetData(model.Artists{*originalArtist})
expectedMBID := "mbid-encoded-bio"
expectedBio := "R&amp;B"
ag.On("GetArtistMBID", ctx, "ar-encoded-bio", "Encoded Bio Artist").Return(expectedMBID, nil).Once()
ag.On("GetArtistImages", ctx, "ar-encoded-bio", "Encoded Bio Artist", expectedMBID).Return(nil, nil).Maybe()
ag.On("GetArtistBiography", ctx, "ar-encoded-bio", "Encoded Bio Artist", expectedMBID).Return(expectedBio, nil).Once()
ag.On("GetArtistURL", ctx, "ar-encoded-bio", "Encoded Bio Artist", expectedMBID).Return("", nil).Maybe()
ag.On("GetSimilarArtists", ctx, "ar-encoded-bio", "Encoded Bio Artist", expectedMBID, 100).Return(nil, nil).Maybe()
updatedArtist, err := p.UpdateArtistInfo(ctx, "ar-encoded-bio", 10, false)
Expect(err).NotTo(HaveOccurred())
Expect(updatedArtist).NotTo(BeNil())
Expect(updatedArtist.Biography).To(Equal("R&B"))
})
It("returns cached info when artist exists and info is not expired", func() {
now := time.Now()
originalArtist := &model.Artist{

View file

@ -45,7 +45,7 @@ func serveIndex(ds model.DataStore, fs fs.FS, shareInfo *model.Share) http.Handl
"variousArtistsId": consts.VariousArtistsID,
"baseURL": str.SanitizeText(strings.TrimSuffix(conf.Server.BasePath, "/")),
"loginBackgroundURL": str.SanitizeText(conf.Server.UILoginBackgroundURL),
"welcomeMessage": str.SanitizeText(conf.Server.UIWelcomeMessage),
"welcomeMessage": str.SanitizeHTML(conf.Server.UIWelcomeMessage),
"maxSidebarPlaylists": conf.Server.MaxSidebarPlaylists,
"enableTranscodingConfig": conf.Server.EnableTranscodingConfig,
"enableDownloads": conf.Server.EnableDownloads,

View file

@ -108,6 +108,18 @@ var _ = Describe("serveIndex", func() {
Entry("extAuthLogoutURL", func() { conf.Server.ExtAuth.LogoutURL = "https://auth.example.com/logout" }, "extAuthLogoutURL", "https://auth.example.com/logout"),
)
It("sanitizes entity-encoded welcomeMessage as html", func() {
conf.Server.UIWelcomeMessage = `&lt;img src=x onerror=alert(1)&gt;&lt;b&gt;Hello&lt;/b&gt;`
r := httptest.NewRequest("GET", "/index.html", nil)
w := httptest.NewRecorder()
serveIndex(ds, fs, nil)(w, r)
config := extractAppConfig(w.Body.String())
Expect(config).To(HaveKey("welcomeMessage"))
Expect(config["welcomeMessage"]).To(Equal(`<img src="x"><b>Hello</b>`))
})
DescribeTable("sets other UI configuration values",
func(configKey string, expectedValueFunc func() any) {
r := httptest.NewRequest("GET", "/index.html", nil)

View file

@ -38,11 +38,20 @@ func SanitizeStrings(text ...string) string {
var policy = bluemonday.UGCPolicy()
// SanitizeText unescapes the input string before sanitizing it as text.
// This should be used for fields rendered as plain text in the UI (e.g. lyrics, song titles, artist names)
func SanitizeText(text string) string {
s := policy.Sanitize(text)
return html.UnescapeString(s)
}
// SanitizeHTML unescapes the input string before sanitizing it as HTML.
// This should be used for fields rendered as HTML by clients (e.g. biographies, welcome messages)
// to prevent XSS bypasses via entity-encoded tags.
func SanitizeHTML(text string) string {
return policy.Sanitize(html.UnescapeString(text))
}
func SanitizeFieldForSorting(originalValue string) string {
v := strings.TrimSpace(sanitize.Accents(originalValue))
return Clear(strings.ToLower(v))

View file

@ -64,6 +64,35 @@ var _ = Describe("Sanitize Strings", func() {
})
})
Describe("SanitizeText", func() {
It("preserves decoded plaintext", func() {
Expect(str.SanitizeText("Tom &amp; Jerry")).To(Equal("Tom & Jerry"))
Expect(str.SanitizeText("Tom & Jerry")).To(Equal("Tom & Jerry"))
})
It("keeps entity-encoded html readable", func() {
Expect(str.SanitizeText(`&lt;b&gt;ok&lt;/b&gt;`)).To(Equal("<b>ok</b>"))
})
})
Describe("SanitizeHTML", func() {
It("removes dangerous content from raw html", func() {
sanitized := str.SanitizeHTML(`<img src=x onerror=alert(1)><script>alert(2)</script><b>ok</b>`)
Expect(sanitized).To(ContainSubstring("<b>ok</b>"))
Expect(sanitized).ToNot(ContainSubstring("onerror"))
Expect(sanitized).ToNot(ContainSubstring("<script"))
})
It("removes dangerous content from entity-encoded html", func() {
sanitized := str.SanitizeHTML(`&lt;img src=x onerror=alert(1)&gt;&lt;script&gt;alert(2)&lt;/script&gt;&lt;b&gt;ok&lt;/b&gt;`)
Expect(sanitized).To(ContainSubstring("<b>ok</b>"))
Expect(sanitized).ToNot(ContainSubstring("onerror"))
Expect(sanitized).ToNot(ContainSubstring("<script"))
})
})
Describe("SanitizeFieldForSortingNoArticle", func() {
BeforeEach(func() {
conf.Server.IgnoredArticles = "The O"