mirror of
https://github.com/navidrome/navidrome.git
synced 2026-10-08 02:17:25 +02:00
850 commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Deluan Quintão
|
95f67d2c4e |
fix(share): reuse cached transcodes for share streams and zip downloads (#6262)
* fix(share): reuse cached transcodes when streaming from share links Public share streams built the stream request with only the share's format and bit rate, leaving sample rate, bit depth and channels at zero. Regular playback resolves those through the transcode decider (e.g. 48000 Hz for Opus), and they are part of the transcoding cache key, so a track already transcoded during normal playback was transcoded again into a separate, identical cache entry when played through a share link. The public router now resolves share stream requests with the same TranscodeDecider.ResolveRequest used by the Subsonic stream endpoint, so both paths produce the same request and share cache entries. Fixes #6261 * fix(archiver): reuse cached transcodes when zipping downloads Zip downloads (album, artist, playlist and share) built the stream request with only the format and bit rate, leaving sample rate, bit depth and channels at zero. Those are part of the transcoding cache key, so a track already transcoded for playback was transcoded again into a separate cache entry when downloaded in a zip, and vice versa. The archiver now resolves each request with TranscodeDecider.ResolveRequest, the same as single-song downloads and streams. This also applies the decider's defaults, so a zip requested without a bit rate uses the target format's default bit rate instead of leaving it to ffmpeg. * fix(archiver): name zip entries after the resolved transcoding format The transcode decider can pick a different format than the one requested (for example a player's forced transcoding, or a fallback to the default downsampling format when the requested one can't be produced). Zip entry names and the playlist M3U were still built from the requested format, so an entry could end in .mp3 or .flac while holding Opus data. Each track's request is now resolved before its entry name is built, and the name uses the resolved format. |
||
|
Deluan Quintão
|
758e64c999 |
feat(scanner): per-library PID configuration (#6252)
* feat(model): add per-library PID config columns * refactor(metadata): pass PID config to ToMediaFile and add spec validation * feat(scanner): rescan only libraries whose PID config changed * feat(server): validate library PID config and rescan on change * feat(ui): edit per-library PID config * fix(ui): label the PID mode selects * fix: tighten per-library PID rescan edge cases An interrupted PID rescan no longer upgrades every library to a full scan, a save that loses the race for the scanner logs at debug, the confirm dialog only shows when the effective PID spec changes, and it now gets translation keys. * refactor(metadata): pass the library to ToMediaFile ToMediaFile and core.Inspect took the library ID and its PID config as separate arguments, so a caller could mix values from two libraries. They now take the model.Library and resolve the effective PID config from it. * chore: tidy per-library PID comments, PropTypes and migration Trim comments that restated the code, add PropTypes to the new UI components, and recreate the migration with make migration-sql. * fix(ui): show the PID spec help under its input * feat(cmd): make inspect use the file's library PID config inspect always used the global PID config, so it showed different IDs than the scanner for files in a library with an override. It now finds the file's library in the DB and uses its effective config, falling back to the global config when there is no DB or the file is outside every library. It never creates a DB. The library path matcher moves from core/playlists to model so both can use it. * refactor: simplify per-library PID code Share the DB-file check between CLI commands, move ErrAlreadyScanning to model so core no longer imports scanner, read the libraries once for insights, and let ValidatePIDSpec accept an empty spec and look tags up directly. In the scanner, use FullScanInProgress instead of a second flag, and skip recomputing album IDs when the album spec did not change. In the UI, share the PID inputs between Create and Edit, and use docsUrl. * feat(ui): add section titles to Library Create and pre-fill Custom PID specs Custom now starts from the global spec, so admins edit a working spec instead of typing one from scratch. * fix(inspect): map files with the library-relative path the scanner uses Inspect gave metadata the file's directory as typed, so folder-based PIDs never matched the DB. It now uses the path relative to the library root, through the scanner's helper, which moves to model. * fix(scanner): say when a PID rescan only covers target folders * fix: reject tag aliases in album PID specs and match root libraries Tags are stored under canonical names, so an alias in a spec always reads as empty. In an album spec that gives every album the same ID, so album specs now require the tag name. Track specs keep accepting aliases, since the default one uses them. LibraryMatcher now matches paths under a library at the filesystem root. * refactor(model): move the tag alias lookup to tag_mappings.go * test: run the library matcher and inspect tests on Windows Build test paths with filepath instead of Unix literals, so they use the OS separator like filepath.Abs output, and drop the Windows skips. * feat(ui): add pt-BR translations for per-library PID settings |
||
|
Deluan Quintão
|
4cdffd5633 |
feat(subsonic): OpenSubsonic API key authentication (#6219)
* feat(persistence): store hashed API keys on players * feat(core): refresh key-bound players without renaming them Add Players.Touch, which records usage for a player already identified by an API key without guessing its identity or overwriting its name. Register also stops renaming players that have an API key. Register no longer returns player save errors (or a stale FindMatch ErrNotFound when the save is rate-limited); save failures are only logged, and only the transcoding lookup error is returned, same as Touch. * feat(subsonic): authenticate with OpenSubsonic API keys Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com> * feat(subsonic): add tokenInfo and advertise apiKeyAuthentication * feat(server): add endpoints to generate and revoke player API keys * feat(ui): manage player API keys Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com> * fix(subsonic): throttle API keys per key and IP A stale key on one device exhausted the shared per-IP bucket and locked out every valid key from the same IP. The limiter only stores a hash of the bucket string, so the key is not retained. Also adds e2e coverage of API key auth through the real repository, and clarifies the player resolution log message. * fix(ui): keep the new API key dialog open until closed The key is shown only once, so Escape and backdrop clicks no longer dismiss it. Also clarifies when the key can be used as a password. * refactor: simplify API key code paths Share the player refresh tail between Register and Touch, fold the ownership-filtered write tail into execOwned, parse the query once for apiKey conflicts, derive HasAPIKey in the player mock, share the player form inputs between create and edit, and pick the delete button by key state instead of spreading conditional props. * feat(players): set API keys through the player record The key is a write-only apiKey field applied on save: required and owner-only on create, optional on edit, empty to revoke. Replaces the generate/revoke endpoints. * fix(players): reject API keys already in use Creating or editing a player with a key another player already has now returns a validation error instead of a 500, and a create that loses the race no longer leaves a keyless player behind. Ownership is checked before the key on create. * feat(ui): edit player API keys as a form field Replaces the show-once dialog, whose icon-less Close button was invisible on mobile. The key is generated in the browser, required and pre-filled on create. * fix(ui): keep new player API keys out of the record cache The json-server create response echoes the request body, and undoable edits merge the payload into the cache, so the key could reappear on the edit page. Strip it from the create result and save player edits pessimistically. Also fall back to a prompt when the clipboard write fails. * fix(ui): polish player API key field Set userId on the created player record so owner actions show immediately, and show a neutral no-key message to non-owners. * refactor: simplify player API key create and field Write the key hash in the create INSERT so the unique index settles races, re-read the created player instead of hand-building the cached record, reuse isWritable for the revoke check, and collapse the key field's derived state and generate/regenerate buttons. * fix(ui): let the API key field size like other inputs fullWidth is now opt-in instead of forced. * fix(ui): align the API key field with other player inputs Apply react-admin's input className, move the actions (now including Copy) below the field, and use a monospace font so the whole key fits. * fix(ui): redirect to the player list after create Matches the other create pages. * refactor(persistence): name the write-access rule for owned rows Owned-row writes now say which row they target and who may write it: ownedRow(rowID, ownerOrAdmin|ownerOnly) builds the WHERE, updateOwnedRow applies it, and SetAPIKey uses ownerOnly instead of a hand-built user_id filter. updateOwned/deleteOwned keep their signatures. * fix(players): apply an edit's key change and fields atomically Update now runs SetAPIKey and the column update in one transaction. Also shares the key format check, drops FindByAPIKey's unneeded empty-key guard, and sets the context username only on the apiKey path. * fix(subsonic): treat any credential param sent with apiKey as a conflict The spec requires error 43 when u, p, t or s is present with apiKey, even with an empty value. * refactor(subsonic): leave the player cookie code unchanged for key-bound requests Return early instead of wrapping the cookie block, so the diff (and CodeQL's view of it) matches master. * fix(subsonic): don't count key lookup errors as failed logins A database error while checking a key sent as the password now surfaces as a server error instead of a bad password, so it no longer feeds the failed-login limiter. * feat(players): use nds_ as the API key prefix Part of a Navidrome secret prefix family (nd + a letter for the kind), alongside ndg_ for API v1 grants. * feat(ui): make player API keys easier to find Label the Settings menu entry "Players & API keys", add an API key filter to the player list, show the key icon in the mobile list, and add Brazilian Portuguese translations for the new player strings. Signed-off-by: Deluan <deluan@navidrome.org> * feat(ui): always show the player API key filter Signed-off-by: Deluan <deluan@navidrome.org> * fix(ui): hide the unset Last Seen date in the player list Players created by hand have no last_seen yet, which showed as 12/31/1. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> Co-authored-by: amCap1712 <amCap1712@users.noreply.github.com> |
||
|
Deluan Quintão
|
c22ce9ebb2 |
feat(api): add the API v1 foundation behind DevAPIv1 (#6227)
* feat(api): add OpenAPI v1 spec skeleton, lint ruleset and bundle tooling vacuum v0.30.6's `bundle --composed` mangles component names for this spec's multi-file layout (duplicates Problem as Problem__schemas etc.), so api-bundle uses the Redocly CLI (npx @redocly/cli bundle) instead. * fix(api): pin the Redocly CLI version Tried moving components out of the root document (per libopenapi's nested_files example) so vacuum's own bundler could produce clean names, but any component declared via $ref inside components.* still gets a __<parent>-suffixed twin regardless of collisions elsewhere, so vacuum's --composed bundler can't cleanly bundle this spec. Pin the already-working Redocly fallback to an exact version instead of @latest. * fix(api): bundle the OpenAPI spec with vacuum vacuum's --composed bundler suffixes any component reached via a $ref written directly inside the root document's own components.* block, regardless of collisions elsewhere. Dropping the root-level schemas/ parameters/responses declarations (keeping only securitySchemes, and leaving every component file under api/openapi/components/ untouched) lets vacuum bundle cleanly with no __ suffixes, going back to Go-only tooling. Components nothing references yet (ListMeta, offset, limit, BadRequest, Unauthorized, Forbidden, NotFound) are absent from the bundle until a later task's operation references them. * fix(api): make spec lint rules cover all schemas and error codes nd-schema-property-descriptions targeted $.components.schemas, but our schemas live in path/response files, not the root document, so it was dead code; switched to $..properties[*] to walk every resolved schema wherever it ends up. nd-error-responses-are-problems only checked a hardcoded status-code list; switched to a patternProperties schema matching the full 4xx/5xx range. Also: api-diff now diffs against the merge-base with API_DIFF_BASE (falling back to its tip with a notice if no merge-base exists), gen no longer depends on api-gen until Task 3 wires up oapi-codegen, and api-lint suppresses vacuum's banner. * feat(api): embed the bundled OpenAPI spec and expose its version * feat(api): generate the v1 server interface with oapi-codegen * feat(api): add RFC 9457 problem responses for API v1 * feat(api): add API v1 router with /server discovery and spec routes * fix(api): serve the OpenAPI document without range support * feat(api): mount API v1 behind the DevAPIv1 flag * chore(ci): lint, regenerate and diff the OpenAPI v1 spec * refactor(api): tighten spec version access, lint rules and test naming * refactor(api): simplify spec routes, tests and OpenAPI tooling Share one If-None-Match parser (utils/req) between the image and spec routes, declare the YAML spec response as an object so tests need no decoder override, and reuse ETag/304 spec components. Install the OpenAPI tools only when missing or at a different version, fail api-diff when its base ref does not exist, and in CI cache the tools, fold regeneration into the go generate check, and fetch only the PR base commit for the breaking-change gate. * refactor(api): raise the list limit maximum to 2000 and drop the flag test * feat(api): treat added enum values as non-breaking Enums in API v1 are open: clients must accept unknown values. api-diff now downgrades response-property-enum-value-added to INFO, while removing a value from a request enum stays breaking. * feat(api): gate breaking changes on x-stability-level Every operation declares x-stability-level (alpha, beta, stable). oasdiff ignores breaking changes to alpha operations and rejects lowering a level, so unreleased endpoints can evolve while beta and stable ones stay additive. All current operations start as alpha. * feat(api): declare loginMethods as an enum Prefix generated enum constants with their type name so enums sharing a value (for example password) cannot collide in package apiv1. * feat(api): send Allow on 405 and answer HEAD wherever GET is routed chi only sets Allow in its default 405 handler, so the problem-format handler now builds it by matching each method against the v1 router. HEAD requests fall back to the GET route, as RFC 9110 expects. * refactor(api): hash the spec ETag with xxh3 The bytes are compiled in, and the digest was truncated to 64 bits anyway, so this matches the artwork ETags instead of paying for cryptographic strength we discard. * docs(api): explain the about:blank problem type * feat(api): make code the problem identifier and omit a blank type RFC 9457 says clients switch on the type URI, but no adopter surveyed ships both a populated type and a separate code. Declare code as an enum, and send type only once a problem has semantics of its own. * fix(api): advertise the configured base path in the served OpenAPI spec With BaseURL=/music the API is mounted at /music/api/v1, but the spec told clients to call /api/v1 at the host root. The server now rewrites servers[0].url to BasePath + /api/v1 when it serves the document. Relative server URLs were tested first: "." and "../v1" work in openapi-generator, Swagger UI and Redoc, but Scalar resolves them against the page origin, so it breaks even without a base path. The committed bundle keeps /api/v1, and a test pins that it appears exactly once, which the rewrite relies on. |
||
|
Deluan Quintão
|
bb7d81a5ea |
refactor(scanner): remove the unused legacy ffmpeg metadata extractor (#6231)
* refactor(scanner): remove the legacy ffmpeg metadata extractor The ffmpeg extractor in scanner/metadata_old has not been wired into the scanner since the taglib-only rewrite, so it was only exercised by its own tests. Remove the package, the FFmpeg.Probe method and its ffmetadata command that only it used, and the startup fallback for Scanner.Extractor="ffmpeg". Configs that still set it keep working: unknown extractors already fall back to taglib with a warning. * fix(conf): warn and fall back to taglib for an unknown Scanner.Extractor Validate the option when loading the config, so invalid values such as the removed "ffmpeg" extractor are reported once at startup instead of only when a library storage is created. |
||
|
Deluan Quintão
|
b293b96256 |
refactor(persistence): stateless repositories with per-call context (#6149)
* refactor(persistence): adopt generic deluan/rest repository API Pin deluan/rest to the refactor branch. REST-facing repository methods take a context and return typed values. Drop DataStore.Resource and ResourceRepository; the native API names typed repositories directly through a per-request adapter that later commits remove. * refactor(persistence): base repository helpers take a context * refactor(persistence): LibraryRepository takes a context per call * refactor(persistence): PropertyRepository takes a context per call * refactor(persistence): UserPropsRepository takes a context per call * refactor(persistence): TranscodingRepository takes a context per call * refactor(persistence): ShareRepository takes a context per call * refactor(persistence): PlayerRepository takes a context per call * refactor(persistence): RadioRepository takes a context per call * refactor(persistence): PlayQueueRepository takes a context per call * refactor(persistence): Tag and Genre repositories take a context per call * refactor(persistence): PluginRepository takes a context per call * refactor(persistence): Scrobble repositories take a context per call * refactor(persistence): FolderRepository takes a context per call * refactor(persistence): Artwork repositories take a context per call * refactor(persistence): UserRepository takes a context per call * refactor(persistence): ArtistRepository takes a context per call ReadAll no longer rewrites the shared sort mappings for the role filter; it works on a per-call copy. * test(persistence): assert artist role sort sanitization in ReadAll * refactor(persistence): AlbumRepository takes a context per call * test(persistence): pass the test context to album repository helpers * refactor(persistence): MediaFileRepository takes a context per call * refactor(persistence): Playlist repositories take a context per call * refactor(persistence): build all repositories once per store * refactor(core): REST repository wrappers are built once * refactor(persistence): repositories are stateless Remove the context field from the base repository and the per-request REST adapter. Enable the containedctx linter so no repository can hold a request context again. * chore(lint): skip containedctx in test files * refactor: share simplifications from the stateless repositories sweep Add deleteOwnedAll on sqlRepository and use it in player/share Delete to remove the duplicated bulk-delete loop; have Share.Repository() return model.ShareRepository so subsonic sharing.go drops its repeated type assertions. * chore(core): assert REST wrappers implement Persistable * chore: reformat imports * perf(persistence): build repositories on first use Each transaction store used to construct all 21 repositories up front, paying for filter and sort mapping setup the block never touched. Fields are now sync.OnceValue thunks, so a store only builds what it uses. * fix(persistence): clean plugin references per deleted user A bulk user delete that fails on a later id had already removed the earlier rows but skipped their plugin cleanup. Cleanup now runs right after each successful delete. * fix(core): unload disabled plugins even when a user delete fails A bulk delete can fail on a later id after earlier users were removed and their plugins auto-disabled. The wrapper returned before unloading, leaving those plugins running until the next successful delete or a restart. * chore(deps): pin deluan/rest to v1.0.1 Replaces the pseudo-version of the refactor branch with the tagged release. REST error messages now name the bare type (Artist, not model.Artist). * test: use the spec context instead of context.Background() Replace the context.Background()/context.TODO() calls this branch added to tests with the spec's ctx, GinkgoT().Context(), or t/b.Context(), so repository calls are bound to the running spec's lifetime. * test: declare the spec context once per Describe Set ctx from GinkgoT().Context() first in each top-level BeforeEach and reuse it, building user contexts on top of it instead of repeating inline calls. |
||
|
Deluan Quintão
|
659d067aba |
fix(archiver): give same-named albums their own folder in artist zips (#6225)
* refactor: add Tags.First and slice.GroupOrdered helpers
Tags.First returns the first value of a tag or an empty string, replacing the inline len-check-then-index pattern in FullTitle, FullAlbumName, Album.FullName and the Subsonic album version mapping.
slice.GroupOrdered is slice.Group returning the groups in first-seen order, for callers that need a deterministic order the map-based Group cannot give.
* fix(archiver): give same-named albums their own folder in artist zips
Artist zips put every album in a folder named after the album, so two albums with the same name (an original and a deluxe edition, or names that only differ in characters the sanitizer replaces) were merged into one folder, with tracks mixed together and duplicate zip entries when file names collided.
The folder is now named after FullAlbumName(), so with AppendAlbumVersion on (the default) the version is part of the name, matching what clients display. Albums whose sanitized names still clash get a " [suffix]" taken from the first field that has a distinct, non-empty value for all of them: album version, year, release type, record label, catalog number, then a short album id. This follows the shape of beets' %aunique{} path function.
Albums are also grouped with slice.GroupOrdered instead of a map, so the zip is deterministic.
* fix(archiver): use the release year to tell same-named albums apart
Taggers often write an edition's date to the Date tag next to an original date, and the scanner then stores the original year in Year and the edition's year in ReleaseYear. Reissues of the same album therefore share Year, so the year disambiguator could not tell them apart and they fell through to the album id suffix. Prefer ReleaseYear and fall back to Year when it is not set.
Found by downloading an artist zip from a live server built from this branch.
* fix(archiver): let one clashing album keep the plain folder name
A disambiguator was only accepted when every clashing album had a non-empty value, so an original and its deluxe edition (with the version not appended to the name) fell through to the album id suffix. Accept a field whose values are distinct across the group even when one of them is empty, as beets' %aunique{} does: that album keeps the plain name, which the suffixed folders cannot clash with. Two or more empty values still count as a tie.
* test(archiver): refactor tests for album naming conventions and query order
|
||
|
Deluan Quintão
|
3f89baaec8 |
test(server): make the handleM3U spec independent of spec order (#6221)
It relied on another spec having set auth.PublicTokenAuth, so it panicked whenever Ginkgo ran it first. |
||
|
Deluan Quintão
|
69b496383d |
docs(jellyfin): refresh the README's known limitations (#6220)
* docs(jellyfin): refresh the README's known limitations Rewrite the Known limitations list against the current code and Jellyfin 12.1, dropping stale entries (synthetic blurhash, unchecked artist access, global genres) and adding the real gaps: search skipping filters, the one-character minimum, the 2,000-item search cap, position-based playlist entry ids, the rating param, missing endpoints and unemitted Fields. Also move the lyrics description into its own section, add the missing routes and filters to the endpoint table, and drop the stale artist-access TODO in resolveItemByID. * docs(jellyfin): list MaxConcurrentStreams env var and all e2e stubs |
||
|
Adrián Sánchez Zapico
|
27483a46dc |
fix(server): fail startup on initial setup errors and fix JSON/M3U response headers (#5897)
* fix(server): stop swallowing errors and correct two response bugs
Four independent bugs found while reviewing the HTTP layer:
initial_setup.go: createInitialAdminUser assigned the users.Put error to a
shadowed err, so the outer err (always nil by then, since a CountAll failure
panics) was returned instead. A failure to create the admin user was reported
as success, and initialSetup went on to commit the "setup complete" property
in the same transaction — so no admin user existed and initial setup was
skipped on every later boot.
auth.go: createAdminUser logged the Put error but returned nil, so createAdmin
fell through to doLogin and answered 401 "Invalid username or password"
instead of surfacing the real failure. It also logged the whole model.User,
which puts the new admin's password in the log in clear text; every other call
site logs user.UserName.
native_api.go: writeDeleteManyResponse did not return after http.Error when
marshaling failed, then wrote a nil body over the 500. It also built the
single-id body by hand with html.EscapeString, which does not escape
backslashes, so an id ending in one produced `{"id":"a\"}` — invalid JSON.
Both shapes now go through json.Marshal. A failed Write is now logged rather
than answered with http.Error, which could not work once the body had started.
handle_shares.go: handleM3U set Content-Type after WriteHeader, so it was
never sent and shared playlists were served with a sniffed type.
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
* fix(server): address review feedback
- writeDeleteManyResponse uses rest.RespondWithJSON, so the response now
has Content-Type: application/json. This also removes a marshal error
branch that could never run.
- createInitialAdminUser returns the CountAll error instead of panicking,
and wraps its errors. initialSetup now stops the server with log.Fatal
when setup fails. Before, the error was dropped and the server started
with a half-done setup.
- Trim comments that described PR history.
---------
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
Co-authored-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan Quintão
|
39028f65c8 |
fix(jellyfin): honor IsPublic when creating a playlist (#6204)
POST /Playlists dropped the client's IsPublic flag, so every playlist was created private. JellyBox Player's create-playlist form defaults its "public" checkbox to true, so JellyBox users could never create a public playlist. Upstream's PlaylistsController passes IsPublic into PlaylistCreationRequest. core/playlists.Create has no visibility parameter and widening it would ripple into the Subsonic and native APIs, so createPlaylist follows the same pattern updatePlaylist already uses: after Create succeeds, a non-nil IsPublic is applied with a follow-up Update. The field is a pointer so an absent one keeps today's default instead of forcing private. If that second write fails the handler surfaces the error through playlistError rather than returning the id: answering 200 for a playlist that is not as visible as the client asked is the same silent drop this fixes. |
||
|
Adrián Sánchez Zapico
|
a3f41fb422 |
sec(server): sanitize user-controlled filenames in Content-Disposition (#5895)
* sec(server): sanitize user-controlled filenames in Content-Disposition
Playlist export, Subsonic download and public share download built the
Content-Disposition header by interpolating a user-controlled name into a
quoted-string with fmt.Sprintf. A name containing a double quote closes the
string early and the rest is parsed as additional parameters, so a playlist
named `party"; filename="evil.html` yielded
attachment; filename="party"; filename="evil.html.m3u"
letting whoever chose the name decide what the browser saves the download as.
The names come from playlists, album/artist names and media file tags.
Go's net/http already rewrites CR and LF in header values to spaces, so
response splitting was not reachable; parameter injection was.
Add str.ContentDispositionAttachment, which emits a sanitized ASCII-only
quoted `filename` plus an RFC 5987 `filename*` carrying the original UTF-8
name, and use it at all four call sites. The `filename*` parameter also fixes
non-ASCII names, which previously went out raw or were mangled by sanitizing.
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
* fix(server): keep download names intact and sanitize filename*
Rework ContentDispositionAttachment after review. Names with no ASCII
letters now fall back to download.<ext> instead of a bare extension
(東京.mp3 gave filename="mp3"). filename* is built from the same
sanitized name as the ASCII fallback, so path separators, reserved
characters, control and bidi characters, and invalid UTF-8 no longer
reach it. The ASCII fallback transliterates accents and typographic
punctuation (Legião -> Legiao, She’s -> She's) through the existing
sanitize.Accents and str.Clear helpers, keeps leading dots, and only
trims trailing ones. Names are capped at 255 bytes, keeping the
extension.
Pure ASCII names now get only the quoted filename parameter, so the
header for them matches the previous output byte for byte. filename*
is encoded with mime.FormatMediaType instead of a hand-written RFC 5987
encoder. Adds tests for the M3U export and Subsonic download headers.
* fix(server): handle dot-only names and long fake extensions
A name made only of dots trimmed down to an empty filename. It now
falls back to download, like an empty stem does.
path.Ext treats anything after the last dot as the extension, so a long
suffix with no real extension was kept whole and replaced the stem with
download, going past the 255-byte cap. Suffixes longer than 16 bytes
are now treated as part of the stem and truncated with it.
Neither case is reachable from the current call sites, which always
append a short extension.
---------
Signed-off-by: zapisanchez <zapisanchez@gmail.com>
Co-authored-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan
|
6b3938b5b6 |
fix(subsonic): warn when a nowPlaying scrobble sends multiple ids
The scrobble endpoint accepts multiple ids, but a nowPlaying notification (submission=false) describes a single track, so only the first id is used. The extra ids were dropped silently, which made client bugs invisible. Log a warning instead, keeping the existing behavior for clients that rely on it. |
||
|
Deluan Quintão
|
c3b9b4ecb3 |
fix(subsonic): rate limit failed authentication attempts (#6185)
* fix(subsonic): limit failed authentication attempts per client IP and username The Subsonic API checked credentials on every request with no limit on failures, so any account could be brute-forced over /rest/*. Failed u+p, t+s and jwt attempts are now capped per (client IP, lower-cased username) using AuthRequestLimit and AuthWindowLength, the same settings that guard the UI login. Every request carries credentials, so only failures count: a slot is taken before the check and given back on success or on a server error, which also stops concurrent guesses from overshooting the limit. Blocked attempts get the same response as a wrong password (HTTP 200, error code 40, no Retry-After), so an attacker cannot tell a block from a wrong guess. Reverse proxy and internal authentication are not limited. The client IP helper behind ClientIPRateLimiter is now exported as server.ClientIP, so spoofed forwarding headers cannot open a fresh bucket. * refactor(subsonic): simplify failed authentication limiter Release the limiter slot from a single place in authenticate(), after the user lookup and credential check, instead of separately in the canceled branch. Store attempt counters by value instead of by pointer, and drop limiter unit tests that only repeated the middleware specs. * fix(subsonic): wait for an in-flight auth check instead of rejecting Slots were reserved before the credential check and only released afterwards, so once AuthRequestLimit checks for the same client IP and username overlapped, the next request was answered with error code 40 even when its credentials were valid. Clients that fan out parallel requests hit this constantly: a burst of six valid logins lost one, a burst of fifty lost forty five, and the web UI authenticates its own /rest calls the same way. A key now carries a slot channel of AuthRequestLimit capacity, and a request waits on it rather than failing when other checks for that key are in flight. Failures are recorded after the check, and a request is only rejected when the key already reached the limit within the window. A waiting request gives up if its context is canceled. Concurrent guesses still cannot run unchecked: at most AuthRequestLimit checks run at once and the rest are turned away as soon as the failures land. * docs(subsonic): state the real guess ceiling of the auth limiter The comment claimed a burst cannot overshoot, which reads as a hard cap of AuthRequestLimit. Allowing concurrent checks means a window admits up to 2*limit-1 guesses, so say that instead. |
||
|
Deluan Quintão
|
8e784b6af7 |
fix: apply the per-user library filter to bookmarks, playlists and now-playing (#6179)
On a multi-library instance, a few reads and writes built their own queries without the per-user library filter that every other media read applies. A user granted only some libraries could see, and store, tracks from libraries they had no access to. - getBookmarks now filters the query. It has to be the query and not the result: the loop below it pre-sizes the response from the bookmark count, so a row dropped afterwards would emit an empty bookmark entry. - createBookmark rejects an id the caller cannot read, returning error 70 to match getSong. Stored rows are left alone rather than purged, so a temporary revoke does not lose saved playback positions. - playlistTrackRepository Read, Count and GetAlbumIDs get the filter their siblings CountAll and GetMediaFileIDs already had. Read is the one that mattered most: its id is the integer playlist position, so it needed no track id at all. - Playlist track writes are filtered in playlistRepository.addTracks, the only writer of playlist_tracks rows apart from smart playlists, so Add, Insert, AddAlbums/AddArtists/AddDiscs and a full replace through Put all go through it. Insert reserves a slot per requested id, so when the filter drops one it renumbers to close the hole. - playTracker.GetNowPlaying honours its context instead of discarding it. The cache is process-global, so the filter belongs in the tracker rather than in the Subsonic handler, and any future caller inherits it. Admins and single-library installs are unaffected: applyLibraryFilter and HasLibraryAccess both short-circuit for them. Scanner playlist sync runs as admin, and M3U and CLI imports already resolve tracks through FindByPaths as the same user, so neither changes. |
||
|
Deluan
|
672c0af580 | docs(README): update Docker networking instructions for UDP discovery | ||
|
Deluan Quintão
|
fb45ad7b9c |
fix(auth): ExtAuth logout redirect on unauthenticated loads, and warning spam from untrusted sources (#6176)
* fix(ui): only redirect to ExtAuth logout URL for proxy-authenticated sessions react-admin calls authProvider.logout() when the boot-time checkAuth fails and after a 401, not only when the user clicks Logout. With ExtAuth.LogoutURL set, every unauthenticated page load (e.g. direct LAN access that bypasses the auth proxy) was sent to the IdP sign-out page and the login form was never shown. Redirect only when the page was authenticated by the reverse proxy (config.auth is present). Other sessions fall back to the login form. Fixes #6175 Signed-off-by: Deluan <deluan@navidrome.org> * fix(server): only warn about untrusted ExtAuth sources when the header is sent UsernameFromExtAuthHeader checked the source IP before looking for the user header, so every request from an IP outside ExtAuth.TrustedSources logged a warning, even when it carried no header at all. With direct LAN access alongside a forward-auth proxy, a single polling client produced a constant stream of warnings (twice per Subsonic request, since the middleware chain resolves the username in both checkRequiredParameters and authenticate). Look for the header first and warn only when an untrusted source actually sends it, which is the case worth seeing: a misconfigured proxy or a spoof attempt. Signed-off-by: Deluan <deluan@navidrome.org> --------- Signed-off-by: Deluan <deluan@navidrome.org> |
||
|
Deluan Quintão
|
b76ae14286 |
feat(jellyfin): add Quick Connect sign-in (#6174)
* feat(jellyfin): add Quick Connect sign-in Jellyfin clients can now sign in without a password: the client shows a 6-digit code, a signed-in user approves it, and the client redeems a secret for its access token. - core/quickconnect: in-memory store shared by both routers through wire. Codes expire after 10 minutes; a secret redeems only once (Jellyfin allows repeats for 10 minutes); at most 1000 pending requests. - Jellyfin API: Initiate, Connect, Authorize and AuthenticateWithQuickConnect. Admins may approve for another user via UserId, like Swiftfin's admin page. Initiate and redeem share the login rate limiter; Connect does not, since Finamp and Streamyfin poll it every second. - Web UI: a Quick Connect item in the user menu looks up the code and shows the app and device before approving, so a user can't be tricked into approving an unknown device blindly. - Jellyfin.QuickConnect option, on by default like Jellyfin. It only matters when the Jellyfin API is enabled. * refactor(jellyfin): tidy Quick Connect naming and route guards Group the Quick Connect routes under one requireQuickConnect guard, make the request's device a named field so req.Device.ID can't be mistaken for a request id, and rename the web API response type to quickConnectDevice. * refactor(jellyfin): remove duplicated Jellyfin date formatting function * test(jellyfin): set play count and starred in the song fixture literal * refactor(jellyfin): inline the Quick Connect redeem body and use the shared date helper * fix(jellyfin): bound the client fields Quick Connect keeps in memory Initiate is unauthenticated and keeps the Client, Device, DeviceId and Version header fields for up to ten minutes. With no header size limit, each pending request could hold about 1 MB, and even a short field kept the whole header alive because the parsed values are substrings of it. Reject fields over 512 bytes and copy the stored values. Also answer 500 instead of 401 when the redeem user lookup fails for a reason other than the user being gone. * fix(jellyfin): rate-limit Quick Connect code approval Any signed-in user could try codes without limit on the Jellyfin Authorize endpoint and the web UI lookup/authorize endpoints, and so could approve another person's pending device for their own account. Apply the same per-IP limiter as the login (AuthRequestLimit/AuthWindowLength) to both surfaces. |
||
|
Deluan Quintão
|
fed35e08de |
feat(jellyfin): add opt-in LAN auto-discovery (#6169)
* feat(jellyfin): compute the address advertised by auto-discovery * fix(jellyfin): use TLSEnabled and path.Join for the auto-discovery address * feat(jellyfin): answer LAN auto-discovery broadcasts * test(jellyfin): e2e check that auto-discovery matches the public server identity * feat(jellyfin): add opt-in AutoDiscovery option and start the listener * fix(jellyfin): close the auto-discovery socket on read errors and quiet reply failures * refactor(jellyfin): build the auto-discovery address with publicurl and log bind failures in place discoveryAddress now hands only the discovery-specific part (the requester-facing host) to publicurl.AbsoluteURL, so the BaseURL, scheme and BasePath rules live in one place. ServeDiscovery logs its own bind failure and returns nothing, so the caller cannot route the error into the server errgroup. Tests use a DescribeTable and no longer wait on a fixed timeout to prove a packet was ignored. * refactor(jellyfin): run auto-discovery as its own service in the main errgroup Discovery is now a small type that needs only a DataStore, started by startJellyfinDiscovery like the other background services, so the errgroup waits for it on shutdown and startServer is back to a one-line mount. The server id resolution moved to resolveServerID with a package-level lock: the Router and Discovery are separate objects, and a per-Router lock would let them persist two different ids on first boot. * refactor(jellyfin): build Discovery through wire and drop the serverName forwarder startJellyfinDiscovery now follows its siblings: negative guard with a DISABLED debug log, and the service comes from a CreateJellyfinDiscovery wire injector instead of an inline constructor. The Router.serverName method only forwarded to the package function, so its four call sites call the function directly. * fix(jellyfin): skip auto-discovery for unix socket servers without a BaseURL host With Address set to a unix socket nothing listens on Port, so the route-facing IP plus Port pointed clients at a dead URL. Discovery now logs a warning and does not start in that mode unless BaseURL names the proxy host, which AbsoluteURL already advertises as-is. * docs(jellyfin): note which address auto-discovery advertises on restricted binds Also stop using a hostname Address in the fallback spec: a hostname like localhost binds a single interface, so it is not an example of the route-facing fallback being right. An empty Address is. |
||
|
Deluan Quintão
|
549dfa7f30 |
feat(jellyfin): advertise Jellyfin 12.1.0 and add the missing 12.x quick wins (#6163)
* feat(jellyfin): advertise Jellyfin server version 12.1.0
Streamyfin, jellyfin-android and jellyfin-androidtv refuse servers older than
10.10, Swiftfin warns below 12.0, and @jellyfin/sdk flags anything below its
minimum as unsupported, so 10.9.11 locked those clients out. 12.1.0 is the
current Jellyfin release (after 10.11 Jellyfin renumbered to 12.0). No client
checked has an upper bound or assumes the major is 10, and the value keeps
three parts because the Kotlin SDK and Swiftfin reject two-part versions.
* feat(jellyfin): acknowledge POST /Sessions/Playing/Ping
Jellyfin clients ping this endpoint to keep a transcode job alive while
paused. Navidrome ties transcodes to the stream request, so there is nothing
to keep alive; answer 204 like Jellyfin instead of a 404. It shares one no-op
handler with Sessions/Capabilities, renamed to acknowledge.
* feat(jellyfin): reorder playlist entries via Items/{entryId}/Move
Adds POST /Playlists/{id}/Items/{entryId}/Move/{newIndex}, which clients use
to reorder playlists. It maps the entry's PlaylistItemId (its position) and
Jellyfin's zero-based newIndex onto the existing core ReorderTrack, which
enforces ownership. As in Jellyfin, an index past the end appends and an
unknown entry is a no-op; out-of-range positions never reach Reorder, which
would otherwise shift unrelated rows.
* feat(jellyfin): honor position when adding items to a playlist
POST /Playlists/{id}/Items takes an optional zero-based position (added to
the Jellyfin spec in 12.0). Match Jellyfin: zero or negative prepends, past
the end appends, otherwise the new items are inserted at that index in the
order they were added.
Adds PlaylistTrackRepository.Insert and core playlists.InsertTracks, which
shift the following entries and insert in one transaction, instead of
appending and moving each new track with its own ReorderTrack call.
* feat(jellyfin): add type-specific InstantMix routes
Jellyfin exposes InstantMix under Songs/, Albums/, Artists/ and Playlists/
as well as Items/, plus the legacy Artists/InstantMix and
MusicGenres/InstantMix forms that take the seed as ?id=. Clients generated
from the Jellyfin SDKs call the type-specific routes, which 404ed. All of them
now share the existing Items/{id}/InstantMix handler; the external provider
already builds mixes from song, album, artist, playlist and genre seeds.
* feat(jellyfin): honor Width, Height and Fill* image size params
The image endpoint only read MaxWidth/MaxHeight, so clients that size covers
with fillWidth/fillHeight (Manet, Finamp) or width/height got the full-size
original on a cold artwork cache: a 578 KB PNG instead of a 13 KB resize.
Jellyfin applies Width/Height, caps them with MaxWidth/MaxHeight, then
shrinks to the smallest size that still covers the Fill box. Navidrome
resizes on one dimension, so the tightest bound wins and a fill box counts
as its larger side.
* feat(jellyfin): answer HEAD on audio, file and image routes
Fintunes sends HEAD to /Audio/{id}/universal to read the content type and
detect direct play (a Content-Length means direct play), and to the audio and
image URLs before a download, aborting the download when it fails. Those routes
were GET-only, so HEAD got a 404. HEAD now reuses the GET handlers. Direct
play goes through Stream.Serve, which already answers HEAD; a transcode
answers with the target content type and no length without starting ffmpeg,
so a probe never costs a transcode.
* fix(jellyfin): clamp playlist move and insert positions before adding one
movePlaylistItem computed min(newIndex+1, SongCount): newIndex=MaxInt wrapped
to a negative position, and Reorder then left the playlist with a gap
(positions 2, 3, 999998), making the moved entry unmovable. Clamp against the
playlist length first. addToPlaylist's min(position, MaxInt32)+1 wrapped on
32-bit builds and req.Int truncated large values there, so a far-past-the-end
position prepended; parse as int64 and clamp before converting.
* fix(playlists): validate reorder positions inside the write transaction
Reorder never checked its positions, so a source outside the playlist or a
destination past its end shifted rows around a missing entry and left a gap
(e.g. ids 1 and 3), which made the moved entry unmovable afterwards. The
Jellyfin Move handler guarded this with a SongCount read before ReorderTrack,
but a concurrent removal between the two reopened it, and the native API
reorder endpoint passed client positions through unchecked.
Reorder now reads the last position in the same transaction, returns
ErrNotFound for a source outside the playlist and clamps the destination.
ReorderTrack uses an immediate transaction so that read and the updates are
atomic against other writers. The Jellyfin handler drops its pre-check and
maps ErrNotFound to Jellyfin's no-op 204; the native API now answers 404 for
an unknown track instead of corrupting the order.
|
||
|
Deluan Quintão
|
16567f147b |
fix(jellyfin): match Jellyfin on login SessionInfo, item types and universal streams (#6161)
* fix(jellyfin): send SessionInfo on login so JellyBox gets past sign-in
JellyBox parses AuthenticateByName's SessionInfo as a required object and
fails silently when it is missing, leaving the user on the login screen.
Real Jellyfin always sends it (SessionManager.AuthenticateNewSessionInternal,
10.10.7 and master), so the login response now carries a full SessionInfo
built from the user and the MediaBrowser auth header. It includes every field
JellyBox (Id, PlayState) and Finamp (UserId, LastActivityDate, the activity
and control bools, PlayState's CanSeek/IsPaused/IsMuted) require once the
object is present. The session Id is derived from client and device id, so
repeated logins from one install share it.
* fix(jellyfin): ignore IncludeItemTypes names that aren't Jellyfin kinds
JellyBox opens an album with ParentId=<album>&IncludeItemTypes=music. Music
is not a BaseItemKind, and Jellyfin's comma-delimited binder drops values it
cannot parse, so real Jellyfin treats the request as having no type filter and
lists the album's tracks. Navidrome returned an empty list, so every album
opened empty. Entries that aren't BaseItemKind names are now dropped before
type resolution, so an all-unknown list behaves like an absent one. Real kinds
Navidrome doesn't serve, such as Boxset, still return nothing.
* fix(jellyfin): treat universal Container as the direct-play list
On /Audio/{id}/universal, Container lists the "container|codec" entries the
client can direct play, and TranscodingContainer/AudioCodec name the target
when it can't (UniversalAudioController builds DirectPlayProfiles from it).
Navidrome passed the whole list to the decider as one target format, which
matched nothing and fell back to DefaultDownsamplingFormat, so JellyBox got
every MP3 transcoded to Opus. /universal now has its own handler: a source
matching an entry keeps its format (still downsampled under a bitrate cap),
anything else is transcoded to TranscodingContainer, then AudioCodec. The
/stream routes keep treating Container as the target format.
* refactor(jellyfin): let the stream decider resolve universal requests
streamUniversal matched the Container list itself with plain string equality
and then asked the legacy resolver for the source format. That skipped the
decider's container and codec aliases (mp4 vs m4a, ogg vs opus), and a
direct-playable source over the bitrate cap was transcoded to its own format
instead of the client's TranscodingContainer.
The shared part of ResolveRequest (server-side player override, player
MaxBitRate cap, decision to Request mapping) moves to a resolve helper, and a
new ResolveClientRequest exposes it for callers that build their own
ClientInfo. streamUniversal now turns Container into DirectPlayProfiles and
TranscodingContainer/AudioCodec into a transcoding profile, so the decision
uses the same rules as the Subsonic getTranscodeDecision path. streamFile and
the /stream routes share a serveStream helper, NewSessionInfo reads the clock
itself, and duplicate comments and tests are trimmed.
|
||
|
Deluan Quintão
|
18205366c8 |
fix(jellyfin): match Jellyfin's item payloads so strict clients can sync (#6151)
* fix(jellyfin): match Jellyfin's item payloads so strict clients can sync Manet (iOS/macOS) aborted its whole library sync on the first item that was missing a key its decoder requires, leaving the library empty (#6147). Every gap was a field real Jellyfin always sends: - dates now use .NET's round-trip layout with 7 fractional digits, which Manet requires and plain RFC3339 failed - playlists carry SortName/DateCreated, and every item carries MediaType, ImageTags, ChannelId and, when Fields asks, Genres/GenreItems/Tags - albums carry Artists and LocationType; songs always carry HasLyrics - the library view is a full CollectionFolder (ChildCount, DateCreated, SortName, Path, LocationType, UserData), read from the library rows rather than the user projection, which has no counts - IncludeItemTypes matches case-insensitively and returns nothing for Jellyfin kinds Navidrome has none of, instead of falling back to every album Verified against a real Jellyfin 10.10.7 server and a live Manet client. * refactor(jellyfin): fold the repeated empty-list defaults into one helper The three mappers each initialised Genres/GenreItems/Tags the same way, and the e2e suite grew three near-identical specs walking every item type. Both now go through a single helper and one table. * refactor(jellyfin): fill the always-present item fields at the serialization edge The defaults real Jellyfin puts on every item were spread across three mappers, so item types nobody had tested yet (playlists, genres, the library view) still shipped payloads a strict client rejects. stampItem now takes the request's Fields and fills them for every item, which is provably the only path to JSON. Also drops the hand-copied BaseItemKind list: only an absent IncludeItemTypes defaults to albums now, so any type Navidrome does not serve returns nothing, as it would from Jellyfin. The synthetic playlists folder matches case-insensitively like the rest, /Items/{libraryId} reads the full library row instead of the count-less user projection, and PremiereDate and LastPlayedDate go through jellyfinDate rather than spelling the layout out again. |
||
|
Deluan Quintão
|
3f4b6a642c |
fix(server): return 404 instead of 500 for missing native API resources (#6131)
* fix: return 404 instead of 500 for missing native API resources The deluan/rest controller only maps rest.ErrNotFound to 404, comparing with ==. Most repositories return model.ErrNotFound, which had the same message but was a different value, so requesting a missing playlist, album, artist, song, radio, player, transcoding or library returned 500. This also applied to other users' private playlists. Make model.ErrNotFound the same value as rest.ErrNotFound. This fixes every REST route at once, with no per-route wrapping. errors.Is checks against either error keep working, and nothing wraps model.ErrNotFound before it reaches the controller. Fixes #6130 * fix(radio): return not found when deleting a missing radio station radioRepository.Delete used the shared delete helper, which never reports a missing row because SQL DELETE on zero rows is not an error. Deleting an unknown id silently succeeded: DELETE /api/radio/{id} returned 200, and the Subsonic deleteInternetRadioStation endpoint returned ok. Delete now checks the affected row count and returns model.ErrNotFound when nothing was deleted. The native API returns 404, and deleteInternetRadioStation returns error 70 (data not found). This matches Subsonic 6.1.6, gonic (both verified live) and Ampache (verified in source). Airsonic-Advanced does not implement this endpoint. The shared delete helper is unchanged, as several callers rely on deletes of absent rows succeeding. * fix(ui): return 404 for missing files that are not missing or do not exist missingRepository.Read filtered media files by bare "id" and "missing" columns. The media file query joins the library table, so SQLite rejected the query as ambiguous and GET /api/missing/{id} returned 500. Read now loads the file with MediaFileRepository.Get, which qualifies the column, and returns not found when the file does not exist or is not marked missing. * fix: report missing rows on single-item deletes and adopt deluan/rest errors.Is Bump github.com/deluan/rest to the version whose controller matches errors with errors.Is and errors.As. model.ErrNotFound stays the same value as rest.ErrNotFound, so the many hand-written conversions from model.ErrNotFound to rest.ErrNotFound in repositories, core services and test mocks did nothing. Remove them, along with the duplicate rest.ErrNotFound check in the Subsonic error mapper. Mappings from model.ErrNotAuthorized stay, as those are different errors. User and transcoding deletes had the same silent success as radio: the shared delete helper never reports a missing row, so their not-found checks never fired and DELETE /api/user/{id} and /api/transcoding/{id} returned 200 for unknown ids. Add deleteByID, which returns model.ErrNotFound when no row matched, and use it for radio, user and transcoding. Also drop the dead sql.ErrNoRows branch from delete, since a DELETE never returns it. The Subsonic deleteUser endpoint is not implemented (501), so this does not change the Subsonic API. Plugin deletes keep the silent helper: there is no REST route for them, and the plugin manager only deletes rows it just read. * chore: drop ErrNotFound comment and its identity test The alias to rest.ErrNotFound is self-explanatory, and the identity test only restated the declaration. * refactor: alias model.ErrNotAuthorized to rest.ErrPermissionDenied Like ErrNotFound, make model.ErrNotAuthorized the same value as the rest library's error, so REST endpoints map it to 403 directly. This removes the ErrNotAuthorized to rest.ErrPermissionDenied mappings in the library and playlist REST adapters and the duplicate check in the Subsonic error mapper. Handlers that check model.ErrNotAuthorized now also recognize rest.ErrPermissionDenied returned by repositories, so writePlaylistError, the image upload handlers and the public share handler return 403 for it instead of their fallback status. The error message changes from "not authorized" to "permission denied". |
||
|
Deluan Quintão
|
2dc0983629 |
fix: miscellaneous fixes for shares, artwork resize, auth limits, and watcher start (#6098)
* fix(artwork): cap declared image dimensions before resizing resizeStaticImage decoded the image with a raw image.Decode, so a small file declaring huge dimensions (e.g. a PNG header claiming 50k x 50k) forced a multi-gigabyte allocation on the serve-time resize path. The processor already guards its own decodes with decodeCapped; use it here too so the same 64M pixel cap applies to uploaded and sidecar images served through the cache. * fix(share): validate every resource ID and reject mixed types when saving Save only resolved the first ID in ResourceIDs to pick the resource type; the remaining IDs were never checked. A non-existent or hidden entity could ride along behind a valid first ID, and IDs of different kinds were accepted as one share. Resolve every ID as the current user and require all of them to be the same kind, returning ErrNotFound or ErrValidation otherwise. * fix(share): scope album and media file shares to the owner's libraries loadMedia already loaded artist and playlist shares as the share owner, but album and media_file shares used the repository context. Public share rendering carries no user, so the library filter was skipped and the share listed albums and tracks from libraries the owner cannot access. Streaming was already blocked, so only metadata leaked. Use ownerContext for all resource types. * fix(server): limit login payload size and surface first-admin creation errors The unauthenticated /login and /createAdmin handlers decoded the request body with no size limit. Add a body-limit middleware to the /auth route group that caps the payload at 8KiB, which is plenty for a username and password. Also make createAdminUser return the datastore error instead of logging it and returning nil, which previously let createAdmin proceed to a login attempt for a user that was never saved. * fix(conf): create the log file readable only by the owner The log file was created with mode 0644, so other local users could read it. Logs can contain usernames, paths and, at trace level, request details, so create it with 0600 instead. Existing files keep their current mode. * fix(lastfm): stop logging the auth token when fetching the session key fails The Last.fm callback token was written to the log as a structured field on failure. The redaction hook only matches value patterns, so it was not masked. Drop the field; the request ID is enough to correlate the failure. * fix(db): allow a music folder path containing a single quote on fresh databases The library table migration interpolated conf.Server.MusicFolder into the SQL with fmt.Sprintf, so a path such as /music/Rock 'n' Roll produced invalid SQL and the migration failed on a brand new database. Bind the path as a parameter instead. * fix(scanner): return an error when the folder watcher cannot start When notify.Watch failed, the watcher goroutine logged the error and exited, but never signalled the started channel, so Start blocked until its context was cancelled and left the watching flag set. Call notify.Watch before spawning the event loop, so Start returns the error right away, the started/failed signalling goes away, and the storage can be watched again later. * fix(jellyfin): limit the login request body size The Jellyfin AuthenticateByName endpoint decoded its JSON body with no size limit, the same gap the native /auth routes had. Export the login body-limit middleware from the server package and apply it to the Jellyfin login route, before the optional per-IP rate limiter, so both unauthenticated login surfaces share the same 8KiB cap. * fix(scanner): share one scanner instance across all injectors Each wire injector built its own scanner controller, so the Subsonic and native API routers held a different instance from the ones used by the startup scan, the periodic scan, the folder watcher and the SIGUSR1 handler. Status reads the in-progress file and folder counters from its own instance, so getScanStatus reported scanning=true with count=0 for every scan not started through the API. Verified live with a startup scan: master reports count 0 while scanning, this branch reports the real counts. Expose the controller through a singleton, as the watcher, broker and play tracker already are, and wire everything to it. New stays available for tests that need isolated controllers. * fix(share): do not panic when a media file share has no visible tracks Share.CoverArtID picked a random track for media file shares without checking that any track was loaded. The tracks are empty when the files went missing, were deleted, or the owner lost access to their library, and the public share page then panicked inside the random pick and returned a 500. Return an empty artwork ID instead, so the page renders with the placeholder cover. The old guard on the split resource IDs was dead code, since SplitN always returns at least one element. |
||
|
Deluan Quintão
|
25e7b5b20d |
Merge branch 'master' into fix/login-rate-limit-ip-spoofing | ||
|
Deluan Quintão
|
08eb46c8ad |
Merge branch 'master' into fix-forceformat-directplay | ||
|
Deluan
|
055fbde3cf |
fix(server): key the login rate limit on a trust-aware client IP
The RealIP middleware rewrote RemoteAddr from the True-Client-IP, X-Real-IP and X-Forwarded-For headers on every request, including when no trusted reverse proxy was configured. The login rate limiters on /auth/login and the Jellyfin /Users/AuthenticateByName derived their bucket from that value, so an unauthenticated client could rotate a forwarding header and get a fresh bucket for every password attempt, defeating the brute-force protection. Resolve the client IP with chi's ClientIPFrom* middlewares instead. The forwarding headers are only honoured when ExtAuth.TrustedSources is set and the connecting peer is in that list, reusing the trust check that external authentication already applies; otherwise the peer address is used. The X-Forwarded-For chain is now walked against the trusted CIDRs rather than taking its leftmost entry, so a spoofed value prepended by the client is skipped. Both limiters now key on the resolved address. The resolved address is still mirrored into RemoteAddr, so request logging, player registration and the Jellyfin local-network check keep reporting the client rather than the proxy. Reported by gehan-psbc. |
||
|
Deluan Quintão
|
72975a95fb |
fix(subsonic): honor DefaultDownloadableShare in createShare (#6121)
* fix(subsonic): honor DefaultDownloadableShare in createShare The DefaultDownloadableShare option was only sent to the web UI, which used it to pre-tick the "Allow Downloads?" checkbox. The Subsonic createShare handler built the model.Share without touching Downloadable, so it fell back to the Go zero value and every share created through the API was stored as non-downloadable, regardless of the configured default. createShare now reads an optional downloadable parameter and falls back to conf.Server.DefaultDownloadableShare when the client omits it, matching the web UI. Fixes #6119. updateShare had a related problem: core's share repository wrapper always writes the downloadable column, but the handler never set the field, so any updateShare call silently reset the share to non-downloadable. It now loads the current share and uses its value as the fallback. * refactor(subsonic): trim the share downloadable lookup and align with the UI updateShare fetched the share with Get to recover the stored downloadable flag, which also runs loadMedia and materializes every album and track the share points at, just to read one boolean. It now uses Read, which skips loadMedia, and only queries at all when the client omitted the parameter. createShare now ANDs the default with EnableDownloads, matching what the web UI already computes, so both paths apply the same rule. The specs collapse the create-path matrix into a DescribeTable, reuse the existing albumIDByName helper, and set the request-time config after setupTestDB so it does not leak into the config snapshot. * fix(subsonic): keep the share description on a downloadable-only update updateShare read the description straight from the request, so a client that sent only id and downloadable got an empty string written over the stored description. shareRepositoryWrapper.Update always writes that column, so the description was silently erased. This predates the downloadable parameter added earlier in this branch: any updateShare that omitted description already cleared it. Adding the parameter just made it easy to hit, since toggling downloads is a natural reason to call updateShare without touching the description. Both fields now use the presence-aware accessors and fall back to the stored share, which still costs at most one read and none when the client sends both. An explicitly empty description still clears the field. |
||
|
Deluan Quintão
|
e7b449b805 |
Merge branch 'master' into fix-forceformat-directplay | ||
|
Deluan Quintão
|
043de7a86c |
docs(jellyfin): correct the rationale for the public image endpoint (#6114)
The comment justified anonymous access with "item ids are unguessable". That is not true: an artist id is a deterministic, unsalted hash of the artist name, id.NewHash(id.NewHash(str.Clear(lower(name)))), so it is computable offline by anyone who knows the name. The real reason the route is public is that upstream Jellyfin's is too. ImageController.GetItemImage carries no [Authorize] attribute (verified on v12.0, master/13.0.0, v10.11.9 and v10.10.7), and an anonymous request reaches LibraryManager.ItemIsVisible with a null user, which returns true unconditionally. Clients build cover URLs with no credentials at all, so requiring auth here would break them. No behavior change. |
||
|
Deluan
|
404837799b |
fix(subsonic): don't re-encode a source already in the player's forced format
When a player has a forced transcoding format, ClientInfo.ForceFormat cleared DirectPlayProfiles unconditionally. A FLAC source on a player configured to transcode to FLAC was therefore re-encoded to FLAC, wasting CPU and bandwidth for no gain. Worse, the transcoder pipes ffmpeg output to stdout, so the resulting FLAC has total_samples=0 and no seek table -- an offline copy of it can never be seeked. Reported against getTranscodeDecision by the Symfonium author. ForceFormat now rebuilds DirectPlayProfiles from the matching transcoding profiles instead of dropping them: a client declaring a transcoding profile for a format is proof it can consume that format, so a source already in it is served as-is. Container and codec come from resolveTargetFormat, so a legacy "oga" target_format yields an ogg/opus profile, and the profile's MaxAudioChannels is carried across. DirectPlayProfile has no bitrate field, so restoring direct play needs a ceiling to keep an over-bitrate source out of it. GetTranscodeDecision now seeds that ceiling from the transcoding row's DefaultBitRate when a format was successfully forced, with the player's own MaxBitRate still taking precedence. This also closes a gap where the new endpoint ignored DefaultBitRate entirely: an mp3 320 source on a player forced to mp3@192 was served at 320, while the legacy /rest/stream path correctly gave 192. Applied via CapBitrate, which only ever lowers, so a client declaring a stricter limit keeps it. The legacy path (applyServerOverride) is untouched -- ForceFormat has no other callers. |
||
|
Shxiao
|
97e1f73cc8 |
docs: fix broken links in Jellyfin and plugin documentation (#6097)
* docs: point jftui client link to canonical repository * docs: fix relative path to webhook-rs example in nd-pdk-host README * docs: fix capability schema paths in plugin examples README --------- Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan Quintão
|
c534aedd0c |
fix(jellyfin): add the /Items/Latest route, and scope it by ParentId (#6090)
Jellify's Discover tab calls GET /Items/Latest and got a 404: we only routed the
/Users/{userId}/Items/Latest form, which real Jellyfin marks [Obsolete] and hides
from its OpenAPI spec, so SDK-generated clients never see it. Add the current
route alongside the legacy one, both served by the same handler.
getLatest also ignored ParentId, so browsing a library or an artist returned the
newest albums across everything the user can see. It now scopes to the library
when ParentId names one, and filters to that artist's albums otherwise, which
also makes a stale id return nothing instead of silently widening back to the
full library set. A malformed ParentId 404s, matching /Items and the contract
decodeFilterParam documents.
|
||
|
Deluan Quintão
|
546302576a |
fix(jellyfin): emit TranscodingUrl without the /jellyfin base path (#6089)
PlaybackInfo returned a TranscodingUrl prefixed with the /jellyfin mount path.
Clients concatenate that value onto a server base URL that already carries the
prefix, producing /jellyfin/jellyfin/Audio/{id}/universal and a 404, so playback
never started. Jellify, jellyfin-web, jellyfin-vue and Streamyfin all consume the
field this way; real Jellyfin emits it server-relative (StreamInfo.ToUrl is called
with a nil baseUrl).
Emit the path server-relative to match. Finamp is unaffected: it builds its own
stream URLs and never reads the field.
|
||
|
Deluan Quintão
|
a7365e119b |
fix(subsonic): update the playlist changed timestamp when renaming a smart playlist (#6082)
`buildPlaylist` reported `evaluated_at` as `changed` for smart playlists, so a rename or comment edit was invisible to clients until the next evaluation. A never-evaluated smart playlist also reported the current time on every call, which never settled. Report `updated_at` for every playlist. `refreshCounters` now syncs the stamp it writes back onto the model, and `refreshSmartPlaylist` reuses it for `evaluated_at`. `changed` therefore still equals the evaluation time for a just-evaluated playlist, and `validUntil` stays anchored to it. Original Subsonic always bumps `changed` on any playlist update, so this also aligns the behavior with upstream. |
||
|
Deluan Quintão
|
1f861d27ef |
fix(plugins): build public URLs on the caller's address instead of localhost (#6059)
* fix(plugins): build public URLs on the caller's address instead of localhost The artwork host service had no `*http.Request`, so it passed `nil` to `publicurl.ImageURL`. With neither `ShareURL` nor `BaseURL` configured, that produced `http://localhost/share/img/...`, which is useless to anything outside the server. The Discord Rich Presence plugin explicitly drops localhost URLs, so it fell back to the Navidrome logo instead of the real cover art. `serverAddressMiddleware` already works out the client-facing scheme and host from the `X-Forwarded-*` headers. It now also records them in the request context, and `publicurl` takes a `context.Context` instead of an `*http.Request` so any caller can reach them. Extism passes the caller's context through to host functions, so plugins invoked during a request now get a reachable URL with no configuration. Switching the parameter also removes the need for a second, parallel entry point: the package previously wanted only a scheme, a host, and a context, and took a whole request to get them. `AbsoluteURL` no longer dereferences a possibly-nil request on its parse-error path. Plugin calls that start from `context.Background()` (scheduler and websocket callbacks, the buffered scrobble drain) still fall back to localhost, since they have no request to learn from. A debug log now points at `ShareURL` when that happens. * fix(publicurl): include the configured port in the localhost fallback The last-resort fallback built `http://localhost/...`, which points at port 80 and so is unreachable for a server listening anywhere else — the default 4533 included. Use `conf.Server.Port` so a consumer on the same machine can actually fetch the URL. * fix(publicurl): use https in the localhost fallback when TLS is configured The fallback hardcoded the http scheme, so a TLS-only server with no BaseURL advertised a URL it does not answer on. Mirror the server's own switch, which requires both a certificate and a key. * refactor(publicurl): tidy the localhost fallback and its tests Use gg.If for the fallback scheme so it reads as an expression, like the BaseScheme branch above it, instead of assigning http and overwriting it. Drop two tests the ctx refactor left redundant: one asserted PublicURL "works without a request" but became a byte-identical copy of the ShareURL spec once the *http.Request parameter went away, and the two port specs differed only in the integer, where the non-default port is the stronger assertion. * refactor(conf): add TLSEnabled and use it instead of repeating the predicate Whether the server speaks HTTPS was decided inline in three unconnected places. This PR added the third, in a URL-building package that has no business inferring the transport config. Move the rule to conf, next to the fields it derives from, and call it from publicurl and the insights collector. server.Run keeps its own expression: it takes the certificate and key as parameters, and its test passes values that do not come from the config. |
||
|
Deluan Quintão
|
9ff0058620 |
fix: assorted scanner, plugin, and server fixes from the Go 1.27 work (#6050)
* fix(plugins): stop the cache janitor when a plugin cache is dropped
newCacheService started a ttlcache janitor goroutine that only stopped via the
explicit Close() path, so a cache service that was discarded without being closed
leaked its janitor for the process lifetime. It now registers the same
runtime.AddCleanup safety net that utils/cache.simpleCache already uses.
* fix(scanner): stop splitting multi-byte characters when truncating tags
sanitize() capped tag values with a byte slice, so a value whose limit falls in
the middle of a multi-byte character was stored as invalid UTF-8. defaultMaxTagLength
is 1024, which is not a multiple of 3, so any sufficiently long CJK title hit this.
Only trailing invalid bytes are trimmed, leaving bad bytes elsewhere in the value
untouched.
* fix(scanner): store MusicBrainz ids in their canonical form
uuid.Parse accepts a UUID wrapped in any two bytes, as well as braced and urn:
forms, but sanitize() returned the raw string. A tag like {<mbid>} or a quoted
value was therefore persisted with its wrapper into the mbz_* columns, where the
exact-match MBID search can never find it. The parsed value is now stored, which
also lowercases uppercase ids and adds the dashes to unhyphenated ones.
* fix(plugins): parse IPv6 hosts correctly in the websocket allowlist
isHostAllowed cut the host at the last colon, which mangles an IPv6 literal:
"[::1]:8080" became "[::1]" and "[::1]" became "[:". A plugin manifest could
therefore never allow an IPv6 host. It now uses net.SplitHostPort, falling back to
unwrapping the brackets when there is no port.
* fix(server): serve pprof profiles when a BaseURL is configured
net/http/pprof's Index resolves the profile name by trimming "/debug/pprof/" from
the raw request path, which never matches once MountRouter prepends the BasePath.
Requests for any profile without an explicit chi route fell through to the index
page, returning HTML with a 200 instead of the profile. The handler now strips the
BasePath first.
* test(scanner): run the goroutine leak check unconditionally
The scanner suite's goleak check only ran when the GOLEAK env var was set, so it
never ran in CI and could not catch a regression. It passes with the existing
ignore list, verified over repeated runs, so the gate is removed.
* fix(server): close the background image body on a non-200 response
serveImage returned early on an unexpected status code without closing the response
body, pinning the connection until the 5s client timeout. The nolint:bodyclose
above the request suppressed the linter that would have caught it, and its
justification only holds on the success path, where the body is handed to the
CachedStream wrapper.
* test(scanner): repair BenchmarkScan so it can actually run
The benchmark failed three ways before reaching its first iteration: it reused a
shared temp DB and tried to repoint the default library, it never loaded the config
defaults so the scanner got a concurrency of 0, and it lacked the notify ignore that
the suite already carries. tests.Init now takes a testing.TB so a benchmark can load
the test config the same way the suites do.
* refactor(artwork): drop the unused sourceFunc Stringer
sourceFunc.String derived a label from the closure's symbol name via reflection, but
nothing called it: the trace output builds its candidate labels from explicit strings.
Whole-program analysis confirms it is unreachable, and dropping it removes a
reflection-based dependency on compiler closure-naming details.
* refactor(plugins): reuse extractHostname in the websocket allowlist
The IPv6 host parsing added for isHostAllowed duplicated extractHostname, which
already lives in the same package and backs the HTTP client's identical allowlist
check. Two copies of a security-relevant parser can drift, so the websocket service
now calls the existing helper. The port-stripping specs move into the URL Validation
block that already covered them.
* perf(scanner): bound the tag truncation trim to a partial rune
The trim loop dropped every trailing byte that failed to decode, so a value ending
in a long run of invalid bytes was walked one byte at a time: a 1 MiB lyrics tag
measured 2.58ms against 45ns for a normal cut. A partial rune is at most 3 trailing
bytes, so the loop is capped there, which also stops it consuming a pre-existing
invalid run.
* test: tighten the tests added with the Go 1.27 bugfixes
Drop the testItem stub in favour of the package's own cacheKey, register the pprof
test profile once at package scope, and replace the hand-rolled goroutine settle
loop with Eventually. Also corrects a comment that credited a TestMain the scanner
suite does not have.
* test(scanner): ignore notify's nonrecursive-tree goroutines on Linux
The goroutine leak check only ignored the recursive tree (macOS/FSEvents).
Linux CI uses inotify, whose nonrecursive tree leaks dispatch and internal
goroutines after Stop(), failing the check.
* fix(scanner): avoid a truncation panic when MaxLength is 1 or 2
A value of only UTF-8 continuation bytes drained the partial-rune loop to
empty, then sliced value[:-1] and panicked. Break when DecodeLastRune returns
size 0 (empty string) by testing size != 1 instead of size > 1.
* fix: address Codex review on the pprof base path and scan benchmark
- profilerHandler: treat a root BasePath ("/") as no prefix, so http.StripPrefix
keeps the leading slash chi needs; without this the profiler 404s when BaseURL
is "/". Cover the root case in the test.
- BenchmarkScan: make it run regardless of test/benchmark ordering. Add
singleton.DeleteInstance so a fresh DB is opened after TestScanner closes the
shared one, guard driver registration with sync.Once so the rebuild does not
re-Register, and ignore the Ginkgo interrupt-handler and Linux notify
goroutines the preceding suite leaves behind.
* fix: address Codex round 2 on BasePath trailing slash and benchmark DB cleanup
- profilerHandler: trim all trailing slashes (TrimRight), not just a bare "/", so
a BaseURL like "/music/" strips correctly instead of 404ing. Cover it in the test.
- BenchmarkScan: keep and defer db.Init's closer so the DB is closed before
b.TempDir cleanup, which otherwise cannot delete the open SQLite/WAL files on Windows.
|
||
|
Deluan Quintão
|
f08b5297ee |
feat(ui): add Refresh Metadata to the album and artist context menus (#6036)
* refactor(artwork): move artworkItemName into core/artwork as ItemName * feat(external): add RefreshInfo to force an external info refresh RefreshInfo re-fetches and re-saves external info for one artist or album, bypassing the TTL check that UpdateArtistInfo/UpdateAlbumInfo use. It is synchronous; callers that must not block detach it themselves. Also makes MockArtistRepo/MockAlbumRepo.UpdateExternalInfo persist to Data (previously a no-op) and adds the new method to the e2e noopProvider, both required so the interface addition compiles and is observable in tests. * feat(external): broadcast RefreshResource after external info is saved populateArtistInfo and populateAlbumInfo now emit the same RefreshResource event the artwork worker uses, so the UI learns about both foreground and background metadata refreshes. * feat(nativeapi): replace artwork refresh endpoint with metadata refresh * feat(ui): add refreshMetadata to the data provider * feat(ui): add a Refresh Metadata item to the album and artist context menus * fix(ui): re-fetch artist info when the record is refreshed * test: fix mislabeled spec, add kind-gate negative case, guard nil mock maps - Rename the RefreshInfo spec that claimed to cover the save-failure/broadcast path: SetError(true) fails Get too, so it only proves RefreshInfo bails out early at getArtist. - Add a spec proving playlist refreshes skip the external-info step, since that asymmetry (al/ar only) was documented but unasserted. - Add lazy nil-map init to MockAlbumRepo/MockArtistRepo.UpdateExternalInfo so a composite-literal-constructed mock doesn't panic on first save. * test: relocate discArtworkName specs from cmd to core/artwork artworkItemName moved into core/artwork as ItemName in an earlier commit, but its disc-name specs stayed behind in cmd/artwork_test.go, reaching across packages. Move them to core/artwork/item_name_test.go where the code now lives. * fix(ui): shape refreshMetadata like a react-admin response react-admin validates custom dataProvider methods and rejects any response without a `data` key, so the raw httpClient promise made every click surface an error toast instead of the success message. The unit test mocked useDataProvider, which skips that validation. Also folds "which kinds have external info" into external.HasInfo so the handler stops restating it, drops the nil-broker guard that only existed for tests, and delegates the mocks' UpdateExternalInfo to Put. * refactor(external): unexport infoKinds Only HasInfo is used outside the package, so the slice itself does not need to be exported. * refactor(artwork): fold ItemName into housekeeping.go next to Refresh ItemName exists to guard Refresh from ids that would orphan a queue row, and both callers invoke them back to back. A separate file hid that pairing; it was only split out to keep the move out of cmd/ legible in review. * fix(nativeapi): return 500 when the refresh lookup fails for a non-ErrNotFound reason A transient repository error told the admin the id did not exist, and the error was dropped without a log line, so nothing pointed at the real cause. Also drops the inherited claim that clearing artwork state shows a placeholder. Reads fall back to local resolution, so that only holds when there is no local art. * fix(ui): move Refresh Metadata above Get Info in the context menu Menu order follows key insertion order in the options object, so the new spec pins the position rather than leaving it to be shuffled by the next addition. |
||
|
Rob Emery
|
3da2b590e7 |
fix: add Navidrome UserAgent in all outgoing requests (#6020)
* There has been a report about navidrome hitting listenbrainz hard
and the listenbrainz guys wanting to be able to distinguish navidrome
* feat: apply Navidrome User-Agent to all outgoing HTTP requests
Add utils/httpclient, a shared http.Client factory whose transport sets
the User-Agent header (Navidrome/{version} - https://github.com/navidrome)
on any request that does not already have one, and use it at every place
the server builds an HTTP client: Last.fm, ListenBrainz and Deezer agents
and auth routers, insights collector, backgrounds handler, and the plugin
host HTTP service. Plugin-set User-Agent values are preserved. The
per-request header lines from the previous commit are superseded by the
transport.
---------
Co-authored-by: Deluan <deluan@navidrome.org>
|
||
|
Deluan Quintão
|
59810c3d59 |
feat(jellyfin): non-expiring, audience-scoped tokens revocable by password change (#6013)
* feat(auth): add per-user token_epoch column and bump method * feat(auth): add aud and ep claims, omitted when zero * feat(auth): add CreateAPIToken for non-expiring, audience-scoped tokens * feat(auth): add CheckClaims for epoch and audience validation * feat(jellyfin): issue non-expiring, jellyfin-scoped access tokens * fix(subsonic): reject API-scoped and revoked tokens on the jwt path * fix(server): reject API-scoped and revoked tokens on the native API * fix(server): pin the token-subject guard and stop leaking test config Adds a regression spec for the DevAutoLogin/ExtAuth guard in tokenAllowed, switches its comparison to case-insensitive to match the user lookup's own COLLATE NOCASE semantics, and restores Subsonic JWT test config after each spec instead of leaking SessionTimeout. * feat(request): add a token epoch holder for handler-to-middleware signalling * refactor(server): write the refreshed JWT header after the handler runs * feat(auth): revoke all tokens for a user when their password changes * fix(server): restore Unwrap on the JWT refresh writer so SSE write deadlines apply * test(auth): pin that non-session tokens reject API access tokens * test(jellyfin): pin token scoping and epoch revocation end to end Exercises auth.CreateAPIToken and CheckClaims against the real Jellyfin router and SQLite DB: the minted token has no exp and is aud-scoped to jellyfin, and bumping token_epoch through the real UserRepository revokes an already-issued token on the next protected request. * test(nativeapi): pin the token-epoch handoff through a real password-change request Drive a self password change through the real Authenticator/JWTRefresher chain and a real SQLite-backed userRepository, so the epoch handoff between Put and the refreshed-token writer is verified end to end, not as two separately-tested halves. Also fix tokenAllowed to read the enriched ctx it was given instead of r.Context(), so its warning log carries the username. * refactor(server): drop tokenAllowed's now-unused request parameter Finding-2 already moved every use to ctx; r was dead weight. Also note in the new nativeapi test why it must stay the package's only real-DB spec: db.Db() is a process-wide singleton its cleanup closes for good. * refactor(auth): remove duplication in claim decoding and token minting * refactor(auth): group aud with the standard JWT claims * refactor(auth): read aud with the standard-claim accessor pattern * fix(log): redact every api_key spelling the Jellyfin API accepts * fix(auth): bind session tokens to the user id, not just the username * fix(auth): return the token epoch from the same atomic increment * fix(auth): bump the token epoch in the same statement as the password write * chore(auth): trim comments to the why-only budget |
||
|
Junker der Provinz
|
c362519f76 |
test: unskip path-separator tests on Windows (#5381) (#5916)
* test: unskip AbsolutePath and i18n path-separator tests on Windows (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * test: unskip metadata folder-PID test on Windows via path.Dir (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * test(storage): make relative-folder assertion cross-platform and unskip on Windows (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * fix(persistence): normalize folder-update-info paths with forward slashes on Windows (#5381) Signed-off-by: junkerderprovinz <jdp@braethoria.com> * review: drop folder-PID change, trim storage_test comment (#5381) Revert model/metadata/persistent_ids.go to master: switching the `folder` PID attribute from filepath.Dir to path.Dir would change the persistent IDs of existing Windows libraries and needs a migration path, so it is out of scope for this PR. The matching test unskip is reverted with it, leaving #TBD-path-sep-metadata open in #5381. Trim the core/storage/storage_test.go comment to two lines. Signed-off-by: junkerderprovinz <jdp@braethoria.com> --------- Signed-off-by: junkerderprovinz <jdp@braethoria.com> Co-authored-by: Deluan Quintão <deluan@navidrome.org> |
||
|
Deluan
|
bd6b7a6686 | test: increase timeout for cache availability checks to 10 seconds | ||
|
Deluan Quintão
|
1f3034f022 |
fix(playlist): block track edits on synced playlists across all APIs (#5984)
* fix(playlist): block track edits on synced playlists across all APIs A synced playlist's tracks come from its source file, so any track edit made through the UI or an API was silently reverted on the next scan. Track mutations funnel through two service guards, checkTracksEditable (incremental edits) and Create (wholesale replace, used by Subsonic createPlaylist and Jellyfin's replace path), which each duplicated the smart-playlist check. Both now consult a shared model.Playlist.TracksEditable() predicate, so the native, Subsonic, and Jellyfin paths are all locked: track edits return ErrNotAuthorized (403, or Subsonic error 50) instead of being accepted and lost. Metadata-only edits (name, comment, public, the sync flag itself) still go through checkWritable and are unaffected. In the UI, a synced playlist's track list becomes read-only, mirroring how smart playlists already behave. * fix(playlist): return 409 Conflict for non-editable playlist track edits The previous commit rejected track edits on smart and synced playlists with ErrNotAuthorized (403). That conflates two different things: a 403 says the caller lacks permission, but a synced or smart playlist's tracks are immutable for everyone, including the owner and admins. It is a property of the resource, not the caller. Introduce ErrPlaylistNotEditable and return it from both track-edit guards. The Native and Jellyfin APIs now map it to 409 Conflict; Subsonic maps it to error 50, the closest code it has (it has no read-only concept). The Native track handlers previously mapped this rejection inconsistently (400 on add, 500 on remove, 403 on reorder) through a new shared writePlaylistError helper. Genuine authorization failures (non-owner, non-admin) still return ErrNotAuthorized. * fix(playlist): surface synced read-only state in picker, Jellyfin, and OpenSubsonic Follow-up to the track-edit lock: the read-only state was enforced but not advertised consistently, so clients still offered edits that the server rejects. - UI: the Add to Playlist picker filtered targets by isWritable only, offering synced playlists that then 409 on add. It now filters with canChangeTracks. - Jellyfin: addToPlaylist/removeFromPlaylist hard-coded every error to 404, so a locked playlist reported "not found" instead of 409. They now return 409 for ErrPlaylistNotEditable while keeping the deliberate anti-probing 404 for every other error (a non-owner never reaches ErrPlaylistNotEditable, so 409 leaks nothing). - OpenSubsonic: buildOSPlaylist marked only smart playlists readonly; owned synced playlists advertised readonly=false. Readonly now also covers !TracksEditable(), matching the existing smart-playlist treatment. * fix(jellyfin): report CanEdit from playlist editability in permission probes getPlaylistUsers and getPlaylistUser returned CanEdit: true unconditionally, so Finamp (which probes this before showing edit controls) offered track editing on synced/smart playlists whose add/remove requests now return 409. Both handlers now fetch the playlist and set CanEdit from TracksEditable(), keeping the deliberate non-owner looseness (CanEdit stays true for a normal playlist a non-owner views) and mapping any lookup error to 404 like the sibling probes. * fix(playlist): check ownership before editability when replacing tracks Create checked TracksEditable() before ownership, so a non-owner replacing another user's public smart/synced playlist (Jellyfin updatePlaylist with a non-empty Ids list) received a 409 read-only conflict instead of a 403 authorization failure. The incremental guards check ownership first via checkWritable; Create now matches that order. Subsonic is unaffected (both errors map to code 50). Owners of their own smart/synced playlists still get the read-only conflict. * fix(jellyfin): return 403 for locked playlists, matching Jellyfin Jellyfin itself refuses edits on its file-backed playlists with Forbid() (403): PlaylistsController gates every mutation on OwnerUserId == caller or a share with CanEdit, and playlists imported from .m3u files satisfy neither. Its CanEdit is an ACL field, not a read-only marker, and Jellyfin core has no server-managed playlist type at all. Our Jellyfin routes exist to imitate that API, so ErrPlaylistNotEditable now maps to 403 there instead of 409. The native API keeps 409 (a resource-state conflict is the accurate REST answer where we define the contract) and Subsonic keeps error 50, its closest code. * chore(playlist): trim comments added by this branch Several comments ran to three or four lines and carried rationale that belongs in the commit history rather than the code: what Jellyfin does with its own file-backed playlists, and restatements of the expressions directly below them. Each block is now one or two lines covering only the non-obvious why. |
||
|
Deluan Quintão
|
7a11ca69bb |
fix(jellyfin): honor the Filters, SortBy and MaxHeight params clients actually send (#5981)
* fix(jellyfin): honor Filters=IsFavorite on /Artists and /Artists/AlbumArtists listArtistsByRole hand-built its itemsQuery and never set favOnly, so the favorites filter was silently dropped on both artist routes while /Items honored it. Finamp's home screen asks for favorite artists once per load and was served the entire artist list instead: 10,298 artists, 6.15 MB, 2.7s on a real library, and the wrong data on screen. Extract the favOnly parsing that parseItemsQuery already did into parseFavOnly and use it in both places. listArtists now adds the starred predicate to notMissing rather than replacing it, matching listAlbums and listSongs, so a favorite artist whose files are gone stays excluded. * fix(jellyfin): map SortBy=Runtime to duration for albums and songs sortColumnsByType had no runtime/runtimeticks key for any type, so Finamp's "Duration" sort silently misbehaved in two different ways. Albums: Finamp sends a bare SortBy=Runtime. Nothing matched, opts.Sort stayed empty, and applyOptions skips OrderBy entirely when Sort is empty — so the query ran with no ORDER BY at all and Ascending and Descending returned identical lists. Songs: Finamp sends SortBy=Runtime,AlbumArtist,Album,SortName. applySort takes the first *recognized* key, so Runtime was skipped and the list came back sorted by album artist while looking correct. Both repos already accept a duration sort (mediafile_repository maps it explicitly; album_repository falls through to the column name), so no migration is needed. Sorting 97k songs by duration costs a temp B-tree (~114ms on a prod-sized copy) — the same cost the Subsonic and UI duration sorts already pay, and correct where the previous behaviour was merely fast. * fix(jellyfin): apply the played/unplayed filters and MaxHeight image bound Filters was matched with a substring test for IsFavorite, so every other token Jellyfin defines was silently dropped and the response kept rows it should have excluded. Finamp sends Filters=IsUnplayed in normal use. Replace the bool with a parsed itemFilters carrying nullable favorite and played flags, so isFavorite=false and isPlayed=false are real filters rather than indistinguishable from an absent param. Standalone params are read first and the Filters list overrides them, the precedence real Jellyfin has. IsFavoriteOrLikes now maps to favorites deliberately instead of by substring accident; Likes, Dislikes, IsFolder, IsNotFolder and IsResumable have no Navidrome equivalent and are dropped rather than half-applied. The negative cases match NULL as well, since annotations are LEFT JOINed and an untouched item has no row. getItemImage read only maxwidth, so a client sending just MaxHeight got the full-size original: measured against a real cover, maxHeight=100 returned 82,570 bytes where maxWidth=100 returned 3,316. Use the tighter of the two bounds. * refactor(jellyfin): share the plain-param parser between /Items and /Artists listArtistsByRole hand-listed the itemsQuery fields it happened to need, which is exactly how the favorites filter went missing: the literal has been amended in four of the five commits that touched it. Extract listParams for the fields that come straight from query params so both paths read one parser, and the next supported param reaches every list path instead of only /Items. Also from the cleanup pass: collapse imageSize to a single clamped comparison and read its bounds through req.Params like the rest of the package, which drops the strconv import; build the artist and playlist filter lists with the flat append shape the album and song paths already use, instead of re-wrapping opts.Filters into a nested And per predicate; drop a nil guard in listPlaylists that no caller can reach, since both paths into queryItemsOfType build QueryOptions without Filters. applySort now logs when no SortBy key resolves at all — a miss inside a fallback list is normal, but none matching means a silently ignored sort, the failure mode that hid the Runtime bug. Its doc comment records why the remaining keys cannot simply be joined. Folds three duplicated test bodies into the tables that already parameterize them, and covers the artist-parent album branch, which reaches notMissing through filter.AlbumsByArtistID rather than the default branch. * docs(jellyfin): correct how applySort describes Jellyfin's SortBy semantics The comment claimed SortBy is a comma-separated fallback list. It is not: RequestHelpers.GetOrderBy (10.10) builds one (ItemSortBy, SortOrder) pair per key, so Jellyfin orders by every key in turn. Navidrome applies only the first recognized one, which is a real divergence — secondary keys never break ties — not the intended reading of the parameter. The assertion that the keys cannot be joined was also wrong. buildSortOrder does split its input on commas; what it maps is the whole string, so joining raw Jellyfin key names misses the mappings. Mapping each key first and joining the results would work, which makes multi-key sorting a real option rather than a blocked one. Documenting the current behaviour as a known divergence until then. * fix(jellyfin): order by every recognized SortBy key, not just the first Jellyfin orders by each SortBy key in turn, so "DatePlayed,SortName" means break ties by name. Navidrome applied only the first recognized key and dropped the rest, which is 28% of the sort traffic on a real server (23 of 82 requests in 12h carry 2-5 keys). Most were harmless because the primary key dominates, but PremiereDate,Album,ParentIndexNumber,IndexNumber,SortName came back unordered within a year. The keys cannot simply be joined: sortMapping keyed on the whole Sort string, so a joined value missed every mapping and fell through to raw column names. Make it resolve a comma list per part, but only when every part is a known key — the four existing callers that pass raw column lists (core/matcher, core/lyrics, core/maintenance, subsonic/browsing) all carry a part that is not a mapping key, several with their own direction, so they keep falling through exactly as before. Verified each one. applySort now collects every recognized key, skipping duplicates so ParentIndexNumber,IndexNumber does not repeat a column. random stays alone: the repo matches it by exact string equality, so joining it would both break that path and emit a bare 'random' column into the ORDER BY. Verified against a prod-sized copy: every multi-key combination seen in real traffic returns 200, and a secondary key now changes the order within a tied year for songs. Albums are unchanged there, because their max_year mapping already ended in ", name". * fix(persistence): resolve sort mappings exactly once Making sortMapping resolve a comma list per part broke an invariant it had been relying on: idempotence. sanitizeSort mapped the sort key up front and applyOptions then ran buildSortOrder over the result, so sortMapping was already being handed its own output. That was harmless only while a mapped value could never look like a key list. media_file's rated_at maps to "rating, rated_at", and both parts are keys, so the second pass expanded it to "rating, rating, rated_at". Found by round-tripping every mapping in all four repositories; it was the only collision, and the duplicate sort key was benign in SQL, but any future mapping of that shape would silently change meaning. sanitizeSort now validates without resolving, leaving buildSortOrder as the single mapping point. The generated SQL is unchanged — the whole suite passes apart from the two specs that asserted the old return value, which are updated and joined by a round-trip guard covering exactly the rated_at shape. Also use the paren-aware splitFunc that buildSortOrder already uses, so an expression carrying commas inside its parentheses cannot be split apart. * refactor(jellyfin,persistence): flatten the sort resolution paths Cleanup pass over the branch, no behavior change. sortMapping loses the len(parts)>1 guard, which existed only to pick between two identical toSnakeCase exits; the single-key case now falls through the same loop. lookupSortMapping hands back the snake_case form it had to derive so the fallback stops recomputing it — toSnakeCase is two regexps, and on a miss it was running twice per call. sanitizeSort now asks lookupSortMapping instead of probing the map itself, so "is this a known sort key" has one answer; the two had already drifted, since sanitizeSort tried one casing where the resolver tries three. applySort folds the nested random branch into the skip condition and the two trailing length tests into one switch. setSortMappings documents the invariant the comma-list rule depends on, where someone adding a mapping will read it. The README line describing SortBy still said only the first key applied, which the commit before last made false. Tests: the twelve near-identical sorting specs become one DescribeTable of (itemType, SortBy, want) triples, 124 lines to 36, and the applyOptions round-trip assertion collapses to the buildSortOrder call its sibling uses. * fix(jellyfin): keep annotation filters out of search, resolve sorts per part Two findings from the Codex review on #5981. The played/unplayed filters turned working requests into 500s when combined with SearchTerm. Search runs a two-phase FTS query whose first phase selects rowids with no annotation join, so a starred or play_count predicate there is "no such column", not a filter. Measured against master: MusicAlbum with SearchTerm and Filters=IsUnplayed went 200 -> 500, likewise IsPlayed and the Audio equivalents. listAlbums and listSongs now skip those predicates on the search path, matching what listArtists already did. That also clears the same 500 master already had for Filters=IsFavorite with SearchTerm. sortMapping resolved a comma list only while every part was a known key, so a list mixing a plain column with a mapped key kept neither: MusicAlbum SortBy=Runtime,SortName arrives as "duration, name", and duration is a plain album column, so name stayed raw instead of expanding to order_album_name. Albums whose name differs from its sort form — 1,366 of 6,987 on a real library — then ordered by the wrong secondary key, and PreferSortTags was ignored. Each part is now resolved on its own, which is what setSortMappings already documents for a single field. Verified every in-tree caller that passes a raw column list still produces its original ORDER BY. Codex also asked for the artist search path to apply the same filters. It would 500 for the reason above, and wrapping the library scope in a compound filter makes requestedLibraryIDs stop recognizing it, silently widening the search past the requested ParentId. * fix(jellyfin): honor the first SortOrder value for a multi-key sort applySort compared the whole SortOrder string with "Descending", so a per-key list like SortOrder=Descending,Ascending failed the match and every key, including the primary, sorted ascending — the exact opposite of the request. Take the first comma-separated value, which Jellyfin also uses for any key past the end of the SortOrder list. True per-key directions can't be expressed through the single opts.Sort string and are left out; no observed client sends a SortOrder list. |
||
|
Deluan Quintão
|
4b1218eec0 |
feat(ui): show translation completion percentage in the language selector (#5979)
The language selector now shows how complete each translation is, so users can see at a glance which languages are lagging behind English. The native API's translation resource gained a termCount field holding the number of non-empty terms in each language file; the UI divides that by the term count of the bundled English file to get the percentage. The percentage is wrapped in a Unicode left-to-right isolate, otherwise it renders as "(%61)" beside right-to-left names such as Arabic and Persian. Sorting runs on the plain language name, before the percentage is appended. This also fixes prepareLanguage() mutating the bundled English translations: for the English locale it received the shared en object and aliased albumSong and playlistTrack onto it, growing en by 94 keys at runtime. That inflated the denominator and made every language read about 14 points low. The aliases now go on the merged copy instead. |
||
|
Deluan Quintão
|
dc40bcaf80 |
feat(cli): add an artwork command group for diagnosing and re-driving artwork (#5957)
* feat(artwork): add a resolution chain trace collector
* feat(artwork): trace the local priority chain
* fix(artwork): record priority candidates the chain never evaluated
* refactor(artwork): report never-evaluated candidates as skipped
* feat(artwork): trace external agents at the gate seam
* feat(artwork): add repository queries to enqueue by current source
* feat(artwork): expose a tracing resolver for the CLI
* feat(artwork): read a single queue row by item
The explain CLI must report whether an item is queued, at what priority and when it
retries; the queue repository could only be drained in eligibility batches, which
cannot see a row that is still backing off.
* feat(cli): add artwork explain
Prints why an item has the artwork it has: the stored state, its queue row, the
governing config, the resolver's priority-chain walk and the verdict. Offline by
default so a diagnostic run cannot add load to an external provider; --live asks
the agents for real. Playlists and radios do not walk a priority chain, so they
report that instead of an empty chain table.
* fix(artwork): trace an external tier that never reaches an agent
A configured 'external' token vanished from the chain when no enabled agent provided
images for that entity type, and for synthetic artists, leaving the trace unable to
say whether the tier was even considered.
* fix(cli): never state an artwork outcome the walk did not observe
A transient external failure traced as 'error' fell through to 'not resolved', which
is the most common state behind a missing-artwork report. It is now indeterminate, and
an offline win that a skipped higher-priority external candidate could have taken says
so instead of naming a winner the live chain might not pick.
* feat(cli): add artwork refresh
* feat(cli): add artwork reprocess
Bulk re-enqueues artwork by kind and/or by the source an item currently
resolves from, previewing the matched count and confirming before queueing.
The preview counts with CountBySource (rows matched) and reports separately
what EnqueueBySource inserted: its DO NOTHING conflict policy leaves an
already-queued row untouched, so the two numbers differ and the output must
not claim the skipped rows were re-queued.
An unknown --source is rejected against the sources present in item_artwork,
rather than silently matching nothing and printing a reassuring 0.
* fix(cli): cover the reprocess selection rule and validate sources table-wide
The reconciliation that makes --source alone target every kind was only
exercised through runReprocess, which no test calls: mutating it to
`all := reprocessAll` left the suite green. It is now reprocessSelectsAll,
covered for all three selectors.
Scoping source validation to the selected kinds made the same well-formed
filter valid or invalid depending on which other kinds were selected, and its
error read the same for a typo as for a source that simply does not apply to
the chosen kind. Validation is now table-wide: a typo still aborts, while a
valid-but-inapplicable source falls through to "Nothing matches".
Also: the prompt now counts only the kinds that reach an external agent as
external cost, and --dry-run on an empty selection reports a dry run.
* fix(cli): cover the reprocess --yes guard and preview the external cost
Mutating the --yes check to `if true` left the suite green, so the one bypass
of the confirmation was unverified. The choice is now reprocessConfirm(yes, in),
covered in both directions.
The external estimate only reached the operator through the prompt, which
--dry-run skips — hiding the number in the one mode that exists to show it
before committing. The preview now carries it, and the prompt drops the clause
when no lookup will be made.
An empty selection says so again under --dry-run.
* feat(artwork): add read-only queue and absent counters
Both are needed by the artwork status CLI: a queue breakdown by kind and priority, and
the absent totals split against the recheck cutoff.
* feat(cli): add artwork status
Reports the queue, where artwork currently resolves from, absent counts against the 24h
recheck window, and the stored config fingerprint versus the current one — the line that
turns 'why is my server re-resolving everything?' into one command.
fingerprint() and staleAbsentAge are exported so the CLI reports the values backfill
itself compares, instead of a second copy of the formula that can silently drift.
* fix(cli): lead the artwork status backfill line with the queued backlog
By the time anyone runs a diagnostic, backfill has usually already stored the new
fingerprint, so 'up to date' was printed while thousands of items churned through external
providers. The backlog is the finding; the fingerprint is context.
Also echoes the config inputs the fingerprint covers, so a change can be traced to the
setting that caused it, and pins the rendered rows: the Absent values, the queue TOTAL and
a queue-scoped kind/priority pair were all unasserted, so kindName and priorityName were
effectively untested. FingerprintInputs is now the single listing ConfigFingerprint hashes;
a pinned hash proves the value did not change.
* refactor(artwork): export the trace outcome vocabulary
The CLI hardcoded the outcome literals and the "external:" prefix, so renaming a
constant's value in core/artwork left cmd compiling and the suite green while
`artwork explain` silently degraded its verdict.
Renaming a value now fails the golden vocabulary test in core/artwork and the
explainResult tests in cmd.
* fix(cli): keep the re-enqueue warning when a backfill is already running
A stale stored fingerprint with items already queued is the worst state the
system can be in: a second full re-enqueue is pending on top of the one running.
The line carried the weakest wording of the three, and was untested.
* refactor(artwork): drop the unreachable breaker branch from the tracing gate
--live wires the tracing gate straight to passthroughGate, so errBreakerOpen can
never reach it; the test only passed by injecting a fake gate.
* refactor(artwork): delete the never-emitted not-reached outcome
Candidates after the winner are lower priority and say nothing about why a source
won; the ones that matter sit above it and are already recorded.
* refactor(artwork): make the trace nil-safe in one place only
add already handles a nil trace, so record's own guard was dead; Steps was the
odd one out and would panic where every other method tolerates nil.
* refactor(artwork): export the trace types directly
ChainTrace and TraceStep were unexported types re-exported through aliases,
which existed only so the CLI had a name to refer to them by. The types are
public API — Resolver.Steps returns []TraceStep and the CLI constructs a
ChainTrace — so name them that way and drop the indirection.
Encapsulation is unchanged: add, mu and steps stay unexported, so only this
package can write a step.
* refactor(cli): simplify parseArtworkKind with slices.Contains
Replaces a nested loop and a manual append with slices.Contains and the
repo's slice.Map helper. Same behaviour, same error message.
* fix(cli): print the absent artwork source under the name --source accepts
`artwork explain` rendered the stored empty source as "(absent)", while
`artwork reprocess --source` only accepts "absent", so pasting what explain
printed straight back into reprocess was rejected as an unknown source.
* refactor(artwork): own the kind list and the chain predicate in the package
Export RecheckKinds and add WalksPriorityChain so the CLI stops keeping its
own copies of both, and unexport externalCandidate, which nothing outside the
package consumes.
* refactor(cli): drop the artwork command's duplicated state and formatting
Reuse artwork.RecheckKinds and artwork.WalksPriorityChain, extract
newTabWriter and externalEstimate, fold reprocessSelectsAll into
selectedKinds, and derive the queue total and the walks-chain flag instead of
carrying them in the report structs.
* test(persistence): drop two artwork-queue specs that cannot fail
One seeded hash and source together and then asserted the two counts agree,
so its setup guaranteed the result; the other repeated the count-does-not-
enqueue property already covered by the CountBySource spec.
* refactor(artwork): rename Resolver to TracingResolver for clarity
* fix(cli): count playlists in the artwork reprocess external estimate
The estimate used WalksPriorityChain, which is true only for artist and album,
so a playlist-only reprocess reported "External lookups: none" and the
confirmation prompt dropped the external-cost warning. Playlists do reach the
network: through the m3u ExternalImageURL fetch when EnableM3UExternalAlbumArt
is on, and — verified by test — through the generated grid, whose tiles resolve
album art via the full album priority chain.
Adds artwork.MayFetchExternal, a config-aware predicate for "can this kind's
resolver reach the network", and uses it for the estimate. WalksPriorityChain
keeps its separate job of deciding whether explain prints a chain block.
* fix(cli): estimate artwork reprocess external lookups per agent, not per item
The reprocess prompt billed one external lookup per externally-capable item.
fetchArtistImage/fetchAlbumImage try every enabled image agent and stop early
only on a hit, and resolvePlaylist can fetch the m3u image and then resolve up
to four sampled albums for the grid, each walking the album agents again. The
number the operator confirmed could understate real provider traffic several
fold, in the prompt whose whole job is to stop a provider flood.
ExternalLookupsPerItem now multiplies by the visible image-agent count and adds
the playlist grid factor. It stays a floor: the CLI never calls Manager.Start(),
so the plugin registry is empty and plugin-provided agents are dropped by
getEnabledAgentNames. On an install with 5 agents of which 3 are plugins the
count is well under the truth, so the wording is now "at least N" rather than
"up to N" — a zero visible count still bills one lookup for the same reason.
Fixing the plugin visibility is out of scope: Manager.Start() needs a Subsonic
router and writes to the DB via syncPlugins, breaking this command group's
read-only guarantee.
* fix(cli): state the artwork reprocess estimate as an estimate, not a bound
Neither bound is true. A ceiling is false because plugin agents are invisible to
a CLI that never starts the plugin manager, and a floor is false because a local
hit ends the walk before any agent is asked and a hit on the first agent skips
the rest. "at least N" traded one wrong claim for another.
The line now names its blind spots instead:
External lookups: ~340 estimated (plugin agents not counted; local hits may
need fewer).
The same line is reused in the confirmation prompt, and the zero case still
reads "External lookups: none." with the prompt dropping the clause entirely.
The count itself is unchanged.
* fix(cli): account for every configured agent in artwork explain
The Agents: line printed the raw config while the Chain only showed the agents the CLI could
construct, with nothing explaining the gap: plugin agents are never registered in a CLI that does
not start the plugin manager, and a built-in without credentials returns nil. Three of five agents
could vanish, including ones ranked above the one shown.
Also treat a live external error before the winning hit like the already-handled would-try case:
the resolver serves such a hit provisionally and retries later, so the verdict is indeterminate.
The Result line is still not qualified when an unavailable agent might have won; that needs agent
ranking, and is left to the follow-up that makes the CLI load plugin agents for real.
* fix(cli): do not call an external artwork win indeterminate
explainResult qualified the verdict whenever an external OutcomeError
appeared before the winning hit. When a later external agent returns an
image, fetchArtistImage/fetchAlbumImage discard the earlier error, so
extError is false: the worker settles the item and schedules no retry.
Telling the operator it may resolve differently on a retry was wrong.
The warning is only correct when a lower-priority local source won while
an external error was recorded, which is the case that carries extError.
* fix(cli): accept --source absent when nothing is currently absent
validateSources checks the requested sources against the ones item_artwork
actually uses, to catch a typo. The reserved empty source (spelled 'absent' on
the CLI) is a valid filter even when it matches nothing, so a scheduled
'artwork reprocess --source absent --yes' stopped working the moment the
library finished resolving. Treat it as intrinsically valid and let the
existing zero-match path report it.
* feat(artwork): explain disc and media file artwork from the CLI
`artwork explain` rejected `dc` and `mf` because it validated against RecheckKinds,
the list of kinds the backfill revisits. Those are different questions: a kind with no
recheck path still has artwork someone can report as wrong.
Disc artwork now walks DiscArtPriority under a trace, so explain reports which entry won
and why the others lost, including entries that map to no source at all (external is
unsupported, a disc with no subtitle, an album folder with no images). Media file artwork
traces its single embedded candidate, separating "EnableMediaFileCoverArt is off" from
"the track has no embedded art" — stored state cannot tell those apart.
Each command now validates against the kinds it can actually serve: explain takes all six,
refresh takes artwork.RefreshableKinds (which nativeapi now shares instead of keeping its
own copy), reprocess still takes RecheckKinds. Disc artwork stays out of refresh: the
worker cannot resolve it, so the queue row would be rejected on every drain.
WalksPriorityChain becomes Explainable, and ResolveArtist/ResolveAlbum collapse into
Resolve(kind, id).
* refactor(artwork): one disc-artwork walk for serving and explain
resolveDisc duplicated the loop selectImageReader already ran: try each source in
priority order, take the first that yields an image. The serving path and the CLI
diverged on two details as a result — only selectImageReader checked ctx between
candidates and logged each attempt.
Both now call discArtworkReader.selectImage, which takes the chainState the CLI already
uses for the other kinds. The serving path passes an untraced one, whose nil trace makes
recording a no-op. selectImageReader had no other caller and is gone.
The disc tests move from fromDiscArtPriority to discCandidates, so they assert the skip
reason for an entry that maps to no source rather than that it silently vanished, and
cancellation mid-walk is now covered.
* fix(artwork): reject a nil reader in the resize cache instead of panicking
resizedItem.Reader closes what open() hands back, so an open() that reports "no image"
as (nil, nil) rather than an error takes the request down with a nil-pointer panic. Every
caller returns an error today, and no test covered it: the resolution e2e harness stubs
the resize reader out entirely, so no e2e path reaches this code at all.
Guard it and cover Reader directly.
* refactor(artwork): move the keeps-state fact into core, drop a redundant guard
keepsArtworkState lived in package cmd and re-derived by hand what RefreshableKinds
already encodes: the same five-of-six kinds. It is now artwork.KeepsState, beside the
list, with a test pinning the two together — nothing else stopped them drifting, and a
drift would have explain report stored state for a kind that keeps none.
serveDisc's closure also hand-rolled a nil-reader error that both consumers of open()
now produce themselves: serveSource for a full-size request, resizedItem.Reader for a
resized one.
* fix(artwork): route disc candidates through the shared resolvers
openCandidate ran its own source loop and threw the error away, so a disc track that
exists but cannot be parsed traced as "miss" — indistinguishable from a track with no
embedded art. fromTag and fromFFmpegTag already report that case as errSourceUnreadable;
only this loop was discarding it. Telling those two apart is what the trace is for.
Candidates now carry a resolve func instead of raw sources: embedded goes to
resolveEmbedded, and the folder-backed entries to resolveFolderSource, extracted from
resolveFolderFile so both callers classify an unopenable file the same way. openCandidate
and its absolute-path special case go away with it.
Disc's own fromExternalFile and fromDiscSubtitle still swallow open errors, so folder
candidates cannot report unreadable yet; that is a change to their error contracts.
* fix(artwork): report an unreadable local candidate as indeterminate
processor.acquire treats resolution.localError exactly as it treats extError: a fault is
not a definitive "no image", so it retries instead of settling absent. explainResult
qualified only the external case, so a chain that ended on an unreadable local candidate
printed "not resolved" — the one verdict that says the walk was conclusive.
The qualification belongs only to the unresolved branch. chainState.try stamps extErr onto
a hit and deliberately drops localErr, so an unreadable step followed by a hit is settled
as found and must not carry a warning; a test pins that.
Found by Codex on
|
||
|
Deluan Quintão
|
aa0824e03b |
feat(jellyfin): add System/Endpoint so Finamp's connection test passes (#5955)
Finamp's connection test GETs /System/Endpoint and treats anything other than a 200 carrying an IsInNetwork key as "not a Jellyfin server", so the test failed against Navidrome and dual-connection setups could never switch to the local address. IsInNetwork mirrors Jellyfin's default LAN set (NetworkManager with no LocalNetworkSubnets configured): loopback, the RFC 1918 ranges, fc00::/7 and fe80::/10. Notably that set omits 169.254.0.0/16, so Go's IsLinkLocalUnicast is deliberately restricted to its IPv6 half. IsLocal mirrors HttpContext.IsLocal(): the caller shares the connection's local address, not merely "is loopback". It falls back to the loopback check when the local address is unavailable. |
||
|
Deluan Quintão
|
6c3e7e268b |
feat(instant-mix): support album, playlist and genre sources (#5948)
* feat(agents): local agent genre-hint similar songs fallback
* feat(external): playlist instant mix via seed-track sampling
* test(external): cover playlist mix never-empty fallback and maxSeeds cap
Adds coverage for the empty-match seed fallback and the maxSeeds
call cap on GetSimilarSongsByTrack, per code review finding.
* feat(external): genre instant mix via seed-track sampling
* feat(external): album instant mix falls back to AudioMuse track similarity
* feat(external): artist instant mix falls back to seed-track sampling
* fix(jellyfin): route genre seeds through instant mix instead of empty
* feat(jellyfin): add /Albums/{id}/Similar route for albumMix radio
* perf(external): bound playlist seed sampling to a random N
samplePlaylistTracks loaded an entire playlist's joined rows just to keep
5 random seeds; push the bound and randomization into the query instead,
matching the other samplers (GetRandom/GetAllByTags with Max).
Fixing this surfaced a real bug: resetSeededRandom's SEEDEDRAND rewrite
assumed every table's id is TEXT, but playlist_tracks.id is an INTEGER
position, so the random sort silently dropped every row. Cast the id to
TEXT before hashing (no-op for the other, TEXT-id tables).
Also trims a changelog-flavored comment and a duplicated rationale in
server/jellyfin/similar_test.go.
* refactor(external): parallelize seed mix and dedup mix helpers
Run the up-to-5 per-seed GetSimilarSongsByTrack calls concurrently (errgroup),
route the four container cases through a shared seedMix helper, flatten the
genre lookup, and sample playlist seeds without forcing a smart-playlist
rebuild. Share the media-file->Song mapping in the local agent.
* perf(agents): use the indexed genre filter for local similarity
Replace GetAllByTags (a json_tree scan of every media_file row) with the
media_file_tags semi-join from #5940, deriving the seed's genre tag ids
locally since they hash from (name, value).
Also carry the library id and the recording MBID on the returned songs:
the matcher resolves by id first and looks up mbz_recording_id, so the
release-track id it got before matched nothing and the local fallback
silently returned no songs.
* refactor: drop redundant MBID and fold mixFromSeeds into seedMix
The local agent returns library tracks, so the id alone resolves them in the
matcher's first phase; the MBID was never consulted. mixFromSeeds had no
caller other than seedMix.
* docs: trim redundant comments
* fix(jellyfin): adopt the GUID id codec in the merged similar routes
getSimilarAlbums still used resolveItemID/DecodeID, which #5942 replaced with
itemIDParam; its tests passed raw ids that the strict codec now rejects.
* fix(external): guard non-positive counts and blend every seed
A negative Subsonic count reached matched[:count] and panicked. The matcher
also keeps input order and stops at count, so seed-grouped results let the
first seed fill the whole mix; interleaving gives every seed a share.
Drops the duplicate playlist-track mock in favour of tests.MockPlaylistTrackRepo,
which pages like the real repository and records the query options.
* fix(external): refresh smart playlists before sampling seeds
A smart playlist materializes no playlist_tracks until it is evaluated, so
sampling without the refresh mixed an empty seed set. The refresh is a no-op
for regular playlists, inside the refresh delay, and for non-owners.
* fix(external): skip missing tracks and a nil playlist-track repo when sampling
Tracks() logs and returns a nil repository when its own lookup fails, so the
chained GetAll panicked. Seeds can also reach the mix verbatim when the agents
find nothing, so a missing file would surface as an unplayable entry.
* fix(jellyfin): never report the seed album as its own similar album
The sampled-seed fallback returns the album's own tracks, which similarAlbums
mapped straight back to the requested album, often as the only result.
* test(agents): assert the genre predicate instead of relying on the mock
MockMediaFileRepo ignores QueryOptions.Filters, so the spec passed even with
no genre filter at all. It now checks the generated predicate carries the
seed's own tag id, the indexed join and the missing exclusion.
* fix(external): clamp the requested count before it becomes a query limit
Subsonic passes the client's count through unbounded. At MaxInt64 the local
agent's count+1 overflows negative, and GetRandom omits the SQL limit unless
Max is positive, so one request would hydrate every matching track. 500 is
what the widest caller (similarAlbums, limit*5) legitimately asks for.
* fix(external): deduplicate playlist seeds by media file
A playlist can hold the same file at several positions, so sampling its rows
could seed the mix twice: a wasted agent call, and a duplicate track whenever
the seed fallback kicks in.
* fix(external): drop tracks two seeds both recommend
The matcher re-emits a track when two inputs are identical, so overlapping
recommendations took several slots in the mix. Match the whole merged set and
dedup before trimming. Playlist sampling now over-fetches before its own
dedup, so repeated positions cannot collapse the seed count.
* test(external): make the seed-blend assertion independent of the shuffle
It matched four tracks and kept two at random, so both could come from the
first seed once in six runs. Keeping three of the four makes a seed-two track
unavoidable.
* fix(external): seed artist mixes from every credited role
media_file.artist_id is the deprecated primary artist, so an artist credited
only on the album, as on compilations, sampled no seeds at all. Use the same
participant filter the artist listings use.
* refactor(external): drop the now-vestigial seed interleaving
Matching the whole merged set removed the early truncation the interleave
guarded against, and the shuffle before the trim makes input order irrelevant.
Its comment described the old behaviour.
* test(agents): give the id-mapping fixture a matching genre
The related track carried no genre, so the real query would never return it;
the spec only passed because the mock ignores QueryOptions.Filters.
* test(agents): drop the MBID from the id-mapping fixture
Local agent candidates are non-missing library rows, so the matcher always
resolves them in its id phase and never reads the MBID. The field guarded a
regression that could not change behaviour.
* test(agents): remove unnecessary comment about MBID in GetArtistTopSongs test
* fix(jellyfin): only let a not-found entity fall through in getInstantMix
Discarding the error conflated a genre id, which never resolves, with a real
lookup failure, which then made a provider call that fails the same way.
* test: pin the invariants the specs only appeared to cover
The missing filter was asserted by substring, so flipping it to true passed
everywhere, including the spec named for it. Matching the whole merged set,
the local agent's over-fetch, and its no-genres early return had no coverage
at all; each is now pinned by a spec that fails when the code is broken.
* test: make the remaining specs say what they actually guard
The playlist-track spec named a sort whitelist it does not exercise; it guards
the integer-id CAST, so it now asserts no rows are dropped. The maxSeeds cap
passed with either bound removed, and the over-fetch was pinned by its literal
value rather than the duplicate positions it exists for. Also drops setup the
count guard returns before reaching.
* fix(external): fall back when the agent's picks are not in this library
A non-empty answer whose songs are all absent locally matched nothing and was
returned as-is, so the mix came back empty with sampleable source tracks
sitting right there.
* refactor(external): name the agent-then-fallback flow once
Each entity case repeated the same error and emptiness plumbing around the
matcher. mixFromAgent states it once and each case supplies only what differs:
how to ask, and what to do when the answer is unusable.
|
||
|
Deluan Quintão
|
c66ef04dd3 |
refactor(jellyfin): emit real 128-bit GUIDs as item ids (#5942)
* test(jellyfin): use canonical ids in dto fixtures
Fixtures used short placeholder strings, which are not valid Navidrome ids. Deriving them from
id.NewHash keeps the labels readable while exercising the real id shape.
* test(jellyfin): use canonical ids in handler fixtures
Fixtures used short placeholder strings, which are not valid Navidrome ids. Deriving them from
id.NewHash keeps the labels readable while exercising the real id shape. Playlist entry positions
stay decimal, matching the integer playlist_tracks.id column.
* test(jellyfin): use canonical ids in e2e fixtures
Fixtures used short placeholder strings, which are not valid Navidrome ids. Deriving them from
id.NewHash keeps the labels readable while exercising the real id shape.
* test(jellyfin): use canonical ids in audiomuse fixtures
audiomuse_test.go passes ids as bare function args (mf(id, ...), call(query, user)) rather than
via ID: struct-literal fields, so the original grep-built file list missed it. Same conversion as
the rest of the fixtures: fake labels through id.NewHash via testID.
* test(jellyfin): convert remaining nonexistent-id sentinels in e2e tests
Reviewer swept for enc("literal") sites the brief's dto.EncodeID grep missed. These "does not
exist" fixtures must stay well-formed GUIDs under the strict codec, or the test degrades from
"resolves to nothing" to "empty path segment".
* refactor(jellyfin): emit real 128-bit GUIDs as item ids
Navidrome ids are now a canonical 22-char base62 encoding of exactly 128 bits, so they map
losslessly onto Jellyfin GUIDs. Previously the API hex-encoded the id string itself, producing
44 hex chars where Jellyfin uses 32.
Integer library ids, the synthetic playlists folder, and playlist entry positions (a
playlist_tracks.id, an integer column) aren't 128-bit values, so they get a reserved GUID space
tagged by kind. DecodeID is now strict: malformed input returns an empty string instead of
passing through unchanged.
BREAKING: Jellyfin clients see entirely new item ids.
* fix(jellyfin): 404 malformed playlist ids instead of silently creating
updatePlaylist decoded a malformed playlistId to "", the same sentinel core/playlists.Create
uses to mean "make a new playlist" — the overload createPlaylist deliberately relies on. A
malformed id now 404s before reaching Create.
Also tightens id-codec test fixtures: several tests set chi params to a raw canonical id, which
now decodes to "" and only passed because the fakes ignore the id argument; and a batch of
not-found sentinels now use well-formed-but-nonexistent GUIDs so they exercise the intended path
instead of the malformed-id path. READMEs "lossless" claim softened to note the reserved space.
* refactor(jellyfin): drop the id truncation workaround
Finamp's saved-queue packing keeps the first 16 bytes of each item id. That was lossy only
because our ids were 44 hex chars; now they are 32, so the packing round-trips exactly and the
server-side prefix recovery is dead code.
Removes an indexed range scan per restored queue and the ambiguous-prefix path that could
resolve to the wrong item.
* fix(jellyfin): emit ServerId and PlaySessionId in Jellyfin's id format
Jellyfin serializes GUIDs without dashes; ServerId was emitting the dashed UUID form. A
ServerId persisted before this change is normalized on read rather than rewritten.
PlaySessionId was emitting a raw internal id instead of the encoded form.
BREAKING: the ServerId change makes clients treat the server as new, so users re-login once.
* fix(jellyfin): 404 on undecodable id filters instead of widening the query
DecodeID collapsed an absent param and an undecodable one into the empty string, and downstream
an empty id means no filter. A client sending a stale pre-upgrade id therefore had its filter
silently dropped: ParentId, ArtistIds and AlbumArtistIds each returned the whole library instead
of a scoped result. Every existing client hits this on first launch after the id format changes.
Scalar id params now distinguish the two cases and report not-found. List-valued params already
failed closed. EncodeID logs a diagnostic when a non-empty id is not canonical, which should not
happen post-migration and would otherwise ship an unaddressable item silently.
* test(jellyfin): drop comments that restate the spec names
* refactor(jellyfin): decode reserved GUIDs from bytes, not hex strings
DecodeID already had the 16 decoded bytes, then re-derived the kind tag and payload by slicing the
hex string and parsing it a second time. Reading them off the byte slice matches how the format is
specified and removes the duplicate parse.
Bounding the payload inside encodeReserved gives both encoders the 32-char guarantee, which only
EncodePlaylistEntryID enforced before.
Playlist entries now decode through DecodePlaylistEntryID, which rejects other kinds. The tag was
being encoded and then discarded, so a song id passed as an EntryId reached RemoveTracks as a
playlist_tracks position.
Drops the per-field log.Warn from EncodeID: it sat in a leaf codec without a ctx and would emit
once per item per request on exactly the bad-data population it was meant to surface.
* refactor(jellyfin): make DecodeID report whether the id was decodable
DecodeID returned the empty string for both an absent param and an undecodable one, and
downstream an empty id means no filter. That conflation is what let a stale id widen /Items to
the whole library; it had been patched at two call sites, leaving three different policies for an
undecodable id in one package and ~16 handlers correct only because a repo Get("") happens to fail.
Returning (string, bool) makes the ambiguity unrepresentable, and the compiler forces each of the
~22 sites to decide. URL params share one itemIDParam helper that 404s; id lists go through
DecodeIDs, which is all-or-nothing because dropping bad entries would empty a list and make its
len() > 0 filter gate vanish — the original bug by another route.
A well-formed but unknown id is still 200 with zero results; only malformed ids 404. Malformed
ids now also 404 on the image and similar/instant-mix routes, which previously answered with a
placeholder or an empty list.
|
||
|
Deluan
|
8978c7b9fa |
Revert "feat(jellyfin): send a synthetic placeholder blurhash for unresolved artwork (#5941)"
This reverts commit
|