From 190a097e2e5809336e02ac2c0d962576bc62ad87 Mon Sep 17 00:00:00 2001 From: ranokay Date: Fri, 17 Jul 2026 10:34:31 +0300 Subject: [PATCH] fix(lyrics): distinguish malformed structured sources - stop malformed structured content from becoming plain lyrics - let source resolution continue to valid lower-priority lyrics Signed-off-by: ranokay --- core/lyrics/lyrics_test.go | 9 +- model/lyrics_lyricsfile.go | 10 +++ model/lyrics_parse.go | 119 ++++++++++++++++++++------- model/lyrics_parse_test.go | 93 +++++++++++++++------ model/lyrics_ttml.go | 4 + plugins/lyrics_adapter_test.go | 16 ++++ plugins/testdata/test-lyrics/main.go | 7 ++ 7 files changed, 198 insertions(+), 60 deletions(-) diff --git a/core/lyrics/lyrics_test.go b/core/lyrics/lyrics_test.go index b00bcd576..7ce09c28a 100644 --- a/core/lyrics/lyrics_test.go +++ b/core/lyrics/lyrics_test.go @@ -230,7 +230,7 @@ var _ = Describe("Lyrics", func() { })) }) - It("returns a non-Lyricsfile YAML sidecar as plain text, shadowing lower-priority sources", func() { + It("skips a non-Lyricsfile YAML sidecar and resolves the next source", func() { dir, err := os.MkdirTemp("", "lyrics-yaml-fallback-*") Expect(err).ToNot(HaveOccurred()) DeferCleanup(func() { @@ -247,14 +247,11 @@ var _ = Describe("Lyrics", func() { Path: "song.mp3", }) - // ParseLyrics falls back to plain text for any suffix when the content - // doesn't match the structured format, so the .yaml hit is non-empty and - // shadows the lower-priority .lrc entirely. Expect(err).To(BeNil()) Expect(list).To(HaveLen(1)) - Expect(list[0].Synced).To(BeFalse()) + Expect(list[0].Synced).To(BeTrue()) Expect(list[0].Line).To(Equal([]model.Line{ - {Value: "title: not lyricsfile"}, + {Start: new(int64(1000)), Value: "Fallback line"}, })) }) diff --git a/model/lyrics_lyricsfile.go b/model/lyrics_lyricsfile.go index 49d416f3f..51073593e 100644 --- a/model/lyrics_lyricsfile.go +++ b/model/lyrics_lyricsfile.go @@ -71,6 +71,16 @@ func parseLyricsfile(lang string, contents []byte) (LyricList, error) { const lyricsfileVersion = "1.0" +func hasLyricsfileVersion(contents []byte) bool { + var header struct { + Version string `yaml:"version"` + } + if err := yaml.Unmarshal(contents, &header); err != nil { + return false + } + return strings.TrimSpace(header.Version) == lyricsfileVersion +} + type lyricsfileDocument struct { Version string `yaml:"version"` Metadata lyricsfileMetadata `yaml:"metadata"` diff --git a/model/lyrics_parse.go b/model/lyrics_parse.go index 8aa095c50..04afd985b 100644 --- a/model/lyrics_parse.go +++ b/model/lyrics_parse.go @@ -3,32 +3,46 @@ package model import ( "bytes" "context" + "errors" "fmt" + "regexp" "slices" "strings" "github.com/navidrome/navidrome/log" ) -// lyricParser returns an empty list (not an error) when the input is not its -// format, so parsers can be tried in order. lang is the default for formats that -// do not carry their own. +// lyricParser parses content already claimed by its format. A nil list with no +// error means a recognized, valid document that contains no lyrics. type lyricParser func(lang string, contents []byte) (LyricList, error) +var errLyricsFormatMismatch = errors.New("lyrics format mismatch") + +type lyricFormat struct { + name string + suffixes []string + claims func([]byte) bool + parse lyricParser +} + // lyricFormats is the structured formats in content-sniff probe order; each // row's suffixes drive sidecar dispatch. LRC/plain is the unlisted fallback floor. -var lyricFormats = []struct { - suffixes []string - parse lyricParser -}{ - {[]string{".ttml"}, parseTTML}, - {[]string{".srt"}, parseSRT}, - {[]string{".yaml", ".yml"}, parseLyricsfile}, +var lyricFormats = []lyricFormat{ + {name: "TTML", suffixes: []string{".ttml"}, claims: claimsTTML, parse: parseTTML}, + {name: "SRT", suffixes: []string{".srt"}, claims: claimsSRT, parse: parseSRT}, + {name: "Lyricsfile", suffixes: []string{".yaml", ".yml"}, claims: claimsLyricsfile, parse: parseLyricsfile}, } +var ( + ttmlRootPrefixRegex = regexp.MustCompile(`(?is)^\s*(?:<\?xml\b[^>]*\?>\s*)?(?:\s*)*<(?:[[:alpha:]_][[:alnum:]_.-]*:)?tt(?:\s|/?>)`) + srtClaimRegex = regexp.MustCompile(`(?m)^\s*(?:\d+\s*\n\s*)?\d{1,2}:\d{2}:\d{2}[,.]\d{1,3}\s*-->\s*\d{1,2}:\d{2}:\d{2}[,.]\d{1,3}(?:\s|$)`) + lyricsfileRegex = regexp.MustCompile(`(?mi)^\s*["']?version["']?\s*:\s*["']?1\.0["']?\s*(?:#.*)?$`) +) + // ParseLyrics is the single entry point for parsing lyrics. A known suffix routes -// to that format's parser; an empty or "auto" suffix content-sniffs. Either way, -// a structured parser that does not match falls back to the LRC/plain-text floor. +// to that format's parser; an empty or "auto" suffix content-sniffs. Explicit +// structured suffixes are strict: malformed or mismatched structured content is +// returned as an error so a source resolver can continue to its next source. // // Parse failures are logged through ctx; callers that know the source should // attach it for attribution, e.g. log.NewContext(ctx, "file", path). @@ -37,37 +51,80 @@ func ParseLyrics(ctx context.Context, suffix, lang string, contents []byte) (Lyr suffix = strings.ToLower(suffix) sniff := suffix == "" || suffix == "auto" - // Sniffing tries every format in order; a known suffix selects just its own. - // Unmatched suffixes leave no candidates, so parseFirstMatch falls to plain. - candidates := make([]lyricParser, 0, len(lyricFormats)) + // Sniffing tries every structured format in order. A known structured suffix + // selects one strict parser; LRC/text and unknown textual suffixes retain the + // longstanding LRC/plain fallback. + candidates := make([]lyricFormat, 0, len(lyricFormats)) for _, f := range lyricFormats { if sniff || slices.Contains(f.suffixes, suffix) { - candidates = append(candidates, f.parse) + candidates = append(candidates, f) } } - return parseFirstMatch(ctx, sniff, lang, contents, candidates...) -} -func parseFirstMatch(ctx context.Context, sniff bool, lang string, contents []byte, candidates ...lyricParser) (LyricList, error) { - for _, parse := range candidates { - list, err := parse(lang, contents) - if err == nil && len(list) > 0 { - return list, nil + if !sniff && len(candidates) > 0 { + format := candidates[0] + list, err := parseClaimedLyrics(format, lang, contents) + if errors.Is(err, errLyricsFormatMismatch) { + err = fmt.Errorf("declared %s lyrics do not match the format: %w", format.name, err) } if err != nil { - // While sniffing, a probe rejecting content it does not own is expected - // control flow, so keep it at trace. A failure under an explicit suffix - // means the declared format is malformed and deserves a warning. - if sniff { - log.Trace(ctx, "Lyrics probe did not match, trying next format", err) - } else { - log.Warn(ctx, "Error parsing lyrics, falling back to plain text", err) - } + log.Warn(ctx, "Error parsing declared lyrics", "format", format.name, err) } + return list, err + } + + return parseFirstMatch(ctx, lang, contents, candidates...) +} + +func parseFirstMatch(ctx context.Context, lang string, contents []byte, candidates ...lyricFormat) (LyricList, error) { + for _, format := range candidates { + list, err := parseClaimedLyrics(format, lang, contents) + if errors.Is(err, errLyricsFormatMismatch) { + log.Trace(ctx, "Lyrics probe did not match, trying next format", "format", format.name) + continue + } + if err != nil { + return nil, fmt.Errorf("parsing claimed %s lyrics: %w", format.name, err) + } + // A claimed, valid-empty document owns the content and deliberately stops + // sniffing instead of being reinterpreted as another format or plain text. + return list, nil } return plainLRC(lang, contents) } +func parseClaimedLyrics(format lyricFormat, lang string, contents []byte) (LyricList, error) { + if !format.claims(contents) { + return nil, errLyricsFormatMismatch + } + list, err := format.parse(lang, contents) + if err != nil { + return nil, err + } + return list, nil +} + +func claimsTTML(contents []byte) bool { + trimmed := bytes.TrimSpace(contents) + if len(trimmed) == 0 || trimmed[0] != '<' { + return false + } + return isTTMLDocument(contents) || ttmlRootPrefixRegex.Match(contents) +} + +func claimsSRT(contents []byte) bool { + raw := bytes.ReplaceAll(contents, []byte("\r\n"), []byte("\n")) + raw = bytes.ReplaceAll(raw, []byte("\r"), []byte("\n")) + return srtClaimRegex.Match(raw) +} + +func claimsLyricsfile(contents []byte) bool { + if lyricsfileRegex.Match(contents) { + return true + } + return hasLyricsfileVersion(contents) +} + func plainLRC(lang string, contents []byte) (LyricList, error) { lyric, err := parseLRC(lang, string(contents)) if err != nil { diff --git a/model/lyrics_parse_test.go b/model/lyrics_parse_test.go index 0b47e55e9..dc2b1b439 100644 --- a/model/lyrics_parse_test.go +++ b/model/lyrics_parse_test.go @@ -1,8 +1,6 @@ package model import ( - "strings" - "github.com/navidrome/navidrome/log" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -34,6 +32,19 @@ var _ = Describe("ParseLyrics", func() { Expect(list[0].Line[0].Value).To(Equal("auto ttml")) }) + DescribeTable("accepts valid TTML XML prologs", + func(suffix, prolog string) { + contents := prolog + `

prolog ttml

` + list, err := ParseLyrics(GinkgoT().Context(), suffix, "eng", []byte(contents)) + + Expect(err).ToNot(HaveOccurred()) + Expect(list).To(HaveLen(1)) + Expect(list[0].Line[0].Value).To(Equal("prolog ttml")) + }, + Entry("explicit suffix with processing instruction", ".ttml", ``), + Entry("content sniffing with doctype", "", ``), + ) + It("empty suffix content-sniffs (YAML)", func() { yaml := "version: \"1.0\"\nmetadata:\n language: eng\nlines:\n - text: auto yaml\n start_ms: 1000\n" list, err := ParseLyrics(GinkgoT().Context(), "auto", "eng", []byte(yaml)) @@ -42,14 +53,62 @@ var _ = Describe("ParseLyrics", func() { Expect(list[0].Line[0].Value).To(Equal("auto yaml")) }) - It("falls back to plain text when a known suffix fails to parse structurally", func() { + DescribeTable("accepts quoted Lyricsfile version keys", + func(suffix string) { + contents := "\"version\": \"1.0\"\nmetadata:\n language: eng\nlines:\n - text: quoted version\n start_ms: 1000\n" + list, err := ParseLyrics(GinkgoT().Context(), suffix, "eng", []byte(contents)) + + Expect(err).ToNot(HaveOccurred()) + Expect(list).To(HaveLen(1)) + Expect(list[0].Line[0].Value).To(Equal("quoted version")) + }, + Entry("explicit YAML", ".yaml"), + Entry("content sniffing", "auto"), + ) + + It("returns an error when an explicit structured suffix does not match", func() { list, err := ParseLyrics(GinkgoT().Context(), ".srt", "eng", []byte("not actually an srt file")) + Expect(err).To(MatchError(ContainSubstring("declared SRT lyrics do not match"))) + Expect(list).To(BeNil()) + }) + + It("retains LRC/plain fallback for textual suffixes", func() { + list, err := ParseLyrics(GinkgoT().Context(), ".lrc", "eng", []byte("not actually timed")) Expect(err).ToNot(HaveOccurred()) Expect(list).To(HaveLen(1)) Expect(list[0].Synced).To(BeFalse()) - Expect(list[0].Line[0].Value).To(Equal("not actually an srt file")) + Expect(list[0].Line[0].Value).To(Equal("not actually timed")) }) + DescribeTable("recognized valid-empty structured documents stop sniffing", + func(contents string) { + list, err := ParseLyrics(GinkgoT().Context(), "", "eng", []byte(contents)) + Expect(err).ToNot(HaveOccurred()) + Expect(list).To(BeEmpty()) + }, + Entry("TTML", ``), + Entry("SRT", "1\n00:00:01,000 --> 00:00:02,000\n"), + Entry("Lyricsfile", "version: \"1.0\"\nmetadata:\n language: eng\n"), + ) + + It("does not reinterpret malformed claimed structured content as plain text", func() { + list, err := ParseLyrics(GinkgoT().Context(), "", "eng", []byte(`

Broken`)) + Expect(err).To(MatchError(ContainSubstring("parsing claimed TTML lyrics"))) + Expect(list).To(BeNil()) + }) + + DescribeTable("generic content remains eligible for plain fallback", + func(contents string) { + list, err := ParseLyrics(GinkgoT().Context(), "", "eng", []byte(contents)) + Expect(err).ToNot(HaveOccurred()) + Expect(list).To(HaveLen(1)) + Expect(list[0].Synced).To(BeFalse()) + }, + Entry("prose", "A plain lyric"), + Entry("generic XML", "A generic lyric"), + Entry("generic YAML", "title: A generic lyric"), + ) + Describe("logging on parser probe failures", func() { var hook *test.Hook @@ -92,10 +151,10 @@ var _ = Describe("ParseLyrics", func() { // failure is worth surfacing loudly — and it must name the file. It("warns and names the file when a requested suffix fails to parse", func() { ctx := log.NewContext(GinkgoT().Context(), "file", "/music/song.yaml") - list, err := ParseLyrics(ctx, ".yaml", "eng", []byte("not: [valid, yaml\n")) + list, err := ParseLyrics(ctx, ".yaml", "eng", []byte("version: \"1.0\"\nnot: [valid, yaml\n")) - Expect(err).ToNot(HaveOccurred()) - Expect(list).To(HaveLen(1)) // still falls back to plain text + Expect(err).To(HaveOccurred()) + Expect(list).To(BeNil()) entry := hook.LastEntry() Expect(entry).ToNot(BeNil()) Expect(entry.Level).To(Equal(logrus.WarnLevel)) @@ -234,25 +293,13 @@ Another subtitle line` Expect(list[0].Line[0].Cue).To(HaveLen(2)) }) - It("should fall back to plain lyrics when embedded TTML is invalid", func() { - content := ` - -

Broken

- -` + It("should reject malformed embedded TTML instead of returning raw markup", func() { + content := `

Broken` list, err := ParseLyrics(GinkgoT().Context(), "", "eng", []byte(content)) - Expect(err).ToNot(HaveOccurred()) - Expect(list).To(HaveLen(1)) - Expect(list[0].Lang).To(Equal("eng")) - Expect(list[0].Synced).To(BeFalse()) - Expect(list[0].Line).ToNot(BeEmpty()) - values := make([]string, 0, len(list[0].Line)) - for _, line := range list[0].Line { - values = append(values, line.Value) - } - Expect(strings.Join(values, "\n")).To(ContainSubstring("Broken")) + Expect(err).To(HaveOccurred()) + Expect(list).To(BeNil()) }) It("detects a Lyricsfile YAML payload via content-sniffing", func() { diff --git a/model/lyrics_ttml.go b/model/lyrics_ttml.go index 43d4699af..8feadc4bd 100644 --- a/model/lyrics_ttml.go +++ b/model/lyrics_ttml.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/xml" "errors" + "fmt" "io" "math" "regexp" @@ -123,6 +124,9 @@ func parseTTML(defaultLang string, contents []byte) (LyricList, error) { // text — isTTMLDocument does a cheap decode that stops at the first element. // Checked after the encoding fixup so UTF-16-declared documents are recognized. if !isTTMLDocument(contents) { + if claimsTTML(contents) { + return nil, fmt.Errorf("malformed TTML document") + } return nil, nil } diff --git a/plugins/lyrics_adapter_test.go b/plugins/lyrics_adapter_test.go index d110665f5..3269f58c9 100644 --- a/plugins/lyrics_adapter_test.go +++ b/plugins/lyrics_adapter_test.go @@ -150,6 +150,22 @@ var _ = Describe("LyricsPlugin", Ordered, func() { Entry("lrc", "lrc", true, "plugin lrc line"), Entry("plain", "plain", false, "plugin plain line"), ) + + It("keeps valid entries when another plugin lyric is malformed", func() { + manager, _ := createTestManagerWithPlugins(map[string]map[string]string{ + "test-lyrics": {"format": "mixed"}, + }, "test-lyrics"+PackageExtension) + + p, ok := manager.LoadLyricsProvider("test-lyrics") + Expect(ok).To(BeTrue()) + + result, err := p.GetLyrics(GinkgoT().Context(), &model.MediaFile{ID: "track-1"}) + Expect(err).ToNot(HaveOccurred()) + Expect(result).To(HaveLen(1)) + Expect(result[0].Line).To(Equal([]model.Line{ + {Start: new(int64(1000)), Value: "valid plugin line"}, + })) + }) }) Describe("PluginNames", func() { diff --git a/plugins/testdata/test-lyrics/main.go b/plugins/testdata/test-lyrics/main.go index 2ee2dabbf..062aab4bc 100644 --- a/plugins/testdata/test-lyrics/main.go +++ b/plugins/testdata/test-lyrics/main.go @@ -48,6 +48,13 @@ func (t *testLyrics) GetLyrics(input lyrics.GetLyricsRequest) (lyrics.GetLyricsR case "plain": lang = "eng" text = "plugin plain line" + case "mixed": + return lyrics.GetLyricsResponse{ + Lyrics: []lyrics.LyricsText{ + {Lang: "eng", Text: `

malformed`}, + {Lang: "eng", Text: "[00:01.00]valid plugin line"}, + }, + }, nil } if text != "" { return lyrics.GetLyricsResponse{