The codebase is in notably good shape for its size: all Go tests pass, go vet ./... is clean (re-run by the lead on the reviewed baseline), the web auth/token design is solid, the signed-manifest updater core uses standard crypto correctly, and the 7z extractor is genuinely hardened. However, this audit found 154 findings concentrated in four systemic weaknesses:
helper.log instead of the terminal (Critical), health is acknowledged before the TUI initializes, the swap protocol has crash windows with no launchable executable, and integrity checks run after executing staged files.BLAKE3SUMS.txt is incompatible with the documented b3sum -c command.| Area | Critical | High | Medium | Low | Info |
|---|---|---|---|---|---|
Web server/API/security (pkg/web) | 0 | 2 | 5 | 6 | 1 |
Installer (pkg/installer) | 0 | 4 | 8 | 4 | 1 |
| Self-update / release stack | 0 | 5 | 12 | 10 | 1 |
TUI (pkg/tui) | 1 | 9 | 15 | 6 | 1 |
Core utilities (pkg/platform, config, keyring, …) | 0 | 1 | 9 | 5 | 0 |
Web frontend (internal/webassets) | 0 | 0 | 6 | 4 | 0 |
| Supporting packages + main | 0 | 2 | 10 | 11 | 0 |
| Build / CI / dependencies / docs | 0 | 3 | 8 | 3 | 1 |
| Total | 1 | 26 | 73 | 49 | 5 |
go vet ./... (exit 0) and go test ./... (all packages pass; exit 0). Every finding is therefore a latent defect not currently caught by the suite — the test-gap pattern is itself a documented finding (§7).helper.log, stealing terminal inputThe TUI hands off via LaunchUpdateHelper("tui"); the helper's stdout/stderr are redirected to helper.log, and the updated binary is launched with those same descriptors while stdin remains the terminal. The relaunched Bubble Tea program puts the terminal into raw mode and consumes keystrokes while all frames/escape sequences go invisibly to the log. To the user it looks like a return to the shell whose input is being eaten by a hidden TUI. The commit can already be permanent at this point.
Fix: preserve/reopen the controlling terminal output handles for the relaunched TUI (Windows-safe CONOUT$ path included); keep helper diagnostics on a separate descriptor. (Lead-verified in source.)
The helper executes target.StagedPath and later the installed target.Path for identity validation before verifyUpdateArtifact/post-swap BLAKE3 checks. Anyone able to replace a target-adjacent file during the parent-exit window gets code execution at helper privilege; the mismatch is only detected afterward. Can cross a privilege boundary when an elevated primary update includes a user-writable secondary.
Fix: size+BLAKE3 before every exec; protected staging (open handle / verified inode) and unsafe-directory rejection.
Apply renames the installed executable to backup, then renames the stage into place; a kill between the two leaves the primary pathname absent, and the advertised startup recovery can't run because there is nothing to launch. Rollback repeats the pattern (remove replacement, then rename backup).
Fix: durably sync the backup without removing the live pathname; platform-atomic replacement (rename over existing path on Unix, ReplaceFile on Windows).
The acknowledgement is written after arg parsing — before platform detection, release fetching (up to 3 manifest attempts), model construction, or tea.NewProgram(...).Run(). With only a 1.5 s stabilization window, the helper can permanently commit an update whose child is still blocked in startup networking or about to fail during TUI init. The qualifier cannot catch this (its fixture only acknowledges and sleeps).
Fix: acknowledge from a real post-first-render readiness event; failures before that trigger rollback. (Also flagged High by two other reviewers — consolidated.)
rollbackUpdateTargets silently continues when the backup stat reports not-exist, including for applied targets. AV/disk/cleanup removing a backup after apply ⇒ rollback "succeeds", journal says rolled_back, helper relaunches the rejected binary; recovery can claim success while the primary is absent.
Fix: missing backup for an applied target = rollback failure; verify restored bytes/identity before recording state.
The manifest deliberately emits legacy URL/hash fields for old clients, but the historical v1.2 decoder performs no signature verification and trusts the BLAKE3 from the same document — an attacker controlling the manifest response supplies both binary and matching hash.
Fix: stop describing legacy bootstrap as signed; use pinned detached signatures or manual replacement; retire the automatic path after migration.
ValidateCustomInstallPath accepts any existing absolute writable non-root non-%WINDIR% directory; uninstall then enumerates and RemoveAlls every top-level entry except portable_config/updater/manager files. Choosing e.g. C:\Users\name\Downloads as install destination makes uninstall erase unrelated files; per-entry errors are warnings and the function still returns success. (Lead-verified in source.)
Fix: ownership manifest at install time; uninstall only manifest-owned paths; fail if cleanup incomplete.
Active exe is renamed to ffmpeg.old, replacement copied directly (not staged+atomic); on copy failure rollback removes the destination and renames the backup back — but a deferred cleanup unconditionally deletes ffmpeg.old, including when that rename failed (AV/sharing race on Windows).
Fix: retain backup until restoration positively confirmed; destination-adjacent staging + atomic replace.
InstallMPVConfigWithOutput logs-and-continues on both preservation and backup failures, then os.WriteFiles over mpv.conf; failure reapplying saved settings is downgraded to a warning after commit. Tests encode the continuation.
Fix: fail closed before replacement; staged+fsynced atomic replacement; failure rollback.
hdiutil attach output is ignored; the code assumes /Volumes/IINA/IINA.app. A pre-existing /Volumes/IINA makes macOS mount the verified image as /Volumes/IINA 1, so the installer validates and copies the wrong volume's app (possibly attacker-prepared), then ad-hoc signs and launches it. Cleanup detaches the hard-coded path, not the attached device.
Fix: attach with -plist -nobrowse, parse device/mount point, validate/copy/detach that exact mount.
The timeout branch's unlabeled break exits only the select, not the collection loop — with two probes still blocked, the next iteration waits on resultChan forever; probes use exec.Command (not CommandContext) so the timeout can't kill a stuck package manager. Runs synchronously during startup and under the cache write lock on refresh. (Lead-verified in source.)
Fix: bounded context per probe, labeled timeout exit, build replacement cache outside the lock.
CancelJob archives/removes the job immediately; CreateJobIfNoneActive then accepts another operation for the same method while the original goroutine still executes (worker calls no-op; several workers have final config writes after their last cancellation check). Cancel-and-retry can overlap install/uninstall/adopt transactions.
Fix: explicit cancelling state; keep job active until worker acknowledges termination; final context check before every commit.
The global Escape branch resets StateInstalling models before the state-specific branch that calls PreparedSelfUpdate.Abort can run; workers use context.Background and keep running; stale completions can be attributed to a new selection; an escaped prepared self-update retains its lock/transaction until process exit.
Fix: single operation state machine that cancels, drains, aborts, and only then returns to menu.
Worker sends buffered installDone then closes outputChan; once both are ready startStreaming selects nondeterministically — a closed-output win leaves the UI stuck "installing" forever; a completion win silently drops remaining output.
Fix: one ordered event channel (closed only after a final result), or receiver tracks closed channels until drained + exactly one terminal result.
executeInstallCmd accepts/logs installDir but never applies it (changing the custom path mid-session installs to the startup path while recording the new one); updateItem drops backend AppID/InstallPath; uninstall uses the global destination — on Windows this can update/delete a different MPV installation than selected.
Fix: carry stable app ID + path; CloneForOperation with immutable snapshot for every operation.
Selects the first MPV app silently, runs the full install path with isUpdate=false, then saveInstalledApp can add a duplicate record instead of updating the selected app's UI type.
Fix: dedicated TUI UI-change operation calling InstallUISafely against the selected app's config dir.
mpv.confbackupMPVConfig renames the active config away before SetConfigValue, which then sees no file and writes fresh embedded defaults — unrelated user settings vanish; a same-day second apply can overwrite the date-named backup with the generated config. Apply returns no error to the state machine, so failures still show "preferences saved".
Fix: unique high-resolution copy backup without removing the source; surface errors before success.
Enter intended to accept a Bubbles filter can immediately start an install/uninstall; some handlers pass Enter to list.Update (changing filter state) and process it as an action; async filter matching can act on a stale item.
Fix: centralize list key routing; never dispatch application Enter/Escape while a filter is being set.
The UI says Ctrl+C cancels; the handler clears a few fields and tea.Quits — no context cancel, no worker join; child package-manager processes can outlive the TUI; a prepared manager transaction is not aborted. (Consolidated with the Web/TUI shutdown finding — §6 theme 2.)
Fix: first interrupt cancels and waits (bounded) for terminal worker result/rollback; second interrupt force-quits.
Without a keyring password, RunSudoCommand falls back to interactive sudo while Bubble Tea's input reader stays active in raw/alternate-screen mode — missing/split password input, broken masking, or a hung install; exactly the first-Linux-install case.
Fix: pre-authenticate via a secure TUI flow or use Bubble Tea's exec/suspend so the child exclusively owns a restored terminal.
mpv.conf missing/partial, and same-day backups collideRestore renames the current config away before validating/reading the source, with no rollback on any failure; the copy is ReadFile+non-atomic WriteFile; safety backups are date-precision and can be overwritten by a second same-day operation.
Fix: one restore transaction under the path lock — validate, unique fsynced snapshot, atomic replace, restore snapshot on every failure.
The manifest job receives only generator artifacts; it reconstructs registry URLs, re-downloads the bytes, and signs those. A duplicate upload, registry writer, or concurrent publication that changes the registry object before signing makes the protected signer authenticate bytes this pipeline never built (GitLab allows duplicate generic-package files by default).
Fix: pass raw build artifacts to the manifest job, sign those local bytes, upload once, then byte/hash-compare published objects before signing.
Every tag (not only protected semantic v\d+\.\d+\.\d+) matches release rules; no duplicate-file preflight or resource_group; concurrent/retried tag pipelines can republish a tag or race the stable-channel PUT; docs assume protected variables but don't require a protected v* tag rule.
Fix: strict tag grammar + CI_COMMIT_REF_PROTECTED; fail if a tag file already exists; serialize stable publication.
Release needs only publish:release-manifest; Linux cross-compilation can publish Windows/macOS artifacts with no machine-verifiable evidence that Authenticode/notarization/native smoke tests passed (acknowledged v1.3 publication blocker, restated for completeness).
Fix: gate release creation on native jobs that sign/verify/smoke-test the exact artifacts, or a protected manual approval recording immutable qualification evidence.
| ID | Finding | Refs |
|---|---|---|
| W-M1 | Cached sudo credentials let any submitted password "pass" validation (sudo -S -v without -k) | api_keyring.go:84-121; command_runner.go:194-229 |
| W-M2 | Backup path validation follows intermediate symlinks outside the backup root | config/validation.go:17-55, 93-99; api_config.go:586-635 |
| W-M3 | Failed restore can leave active mpv.conf missing (see H-20) | api_config.go:586-608 |
| W-M4 | Global locale cache not concurrency-safe (unsynchronized lazy load; shared backing slice sorted in place) | locale.go:13-33, 57-75, 111-123, 305-313 |
| W-M5 | Multi-setting writes validated together, committed independently (up to 22 writes; partial apply on mid-failure) | api_config.go:113-466; api_languages.go:28-120 |
| ID | Finding | Refs |
|---|---|---|
| I-M1 | Windows discovery executes untrusted candidate binaries during a read-only scan (no context/timeout; PATH/registry-supplied paths) | windows_detection.go:69-105, 148-164 |
| I-M2 | Uncertain probes treated as "not installed" and records synced away; method-keyed identity collapses multiple installs; locale-varying output parsing | detection.go:167-465; config/config.go:711-744 |
| I-M3 | MPC-QT install uses Start-Process without -Wait -PassThru; returns before installer completes | windows_mpcqt.go:516-572 |
| I-M4 | Restore moves active config away before validating/staging replacement (dup of H-20) | installer.go:1165-1190 |
| I-M5 | No destination-scoped exclusion between installer operations — different methods can concurrently mutate shared config/UI paths | installer.go:72-120; common.go:367-390 |
| I-M6 | ZIP/tar extraction lacks the 7z preflight policy (no entry/size/symlink policy; direct extraction into live destinations) | installer_archive.go:15-68; real.go:191-253 |
| I-M7 | Runtime rollback transactions not crash-durable (in-memory state; .txn-*/.bak-* debris not recovered at startup) | file_transaction.go:23-205; ui_config_update.go:269-488 |
| I-M8 | Cached sudo timestamp issue (dup of W-M1) | command_runner.go:169-229 |
| ID | Finding | Refs |
|---|---|---|
| U-M1 | Target-directory metadata not durably ordered with journal (no dir fsync after renames; recovery doesn't revalidate committed targets) | transaction.go:491-540, 803-870 |
| U-M2 | Crash before first journal write permanently blocks future updates (orphan dir never cleaned) | transaction.go:225-318, 905-916 |
| U-M3 | PrepareSelfUpdateFromCheck accepts a forgeable mutable DTO (any non-empty ManifestKeyID = "authenticated"; qualifier proves HTTP/synthetic values accepted) | version.go:84-210; transaction.go:175-203 |
| U-M4 | No freshness/anti-replay on signed metadata (replay older-higher release or suppress security updates) | manifest.go:65-71, 229-245 |
| U-M5 | Static single-key authorization; no revocation/threshold/validity epochs | manifest.go:54-71, 191-224 |
| U-M6 | Stale/tampered manager_bin_path secondary can overwrite an unrelated regular file (no identity proof) | config.go:692-713; transaction.go:151-168, 265-284 |
| U-M7 | Detached helper failures never reach the initiating UX (Web job completes before any replacement occurs) | transaction.go:328-500; api.go:278-301 |
| U-M8 | Health-failure rollback races a still-running child on Windows (kill without wait) | transaction.go:479-488, 620-657 |
| U-M9 | Release-generator downloads: no whole-transfer deadline, unbounded body, Proxy nil | generate-info/main.go:56-68, 546-634 |
| U-M10 | Qualifier's hidden --qualifier-driver mode destructive outside controller (no capability token/containment) | qualify-selfupdate/main.go:97-126, 299-381 |
| U-M11 | BLAKE3SUMS.txt not in b3sum -c format (blake3: prefix; documented verify command unusable) | gen-checksums/main.go:49-90; CI :609-623 |
| U-M12 | Auto publication promotes unreviewed upstream "latest" releases into signed trust | generate-info/main.go:358-448, 1202-1269 |
| ID | Finding | Refs |
|---|---|---|
| T-M1 | App-update bookkeeping uses stale menu ID "updates"; successful updates stay at old version and are re-offered | models_update.go:43-48, 614-618, 999-1037 |
| T-M2 | Persistence failures ignored or printed straight to stdout (corrupts frames); success rendered anyway | models_update.go:545-596, 977-989 |
| T-M3 | Restore removes active config before replacement durable (dup of H-20) | models_update.go:1588-1606 |
| T-M4 | Detached worker panics bypass Bubble Tea terminal recovery (10 raw goroutines, no recover) | models_messages.go:180-309 |
| T-M5 | Progress bar fed 0–100 values to ViewAs, which expects 0–1 (≥1% renders as 100%) | models_update.go:491-503 |
| T-M6 | Output not keyboard-scrollable; mouse mode never enabled; auto-scroll never reset | models_views.go:124-128; models_update.go:679-689 |
| T-M7 | Output rendering unbounded and quadratic (full transcript re-joined per line) | models_init.go:345-348 |
| T-M8 | Resize omits many active lists; fixed deductions without minimums | models_update.go:360-394 |
| T-M9 | "Install to PATH" broken on Windows (no .exe, PATH untouched); non-idempotent Unix aliases; failures reported as success | models_messages.go:77-309 |
| T-M10 | Job history not safe across TUI/Web processes (§6 theme 4) | job_history.go:91-98 |
| T-M11 | Ctrl+V overlay shows hard-coded 0.1.0/Go 1.21/linux/amd64 | models_views.go:736-755 |
| T-M12 | Offline startup not offline-first (~33 s of failed fetches before first render; network actions not gated) | main.go:209-238 |
| T-M13 | Reset erases data after backup failure while claiming "backup saved" | reset_data.go:33-47 |
| T-M14 | Unicode search backspace removes one byte, not one character | language_preferences.go:264-268 |
| T-M15 | Raw subprocess output rendered without stripping terminal control sequences (CSI/OSC injection) | models_update.go:505-511 |
| ID | Finding | Refs |
|---|---|---|
| P-M1 | Backup validation symlink/race escape (dup of W-M2) | config/validation.go:33-99 |
| P-M2 | Setters mutate memory then fail to persist without rollback; reset ignores backup failure | config/config.go:459-1090 |
| P-M3 | mpv.conf editor truncates quoted values at #; edits first-not-effective duplicate | config/editor.go:46-213 |
| P-M4 | Linux GPU detection truncates models at first (; skips other probes when glxinfo returns anything; empty PCI-ID stub | platform/gpu.go:85-1218 |
| P-M5 | macOS AV1 over-reported (unknown models fail open; M2 wrongly grouped with M3+; CGO-off heuristic shipped) | platform/gpu_darwin.go:22-64 |
| P-M6 | Linux arm64 CPUs can be labeled x86-64-v2 (asimd unrecognized) | platform/cpu.go:38-177 |
| P-M7 | Job history cross-process safety (§6 theme 4) | jobhistory/jobhistory.go:64-157 |
| P-M8 | Hotkey round-trip corrupts quoted # args; # accepted as a key and serializes to a comment | hotkeys/inputconf.go:109-220 |
| P-M9 | Locale data/selection disagree (hif/te/fil unresolvable; wrong regional assignments; es-419 flag) | locale/selection.go:49-111; locales.json |
| ID | Finding | Refs |
|---|---|---|
| F-M1 | Stored-password installs dispatch the privileged operation twice (callback invoked and true returned; 409 corrupts button state) | password-modal.js:398-416; install.js:287-315 |
| F-M2 | Lost SSE events never reconciled after reconnect (no IDs/replay/snapshot; stale jobs/badges/buttons until refresh) | jobs.js:27-344; sse.go:35-49 |
| F-M3 | Config apply marks concurrent edits as saved (baseline rebuilt from current controls, not submitted payload) | config-page.js:74-108 |
| F-M4 | UI option save/reset can overwrite newer intent (no generation/abort per key) | ui-settings.js:424-590 |
| F-M5 | Regional-language cancellation loses the AbortController; out-of-order responses render wrong variants | languages.js:424-887 |
| F-M6 | Alpine job modal opens focus controller while still hidden (focus targets rejected as invisible) | base.html:32-45; jobs-modal.js:49-78 |
| ID | Finding | Refs |
|---|---|---|
| S-M1 | Persistent SSE handlers defeat graceful HTTP shutdown (2 s timeout → "failure", exit 1) | main.go:345-387; server.go:229-254 |
| S-M2 | Command parsing misroutes positionals/conflicting modes (--verbose cli starts Web; internal flags leak into usage) | main.go:56-99, 162-184 |
| S-M3 | --verbose/--debug don't produce promised console logging | main.go:127-135; logger.go:48-196 |
| S-M4 | Accepted string values not round-trip safe (" #" split corrupts quoted values) | scriptopts/options.go:29-75 |
| S-M5 | BOM misread as part of first key; edits produce mixed CRLF/LF | scriptopts.go:73-280 |
| S-M6 | Atomic replacement destroys symlinks and original metadata (mode forced 0644) | fileops.go:66-102 |
| S-M7 | Config locking doesn't cover stale public snapshots or multiple processes | fileops.go:20-64; scriptopts.go:73-159 |
| S-M8 | Job history cross-process (dup of P-M7) | jobhistory.go:64-157 |
| S-M9 | CLI --path ignored by component dispatch but recorded as InstallPath | main.go:534-663 |
| S-M10 | Browser launch/banner race the actual bind; httpServer assign/read race | main.go:345-375; server.go:116-235 |
| ID | Finding | Refs |
|---|---|---|
| B-M1 | Distributed archives omit LICENSE and third-party notices | .gitlab-ci.yml:331-392 |
| B-M2 | Windows resource generation stale/fail-open/incomplete for arm64 (|| true; committed 1.0.0.0/Win7) | Makefile:64-67; winres.json |
| B-M3 | Standalone make release-build works without trust ring → release binaries that reject all metadata | Makefile:51-54, 197-227 |
| B-M4 | Release generator unbounded downloads (dup of U-M9) | generate-info/main.go:56-67, 594-632 |
| B-M5 | No live-browser job/SSE E2E in CI (jsdom/fakes only) | vitest.config.mjs:3-7; TESTING.md:129-155 |
| B-M6 | Release jobs execute mutable container-image tags (alpine:latest, release-cli:latest) | .gitlab-ci.yml:51-553 |
| B-M7 | Tag grammar validated only after public package publication begins | .gitlab-ci.yml:191-335 |
| B-M8 | BLAKE3SUMS.txt format (dup of U-M11; cmd/gen-checksums has no tests) | gen-checksums/main.go:49-90 |
check-updates/refresh-installed/settings/shortcut)ui_type unvalidated on install/update → invalid durable stateFileInfo)NativeSigning fields not enforcedPostInstallMessage normalized for history only, not control flowStateSuccess, installSuccessMsg, …)volume-up describes decrease, etc.)formatKeys builds template.HTML from unescaped fragments (trust-boundary hazard)uoscconf.KnownOptions exposes mutable global slices (ModernZ deep-copies; divergence)os.Exit bypasses deferred cleanup; cancellation exit codes inconsistent--help/--version --json initialize the file logger (stateful identity probes)1.0.0.0make lint installs golangci-lint@latest, not mirrored in CImpv.conf moves the original away before the replacement is validated/durable, with fail-open backups and date-collision names.sudo -S -v without -k accepts any password while credentials are cached; both duplicated code paths share the bug.Store mutexes don't span processes; fixed .tmp name enables lost records between Web and TUI.mpv.conf, input.conf, and script-opts each split on # without tracking quotes — three independent implementations of the same bug.All findings are latent (suite green, vet clean). The reviews consistently traced why: the highest-risk seams are precisely the untested ones — stored-password double dispatch, SSE reconnect reconciliation, cancel/retry overlap, stream channel races, terminal descriptor inheritance, quoted-# round-trips, b3sum -c compatibility (cmd/gen-checksums has no tests), cross-process history, and any browser-level E2E for jobs. Several existing tests encode the defective behavior (backup-failure continuation, glxinfo truncation, #-key acceptance, regionless country-filter leak), so fixes will need expectation changes, not just new tests.
crypto/rand token, constant-time compare, HttpOnly/SameSite=Strict cookie, Host allowlist + exact Origin match, OPTIONS rejected, loopback-only listener without DNS resolution, 1 MiB body cap, header-count/timeouts. No bypass found.os.Root.O_EXCL staging, correct flock/LockFileEx serialization.html/template + textContent/x-text/escapeHtml discipline held everywhere reviewed; no eval/new Function/document.write; vendor sync byte-for-byte with CI freshness checks.sudo -k), theme 4 (cross-process history lock), locale cache race (W-M4).b3sum -c-compatible checksums, notices in archives, pinned digests, browser E2E.| Area | Artifact | Counts |
|---|---|---|
| Web security/correctness | .opencode/reviews/web-security-correctness-review.md | 2H/5M/6L/1I |
| Installer | .opencode/reviews/installer-deep-review.md | 4H/8M/4L/1I |
| Self-update/release stack | .opencode/reviews/self-update-release-version-stack.md | 5H/12M/10L/1I |
| TUI | .opencode/reviews/tui-deep-correctness.md | 1C/9H/15M/6L/1I |
| Core utilities | .opencode/reviews/core-utility-packages.md | 1H/9M/5L |
| Web frontend | .opencode/reviews/web-frontend-review.md | 6M/4L |
| Supporting packages + main | .opencode/reviews/supporting-packages-main-entrypoint.md | 2H/10M/11L |
| Build/CI/deps/docs | .opencode/reviews/build-ci-dependency-docs-review.md | 3H/8M/3L/1I |