diff --git a/contracts/settings/v1/manifest.json b/contracts/settings/v1/manifest.json index 195802a4..0d30a519 100644 --- a/contracts/settings/v1/manifest.json +++ b/contracts/settings/v1/manifest.json @@ -20,7 +20,7 @@ "label": "Preferred audio language", "description": "Choose which spoken language Silo should prefer first.", "recommended_control": "select", - "notes": "Migrates user_profiles.language as the roaming fallback. Existing user_device_settings values become real overrides, and the per-series value comes from AudioPreference.audio_language. AudioPreference.audio_track_index and track_signature stay specialized: they identify a concrete track, not a default." + "notes": "Migrates user_profiles.language as the roaming fallback. Existing user_device_settings values become real overrides, and the per-series value comes from AudioPreference.audio_language. AudioPreference.audio_track_index and track_signature stay specialized: they identify a concrete track, not a default. The legacy string-only endpoint has no way to send null, so it spells \"no preference\" as the empty string and both Android and web send that to clear the choice; its validator accepts \"\" and otherwise requires a well-formed tag via settingscontract.NormalizeLanguageTag. Migration maps \"\" to no stored row, the same way playback.subtitle_mode handles it." }, { "key": "playback.subtitle_language", diff --git a/internal/api/handlers/jellyfin_compat_test.go b/internal/api/handlers/jellyfin_compat_test.go index 8d1631ad..778ead47 100644 --- a/internal/api/handlers/jellyfin_compat_test.go +++ b/internal/api/handlers/jellyfin_compat_test.go @@ -4,6 +4,7 @@ import ( "context" "net/http" "net/http/httptest" + "os" "strings" "testing" @@ -234,7 +235,7 @@ func TestRemoveJellyfinCompatWebDisablesWebSetting(t *testing.T) { settings := &fakeServerSettingsStore{values: map[string]string{ "jellyfin_compat.enabled": "true", "jellyfin_compat.web_enabled": "true", - "jellyfin_compat.web_install_dir": t.TempDir(), + "jellyfin_compat.web_install_dir": asyncWebInstallRoot(t), }} published := map[string]string{} handler := &AdminHandler{ @@ -337,3 +338,21 @@ func TestPersistJellyfinCompatWebInstallSettingsEnablesWebUI(t *testing.T) { t.Fatalf("jellyfin_compat.web_source_url = %q", got) } } + +// asyncWebInstallRoot returns a temp dir for a handler that removes or installs +// Jellyfin Web assets in a background goroutine. +// +// t.TempDir is wrong here: the endpoint returns 202 and its goroutine keeps +// writing into the root after the test body returns, so t.TempDir's cleanup +// intermittently trips "directory not empty" and fails an otherwise passing +// test. os.RemoveAll recurses, so a late write is harmless, and cleanup errors +// are ignored rather than failing the test. +func asyncWebInstallRoot(t *testing.T) string { + t.Helper() + dir, err := os.MkdirTemp("", "jellyfin-web-root-*") + if err != nil { + t.Fatalf("MkdirTemp: %v", err) + } + t.Cleanup(func() { _ = os.RemoveAll(dir) }) + return dir +} diff --git a/internal/api/handlers/settings.go b/internal/api/handlers/settings.go index 1a430fa4..9111bdcb 100644 --- a/internal/api/handlers/settings.go +++ b/internal/api/handlers/settings.go @@ -15,6 +15,7 @@ import ( apimw "github.com/Silo-Server/silo-server/internal/api/middleware" "github.com/Silo-Server/silo-server/internal/cache" + "github.com/Silo-Server/silo-server/internal/settingscontract" "github.com/Silo-Server/silo-server/internal/userstore" ) @@ -157,12 +158,7 @@ var settingsRegistry = map[string]settingSpec{ "playback.audio_language": { Scope: scopeDevice, DefaultValue: "", - Validate: func(value string) error { - if len(strings.TrimSpace(value)) > 32 { - return fmt.Errorf("playback.audio_language must be 32 characters or fewer") - } - return nil - }, + Validate: validateLanguageTagSetting("playback.audio_language"), }, "playback.auto_skip_intro": { Scope: scopeDevice, @@ -253,7 +249,7 @@ var settingsRegistry = map[string]settingSpec{ "player.playback_speed": { Scope: scopeDevice, DefaultValue: "1", - Validate: validateFloatRange("player.playback_speed", 0.25, 3.0), + Validate: validateFloatRangeStep("player.playback_speed", 0.25, 3.0, 0.05), }, "player.audio_sync_ms": { Scope: scopeDevice, @@ -862,6 +858,18 @@ func validateIntRange(key string, min, max int) func(string) error { } func validateFloatRange(key string, min, max float64) func(string) error { + return validateFloatRangeStep(key, min, max, 0) +} + +// validateFloatRangeStep enforces the range and, when step is positive, that +// the value sits on the step grid anchored at min. +// +// The step check delegates to settingscontract.StepAligned so this endpoint +// enforces exactly what contracts/settings/v1/manifest.json declares. Before +// this, player.playback_speed advertised a 0.05 step that nothing enforced, so +// the server happily stored 0.26 — a value no client's stepper can represent +// and that every client would silently snap on the next write. +func validateFloatRangeStep(key string, min, max, step float64) func(string) error { return func(value string) error { parsed, err := strconv.ParseFloat(value, 64) if err != nil { @@ -870,6 +878,33 @@ func validateFloatRange(key string, min, max float64) func(string) error { if math.IsNaN(parsed) || parsed < min || parsed > max { return fmt.Errorf("%s must be between %g and %g", key, min, max) } + if !settingscontract.StepAligned(parsed, min, step) { + return fmt.Errorf("%s must be a multiple of %g starting from %g", key, step, min) + } + return nil + } +} + +// validateLanguageTagSetting accepts a BCP 47 language tag, or the empty string. +// +// The empty string is the legacy wire form for "no preference": the string-only +// settings API has no way to send null, and both the Android and web clients +// send "" to clear the choice. The contract expresses the same state as null, +// which is why every language definition there is nullable. +// +// Anything else must be a well-formed tag. The previous check was "32 +// characters or fewer", so the server accepted "!!!" for a field the manifest +// declares as language_tag — and stored it where track matching would silently +// never match. +func validateLanguageTagSetting(key string) func(string) error { + return func(value string) error { + trimmed := strings.TrimSpace(value) + if trimmed == "" { + return nil + } + if _, ok := settingscontract.NormalizeLanguageTag(trimmed); !ok { + return fmt.Errorf("%s must be a BCP 47 language tag such as en or en-US", key) + } return nil } } diff --git a/internal/api/handlers/settings_contract_test.go b/internal/api/handlers/settings_contract_test.go index cd9c566f..e03bf049 100644 --- a/internal/api/handlers/settings_contract_test.go +++ b/internal/api/handlers/settings_contract_test.go @@ -139,3 +139,76 @@ func TestContractLoadsUnderTheServerBuild(t *testing.T) { } } } + +// TestAudioLanguageRejectsMalformedTags closes a drift measured against the +// live server: the manifest declares playback.audio_language as language_tag, +// but the registry check was "32 characters or fewer", so "!!!" was stored for +// a field track matching would then silently never match. +func TestAudioLanguageRejectsMalformedTags(t *testing.T) { + const key = "playback.audio_language" + + // The empty string is how the string-only API says "no preference", and + // both Android and web send it to clear the choice. It must keep working. + accepted := []string{"", " ", "en", "EN", "en-US", "en_US", "pt-BR", "zh-Hant-TW", "es-419"} + for _, v := range accepted { + if err := validateRegisteredSetting(key, v, scopeDevice); err != nil { + t.Errorf("value %q was rejected: %v", v, err) + } + } + + rejected := []string{"!!!", "english please", "e", "en-", "-US", "en--US", "123", "