fix(security): bind analysis paths to filesystem authority

[seonghobae]

Security gap

Analysis path strings are authority-bearing inputs. This Draft addresses #852 from protected develop without dependency, lockfile, workflow, model, network, database, or .trivyignore changes.

Exact current state

• Exact head: 30293ac30680f9cff5b3888bb9399d6b611f7e75.
• Protected base: develop@acdbea6344fe1231c39535b575f4de35e4c607c9.
• The branch remains Draft. fix(security): establish canonical npm, PDF.js, Nanoid, and Undici baseline #783 owns the protected JavaScript dependency baseline, test(quality): enforce Python branch coverage #861 owns Python branch-coverage baseline integration, central required-workflow fixes must first reach protected central main, and no qualifying independent non-author last-push approval exists.

Bounded authority contract

The branch rejects relative/drive-relative paths, traversal, UNC/network roots including mixed separator spellings, Win32 device namespaces and console aliases, alternate streams, foreign-host absolute authority at native preflight, direct selected-source symlinks, non-directory writable roots, and already-present derived cache/temp symlink escapes. Lexical cross-platform validation stays separate from native filesystem authority and diagnostics stay payload-safe.

Exact derived feature-cache files, atomic-write siblings, stem-work arrays, and the stem-work metadata sidecar are revalidated against app-owned authority before repository-owned I/O. Canonicalization remains a point-in-time check and does not claim descriptor/handle-level race freedom against a privileged local actor replacing entries after validation.

Current-head repair adopted from validated successor commit

Exact-current-head review on predecessor 8a9321d3eeadbf4a35c996d3d3a013b294af38ca found two still-valid BandScope-owned defects:

1. _run_stem_separation_with_timeout trusted the worker-returned arraysPath as writable sidecar/reload authority, allowing a mismatched ok_file envelope to target a sibling outside the parent-helper-authorized stem-work child.
2. A late path-authority ValueError from _build_local_audio_features after progress remained fail-closed but was reported as engine_unavailable, misleading the musician instead of returning the existing payload-safe invalid_request contract.

Successor commit 30293ac30680f9cff5b3888bb9399d6b611f7e75 was independently verified to be a direct child of 8a9321d.... Rather than open another #852 PR, the canonical #858 branch was fast-forwarded non-forced to that commit.

The repair:

• compares the worker arraysPath to the already-authorized parent-helper arrays_path and rejects missing/mismatched values as Invalid analysis job request: invalid field 'tempRoot' before sidecar write or unlink;
• writes the {digest}.json sidecar and reloads arrays only through the authorized arrays_path;
• preserves ordinary media ValueError / FileNotFoundError on the existing engine-unavailable path, but keeps ValueError messages beginning Invalid analysis job request: as invalid_request even when discovered after progress;
• adds realistic regressions including an outside sidecar sentinel and late invalid-request classification; and
• aligns doctoring and CHANGELOG.md with the exact authority contract.

The two review threads that described these defects were rechecked against 30293ac... and resolved only after the production code matched the requested invariant. The predecessor-head Cursor CHANGES_REQUESTED review was then dismissed as stale after this exact-current-head verification; that dismissal is not an approval and does not satisfy the independent-review gate.

TDD / verification history

Earlier RED/GREEN slices on this branch cover Windows leading-space normalization, CONIN$ / CONOUT$, mixed-separator UNC/device authority, derived cache/temp symlink escapes, exact feature-cache sibling authority, atomic-write siblings, and stem-work sidecar symlinks. Historical successful or failed predecessor checks remain historical and do not transfer.

Exact-current-head repository evidence

Repository workflows for unchanged head 30293ac30680f9cff5b3888bb9399d6b611f7e75 are now terminal.

Terminal-success repository lanes:

• ci 31960639721;
• build-baseline 31960639710;
• release 31960639743;
• sbom 31960639752;
• bandit 31960639766;
• secret-scan-gate 31960639773; and
• SAST Semgrep 31960639817.

security-audit 31960639797 and aggregate Security Scan 31960639724 are terminal-failure/non-passing. This branch has no JavaScript dependency/root-lock or vulnerability-suppression delta; those whole-tree failures remain the protected-base dependency-security baseline owned by canonical #783 and must be regenerated after #783 integrates. They are neither suppressed nor counted as success here.

Merge gate

Keep Draft until one unchanged exact head has all applicable repository/central required workflows terminal-success, exact 100% owned production statement and branch coverage plus other exposed metrics, docstrings/type/lint/build evidence, SAST/security/SBOM/supply-chain/package/release evidence, zero valid unresolved threads/findings, #783/#861/central prerequisites integrated and regenerated, a qualifying independent non-author last-push approval, and ordinary protected-branch acceptance without bypass.

Closes #852 only after protected merge.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 9d57fa8e-8848-4a3b-9ded-7f86f76b88aa

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[seonghobae]

@jules Implement the production half of this Draft on the existing fix/path-authority-boundary-852 branch only, starting from exact head db92b8b1b3698ed8eb03335f9bbe6e24685186dc. The RED contract is already in services/analysis-engine/tests/test_path_authority.py; do not weaken or delete those tests to get green.

Scope is strictly #852: services/analysis-engine/src/bandscope_analysis/api.py, a small dedicated path-authority module if useful, focused tests, CHANGELOG.md, and docs/doctoring/filesystem-path-authority.md. No dependency, lockfile, .trivyignore, workflow, model, generated patch script, or unrelated cleanup changes.

Required behavior:

• localSource.sourcePath, cacheRoot, and tempRoot must reject relative paths, Windows drive-relative forms (including C:..\\x), UNC/network roots, Windows device paths, and lexical ./.. traversal without echoing attacker-controlled path values in errors.
• Preserve ordinary native absolute POSIX paths and ordinary native absolute Windows paths on Windows; do not reject valid local absolute paths merely because CI runs on another OS. Implement lexical shape classification cross-platform, then enforce native-path authority at the actual filesystem-use boundary.
• Before stem separation, resolve the selected local source using a fail-closed helper: expand user, require native absolute local path, reject a direct symlink, resolve(strict=True), require a regular file, and pass only the canonical resolved path to the separator. Keep missing-file vs invalid-request behavior safe and existing public error envelopes compatible.
• Cache/temp derived paths must stay under the canonical authorized root even if the fixed analysis-cache-v1 or stem-work-v1 child already exists as a symlink. Use fixed digest filenames and canonical containment; do not introduce caller-controlled child names.
• Keep source absolute paths out of persisted cache metadata and out of user-facing errors/logs.
• Document the residual TOCTOU boundary honestly rather than claiming descriptor-level race freedom.
• Maintain 100% owned production statement/branch coverage and public docstrings.

Use current primary references in doctoring: Microsoft Learn Windows path formats/namespaces, MITRE CWE-22, and Python 3.14 pathlib documentation, formatted APA 7. First prove the existing RED test fails for the intended reason, then make the narrowest implementation and run focused tests plus full repository quickcheck. If exact head or branch moves before your write, refetch and reconcile rather than overwriting. Keep the PR Draft; do not merge, bypass protection, change reviewer credentials, or touch #783/#1008.

[seonghobae]

Exact-current-head security review on 7655f6cc8b16b6a24fba754a19ea8e45292bc6d1 found a remaining branch-introduced path-authority escape in the cache layer. _analysis_cache_path() resolves the exact <digest>.json child safely, but _feature_cache_paths() then derives <digest>.features.json/.npz with Path.with_suffix() instead of re-running canonical containment. A pre-existing symlink at either derived feature path can therefore be followed by _load_cached_local_audio_features() outside cacheRoot. The atomic-write helpers have the same gap for repository-owned temporary siblings: _store_cached_analysis() opens <digest>.tmp, and _store_cached_local_audio_features() opens *.tmp, without revalidating those exact paths; a pre-existing symlink can be followed and truncate/write a file outside the authorized root before replace() runs. This is distinct from the documented post-validation TOCTOU residual because the escaping symlink may already exist before the helper derives/opens the path.

I am treating this as blocking current #858 semantics. Minimal repair is to regression-lock the exact feature and temporary children first, then resolve each repository-owned child through the existing canonical containment helper immediately before use; do not broaden authority or suppress the race caveat.

[seonghobae]

Exact-head gate refresh for e020e4c77db473b9578746a356a4c3fba391356a after the previously non-terminal runs completed:

• ci run 31930865376 is terminal-success. ci / build-and-test job 95125363073 successfully installed the native Rust numeric extension and completed repository quickcheck; gate / ci / rust-check job 95125363103 also completed successfully, including frontend build, Tauri check, and Tauri tests.
• release run 31930865342 is terminal-success.
• build-baseline run 31930865322 is terminal-success.
• sbom, Semgrep, Bandit, and secret-scan remain terminal-success on this exact head.
• security-audit run 31930865304, job 95125362768, is terminal-failure specifically at Audit npm dependencies; setup/install completed first, and Python/Rust audit stages are skipped after npm exits. The exact check-run annotation is only the generic exit-code-1 marker. Both the exact job-log and run-log endpoints were inspected and return an empty payload through the connector, so no unobserved log text is being represented as evidence.
• Aggregate Security Scan run 31930865354 remains terminal-failure only in trivy-fs job 95125364112. The exact job sequence shows filesystem scan and SARIF-output requirement succeed before the finding gate fails; OSV base-vs-head/PR-introduced comparison, dependency review, and Scorecard are terminal-success. The exact Trivy annotation is likewise only the generic exit-code-1 marker and the job-log endpoint returns an empty payload through the connector.

This seven-file branch has no JavaScript dependency/root-lock delta. Combined with the clean exact-head PR-introduced OSV comparison and the already inspected protected-base pdfjs-dist finding on this same run, the remaining whole-tree npm/Trivy failures stay classified as inherited protected-develop evidence owned by canonical #783. They are not suppressed, duplicated, or counted as success here.

Review refresh: both visible inline threads remain resolved and the formal review list still contains no qualifying independent non-author approval. Python branch coverage is also not inferred from this protected-base-lineage quickcheck; #861 remains the measurement prerequisite. Keep Draft; no merge/auto-merge is eligible.

[cursor]

Exact-head review of 8a9321d3eeadbf4a35c996d3d3a013b294af38ca.

The prior stem-work {digest}.json sidecar residual is fixed on this head: a pre-existing symlink next to the authorized arrays file is rejected before write, and the job fails as invalid_request before progress. Mixed-separator UNC/device prefixes, CONIN$/CONOUT$, leading ASCII-space components, and feature-cache sibling re-resolve still hold.

Do not merge this head as closing #852. _run_stem_separation_with_timeout still treats a worker-supplied arraysPath as writable tempRoot authority. A mismatched ok_file envelope can write or unlink a sidecar outside the authorized stem-work child. I reproduced that write/unlink against an outside sentinel, then bound sidecar write and array reload to the parent-helper authorized arrays_path on successor 30293ac30680f9cff5b3888bb9399d6b611f7e75.

Late _build_local_audio_features path-authority ValueError after progress is still mapped to engine_unavailable on this head. That is fail-closed but tells the musician the engine died instead of asking them to pick another temp folder. The successor keeps that failure as invalid_request.

Inherited JavaScript findings stay with #783. Python branch-coverage ownership stays with #861. Canonicalization remains point-in-time; descriptor-level TOCTOU is still out of scope.

Prefer the successor over this head. Do not open a third #852 PR.

  

_{Sent by Cursor Automation: Fix Issues}

Findings were verified against successor head 30293ac and addressed on the canonical #858 branch: worker arraysPath is now required to equal the parent-authorized arrays_path before sidecar/reload I/O, and late Invalid analysis job request: ValueError remains invalid_request. Both corresponding threads are resolved. Dismissing this predecessor-head CHANGES_REQUESTED review only clears stale review state; it is not approval, and current-head required checks plus qualifying independent last-push approval remain required.