Fix plugin-first runtime boundaries and CodeQL findings - #46
Draft
Iranman wants to merge 20 commits into
Draft
Conversation
Iranman
force-pushed
the
fix/plugin-first-runtime-and-codeql
branch
from
July 30, 2026 14:51
538b445 to
0b11a26
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| self._send_json(403, {"ok": False, "status": "analysis_error", "error": f"Access denied for path outside allowed roots: {file_path}"}) | ||
| return | ||
|
|
||
| if not safe_file_path.exists(): # codeql[py/path-injection] -- sanitized by resolve_safe_path() above |
| self._send_json(404, {"ok": False, "status": "invalid_media", "error": f"File not found: {file_path}"}) | ||
| return | ||
|
|
||
| if not safe_file_path.is_file(): # codeql[py/path-injection] -- sanitized by resolve_safe_path() above |
Iranman
added a commit
that referenced
this pull request
Jul 30, 2026
…view Independent final review of PR #46 found and fixed: - backend/beets_control_agent.py had TWO conflicting definitions of is_safe_path(): a broken one (line ~488, calling resolve_safe_path(...) is not None -- which never returns None, since resolve_safe_path() raises UnsafePathError instead, so this would crash rather than return False for unsafe input) left over from commit 0b11a26's rebase reconciliation, silently shadowed by the correct try/except version already present on the rebased base (refactor/external-beets-engine @ 125752d). Removed the broken, dead first definition and its accompanying dead `except Exception: return None` duplicate clause in _path_is_within(), which was unreachable but violated that function's own bool return contract. - Two duplicate bare `return` statements (tag-write's "Unsupported tag field" branch, and the new /audio/acoustid-lookup endpoint's "Path is not a regular file" branch) -- both dead code from the same rebase, removed. - /audio/acoustid-lookup echoed the raw request path (`file_path`) into 403/404/400 error bodies and a raw Python exception string into a 500 body, inconsistent with every other sink in this file. Changed to the same generic-message pattern already used by /files/delete, /files/move, /commands/execute, etc. - Removed the non-functional `# codeql[py/path-injection] -- sanitized by resolve_safe_path() above` comments: GitHub Code Scanning has no inline source-comment suppression mechanism, so these had zero actual effect and misrepresented the alert as handled. A live temporary CodeQL-validation PR against main (closed without merging) confirmed 2 open py/path-injection alerts remain on these exact lines for this exact head. Documented as ARCH-014 in docs/TECHNICAL_DEBT.md with the real remediation options (advanced model pack, blocked while Default Setup is enabled; or an owner-reviewed dismissal). - Escalated ARCH-007 with a confirmed, more severe finding unrelated to this PR's own diff: backend/beets_client.py's raw_sqlite_query() unconditionally raises (already true on the rebased base, not introduced here), so every one of the ~18 remaining _db()/con.execute(...) call sites in app.py always raises or is silently swallowed by a broad except. Flagged P0 for separate follow-up; out of scope for this PR to fix. All new tests pass; full suite unaffected (1553 tests, OK, skipped=6 locally / 61 on Linux CI -- unchanged).
Iranman
added a commit
that referenced
this pull request
Jul 30, 2026
…view Independent final review of PR #46 found and fixed: - backend/beets_control_agent.py had TWO conflicting definitions of is_safe_path(): a broken one (line ~488, calling resolve_safe_path(...) is not None -- which never returns None, since resolve_safe_path() raises UnsafePathError instead, so this would crash rather than return False for unsafe input) left over from commit 0b11a26's rebase reconciliation, silently shadowed by the correct try/except version already present on the rebased base (refactor/external-beets-engine @ 125752d). Removed the broken, dead first definition and its accompanying dead `except Exception: return None` duplicate clause in _path_is_within(), which was unreachable but violated that function's own bool return contract. - Two duplicate bare `return` statements (tag-write's "Unsupported tag field" branch, and the new /audio/acoustid-lookup endpoint's "Path is not a regular file" branch) -- both dead code from the same rebase, removed. - /audio/acoustid-lookup echoed the raw request path (`file_path`) into 403/404/400 error bodies and a raw Python exception string into a 500 body, inconsistent with every other sink in this file. Changed to the same generic-message pattern already used by /files/delete, /files/move, /commands/execute, etc. - Removed the non-functional `# codeql[py/path-injection] -- sanitized by resolve_safe_path() above` comments: GitHub Code Scanning has no inline source-comment suppression mechanism, so these had zero actual effect and misrepresented the alert as handled. A live temporary CodeQL-validation PR against main (closed without merging) confirmed 2 open py/path-injection alerts remain on these exact lines for this exact head. Documented as ARCH-014 in docs/TECHNICAL_DEBT.md with the real remediation options (advanced model pack, blocked while Default Setup is enabled; or an owner-reviewed dismissal). - Escalated ARCH-007 with a confirmed, more severe finding unrelated to this PR's own diff: backend/beets_client.py's raw_sqlite_query() unconditionally raises (already true on the rebased base, not introduced here), so every one of the ~18 remaining _db()/con.execute(...) call sites in app.py always raises or is silently swallowed by a broad except. Flagged P0 for separate follow-up; out of scope for this PR to fix. All new tests pass; full suite unaffected (1553 tests, OK, skipped=6 locally / 61 on Linux CI -- unchanged).
Iranman
force-pushed
the
fix/plugin-first-runtime-and-codeql
branch
from
July 30, 2026 16:10
3207bd8 to
819ac21
Compare
Iranman
added a commit
that referenced
this pull request
Jul 30, 2026
Retargeting PR #46's base (refactor/external-beets-engine -> main) is a GitHub "edited" PR event, which does not fall in the default trigger set (opened, synchronize, reopened) for pull_request-triggered workflows, including GitHub's own Default Setup CodeQL analysis. No analysis appeared for this PR's head after 5 minutes of polling post-retarget. This empty commit produces a genuine synchronize event so Default Setup analyzes the current exact head under its now-current base (main), without creating another temporary validation PR.
Iranman
added a commit
that referenced
this pull request
Jul 30, 2026
…view Independent final review of PR #46 found and fixed: - backend/beets_control_agent.py had TWO conflicting definitions of is_safe_path(): a broken one (line ~488, calling resolve_safe_path(...) is not None -- which never returns None, since resolve_safe_path() raises UnsafePathError instead, so this would crash rather than return False for unsafe input) left over from commit 0b11a26's rebase reconciliation, silently shadowed by the correct try/except version already present on the rebased base (refactor/external-beets-engine @ 125752d). Removed the broken, dead first definition and its accompanying dead `except Exception: return None` duplicate clause in _path_is_within(), which was unreachable but violated that function's own bool return contract. - Two duplicate bare `return` statements (tag-write's "Unsupported tag field" branch, and the new /audio/acoustid-lookup endpoint's "Path is not a regular file" branch) -- both dead code from the same rebase, removed. - /audio/acoustid-lookup echoed the raw request path (`file_path`) into 403/404/400 error bodies and a raw Python exception string into a 500 body, inconsistent with every other sink in this file. Changed to the same generic-message pattern already used by /files/delete, /files/move, /commands/execute, etc. - Removed the non-functional `# codeql[py/path-injection] -- sanitized by resolve_safe_path() above` comments: GitHub Code Scanning has no inline source-comment suppression mechanism, so these had zero actual effect and misrepresented the alert as handled. A live temporary CodeQL-validation PR against main (closed without merging) confirmed 2 open py/path-injection alerts remain on these exact lines for this exact head. Documented as ARCH-014 in docs/TECHNICAL_DEBT.md with the real remediation options (advanced model pack, blocked while Default Setup is enabled; or an owner-reviewed dismissal). - Escalated ARCH-007 with a confirmed, more severe finding unrelated to this PR's own diff: backend/beets_client.py's raw_sqlite_query() unconditionally raises (already true on the rebased base, not introduced here), so every one of the ~18 remaining _db()/con.execute(...) call sites in app.py always raises or is silently swallowed by a broad except. Flagged P0 for separate follow-up; out of scope for this PR to fix. All new tests pass; full suite unaffected (1553 tests, OK, skipped=6 locally / 61 on Linux CI -- unchanged).
Iranman
force-pushed
the
fix/plugin-first-runtime-and-codeql
branch
from
July 30, 2026 21:19
39fa5cc to
f1b44ea
Compare
Iranman
added a commit
that referenced
this pull request
Jul 30, 2026
The 4 job-log carve-out alerts (#43/#46/#68/#83) plus a 5th instance that surfaced after the delete_import_review_folder fix (#413, same _delete_review_source_folder log sources, new alert number once the sibling "error" field became a static string) are dismissed individually on GitHub as won't-fix with the CLAUDE.md raw-debug-detail justification, rather than left open -- the hard zero-open-PR-alerts merge gate requires a real disposition. Corrects the dismissal count (10, not 5) and remaining backlog (225, not 220).
Iranman
added a commit
that referenced
this pull request
Jul 30, 2026
* fix: sanitize app.py public exception responses (SEC-002 wave 3)
Independently traced and closed all 62 open py/stack-trace-exposure
alerts in app.py (scope agreed with repo owner; path-injection, ReDoS,
URL-substring, and other rule families in app.py remain out of scope
for a future wave, as do index.html/Playlists.tsx).
Genuine fixes (~48 call sites/helpers): the established pattern
throughout -- log the real exception server-side (app.logger.warning,
type name only, never the message) and return a fixed safe message.
Notable:
- /api/config GET/POST and /api/config/revert (the beets config.yaml
file itself, the single most sensitive file in the container since
it can hold provider credentials) previously leaked raw filesystem
exception text on any read/write/backup failure.
- _classify_openai_error()'s fallback branch (f"AI provider error
({exc})") now returns only the exception type name; this closed
every downstream alert that traced through the shared helper.
- _folder_cleanup_path()'s except-branch (f"Invalid path: {exc}") now
returns a fixed message; this closed 7 downstream alerts across the
folder-move/rename/merge maintenance route sharing this one source.
- _SpotifyFetchError's two raise sites used to wrap the underlying
urllib/json exception in violation of the "never wrap another
exception" rule; both now raise fixed messages.
- One yt-dlp cookie smoke-test probe error wasn't yet routed through
the codebase's existing _redact_security_text() helper (already used
elsewhere for auth-rejection reasons); now is.
Proven false positives (5 total, dismissed on GitHub, each with an
individual justification and a passing behavioral test):
- #38/#100: _start_metadata_apply_transaction() (two call sites) raises
ValueError only with fixed, deliberate messages.
- #44: delete_import_review_folder()'s ValueError branch, verified
against _delete_review_source_folder()'s raise sites.
- #61/#62: _folder_clean_root() (two call sites) raises RuntimeError
only with fixed messages; its one path-embedding variant was
tightened to a fixed string this wave.
Won't-fix, left open (4, job-log-only taint, no top-level response
field affected, none provider/credential-adjacent -- CLAUDE.md
explicitly requires jobs to expose raw debug detail): #43, #46, #68,
#83.
Also fixes a real test regression: tests/test_ai_auth_setup_hardening.py
ClassifyOpenAiErrorBehaviorTests used an AST-extraction harness that
didn't stub `app` (needed by the new app.logger.warning() call) and
explicitly asserted the old unsafe fallback behavior. Updated the
harness and rewrote the assertion to require the exception type name
while explicitly asserting the raw message and an embedded secret are
both absent.
* docs: update SEC-002 for app.py stack-trace-exposure wave
Records wave 3's full accounting: starting/alert counts, root-cause
groups, fixes, dismissals, the 4 accepted-by-design job-log exceptions,
the test-harness regression found and fixed, remaining 220-alert
backlog by rule/file, and the next recommended wave (app.py
py/path-injection, 184 alerts).
* fix: make FolderCleanRootFalsePositiveTests platform/CI independent
FOLDER_CLEAN_ROOTS includes /tmp by design, so the "outside allowed
roots" test's use of a bare tempfile.TemporaryDirectory() (which lives
under /tmp on Linux CI) was actually inside the allowed roots there,
even though it happened to be outside them on Windows. Pin
FOLDER_CLEAN_ROOTS to a disjoint sibling directory instead, so the
test is deterministic on every platform.
* docs: dismiss job-log alerts as won't-fix, correct SEC-002 counts
The 4 job-log carve-out alerts (#43/#46/#68/#83) plus a 5th instance
that surfaced after the delete_import_review_folder fix (#413, same
_delete_review_source_folder log sources, new alert number once the
sibling "error" field became a static string) are dismissed
individually on GitHub as won't-fix with the CLAUDE.md raw-debug-detail
justification, rather than left open -- the hard zero-open-PR-alerts
merge gate requires a real disposition. Corrects the dismissal count
(10, not 5) and remaining backlog (225, not 220).
… end-to-end
The engine-side /audio/acoustid-lookup adapter (ARCH-012) read
ACOUSTID_API_KEY from its own process environment, but both compose files
only passed it to beets-web-manager -- the engine container never received
it, so the adapter would always report unavailable in a real deployment
regardless of configuration. Added it to the beets service in both
docker-compose.yml and docker-compose.arrs.yml, and corrected the /status
capability diagnostic (it required pyacoustid+chroma, which this fpcalc/
HTTP-based adapter never touches, instead of the key it actually needs).
Fixed several places where the adapter's distinct status values (matched/
no_match/unavailable/invalid_media/timeout/provider_error/analysis_error)
collapsed into a bare [] indistinguishable from a genuine no-match:
- helpers_mb._acoustid_lookup's except-block called logging.warning() with
no `logging` import, so a control-agent hiccup raised an unhandled
NameError instead of degrading gracefully (reproduced and fixed).
- Added _acoustid_lookup_status()/_acoustid_capability_available() so
callers can tell "not attempted" apart from "attempted, no match"; wired
the AI-evidence "acoustid_available" field to the latter instead of
bool(candidates).
- _acoustid_lookup_cached's disk cache (never expires) persisted every
outcome including transient failures; now only matched/no_match/
invalid_media are cached, so a temporary engine outage can't freeze in
as a permanent false no-match for a file.
- BeetsClient.acoustid_lookup() called the shared _request(), which
collapses any >=500 response into a generic BeetsUnavailableError,
discarding the engine's real {"status": "unavailable"} body -- found via
live two-container testing, not unit tests (which had mocked _request
directly and never exercised this path). Rewritten to preserve the
structured body from a non-2xx response while still raising for 401 and
genuine transport failures.
Also refactored is_safe_path() into resolve_safe_path(), which returns the
canonical (symlink-resolved) path instead of a bare bool, and updated every
direct filesystem sink (tags/read, tags/write, files/move, files/delete,
files/mkdir, acoustid-lookup, album deletion) to operate on that resolved
path instead of re-using the original caller-supplied string, closing the
gap where a sink revalidated nothing between the safety check and the
actual filesystem call.
Verified live: valid/invalid/missing auth, missing-key -> unavailable (not
no_match), path traversal/outside-root/nonexistent/directory/null-byte/
symlink-escape rejection, and the full BeetsClient -> helpers_mb chain,
against a disposable two-container engine+client stack (no production
paths, no real AcoustID network calls).
yt-dlp==2024.11.4 predates the yt_dlp.extractor.youtube.pot package that bgutil-ytdlp-pot-provider imports at plugin-discovery time. yt-dlp's plugin loader swallows the resulting ModuleNotFoundError and continues, so the PO-token provider silently never registered on every yt-dlp invocation -- only a console warning was ever printed, easy to miss in job logs. Bumped to yt-dlp==2025.6.9, the earliest version verified here to both import the provider and actually register it (BgUtilHTTP appears in yt_dlp.extractor.youtube.pot._registry._pot_providers). Added a real import/registration regression test instead of relying on yt-dlp to keep swallowing the failure quietly.
This check has never had real CI coverage: .github/workflows/unit-tests.yml (the job that runs this test suite) never builds any web-manager image -- only docker-build.yml does, tagged beets-web-manager:ci, in a separate job that doesn't run these tests. The test hardcoded beets-web-manager:0.1.0, a tag nothing in CI ever produces, so it silently skipped (or, before the prior commit's guard, coincidentally "passed" against a nonexistent image) on every CI run. Checking both beets-web-manager:0.1.0 and beets-web-manager:ci doesn't close the CI coverage gap by itself, but it means the check is at least capable of running for real in whichever environment has either image built, instead of being permanently tied to a tag that never exists in the job that runs it. Documented the gap in the test's docstring so it isn't mistaken for real CI coverage.
…view Independent final review of PR #46 found and fixed: - backend/beets_control_agent.py had TWO conflicting definitions of is_safe_path(): a broken one (line ~488, calling resolve_safe_path(...) is not None -- which never returns None, since resolve_safe_path() raises UnsafePathError instead, so this would crash rather than return False for unsafe input) left over from commit 0b11a26's rebase reconciliation, silently shadowed by the correct try/except version already present on the rebased base (refactor/external-beets-engine @ 125752d). Removed the broken, dead first definition and its accompanying dead `except Exception: return None` duplicate clause in _path_is_within(), which was unreachable but violated that function's own bool return contract. - Two duplicate bare `return` statements (tag-write's "Unsupported tag field" branch, and the new /audio/acoustid-lookup endpoint's "Path is not a regular file" branch) -- both dead code from the same rebase, removed. - /audio/acoustid-lookup echoed the raw request path (`file_path`) into 403/404/400 error bodies and a raw Python exception string into a 500 body, inconsistent with every other sink in this file. Changed to the same generic-message pattern already used by /files/delete, /files/move, /commands/execute, etc. - Removed the non-functional `# codeql[py/path-injection] -- sanitized by resolve_safe_path() above` comments: GitHub Code Scanning has no inline source-comment suppression mechanism, so these had zero actual effect and misrepresented the alert as handled. A live temporary CodeQL-validation PR against main (closed without merging) confirmed 2 open py/path-injection alerts remain on these exact lines for this exact head. Documented as ARCH-014 in docs/TECHNICAL_DEBT.md with the real remediation options (advanced model pack, blocked while Default Setup is enabled; or an owner-reviewed dismissal). - Escalated ARCH-007 with a confirmed, more severe finding unrelated to this PR's own diff: backend/beets_client.py's raw_sqlite_query() unconditionally raises (already true on the rebased base, not introduced here), so every one of the ~18 remaining _db()/con.execute(...) call sites in app.py always raises or is silently swallowed by a broad except. Flagged P0 for separate follow-up; out of scope for this PR to fix. All new tests pass; full suite unaffected (1553 tests, OK, skipped=6 locally / 61 on Linux CI -- unchanged).
Codex's independent review of the frozen head found a P1 crash: the control agent's /audio/acoustid-lookup endpoint uses urllib.request.Request/urlopen and urllib.error.HTTPError, but the file only imported urllib.parse. Importing a submodule of a package does not make sibling submodules accessible as attributes of the shared package object unless something else in the process happens to have imported them first -- so this worked by accident in the full test suite (another test module's own `import urllib.request` had already bound that attribute onto the process-wide `urllib` module) but crashed with `AttributeError: module 'urllib' has no attribute 'request'` in any genuinely isolated process, including the real container: a valid file passed authentication, path validation, and fingerprinting, then crashed the instant a configured ACOUSTID_API_KEY reached the actual provider request. Added the missing `import urllib.error` / `import urllib.request` (matching the pattern already used in helpers_mb.py), and restructured the request's exception handling to separate three previously-conflated outcomes: HTTPError and URLError both now map to `provider_error` immediately (no pointless retry against a rejection or connection failure); a genuine TimeoutError still retries once before reporting `timeout`; anything else (including a malformed JSON payload) reports `analysis_error`. No response includes the raw HTTP status detail, provider text, or path. Regression coverage: AcoustidLookupProviderRouteTests exercises the real handler through mocked urllib.request.urlopen only (matched, no_match, HTTPError, URLError, timeout-after-retry, malformed payload), and AcoustidLookupIsolatedImportTests spawns a genuinely fresh subprocess that imports only backend.beets_control_agent -- the one check immune to cross-test-module import contamination, verified to fail with the exact reported AttributeError against the unfixed file and pass against the fix.
Codex found helpers_mb._acoustid_lookup_status()'s except-block logging the complete media path and the raw exception string on every control-agent/network failure -- the exception text can itself embed a provider URL, response body, or query parameters (e.g. an API key in a failed request URL). The returned dict also echoed the same raw exception text back to the caller via an "error" field. Replaced both with a stable 12-character hash of the path (sufficient to correlate repeated failures on the same file across log lines without revealing its content) and the exception's type name only. Removed the "error" field from the returned dict entirely -- no caller reads it (confirmed by grep), so nothing downstream relied on it echoing raw text. Regression test: captures real log output via assertLogs with a distinctive full path and a sensitive exception message (embedding a fake internal hostname and token), asserts the complete path, the artist name, the raw exception text, and its embedded secret are all absent from the log, while the exception's class name remains. Verified to fail against the prior implementation and pass against the fix.
Codex found several stale/inaccurate AcoustID statements: - README.md's "Required vs. optional integrations" table claimed AcoustID "falls back to a shared, rate-limited test key" -- directly contradicting the correct statement three lines above it in the same file (ACOUSTID_API_KEY's own description, which has always said the engine fails closed to "unavailable" with no fallback). No such shared key exists anywhere in this codebase. Corrected to match reality. - Four AcoustID-adjacent comments/references (README.md, docs/CONFIGURATION.md, backend/beets_control_agent.py, tests/test_audio_identity_pipeline.py) pointed to ARCH-012 (a disk-walk library gap, unrelated to AcoustID) instead of ARCH-013 (the actual AcoustID capability-gap entry). Corrected all four. - docs/TECHNICAL_DEBT.md's ARCH-014 entry still said alerts #404 and #405 "remain open and unaddressed," describing a state that predates this same review cycle -- both were already independently re-verified and dismissed as false positives with project-owner authorization. Corrected to describe the dismissal, its evidence basis, and that GitHub's alert-fingerprinting carries it forward automatically across rebases, without claiming the underlying CodeQL modeling gap itself is resolved. No claim is made that the broader SEC-002 backlog, ARCH-007, or the overall application is resolved or fully healthy.
Iranman
force-pushed
the
fix/plugin-first-runtime-and-codeql
branch
from
July 31, 2026 02:07
f1b44ea to
66c2566
Compare
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.
Summary
POST /audio/acoustid-lookup), eliminating directfpcalcexecution and directapi.acoustid.orgHTTP calls in the web manager.matched,no_match,unavailable,invalid_media,timeout,provider_error,analysis_error) end-to-end through the web manager workflow.ACOUSTID_API_KEYto the engine container environment, failing closed when missing without crashing unrelated setup or diagnostics.ffmpegin the web manager for yt-dlp source acquisition postprocessing while removinggitand plugin-only dependencies.AGENTS.md,CLAUDE.md,docs/AGENT_WORKFLOW.md,docs/ARCHITECTURE.md, andREVIEW.md.resolve_safe_path) in control agent endpoints.Branch topology -- current with
mainRebased onto
main@92a7ea62274eb9de3e3a4aafd4e18152e7235109(which includes PR #55's non-app-backend CodeQL remediation and PR #56's app.py stack-trace-exposure remediation, both separate SEC-002 waves).docs/TECHNICAL_DEBT.md's SEC-002 section (Waves 1-3, full alert accounting, remaining backlog) is preserved verbatim frommain; this PR's own ARCH-007 escalation, ARCH-013, and ARCH-014 entries are appended after it, each exactly once. Backup refbackup/pr46-before-main-92a7ea6-20260730-184625preserved before the rewrite.Codex independent review -- REQUEST CHANGES addressed
An independent Codex review of the previously frozen head (
f1b44ea1...) returned REQUEST CHANGES with two P1 blockers and two P3 findings, all now fixed:P1 -- base invalidated:
mainhad advanced past this branch's base. Fixed by the rebase above.P1 -- configured AcoustID provider path crashed:
backend/beets_control_agent.pyusedurllib.request.Request/urlopenandurllib.error.HTTPErrorbut only importedurllib.parse. This worked by accident in the full test suite (another test module's own import had already bound the attribute onto the sharedurllibpackage object) but crashed withAttributeError: module 'urllib' has no attribute 'request'in any genuinely isolated process, including the real container -- a valid file would pass auth/path-validation/fingerprinting, then crash the instant a configured key reached the actual provider request. Fixed: added the missing imports and restructured exception handling soHTTPError/URLErrorboth map toprovider_errorimmediately, a genuineTimeoutErrorstill retries once before reportingtimeout, and nothing echoes raw provider/exception text. Verified three independent ways: (1)AcoustidLookupProviderRouteTests, mocking onlyurllib.request.urlopen, exercises the real handler through matched/no_match/HTTPError/URLError/timeout/malformed-JSON; (2)AcoustidLookupIsolatedImportTestsspawns a genuinely fresh subprocess importing only this module -- proven to fail with the exact reportedAttributeErroragainst the unfixed file; (3) live in a disposable container with a real syntheticACOUSTID_API_KEY, realfpcalcfingerprinting a generated WAV file, andapi.acoustid.orgDNS-redirected to an unreachable local address (never contacts the real service) -- the request reached a clean502 provider_error, and the engine logs contain zeroAttributeError/traceback lines.P3 -- stale documentation: fixed 4 incorrect
ARCH-012->ARCH-013AcoustID references (README.md,docs/CONFIGURATION.md,backend/beets_control_agent.py,tests/test_audio_identity_pipeline.py); removed a README claim of a "shared, rate-limited test key" fallback that directly contradicted the correct statement three lines above it in the same file (no such fallback exists anywhere in this codebase); correctedARCH-014to state alerts #404/#405 were dismissed as false positives (not "remain open").P3 -- media path disclosure in helper log:
helpers_mb._acoustid_lookup_status()'s failure path logged the complete media path and raw exception text (which can itself embed a provider URL or query string). Fixed: logs a 12-character path hash plus the exception's type name only; removed the same raw text from the returned dict'serrorfield (no caller read it). Regression test captures real log output viaassertLogswith a distinctive path and an exception embedding a fake secret, and asserts both are absent while a useful diagnostic remains -- verified to fail against the prior implementation.Validation (frozen head
66c2566efe5db21cdf52db10fa7335d1c4f8d0a7)OK, 0 failures, 0 errors, 7 skipped.npm audit --audit-level=high0 vulnerabilities.git diff --check, bothdocker compose configfiles -- all clean.pushandpull_requesttriggers (14 executions).CodeQL -- analyzed on the exact frozen head, zero open alerts
Alerts #404 and #405 (
py/path-injection, the.exists()/.is_file()sinks in/audio/acoustid-lookup) were re-verified against the unchanged data path and remaindismissed/false positive-- GitHub's alert-fingerprinting continues to carry the dismissal forward automatically across rebases, since the underlying sink code is unchanged (only shifted by 2 lines from the new imports). 0 open PR-introduced alerts. AggregateCodeQLcheck: green.PR #50 remains closed, not merged. Issue #51 (ARCH-007) remains open and separate.
Remaining limitations
main's much larger pre-existing CodeQL backlog (SEC-002; 220 alerts remain after PR fix: remediate main CodeQL security backlog (critical + newest batch) #52/fix: remediate non-app backend CodeQL baseline #55/fix: remediate app stack-trace CodeQL baseline #56's waves) is separate, unresolved, and unrelated to this PR.