Fix review findings: concurrency, plugin sandbox, provider bugs, and dedup#254
Merged
Conversation
Lua plugin callbacks run on goroutines (hooks, timers, keybinds) and read playlist state through StateProvider closures, while the Bubbletea loop mutates the same *Playlist. The struct had no synchronization, so reads of p.tracks/p.order/p.pos raced with Next/Prev/Add/Remove/shuffle, and Current()'s unguarded p.tracks[idx] could panic on a stale index. Add a sync.Mutex and guard every exported method. Reads and writes are now serialized; no exported method calls another, so defer-unlock cannot deadlock. Fixes the high-severity track/player getter race and the playlist-mutation data race.
doHTTP passed plugin-supplied URLs straight to the HTTP client with no validation, letting a plugin reach loopback, RFC1918, and link-local 169.254.169.254 (cloud metadata), and follow redirects into those targets. Add an ssrfGuard on the dialer Control hook so every connection attempt (including redirects and DNS-rebound hosts) is checked against the resolved IP and rejected if non-public, and reject non-http(s) schemes up front. A per-plugin network permission gate remains a possible follow-up; it is omitted here to avoid breaking existing plugins that legitimately use http.
extractMetadata ran DoFile on the entire plugin file with full stdlib, so any top-level os.execute/io call ran unsandboxed the moment a user listed installed plugins. Apply the same luaplugin.Sandbox used by the runtime before executing the file. Exports luaplugin.Sandbox for reuse.
fs.read used os.ReadFile (whole file into memory) then sliced to 1MB, returning a silently truncated value as success and not bounding memory for huge files. Stream through io.LimitReader(maxSize+1) and return an explicit error when the file exceeds the limit.
isWriteAllowed validated paths lexically: the strings.Contains(abs, "..") check was dead (filepath.Abs already cleans "..", and it false-positived on filenames like "..bar"), and a symlink planted inside an allowed dir could redirect a write outside it. Resolve symlinks on the deepest existing ancestor of the target and on the allow dirs themselves before the prefix check. Also fixes the same gap in the exec cwd check, which reuses this function.
timer.after/every called CallByParam directly with no context timeout, so a runaway callback held the plugin mutex forever, blocking every other hook, keybind, and visualizer for that plugin. Add a shared callBounded helper that applies hookTimeout (mirroring invokeHook) and use it from both timer paths.
RenderVis runs on the UI render loop and called CallByParam with no timeout while holding the plugin mutex; a hanging render() froze the UI with no escape. InitVis/DestroyVis had the same gap. Route all three through callBounded so a misbehaving visualizer times out and render falls back to the previous frame.
Emit spawned untracked goroutines that call CallByParam on a plugin LState. Close stopped timers/execs then closed every LState without waiting, so a UI event dispatched just before shutdown could run against an already-closed LState (p.L.Close does not take the plugin mutex). Track Emit goroutines in a WaitGroup, set a closing flag under mu to reject late dispatch, and Wait before closing the LStates.
renderFirework/renderBubbles/renderSakura compute dotCols = PanelWidth*2 and then mod by dotCols/dotRows/len(bands); on a narrow terminal PanelWidth can be 0, so seed % uint64(dotCols) panicked and crashed the TUI. Add the same dotRows<4 || dotCols<4 early-out the braille-grid renderers already use.
Record discarded loadLocked's error and proceeded as if history were empty, so a one-off read failure (permission/I-O glitch) made saveLocked atomically rewrite the file with only the new entry, destroying all prior rows. Propagate the error instead, leaving the on-disk file untouched.
saveLocked ignored errors from writeEntry's formatted writes, so a failure mid-write (e.g. ENOSPC) could still rename a truncated temp file over the real history. Thread writes through an errWriter and abort the rename when a write fails.
The noise regex matched official/lyric/audio/video as bare substrings followed by .*, so 'Videotape', 'Audioslave', and 'Video Games' were cleaned to empty or truncated queries, breaking lyric lookups. Restrict bare-label stripping to genuine trailing labels (dash suffix, or 'official video/audio' and 'lyric(s) video' phrases). Add regression test cases.
Client.token, userID, and albumCache were lazily read and written from concurrent tea.Cmd goroutines (Playlists, album browse, SearchTracks all run in their own goroutines) with no synchronization, a data race on the lazy auth writes and the cached slice. Add a mutex guarding those fields, taken only around field access and never across network I/O, with double-checked auth so at most a redundant (not racy) auth can occur.
handleConn's read loop only observed the 60s per-request read deadline, never s.done, and Close closed only the listener. A still-connected client (e.g. a vis-bands polling client) kept its handler goroutine alive, so Close blocked on wg.Wait for up to 60s. Track live connections and close them in Close so their scanner.Scan unblocks immediately; addConn shares the conn mutex with the done check to close the accept-during-shutdown race.
The reply/timeout/shutdown select was open-coded in load/theme/vis/bands/ status and waitReply was used elsewhere with a generic 'timeout' message. Parameterize waitReply with a label and timeout and use it everywhere, which also gives the previously-generic commands specific timeout messages.
acceptLoop swallowed every non-shutdown Accept error and retried at 10/s, treating a permanently closed listener as transient. Return on net.ErrClosed and log other errors before backing off.
The jump enter handler called notifyPlayback (MPRIS only) plus notifier.Seeked, skipping plugin notification. Use finishSeek, which calls notifyAll, so a jump seek emits the playback-state event to plugins like every other completed seek. Seek stays synchronous (immediate seek), matching the existing test.
Args runs on the startup path and called sniffFeedURL, which did a HEAD on the 30s feed/M3U client, so one slow CDN could stall cliamp startup for up to 30s during pure URL classification. Give the sniff a dedicated 5s client.
An empty 200 response left download returning a non-nil empty slice, so Install wrote a 0-byte plugin file. Treat an empty body as a download error so the next candidate URL is tried (or the install fails cleanly).
A raw URL ending in '/' made path.Base yield '.' or '/', producing a degenerate plugin filename. Return a clear error instead.
The override only set LowPower when the flag was true, so --low-power=false could not disable a config.toml low_power=true. Assign the flag value directly, matching the other boolean overrides.
wireMediaCtl's error was silently discarded in TUI mode, so a dbus/MPRIS setup failure left media-control silently disabled with no trace. Log it to the app log (not stderr, which would corrupt the TUI).
cliamp.player.eq_preset(), visualizer(), and theme() were documented but never registered in the player API (calling them errors). Implementing them would require exposing model state to plugin goroutines, which would add a data race; remove them from the docs so the reference matches the API.
A bands table missing entries silently zeroed the unset bands. Require all 10 values and raise an arg error otherwise so partial input can't corrupt the EQ curve.
EmitKey spawned fire-and-forget goroutines per keypress that were untracked and not gated on shutdown, so a keypress during Close could call into a closed LState. Track them in the manager WaitGroup and skip dispatch once closing, matching Emit.
Each volume Change spawned its own goroutine to send SetVolumeMsg, so rapid changes could be delivered out of order and an older volume win. send (prog.Send) is goroutine-safe and non-blocking, so call it directly.
trackid was a fixed constant for every track, so SetPosition ignored its trackID argument and would seek whatever track became current after the client read its position. Assign a unique track object path per track change and ignore SetPosition when its trackID does not match the current track.
save() truncated the real favorites file with os.Create and ignored every write error, so a partial write could lose all favorites. Build the content in memory, write a temp file, and rename so a failed write can't corrupt the existing file and the error surfaces to the caller.
ToggleFavorite discarded the error from Add/Remove (which persist favorites), so a failed save was silently swallowed. Return it to the caller.
The mutex around appending the Your Music entry only touched the function-local 'all' slice, guarding nothing shared (the other locks in the loop legitimately guard trackCache).
Playlists returned the cached slice directly (both on cache hit and after building it), so a caller mutating the result corrupted the cache. Return a clone on both paths.
Tracks handed out the cached track slice directly, so a caller mutating it corrupted the per-playlist cache. Clone on both the cache-hit and freshly built return paths. (The snapshot-based invalidation within the playlist-list TTL is an intentional caching tradeoff and is left as-is.)
On the final retry the loop slept the full backoff (up to 128s) and then returned the rate-limited error without making another request. Break out immediately on the last attempt.
A negative offset passed the offset>=len check and then panicked at out[offset:end]. Clamp offset to 0 first, matching the Emby client.
MoveQueue swaps any two positions, not only adjacent ones.
math.Min(0.65, 0.55) is a constant 0.55; replace with the literal and drop the now-unused math import.
resp.Code is NetEase's application-level status, not an HTTP status; it was compared against http.StatusOK which only coincidentally shares the value 200. Introduce neteaseCodeOK and use it at all four comparison sites.
humanizeBasename left a trailing extension (e.g. .mp3) in the title derived from a URL basename. Drop a recognized audio extension first, leaving non-media dotted suffixes untouched.
resolveFeed decoded the response body with no size limit. Wrap it in an io.LimitReader (32 MB) so an oversized or malicious feed can't exhaust memory.
Playlists and Tracks handed out the internal cached slices, so a caller mutating the result corrupted the cache. Return slices.Clone on the cache-hit and freshly built return paths.
decodeFFmpeg wrapped the exec error but dropped ffmpeg's stderr, where the actual failure reason is written. Surface ExitError.Stderr in the message.
beep.Streamer already declares Err() error, so the local errorer type assertion was redundant. Call ss.s.Err() directly, matching the eq and volume streamers.
When the scheduled stream reconnect fired, the tick handler returned early with only playTrack + tick, discarding the seekCmd/lyricCmd computed earlier in the same tick. Fold them into the reconnect batch.
The footer only showed the filtered count while the search input bar was open (searching); once a filter was committed with Enter it fell back to the full count. Add navFilteredTotal and use the filtered count whenever a query is active, across the artist, album, and track footers.
internal/control duplicated the message types in internal/playback and had no importers (only its own test); the live code uses internal/playback. Remove it.
Navidrome, Plex, Jellyfin, and Emby cache playlist/track (and album) data but none implemented playlist.Refresher, so the UI refresh key was a silent no-op for them. Add Refresh to clear the caches (including the jellyfin/emby client album cache) so the next fetch hits the server.
The [[section]] parse loop was duplicated three times (radio loadStations, radio loadFavoriteStations, local loadTOML). Extract tomlutil.ParseSections and rewrite all three against it. Behavior is preserved, including the single-section-type assumption of the original parsers.
The emby and jellyfin clients were ~95% identical (~700 duplicated lines) and had already drifted (jellyfin lacked the negative-offset guard and error wrapping). Extract one shared Client into internal/embyapi, isolating the real differences behind a dialect: auth header scheme (Emby Authorization vs Jellyfin X-Emby-Authorization), ping endpoint, user-id discovery, error prefix, and metadata key. emby and jellyfin become thin wrappers (NewClient, IsStreamURL, type aliases) plus their providers. Client tests are consolidated in embyapi: shared behavior once, dialect specifics per dialect. The client now uses a per- instance http.Client (SetHTTPClient) instead of a package global, which is what makes it testable from the provider packages.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (55)
📝 WalkthroughWalkthroughThis PR consolidates Emby and Jellyfin HTTP client implementations into a shared ChangesEmby/Jellyfin consolidation, playlist concurrency, and plugin lifecycle
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the findings from a full-codebase review. 48 commits, one fix per commit. Build,
go vet, and the full test suite (with-raceon the concurrency fixes) are green.High severity
Playlisthad no synchronization, so Lua plugin goroutines reading state raced with the Bubbletea loop mutating it, andCurrent()could panic on a stale index. Added a mutex guarding every exported method.cliamp.http— plugin URLs went straight to the HTTP client. Added a dial-time guard rejecting loopback/RFC1918/link-local (cloud metadata) and a scheme allowlist; also covers redirects.Medium severity
plugins listmetadata extraction (was running plugin top-level code unsandboxed).fs.readerrors on oversized files instead of silently truncating.hookTimeout(a stuck callback could freeze a plugin / the UI).tea.Cmdgoroutines).cleanQueryregex from erasing titles like "Videotape"/"Audioslave"/"Video Games".Low severity (selection)
set_eq_presetbands.SetPositionrejection.waitReplydedup.AlbumListnegative-offset guard;Refresh()implemented for the self-hosted providers (was a silent no-op).internal/controlpackage; assorted doc/idiomatic cleanups.Refactors
[[section]]parser (tomlutil.ParseSections) replacing three copies.internal/embyapi) replacing ~700 duplicated lines; the real differences (auth scheme, ping endpoint, user-id discovery, error prefix, metadata key) are isolated behind a dialect.emby/jellyfinare now thin wrappers; external API unchanged.Deliberately not included
fs.read/httppermissions) — left as-is to avoid breaking existing plugins.navBufferunbounded growth — by design (retained for seek-back).Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Improvements