Refactor: separate web manager from Beets engine - #39
Merged
Conversation
…ol agent supervision
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
This was referenced Jul 28, 2026
* fix: restore architecture CI baseline Fixes the three real CI failures on refactor/external-beets-engine (compose-verification, security, python-tests) and the false-negative readiness gate, rather than only adjusting assertions to match them: - docker-compose.yml/docker-compose.arrs.yml: reverts the invalid beets-engine:2.4.0@sha256:... image reference. Attaching a digest to a locally-built image's tag is not just misleading (a multi-layer custom build can never share the upstream base image's own manifest digest) -- `docker compose build` fails outright with "build tag cannot contain a digest", independently reproduced. Immutable upstream provenance already lives in Dockerfile.beets's digest-pinned FROM line, which is where it belongs. Also removes BEETS_OUTBOUND_ALLOWLIST/SPOTIFLAC_AUTO_INSTALL from the beets (engine) service, where neither is read by any code that runs in that container (the SSRF allowlist is enforced by backend/security.py, imported only by app.py in the web-manager process; SpotiFLAC auto-install is a hardcoded-False constant in app.py, never read from this env var at all) -- both were dead configuration on the wrong service. Adds the real, enforced BEETS_OUTBOUND_ALLOWLIST passthrough to docker-compose.arrs.yml's beets-web-manager service (docker-compose.yml's already had it). - .env.example: removes the project owner's own LAN IP (192.168.0.250:32400,192.168.0.250:8686) as an active default for BEETS_OUTBOUND_ALLOWLIST, replacing it with an empty default plus a commented illustrative example -- a reusable public template must not ship one user's real home-network address. - scripts/validate_compose_security.py: distinguishes a locally-built image (validates the Dockerfile's FROM digest instead of requiring an invalid digest on the image: line) from a pulled third-party image (still requires @sha256:); adds a check rejecting a hardcoded private/LAN IPv4 literal as an active BEETS_OUTBOUND_ALLOWLIST default; extends coverage to both compose files instead of only docker-compose.arrs.yml; drops the two now-removed dead-variable requirements. - scripts/security_secret_scan.py: narrows the "changeme"-style placeholder allowlist so it is only accepted in .env.example (or *.example files) and in .github/workflows/ CI-only env blocks, instead of globally across the whole repository -- the previous blanket allowlist meant a real secret committed as SOME_PASSWORD=changeme anywhere in the repo would never be flagged. - backend/beets_control_agent.py: BEETS_API_TOKEN now requires a minimum length and rejects known placeholder values (e.g. "changeme") at both startup and per-request auth, closing the gap where a non-empty but trivially weak token (the scanner-allowlisted example value) would silently grant real authenticated control-agent access. - routes_setup.py: _fetchart_integration_status() no longer gates `operational` on local Beets package importability (find_spec-based probe results), which the web-manager process is architecturally not guaranteed to have. `operational` now depends only on the real plugin-loader probe result and configuration state; the namespace-probe fields remain in the response for diagnostics but no longer block a correct readiness determination. Also documents (docs/TECHNICAL_DEBT.md ARCH-011) a related, deeper, pre-existing gap found while verifying this fix but explicitly out of scope for a CI-baseline-restoration slice: _beets_plugin_diagnostics() shells out to a local `beet` binary instead of querying the remote control agent the way routes_submissions._submission_readiness() already does, which means these integration statuses can never report operational in a correctly-configured (Beets-free) web-manager container. * ci: verify beets engine image Adds a reusable, safe CI verifier for the Beets engine Docker image and wires it into its own workflow job, replacing the earlier version's shell-injection-prone, weak-duplicate-check implementation: - scripts/verify_beets_engine_image.py: every value that could contain CI-input-controlled text (image name, plugin names, command names) is validated against a strict safe-character allowlist before use, and passed to subprocesses as discrete argv list elements -- container Python probes are piped via stdin, never interpolated into a shell -c string. The forbid-duplicate-plugin check now actually detects duplicates: it loads the full configured plugin set through a real `beet -vv version` and asserts the plugin appears at most once in the final "plugins:" line, rather than just re-confirming _get_plugin(name) resolves to itself in isolation (which is true whether or not a real duplicate exists elsewhere, and could never have caught the musicbrainz-duplication defect this whole verifier exists to guard against). All checks run with --network none (none of them need network access) and every Docker invocation is wrapped so a missing/broken Docker daemon produces one clear error instead of an unhandled traceback. Removes the unused, unimplemented --require-command argument. - .github/workflows/docker-build.yml: adds the beets-engine-verification job (builds Dockerfile.beets independently of the web-manager image job, then runs the verifier). The verification step activates Chroma-specific assertions (--require-plugin chroma, --forbid-duplicate-plugin musicbrainz, --require-command-help submit, --check-bpsync) automatically whenever docker/beets/apply_patches.py is present on the checked-out tree, rather than requiring a follow-up workflow change on whichever branch adds that file -- confirmed by temporarily integrating the current fix/chroma-acoustid-plugin remote head with this branch and observing the same workflow logic pick up full Chroma coverage with no changes to that branch itself. Tests exercise the real production functions and failure paths (missing image, wrong version, missing/duplicate/malformed plugin resolution, bpsync mismatch, command-help failure, unsafe-identifier rejection, --network none usage, Docker-binary-missing handling) rather than only a single happy-path --help smoke check. * fix: add missing BEETS_API_TOKEN dummy value to compose-verification job The compose-verification job's env block was missing BEETS_API_TOKEN entirely, so `docker compose -f docker-compose.arrs.yml config` / `docker compose config` always failed interpolation (`required variable BEETS_API_TOKEN is missing a value`). This was previously masked because scripts/validate_compose_security.py's own earlier hard failures (fixed in the prior commit on this branch) stopped the job before this step ever ran. Adds the same narrow, CI-only "changeme" dummy value already used for every other required variable in this job -- covered by the existing .github/workflows/ scanner exception.
* fix: restore architecture CI baseline Fixes the three real CI failures on refactor/external-beets-engine (compose-verification, security, python-tests) and the false-negative readiness gate, rather than only adjusting assertions to match them: - docker-compose.yml/docker-compose.arrs.yml: reverts the invalid beets-engine:2.4.0@sha256:... image reference. Attaching a digest to a locally-built image's tag is not just misleading (a multi-layer custom build can never share the upstream base image's own manifest digest) -- `docker compose build` fails outright with "build tag cannot contain a digest", independently reproduced. Immutable upstream provenance already lives in Dockerfile.beets's digest-pinned FROM line, which is where it belongs. Also removes BEETS_OUTBOUND_ALLOWLIST/SPOTIFLAC_AUTO_INSTALL from the beets (engine) service, where neither is read by any code that runs in that container (the SSRF allowlist is enforced by backend/security.py, imported only by app.py in the web-manager process; SpotiFLAC auto-install is a hardcoded-False constant in app.py, never read from this env var at all) -- both were dead configuration on the wrong service. Adds the real, enforced BEETS_OUTBOUND_ALLOWLIST passthrough to docker-compose.arrs.yml's beets-web-manager service (docker-compose.yml's already had it). - .env.example: removes the project owner's own LAN IP (192.168.0.250:32400,192.168.0.250:8686) as an active default for BEETS_OUTBOUND_ALLOWLIST, replacing it with an empty default plus a commented illustrative example -- a reusable public template must not ship one user's real home-network address. - scripts/validate_compose_security.py: distinguishes a locally-built image (validates the Dockerfile's FROM digest instead of requiring an invalid digest on the image: line) from a pulled third-party image (still requires @sha256:); adds a check rejecting a hardcoded private/LAN IPv4 literal as an active BEETS_OUTBOUND_ALLOWLIST default; extends coverage to both compose files instead of only docker-compose.arrs.yml; drops the two now-removed dead-variable requirements. - scripts/security_secret_scan.py: narrows the "changeme"-style placeholder allowlist so it is only accepted in .env.example (or *.example files) and in .github/workflows/ CI-only env blocks, instead of globally across the whole repository -- the previous blanket allowlist meant a real secret committed as SOME_PASSWORD=changeme anywhere in the repo would never be flagged. - backend/beets_control_agent.py: BEETS_API_TOKEN now requires a minimum length and rejects known placeholder values (e.g. "changeme") at both startup and per-request auth, closing the gap where a non-empty but trivially weak token (the scanner-allowlisted example value) would silently grant real authenticated control-agent access. - routes_setup.py: _fetchart_integration_status() no longer gates `operational` on local Beets package importability (find_spec-based probe results), which the web-manager process is architecturally not guaranteed to have. `operational` now depends only on the real plugin-loader probe result and configuration state; the namespace-probe fields remain in the response for diagnostics but no longer block a correct readiness determination. Also documents (docs/TECHNICAL_DEBT.md ARCH-011) a related, deeper, pre-existing gap found while verifying this fix but explicitly out of scope for a CI-baseline-restoration slice: _beets_plugin_diagnostics() shells out to a local `beet` binary instead of querying the remote control agent the way routes_submissions._submission_readiness() already does, which means these integration statuses can never report operational in a correctly-configured (Beets-free) web-manager container. * ci: verify beets engine image Adds a reusable, safe CI verifier for the Beets engine Docker image and wires it into its own workflow job, replacing the earlier version's shell-injection-prone, weak-duplicate-check implementation: - scripts/verify_beets_engine_image.py: every value that could contain CI-input-controlled text (image name, plugin names, command names) is validated against a strict safe-character allowlist before use, and passed to subprocesses as discrete argv list elements -- container Python probes are piped via stdin, never interpolated into a shell -c string. The forbid-duplicate-plugin check now actually detects duplicates: it loads the full configured plugin set through a real `beet -vv version` and asserts the plugin appears at most once in the final "plugins:" line, rather than just re-confirming _get_plugin(name) resolves to itself in isolation (which is true whether or not a real duplicate exists elsewhere, and could never have caught the musicbrainz-duplication defect this whole verifier exists to guard against). All checks run with --network none (none of them need network access) and every Docker invocation is wrapped so a missing/broken Docker daemon produces one clear error instead of an unhandled traceback. Removes the unused, unimplemented --require-command argument. - .github/workflows/docker-build.yml: adds the beets-engine-verification job (builds Dockerfile.beets independently of the web-manager image job, then runs the verifier). The verification step activates Chroma-specific assertions (--require-plugin chroma, --forbid-duplicate-plugin musicbrainz, --require-command-help submit, --check-bpsync) automatically whenever docker/beets/apply_patches.py is present on the checked-out tree, rather than requiring a follow-up workflow change on whichever branch adds that file -- confirmed by temporarily integrating the current fix/chroma-acoustid-plugin remote head with this branch and observing the same workflow logic pick up full Chroma coverage with no changes to that branch itself. Tests exercise the real production functions and failure paths (missing image, wrong version, missing/duplicate/malformed plugin resolution, bpsync mismatch, command-help failure, unsafe-identifier rejection, --network none usage, Docker-binary-missing handling) rather than only a single happy-path --help smoke check. * fix: add missing BEETS_API_TOKEN dummy value to compose-verification job The compose-verification job's env block was missing BEETS_API_TOKEN entirely, so `docker compose -f docker-compose.arrs.yml config` / `docker compose config` always failed interpolation (`required variable BEETS_API_TOKEN is missing a value`). This was previously masked because scripts/validate_compose_security.py's own earlier hard failures (fixed in the prior commit on this branch) stopped the job before this step ever ran. Adds the same narrow, CI-only "changeme" dummy value already used for every other required variable in this job -- covered by the existing .github/workflows/ scanner exception. * fix: resolve frontend router security advisories Resolves the three real npm audit findings on the frontend (GHSA-mh99-v99m-4gvg brace-expansion, GHSA-r28c-9q8g-f849 postcss, GHSA-qwww-vcr4-c8h2 react-router) with the actual supported React Router v8 package structure, not an alias: - Removes `react-router-dom` entirely (previously `"npm:react-router@8.3.0"`, an unsupported alias that kept every import dependent on a package name officially removed in v8, misrepresenting the installed package graph and hiding an incomplete migration). All 14 files that imported from `react-router-dom` (BrowserRouter, Navigate, Route, Routes, NavLink, Outlet, useNavigate, useSearchParams, useLocation, Link) now import directly from `react-router` -- confirmed these are the correct v8 exports by inspecting the installed package directly; `react-router/dom` is not needed anywhere in this app (it only exports RouterProvider/ HydratedRouter/unstable RSC APIs, none of which this app uses -- it uses the classic declarative BrowserRouter/Routes/Route pattern throughout, so GHSA-qwww-vcr4-c8h2's unstable-RSC-execution-path advisory does not presently apply here regardless -- the patched version is kept anyway since that's what actually satisfies the audit). - `react-router@8.3.0`'s peer/engine requirements (react/react-dom >=19.2.7, node >=22.22.0) are satisfied by this repo's pinned react/react-dom 19.2.7, CI's Node 24, and Docker's node:22-bookworm-slim (currently resolves to 22.23.1). - postcss@8.5.24 and the matching override: single deduped copy under both @tailwindcss/postcss and next, confirmed via `npm explain`. - brace-expansion@5.0.8: single copy via minimatch@10.2.6, confirmed via `npm explain`; eslint/minimatch remain compatible. - Extracts `AppRoutes` from `App` (same route tree, same paths, same redirects) so it can be mounted under MemoryRouter in tests instead of the production BrowserRouter -- no behavior change. - Adds a minimal Vitest + @testing-library/react stack (no prior frontend test runner existed) and frontend/tests/router.test.tsx: 15 real component/behavior tests covering the redirects (/ -> /library, /clean -> /jobs, /setup -> /system), every top-level route rendering its real page component, query-parameter propagation, NavLink destinations and active state, and the actual (previously undocumented, now verified) unknown-route behavior -- react-router renders nothing at all, not even the Shell layout, since there is no wildcard/catch-all route. - Replaces the prior source-text-only Python test class (which asserted file contents, not routing behavior, and asserted the alias as if it were policy) with RouterDependencyPolicyTests: narrow, honest governance assertions (react-router-dom absent, no npm-alias workaround, patched versions pinned) that explicitly defer routing-behavior verification to the real Vitest suite. - Verified via: clean `npm ci` (0 vulnerabilities), lint, typecheck, build (13 static pages, matching pre-migration count), Docker build of both images, a real disposable Compose-less container smoke test, and manual browser smoke testing of every listed route plus query params, redirects, refresh, and back/forward navigation against the built static export -- zero console errors. Documents the React Router v8 migration and adds `npm run test` to the frontend validation commands in CLAUDE.md.
* fix: correct beets chroma plugin resolution * fix: skip Chroma plugin-resolution tests when beets is not installed CI's python-tests job intentionally has no `beets` package installed, matching the web manager's zero-direct-Beets-dependency architecture. TestChromaPluginClassResolution imports docker/beets/apply_patches.py (which does `import beets` at module level) and exercises real Beets plugin-loader behavior, so it can only run where beets happens to be importable. Guard it the same way tests/test_plugin_path_precedence.py already guards its own real-Beets-dependent class, instead of letting all 17 tests error out with ModuleNotFoundError in CI.
* fix: use remote beets diagnostics for setup status * docs: align project documentation with external beets architecture * fix: harden remote beets readiness review Independent final review of PR #44 found and fixed three defects beyond the original ARCH-011 implementation, all verified with real Docker runtime validation, not source inspection alone: - setup.sh/setup.ps1 generated a random BEETS_WEB_AUTH_TOKEN but never touched BEETS_API_TOKEN, which ships as the placeholder "changeme" in .env.example. The Beets control agent's beets_api_token_is_usable() rejects that placeholder at startup, so a clean install following the documented ./setup.sh or .\setup.ps1 path built both images successfully but the beets engine container failed to start. Both scripts now also generate a strong, independent BEETS_API_TOKEN when creating .env, and a new regression test (tests/test_fresh_install_token_generation.py) runs the real scripts end-to-end against a stubbed `docker` and proves the fix (and fails against the pre-fix scripts). - Neither script pre-created ./web-manager-data, so a native Linux Docker host could auto-create it root-owned and unwritable to the container's UID when the opt-in legacy scan writes its state file there. - Dockerfile.beets's removal of the inherited LinuxServer svc-beets service was only covered by a static Dockerfile-text assertion. Added a real runtime check (scripts/verify_beets_engine_image.py --check-s6-supervision, wired into the beets-engine-verification CI job) that starts the actual image, confirms svc-beets stays down under S6 while the control agent runs, kills the control agent and confirms S6 restarts it, and confirms clean shutdown. Also fixed a test-isolation regression this PR's own test_routes_setup.py tearDownModule() introduced: popping sys.modules["app"] without also clearing routes_submissions/routes_jobs/routes_lidarr let a later test file's fresh `from app import app` build a new Flask instance while those already-cached route modules stayed bound to the old one, silently dropping routes (observed as spurious 405s in tests/test_external_beets_architecture.py when run after test_routes_setup, exactly the order the project's own required validation command uses). Clearing all four route modules together forces one consistent rebuild. Independently re-verified the ARCH-011 remote-status contract itself (one-snapshot-per-request reuse, fail-closed auth/timeout/malformed- response handling, submit/mbsubmit independence, liveness/readiness semantics, legacy scan gating, outbound-allowlist necessity, and the CodeQL default-setup gap being expected for a non-main-targeting PR) via disposable Docker containers and found no further defects. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
Beets stores SQLite path columns (items.path, albums.path, albums.artpath) as raw bytes. Any response echoing a row directly from the database hit TypeError: Object of type bytes is not JSON serializable, since _send_json's json.dumps had no default= handler. _json_default decodes bytes as UTF-8 with a str() fallback for anything that can't be decoded, and is now passed to json.dumps as default=. (cherry picked from commit e5d85996131b9eca01cb6574ea9ad9f99815285d)
Reviewed _json_default against a stricter checklist: confirms it is not an overly broad serializer (only bytes get special handling; every other unsupported type still raises TypeError -- unsupported objects fail loudly, as required), and adds coverage for nested bytes inside lists/dicts, normal JSON-native types passing through unchanged, and a check that the function never reads or leaks credential-shaped values (it only ever transforms bytes -> str; a plain string token passes through json.dumps untouched by this function). (cherry picked from commit 9631c8f521f56ea2313b2e887782858b90bf01b9)
Root cause of the 2026-07-28 TrueNAS production incident: Beets silently creates a brand-new, valid, empty SQLite library the moment it opens a configured library: path that doesn't exist. That's correct behavior for a genuine first run, but indistinguishable from a production deployment whose real database went missing mid-migration. On the affected deployment the real database (and most of config.yaml) was removed from the live path during an incomplete decommission step; the next beet invocation against that path quietly created a fresh empty library, which then served as "production" for ~29 hours. beets_startup_guard.check_library() is a read-only decision function: given BEETS_EXPECT_EXISTING_LIBRARY=1, it refuses (non-zero exit, no side effects) unless the configured database is a real file, passes PRAGMA integrity_check, has the required items/albums tables, and meets BEETS_MIN_EXPECTED_ITEMS/BEETS_MIN_EXPECTED_ALBUMS. An absent or zero-item database is only accepted when existing-library mode is not requested (explicit first-run). A symlink is only accepted if it resolves inside the approved config root. Deliberately dependency-free (stdlib only) so the identical file can be dropped onto a bare LinuxServer.io Beets image as a custom-cont-init.d hook, not just imported by this application. Reusable code intentionally does not hard-code this deployment's current row counts; a specific deployment supplies its own minimums via environment configuration outside source control. (cherry picked from commit 15c764326f899ffdb22d165952d8e2c5566db895)
Per review: the guard must run natively inside the beets-engine image, not only via the temporary bind-mounted override used to protect the current legacy production container. Wires beets_startup_guard.py into Dockerfile.beets and docker/beets-agent-s6.sh -- the control agent's own S6 service script, which is the process that actually opens musiclibrary.blb in this architecture -- so the guard runs before the control agent starts, not via custom-cont-init.d (confirmed by reading the base image's init-custom-files runner: it logs a non-zero exit from a custom-cont-init.d script but does not stop the rest of the startup chain, so a failing guard placed there would not actually block anything). BEETS_EXPECT_EXISTING_LIBRARY=1 is now the image default: a rebuilt engine must refuse to silently treat a missing/empty database as a fresh install, which is the exact failure mode of the 2026-07-28 incident. A genuine new install must override this explicitly. tests/test_beets_engine_docker_guard.py is an opt-in (RUN_DOCKER_TESTS=1) end-to-end test: builds the real image, proves the guard blocks the control agent from starting against a missing database, and proves it starts (with integrity/counts unchanged) against a valid one. Verified manually and via this test against a real local build: negative case blocks the control agent entirely (container idles on `sleep infinity`, confirmed via `docker top`); positive case starts the control agent for real, PRAGMA integrity_check stays "ok", and item/album counts are unchanged (a benign byte-level write on first open, matching what was observed on the real production restore this session, is expected and is not a data-loss signal by itself). (cherry picked from commit dc441248aab90beffbd14d8f9c2b8ec6b8b5f607)
…e hang Root cause of the full pytest suite appearing to hang: app.py is ~49.5k lines, and a single ast.parse() call on it costs roughly 10-15 seconds by itself (confirmed by timing read_text vs ast.parse in isolation -- read is ~0.02s, parse is 10-15s). Seven test files each extract pure helpers out of app.py via their own local ast.parse(APP_SOURCE) call to unit-test them without importing the whole Flask app, and several call it once per test method. Across a 1,400+ test run this made those files, and therefore the whole suite, take dramatically longer than necessary -- long stretches with no visible per-test output between pytest's percentage ticks were indistinguishable, in a live run, from a genuine deadlock. There was no leaked subprocess, thread, or lock; parsing a 49k-line file repeatedly is just that expensive. tests/_app_ast_cache.py adds get_app_ast()/get_app_source(), parsing app.py exactly once per test process and handing every caller the same tree; each of the 7 files now calls it instead of running its own ast.parse(). tests/test_app_ast_cache.py is the regression test: proves parse_count() never exceeds 1 no matter how many times the cache is hit, across this test and cumulatively with every other file that already imported it in the same process. Verified: all 7 files together now run in ~17s (previously each paid the 10-15s cost independently); the full suite completes in ~95-115s with 0 failures (previously killed after 10+ minutes at ~15% with no forward progress). (cherry picked from commit 2659ead30940675efac3683c5579ea2b62a8b168)
Both images now require a non-blank VCS_REF build-arg (the build fails loudly otherwise) and stamp org.opencontainers.image.revision and .source. The running production beets-web-manager:0.1.0 image has no OCI labels at all -- its exact source commit is unverifiable -- which is what this prevents from happening again for future builds. Built and verified from this integration branch (55eced43efa64b03a0eecb9c4d849fc2a50b3a00): - beets-engine / beets-web-manager both carry the exact integration SHA as org.opencontainers.image.revision, correct .source, unique image IDs. - chroma resolves to beetsplug.chroma.AcoustidPlugin (not MusicBrainzPlugin); musicbrainz resolves to its own plugin, not duplicated. - `beet submit --help` and `beet mbsubmit --help` both succeed with chroma/mbsubmit/musicbrainz enabled in config. - fpcalc present in the engine image. - web-manager image has no `beet` binary and no importable `beets` module. (cherry picked from commit 289226df57d91b3875c38c46b811a56a008c3f0f)
Both Dockerfiles now fail the build if VCS_REF is blank (previous
commit). Neither CI build step passed it, which would have broken
docker-build.yml the next time it ran. Uses ${{ github.sha }}, matching
how GitHub Actions already exposes the commit being built.
(cherry picked from commit 5639a71e66e5cbc41e7243c937403338a04c3d3b)
The previous --check-bpsync only imported beetsplug.bpsync and checked the class name/module -- a false positive. Real instantiation (exactly what Beets' own loader does: _get_plugin() calls obj() with zero args) fails on every attempt: BPSyncPlugin.__init__ unconditionally calls self.beatport_plugin.setup() with no arguments, but the installed BeatportPlugin.setup(self, session) requires one. Confirmed live against the real production engine image and by reading both plugin sources -- this is a genuine upstream signature mismatch, independent of Beatport credentials, not a credentials-gate. check_bpsync() now runs bpsync through Beets' real loader (a disposable BEETSDIR config with `plugins: bpsync`, `beet -vv version`) and reports one of: operational (appears in the loaded plugins line), the known setup()-signature incompatibility (non-fatal by default -- bpsync is not enabled in production and does not affect Chroma, submit, or mbsubmit), or an unexpected failure (always fatal, since silently bucketing a new regression into "known issue" would hide it). New --require-bpsync-operational flag escalates the known incompatibility to fatal for any deployment that actually needs bpsync to work. docker-build.yml: bpsync verification is no longer bundled under the chroma-patch-detected condition (they're unrelated defects that happened to share a "same defect class" description in an earlier PR draft); it always runs and reports status, without requiring bpsync to be operational. Tests added/updated in tests/test_external_beets_architecture.py's BeetsEngineImageVerifierTests: resolution failure (fatal), operational via real loader (passes), known incompatibility (non-fatal by default, fatal when required), and an unexpected/different failure (always fatal, proving the known-issue bucket can't silently swallow a new bug). (cherry picked from commit 93d88f75c257c82b36c273c81693e437cefa66e1)
…points PATCH /items/<id> and /albums/<id> built a SQL SET clause directly from caller-supplied JSON field-name keys with no allowlist (CodeQL py/sql-injection, alerts #350/#351). SQL parameters can only bind values, not column names, so an attacker-chosen key was interpolated straight into the SQL text. The web-manager already filters this via app.py's EDITABLE_FIELDS, but the control agent is a separately-authenticated network service and must not depend solely on the caller's honesty. Adds ITEM_EDITABLE_FIELDS/ALBUM_EDITABLE_FIELDS allowlists and rejects (403) any field name outside them before the SET clause is built.
…alation The /library/raw_query endpoint is deliberately a caller-supplied read-only SELECT passthrough (an admin/power-user query tool), gated by comment-stripping, single-statement enforcement, a mutating-keyword denylist, and a SELECT/WITH-only prefix check. CodeQL flags two distinct issues here: - py/polynomial-redos (#353/#354): the comment-stripping regexes are worst-case polynomial in input size on pathological input. Fixed by capping the query string to 10,000 characters before any regex runs, bounding worst-case cost to a fixed constant. - py/sql-injection (#348/#349): the query text is interpolated into SQL by design, which CodeQL can't verify is safe from the denylist alone (denylists are a known-weak mitigation class). Added a genuine defense-in-depth backstop: the connection is now opened read-only at the SQLite engine level via URI mode, so even a successful bypass of every check above cannot mutate the database. Documented with narrow codeql[py/sql-injection] suppressions at the two flagged execute calls, citing the full layered defense.
CodeQL flags 11 alerts (py/path-injection) across /tags/read, /tags/write, /files/move, /files/delete, and /files/mkdir: file_path/src/dst/target flow from the request body to filesystem operations (MediaFile, mutagen, os.path.exists, shutil.move/rmtree, os.unlink, os.makedirs). Every one of these is already gated by is_safe_path() immediately before use -- a custom sanitizer (URL-decode, reject null bytes/backslashes, require absolute path, reject '..'/'.' segments, then os.path.realpath containment check against an allowed root) proven by TestBeetsControlAgentSecurity (traversal, symlink escape, null bytes, backslashes). CodeQL's default py/path-injection query doesn't model a custom boolean-returning helper function as a taint barrier, so it correctly can't see that the guard already applies. Added narrow codeql[py/path-injection] suppression comments at each sink, citing the existing tested barrier.
…ace-exposure)
Two call sites echoed str(exception) directly into a client-facing JSON
response:
- app.py:10616 (album art-removal route): 'Could not clear artpath: {ex}'
returned verbatim in the 500 response.
- routes_submissions.py's _submission_readiness(): a failed remote
beets_client.get_status() call stored str(exc) in the 'reason' field,
which flows into /api/submissions/target's response (alert #355 flags
the exact jsonify() call this reaches).
Both can leak internal connection details, hostnames, or paths to an
external client. Both now log the full exception server-side
(app.logger.error, matching the project's existing global-handler
pattern) and return a generic client-facing message instead. Updated the
one existing test that had encoded the old (leaky) behavior as expected.
^[^:@/]+(?:/[^:@]+)*$ was used twice to detect an image reference with no
tag/digest (CodeQL py/polynomial-redos, alerts #356/#106). Both character
classes accept the same non-colon, non-at characters, so the regex is
ambiguous about how to split the string between the two groups -- a
classic superlinear-backtracking shape on crafted input.
The actual intent is just 'the whole string has no : and no @', which
needs no regex at all: replaced with a plain substring check
(':' not in image and '@' not in image), equivalent for every legitimate
image reference, verified against the real docker-compose.yml, and
immune to backtracking by construction.
… without regex CodeQL re-flagged both findings even after the earlier mitigations landed: - py/sql-injection (#350/#351): rejecting invalid `fields` keys up front (4f8ca18) doesn't give the dataflow analysis a way to prove the SET clause text never contains caller-controlled data, because set_clause was still built by iterating fields.keys() (the tainted dict) after the check passed. Restructured to iterate the ITEM_EDITABLE_FIELDS/ ALBUM_EDITABLE_FIELDS constants directly (checking membership in the fields dict, not iterating fields.keys()), so the column-name source in the generated SQL text is provably a hardcoded set literal. - py/polynomial-redos (#353/#354): a length cap (0e38b17) bounds worst-case cost but doesn't change that the regex pattern itself is still ambiguous, which is what the query actually flags. Replaced both r'/\*.*?\*/' (DOTALL) and r'--.*$' (MULTILINE) with _strip_sql_comments(), a single linear-time scan with no regex at all.
…losed default beets-engine-verification failed in CI: check_s6_supervision() starts a fresh container with no pre-existing /config/musiclibrary.blb, and the Dockerfile's production-safe default (BEETS_EXPECT_EXISTING_LIBRARY=1) correctly refused to start the control agent against a missing database -- exactly the behavior the guard exists for. That refusal blocked S6 from ever reporting the container healthy, failing a check whose actual job is verifying process supervision (svc-beets stays down, the control agent stays up and restarts after a crash), not the guard itself (which has its own dedicated tests: test_beets_startup_guard.py, test_beets_engine_docker_guard.py). This is a genuine gap from porting the startup guard forward onto the current architecture head: it never ran alongside PR #44's newer --check-s6-supervision test before now, since neither existed on the same branch previously. Fix: pass -e BEETS_EXPECT_EXISTING_LIBRARY=0 to this specific test container. The Dockerfile's production default is unchanged.
Discovered by live verification against the real production library (caught exactly the class of bug the "verify with an out-of-band API call, not just a UI success message" practice exists to catch): deployed tracks=4687 against a database that actually has 3144 items. _build_library_payload's "track_count" for a card is the total file count in that disk folder -- imported items + not-yet-imported extras + missing -- not the imported item count alone. _library_stats_for_artists summed raw track_count for every real-album (album_id>0) card, so any real album whose folder also contains extra never-imported audio files had those extras counted as if they were Beets item rows. Since not_imported already isolates that count per card, subtract it: tracks_total += track_count - not_imported for real-album cards. Added a regression test for a real album with unimported extras alongside its genuine tracks. Known remaining gap (not fixed here, documented for follow-up): live verification also found albums=368 against a database with 413 real albums -- 45 real albums whose entire disk folder has gone missing appear to fail the artist/album name-based lookup in _build_library_payload's leftover-missing-item injection (a separate, pre-existing traversal-logic issue, out of scope for a change to _library_stats_for_artists alone, and explicitly marked in CLAUDE.md as requiring a side-by-side parity check before modification).
Follow-up to the library-summary aggregation fix: live production verification found 45 real albums (whose entire folder is missing from disk) never get matched back to their real album_id in the leftover- missing-item injection pass, landing in the singleton bucket instead. Recorded per CLAUDE.md's guidance to log design issues surfaced during implementation work rather than expand the current change's scope.
This was referenced Jul 29, 2026
Owner
Author
Credential incident response (2026-07-29)Two production credentials (
PR #39 remains draft. Old Separately, this comment does not attempt to address the CodeQL alerts (#370-#382) or |
Merges origin/main (36cb0f6) into the PR branch. main's package.json predates the React Router v7-to-v8 security migration and the postcss GHSA-r28c-9q8g-f849 remediation done on this branch -- it still has react-router-dom, postcss 8.5.15, and no vitest test infrastructure. Dependabot had only bumped react (19.2.7->19.2.8) and @tailwindcss/postcss (4.3.2->4.3.3) within that older baseline. Resolution: took only the two legitimate newer patch versions from main (react, @tailwindcss/postcss) and kept everything else from this branch (react-router 8.3.0, postcss 8.5.24, the full vitest/testing-library/jsdom test toolchain, the "test" script). Also bumped react-dom to 19.2.8 to match react -- React 19 refuses to run when react/react-dom versions differ, which the test suite caught immediately. package-lock.json was not manually spliced: removed and regenerated via a clean npm install, then verified with npm ci. Confirmed: 0 npm audit vulnerabilities, react-router-dom absent from the lockfile, react-router at 8.3.0, lint/typecheck/build (13 static pages)/tests (15/15) all pass.
PR #45's resolve_safe_path() (returning Optional[str]) still left 13 open CodeQL py/path-injection alerts (#370-#382): every call site checked `if safe_x is None` and then operated on the returned string, but the static analysis apparently doesn't reliably treat a same-shaped-type (str-in, str-or-None-out) helper as a taint barrier -- the raw tainted input and the "sanitized" output are the same type, so nothing in the type system forces callers to stop touching the original string. Replaced the contract entirely: resolve_safe_path() now raises UnsafePathError for every unsafe condition and returns a pathlib.Path (not the original str) for every safe one. There is no code path that produces a Path from untrusted input without every check (decode, null-byte/backslash rejection, traversal-segment rejection, absolute-path requirement, realpath + containment-via-commonpath against the allowed roots) having passed -- the type change itself severs the direct string-in/string-out taint shape. Every affected handler (/tags/read, /tags/write, /files/move, /files/delete, /files/mkdir, /albums/<id>/artpath) now uses try/except UnsafePathError and operates exclusively on the returned Path, never the original raw string. Also: removed three redundant re-resolutions of the same already-trusted value that added no safety (files/move, files/mkdir each re-resolved the same path 2-3 times for no reason); added a root- itself deletion refusal for /files/delete; is_safe_path() is now a thin bool wrapper over resolve_safe_path() for the few callers that only need a check (job source_path/args validation), not the resolved Path. Extended tests/test_pr39_codeql_alerts.py for the new contract: empty/ non-string input, relative-path rejection, require_exists, expected_type (file vs dir), multiple allowed roots, unicode/spaces, root-itself deletion refusal, and /files/move source/destination escape rejection (previously untested). All existing tests updated from assertIsNone to assertRaises(UnsafePathError).
The Plex integration authenticated with a bare token and no distinct client identity headers -- Plex's account/device model registers a "device" per client identifier, and with none set, this integration's usage was indistinguishable from whatever session originally produced the token (confirmed during Track B attribution: the token has visibility into 35 authorized devices with no way to isolate which one is "this app"). This doesn't retroactively fix that, but prevents the same ambiguity for the replacement token and any future installation. Adds _plex_client_identifier(): a UUID generated once and persisted to /web-manager-data/.plex_client_identifier (override via PLEX_CLIENT_IDENTIFIER_FILE), so it survives container recreation instead of registering a new device on every restart. _plex_request() now sends it plus X-Plex-Product/X-Plex-Device-Name/X-Plex-Version/X-Plex-Platform on every call. Also moved X-Plex-Token from a URL query parameter to a request header -- query-string tokens are far more likely to end up verbatim in access logs/proxies than headers, and this repo had no other client-facing consumer of the URL (no <img src> or similar) needing it there.
The production PLEX_TOKEN appeared in private diagnostic session output. Investigation found the token has account-level visibility into 35 authorized Plex devices with no distinct client identifier ever sent, so no single device could be safely attributed and revoked without risking disruption to an unrelated session or requiring a broad account sign-out. Owner reviewed this and explicitly chose to retain the current token, accepting the risk. Recorded per this repo's existing technical-debt register convention (see ARCH-012) rather than treating this as an unresolved release blocker.
Iranman
marked this pull request as ready for review
July 30, 2026 04:38
Iranman
added a commit
that referenced
this pull request
Jul 30, 2026
* fix: preserve canonical path compatibility Ports only the two independently-reproduced compatibility regressions found during PR #47's review onto the current parent (refactor/external-beets-engine @ f6485a2), plus the remaining boolean-check/original-value sink it also missed. PR #47 could not be merged wholesale: the parent has since developed its own, independently authored path-security implementation (commit 94e05fa) with a stricter contract (resolve_safe_path() returns pathlib.Path and raises UnsafePathError, rather than PR #47's Optional[str]/None), which this change preserves rather than reverts. 1. resolve_safe_path() canonicalized from the URL-decoded form of the input, not the original raw path -- the same bug independently present in both the parent and PR #47's original code. A legitimate literal filename containing a percent-sign byte sequence (e.g. "convention%20album.flac") was silently rewritten to a different string ("convention album.flac") before reaching the filesystem sink. Decoding is still used to detect a hidden traversal/separator/null-byte attack; the canonical return value is now always derived from the original path. Verified this does not weaken detection: plain and single/double-encoded traversal, encoded slash/null/backslash, and nested-encoded "../" up to 6 layers (beyond the resolver's decode budget) are all still rejected via direct adversarial testing against the parent's actual (unmodified except for this fix) resolver. 2. `/commands/execute` and `/jobs/create` rejected any argument containing ".." as a bare substring, incorrectly blocking Beets' own supported range-query syntax (`year:2020..2023`, `added:2020-01-01..2020-02-01`) even though non-absolute args are never filesystem-joined here. Narrowed to reject ".." only as an actual path segment via a shared `_sanitize_command_path_args()` helper (adapted to raise UnsafePathError, matching the parent's contract, rather than returning an error tuple). 3. The same two endpoints also validated `source_path`/absolute args with a boolean is_safe_path() check and then appended the *original* request string to the command array -- `_sanitize_command_path_args()` now returns the canonical resolved value for every absolute-path argument, and `source_path` is replaced by its resolved Path before being added to `cmd_list`. 4. `_handle_delete_album()` treated database-stored item/album paths as trusted after only a boolean is_safe_path() check, then deleted the *original* stored string. Stored paths are database content, not request input, but are exactly as untrusted -- a corrupted or maliciously-written row must not be able to direct a delete outside the approved roots. Now resolves each stored path through resolve_safe_path() and operates only on the returned Path, refuses an approved root itself (matching /files/delete's existing protection), and no longer echoes raw paths or exception text into file_errors. Database row deletion remains unconditional regardless of any individual file's safety, so a hostile stored path cannot block legitimate cleanup. Intentionally not ported: PR #47's CodeQL barrier model (.github/codeql/extensions/pr39-path-sanitizers/) and its advanced-setup workflow files. This repository currently uses GitHub's default CodeQL setup, under which PR #39 (base: main) already records a green authoritative analysis; switching to advanced setup to load a local model pack is a repository-wide governance decision, not part of this bug salvage, and is recorded as a technical-debt recommendation instead. Regression tests added: literal-percent-filename preservation (including survival through the real /files/delete endpoint), nested-encoding rejection at multiple depths, legitimate Beets range-query arguments passing through both /commands/execute and /jobs/create's real subprocess/job command arrays unchanged, real traversal/option-injection still rejected, and a hostile-SQLite-database album-cleanup test (mixed valid/outside-root/literal-percent/encoded-traversal stored paths) proving no outside-root deletion occurs and the database rows are removed unconditionally. * fix: harden path regression salvage review Independent final review of PR #48 found three narrow, correctable defects on top of the salvaged fixes: - resolve_safe_path() enforced the absolute-path requirement only on the decoded copy of the input, while canonicalizing from the raw string. A value whose raw form was relative but whose fully-decoded form happened to be absolute (a fully percent-encoded leading separator) could fall through to os.path.abspath(), which silently anchors a relative string to the process cwd instead of being rejected. The raw string must now also start with "/". - _handle_delete_album() committed the database deletion before attempting best-effort filesystem cleanup, but an unguarded os.listdir()/is_dir() call on the album directory could raise and propagate to the outer handler, which would then report database_deleted: False for a delete that had already succeeded. The album-directory emptiness check and the whole delete_files block are now exception-safe so a filesystem error can never misreport an already-committed database result. - /commands/execute validated target_path with resolve_safe_path() but never used the result -- it was never appended to the constructed command array, and no current caller (beets_client.run_command's own only caller never passes it) ever sends it. Removed the dead, misleading validation and the unused client-side parameter. Added regression tests for all three.
Owner
Author
Automated task note: out-of-band merge detected mid-taskThis PR was merged (
Steps 1-2 completed successfully: PR #48 was synced (normal merge, no rebase/force-push), fully revalidated, and squash-merged into However, this PR was merged into
No further action was taken on |
Iranman
added a commit
that referenced
this pull request
Jul 30, 2026
* fix: preserve canonical path compatibility Ports only the two independently-reproduced compatibility regressions found during PR #47's review onto the current parent (refactor/external-beets-engine @ f6485a2), plus the remaining boolean-check/original-value sink it also missed. PR #47 could not be merged wholesale: the parent has since developed its own, independently authored path-security implementation (commit 94e05fa) with a stricter contract (resolve_safe_path() returns pathlib.Path and raises UnsafePathError, rather than PR #47's Optional[str]/None), which this change preserves rather than reverts. 1. resolve_safe_path() canonicalized from the URL-decoded form of the input, not the original raw path -- the same bug independently present in both the parent and PR #47's original code. A legitimate literal filename containing a percent-sign byte sequence (e.g. "convention%20album.flac") was silently rewritten to a different string ("convention album.flac") before reaching the filesystem sink. Decoding is still used to detect a hidden traversal/separator/null-byte attack; the canonical return value is now always derived from the original path. Verified this does not weaken detection: plain and single/double-encoded traversal, encoded slash/null/backslash, and nested-encoded "../" up to 6 layers (beyond the resolver's decode budget) are all still rejected via direct adversarial testing against the parent's actual (unmodified except for this fix) resolver. 2. `/commands/execute` and `/jobs/create` rejected any argument containing ".." as a bare substring, incorrectly blocking Beets' own supported range-query syntax (`year:2020..2023`, `added:2020-01-01..2020-02-01`) even though non-absolute args are never filesystem-joined here. Narrowed to reject ".." only as an actual path segment via a shared `_sanitize_command_path_args()` helper (adapted to raise UnsafePathError, matching the parent's contract, rather than returning an error tuple). 3. The same two endpoints also validated `source_path`/absolute args with a boolean is_safe_path() check and then appended the *original* request string to the command array -- `_sanitize_command_path_args()` now returns the canonical resolved value for every absolute-path argument, and `source_path` is replaced by its resolved Path before being added to `cmd_list`. 4. `_handle_delete_album()` treated database-stored item/album paths as trusted after only a boolean is_safe_path() check, then deleted the *original* stored string. Stored paths are database content, not request input, but are exactly as untrusted -- a corrupted or maliciously-written row must not be able to direct a delete outside the approved roots. Now resolves each stored path through resolve_safe_path() and operates only on the returned Path, refuses an approved root itself (matching /files/delete's existing protection), and no longer echoes raw paths or exception text into file_errors. Database row deletion remains unconditional regardless of any individual file's safety, so a hostile stored path cannot block legitimate cleanup. Intentionally not ported: PR #47's CodeQL barrier model (.github/codeql/extensions/pr39-path-sanitizers/) and its advanced-setup workflow files. This repository currently uses GitHub's default CodeQL setup, under which PR #39 (base: main) already records a green authoritative analysis; switching to advanced setup to load a local model pack is a repository-wide governance decision, not part of this bug salvage, and is recorded as a technical-debt recommendation instead. Regression tests added: literal-percent-filename preservation (including survival through the real /files/delete endpoint), nested-encoding rejection at multiple depths, legitimate Beets range-query arguments passing through both /commands/execute and /jobs/create's real subprocess/job command arrays unchanged, real traversal/option-injection still rejected, and a hostile-SQLite-database album-cleanup test (mixed valid/outside-root/literal-percent/encoded-traversal stored paths) proving no outside-root deletion occurs and the database rows are removed unconditionally. * fix: harden path regression salvage review Independent final review of PR #48 found three narrow, correctable defects on top of the salvaged fixes: - resolve_safe_path() enforced the absolute-path requirement only on the decoded copy of the input, while canonicalizing from the raw string. A value whose raw form was relative but whose fully-decoded form happened to be absolute (a fully percent-encoded leading separator) could fall through to os.path.abspath(), which silently anchors a relative string to the process cwd instead of being rejected. The raw string must now also start with "/". - _handle_delete_album() committed the database deletion before attempting best-effort filesystem cleanup, but an unguarded os.listdir()/is_dir() call on the album directory could raise and propagate to the outer handler, which would then report database_deleted: False for a delete that had already succeeded. The album-directory emptiness check and the whole delete_files block are now exception-safe so a filesystem error can never misreport an already-committed database result. - /commands/execute validated target_path with resolve_safe_path() but never used the result -- it was never appended to the constructed command array, and no current caller (beets_client.run_command's own only caller never passes it) ever sends it. Removed the dead, misleading validation and the unused client-side parameter. Added regression tests for all three. (cherry picked from commit 125752d)
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
2 participants
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.
Refactor: separate web manager from Beets engine
Current Verified Status (2026-07-29)
5a7e871ce3c53694452364cc85db493722bce19c.MERGEABLE.npm audit: 0 vulnerabilities. ShellCheck: 0 findings.BEETS_API_TOKEN: rotated (prior pass) and reverified this pass -- old value rejected, current value accepted.PLEX_TOKEN: retained by explicit owner decision; not rotated. See "Owner-accepted Plex credential risk" below. This is not a release blocker.8c642b108489a5ce841fb59a8039168cbb5f47ff6dcdc7447486f20984fea70f,PRAGMA integrity_check: ok, 3,144 items, 413 albums -- unchanged throughout.Owner-accepted Plex credential risk
During deployment diagnostics, the production
PLEX_TOKENappeared in private session output.The owner has explicitly chosen not to revoke or replace the token and accepts the risk of continued use.
This token remains active and is not represented as rotated or invalidated.
Mitigations included in this PR:
beets-web-managerconsumesPLEX_TOKEN.This accepted risk is not treated as a technical blocker for PR readiness. Recorded in
docs/TECHNICAL_DEBT.mdas SEC-001.Production deployment
/mnt/PLEX/Apps/Arrs/_backups/beets-f6485a-risk-accepted-predeploy-20260729-211704/(database checksum-verified match, config, compose, env, image/container inspections, resolved compose, ports, health, rollback instructions; directory mode 700, secret-bearing files mode 600).--no-cachefromf6485a2(later re-tagged conceptually to the docs-only5a7e871, which needed no rebuild),org.opencontainers.image.revisionverified to match on the TrueNAS Docker daemon before deployment.submit/mbsubmit --helpboth succeed, BPSync accurately reports the known incompatibility (non-fatal, not enabled), no published host ports. Restarted twice; second restart healthy in 5s (first restart showed a transient ~90s health-check delay under investigation -- did not reproduce on the second restart, database unaffected throughout, resolved on its own).beetexecutable, nobeetsPython module, Plex read-only request succeeds (HTTP 200) with the retained token, stable client identifier file created (32-byte UUID) and confirmed byte-identical after a restart, control-agent communication verified with the currentBEETS_API_TOKEN, port 8337 loopback-bound, 0 token matches in container logs.beets-engine:b6cf0b0thenbeets-web-manager:f143691, both healthy, database checksum/integrity/counts unchanged, Plex connectivity verified with the retained token, zero orphaned containers, all 36 total containers on the host unaffected. Then redeployedf6485a2/current-head images and repeated all read-only validation -- both healthy within 5-11 seconds.Note on a pre-existing, unrelated finding:
beets-web-manager's/configmount contains stale, unused leftover database-shaped files (different checksum than the real production database, not referenced by any environment variable the running code reads) from before the two-service architecture migration. Not part of this PR's changes; flagged for future cleanup, not a functional issue (the application never reads this path).Main conflict resolution
main'sfrontend/package.jsonpredates this branch's React Router v7-to-v8 security migration and the postcssGHSA-r28c-9q8g-f849remediation -- merging it naively would have reintroducedreact-router-dom, downgradedpostcssto the vulnerable8.5.15, and dropped the entire vitest/testing-library/jsdom test toolchain. Dependabot (PRs #37/#38, legitimate) had only bumpedreact(19.2.7->19.2.8) and@tailwindcss/postcss(4.3.2->4.3.3) within that older baseline.Resolution: merged
origin/main, took only the two legitimate newer patch versions, kept everything else from this branch. Also bumpedreact-domto 19.2.8 to matchreact-- React 19 refuses to render when the two versions differ, which the test suite caught immediately.package-lock.jsonwas not manually spliced: removed and regenerated via a cleannpm install, then verified withnpm ci. Final dependency state:react-router8.3.0,react-router-domabsent,postcss8.5.24,react/react-dom19.2.8, full test toolchain intact,npm audit: 0 vulnerabilities.CodeQL Remediation State
a4b347c6) introduced aresolve_safe_path()helper returningOptional[str]; CodeQL still flagged 13 openpy/path-injectionalerts (#370-#382) against it.resolve_safe_path()now raisesUnsafePathErrorfor every unsafe condition and returns apathlib.Path(not the original string) for every safe one -- every affected handler (/tags/read,/tags/write,/files/move,/files/delete,/files/mkdir,/albums/<id>/artpath) now operates exclusively on the returnedPath.resolve_safe_path's own post-containment-check logic, on aPathalready proven root-contained by the same function. That's a definitive false-positive demonstration.py/path-injectionquery, all 18 alerts were dismissed as false positives with per-alert justification citingtests/test_pr39_codeql_alerts.py(traversal, symlink escape, prefix collision, encoded/double-encoded traversal,/files/movesource and destination escape).BPSync and library-summary status (unchanged from prior validation)
BPSync remains unsupported and not enabled in production (confirmed live on the current-head deployment:
[INCOMPATIBLE, non-fatal]). Library summary aggregation is corrected (tracks/albumsno longer double-count split/unimported-extra content); ARCH-012 (45 real albums with wholly-missing disk folders) remains an open, documented, separate limitation in the disk-walk's name-matching logic.Merge Status
PR #39 remains draft and unmerged, pending separate, explicit publication authorization. All technical gates pass; the Plex credential is a documented, owner-accepted risk rather than a blocker.