fix(cache): atomic version-dir publish to close cache TOCTOU (DEVA11Y-482)

[Crash0v3rrid3]

What & why

Fixes DEVA11Y-482 (F-013, CWE-362, Medium) — a check‑delete‑recreate TOCTOU in the SPM plugin's CLI cache.

prepareArtifact did fileExists → removeItem → createDirectory → extract on the shared ~/.cache version directory. Two concurrent SPM builds (Xcode parallel targets, or a CI matrix sharing a cache volume) can both miss the isExecutableFile fast‑path and enter this path; one instance's removeItem/createDirectory then wipes the other's in‑progress extraction. Result: non‑deterministic build failures, or locateExecutable's fallback running a partially‑written (or planted) binary.

Change

• Extract into a unique staging directory (.tmp.<version>.<globallyUniqueString>) under the cache root, normalise the binary name and set permissions inside staging.
• Atomically publish with FileManager.moveItem (rename(2)) so a version directory only ever becomes visible fully‑formed.
• New publishVersionDirectory(...) helper: if the destination already exists (another build won, or forceDownload is replacing a stale copy) reuse the valid published binary, otherwise replace and retry the rename once.
• defer cleans up the staging directory on every exit path.

Testing

• swiftc -parse clean; brace balance verified. The package vends only a plugin (no buildable target), so full compilation happens via a consumer — not run here. Uses only Foundation APIs already present in the file (moveItem, removeItem, createDirectory, ProcessInfo.processInfo.globallyUniqueString).

Draft for review — please sanity‑check the forceDownload concurrent‑refresh branch.

🤖 Generated with Claude Code

[Crash0v3rrid3]

Claude Code PR Review

PR: #32 • Head: 833eff8 • Reviewers: stack:pr-review (aggregated correctness/security/testing/perf/quality)

Summary

Fixes a cache-directory TOCTOU (DEVA11Y-482): the SPM plugin now extracts the CLI into a unique staging directory and atomically renames it into the final version directory (publishVersionDirectory), with a sweepStaleStaging helper, a write-probe, and race-loser reuse — so a version directory only ever becomes visible fully-formed.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced
High	Security	Authentication/authorization checks present	N/A	No auth surface
High	Security	Input validation and sanitization	Pass	Version sanitized vs traversal; HTTPS-only download retained
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	—
High	Correctness	Logic is correct, handles edge cases	Pass	Staging+rename publish sound; race-loser reuse + forceDownload replace traced
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	try? swallow limited to Windows force-refresh edge (Low)
High	Correctness	No race conditions or concurrency issues	Pass	Closes the prior check-delete-recreate TOCTOU
Medium	Testing	New code has corresponding tests	Fail	Publish/sweep branches have no automated coverage
Medium	Testing	Error paths and edge cases tested	Fail	Same gap
Medium	Testing	Existing tests still pass (no regressions)	Pass	Demo fixtures unaffected
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	—
Medium	Performance	Long-running tasks use background jobs	N/A	—
Medium	Quality	Follows existing codebase patterns	Pass	Uses PluginError/Diagnostics conventions
Medium	Quality	Changes are focused (single concern)	Pass	Scoped to artifact preparation
Low	Quality	Meaningful names, no dead code	Pass	Old in-place-extract path removed
Low	Quality	Comments explain why, not what	Pass	Rationale documented
Low	Quality	No unnecessary dependencies added	Pass	—

Findings

• File: Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift:222

• Severity: Medium

• Reviewer: stack:pr-review

• Issue: The new publish/sweep concurrency logic (publishVersionDirectory fast-path/reuse/replace branches, sweepStaleStaging) has no automated test coverage; the demo fixtures exercise scan output, not the downloader.

• Suggestion: Extract the pure filesystem-publish logic into an internally-visible helper and add tests for sequential publish, reuse-when-present, and forceDownload replace.

• File: Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift:317

• Severity: Low

• Reviewer: stack:pr-review

• Issue: On Windows, if removeItem(versionDirectory) fails (locked dir) on the forceDownload path, try? swallows it and the catch returns the stale binary as success — a forced refresh silently no-ops.

• Suggestion: On the forced path, surface a diagnostic when removal+move both fail rather than treating stale-present as success.

• File: Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift:222

• Severity: Low

• Reviewer: stack:pr-review

• Issue: sweepStaleStaging runs before the cache-hit fast-path return, so a warm-cache invocation pays a directory enumeration + per-entry stat unnecessarily.

• Suggestion: Move the sweep after the cache-hit early return (only when extraction is needed).

Residual (non-gating): the moveItem atomicity relies on same-volume rename(2) (holds for the current cache layout); a pre-existing locateExecutable fallback can promote a non-executable payload — out of scope for this PR.

---

Verdict: PASS — closes the TOCTOU correctly; no High/Critical. Recommend the Medium test-coverage gap as follow-up.