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

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

Extract the CLI into a unique staging directory and atomically rename it
into place instead of the check-delete-recreate sequence. Concurrent SPM
builds sharing ~/.cache can no longer wipe each other's in-progress
extraction; a build that loses the publish race reuses the winner's
fully-formed binary rather than running a partially-written one.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(review): correct nested-binary publish + harden publish race (DEVA11Y-482)

Review of PR #32 found a regression and residual races in the atomic-publish
change:

- P1: locateExecutable recurses, so a binary with the right name can sit in a
  nested subdir; the old lastPathComponent check skipped relocating it, leaving
  stagedExecutableURL/expectedExecutableURL pointing at a non-existent top-level
  path (ensureExecutablePermissions would throw). Compare full standardized URLs
  so any not-already-in-place binary is relocated.
- P2: the publish catch did a non-atomic fileExists->removeItem->moveItem that
  could ENOENT-crash or fail a build when a peer republished concurrently. Make
  removeItem best-effort and, on retry failure, reuse a peer's valid binary
  instead of throwing.
- P3: reword the publishVersionDirectory doc to stop overstating atomicity of the
  replace path; use UUID().uuidString for the staging suffix to match the file's
  existing convention.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

* fix(review): resolve residual cache-publish findings (DEVA11Y-482)

Follow-up to the PR #32 review, closing the report-only findings:

- Staging leak (#4): extraction helpers exit() via forwardExit on failure and
  SIGKILL bypasses defer, leaking .tmp.* staging dirs. Add sweepStaleStaging(),
  a best-effort mtime-gated sweep (>1h old only, so a concurrent build's in-flight
  staging is never deleted) run at the start of prepareArtifact.
- Cross-platform moveItem (#5): stop depending on moveItem's throw-on-existing
  semantics (Darwin throws; POSIX rename silently replaces an empty dir). Check
  versionDirectory existence explicitly; absent -> atomic rename, present ->
  reuse/replace. Removes the untestable platform assumption.
- Windows archive (#6): download the .zip to a sibling temp file outside the
  staging dir (with its own defer cleanup) so a failed removal can never bake the
  archive into the published version directory; a leftover is swept later.

Not addressed here: binary integrity/signature verification (#7) is pre-existing,
needs a trusted out-of-band digest source, and belongs with APPSEC-415 — not this
TOCTOU fix.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

Author: Crash0v3rrid3

Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift

@@ -192,8 +192,34 @@ private struct BrowserStackCLIDownloader {
         return cacheRoot
     }
 
+    /// Best-effort removal of stale staging artifacts (`.tmp.*` files and directories) left
+    /// behind when a previous extraction was interrupted. The extract helpers call
+    /// forwardExit()/exit() on failure and SIGKILL can hit at any point, both of which
+    /// bypass the `defer` cleanup in prepareArtifact. Only entries older than one hour are
+    /// removed, so a concurrent build's in-flight staging directory is never deleted
+    /// mid-extraction.
+    private func sweepStaleStaging(in cacheRoot: URL) {
+        let staleStagingAge: TimeInterval = 3600
+        let now = Date()
+        guard let entries = try? fileManager.contentsOfDirectory(
+            at: cacheRoot,
+            includingPropertiesForKeys: [.contentModificationDateKey],
+            options: []
+        ) else {
+            return
+        }
+        for entry in entries where entry.lastPathComponent.hasPrefix(".tmp.") {
+            let modified = (try? entry.resourceValues(forKeys: [.contentModificationDateKey]))?.contentModificationDate
+            guard let modified, now.timeIntervalSince(modified) > staleStagingAge else {
+                continue
+            }
+            try? fileManager.removeItem(at: entry)
+        }
+    }
+
     private func prepareArtifact(using info: ArtifactInfo) async throws -> BrowserStackCLIArtifact {
         let cacheRoot = try ensureCacheRootExists()
+        sweepStaleStaging(in: cacheRoot)
         let versionDirectory = cacheRoot.appendingPathComponent(info.version, isDirectory: true)
         let executableName = info.executableName
         let expectedExecutableURL = versionDirectory.appendingPathComponent(executableName, isDirectory: false)
@@ -202,37 +228,104 @@ private struct BrowserStackCLIDownloader {
             return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL)
         }
 
-        if fileManager.fileExists(atPath: versionDirectory.path) {
-            try fileManager.removeItem(at: versionDirectory)
-        }
-        try fileManager.createDirectory(at: versionDirectory, withIntermediateDirectories: true)
+        // Extract into a unique staging directory and atomically publish it to the final
+        // version directory (DEVA11Y-482). The previous check-delete-recreate sequence was
+        // a TOCTOU: two concurrent builds sharing ~/.cache could both fall through the
+        // isExecutableFile check, then one instance's removeItem/createDirectory would wipe
+        // the other's in-progress extraction, corrupting the binary or leaving a partial
+        // file that locateExecutable's fallback would happily run. Staging + rename means a
+        // version directory only ever becomes visible fully-formed, and a loser of the
+        // publish race reuses the winner's binary instead of clobbering it.
+        let stagingDirectory = cacheRoot.appendingPathComponent(
+            ".tmp.\(info.version).\(UUID().uuidString)",
+            isDirectory: true
+        )
+        defer { try? fileManager.removeItem(at: stagingDirectory) }
+        try fileManager.createDirectory(at: stagingDirectory, withIntermediateDirectories: true)
 
         Diagnostics.remark("BrowserStackAccessibilityLint: Downloading CLI \(info.version)...")
 
         #if os(Windows)
-        let archiveURL = versionDirectory.appendingPathComponent("browserstack-cli.zip")
+        // Download the archive to a sibling temp file *outside* the staging directory so a
+        // failed cleanup (e.g. an AV scanner or indexer holding a handle on Windows) can
+        // never bake the .zip into the published version directory. A leftover is a `.tmp.*`
+        // sibling that sweepStaleStaging reclaims later.
+        let archiveURL = cacheRoot.appendingPathComponent(".tmp.\(info.version).\(UUID().uuidString).zip")
+        defer { try? fileManager.removeItem(at: archiveURL) }
         try await download(from: info.resolvedURL, to: archiveURL)
         Diagnostics.remark("BrowserStackAccessibilityLint: Extracting CLI \(info.version)...")
-        try unzip(archive: archiveURL, into: versionDirectory)
-        try? fileManager.removeItem(at: archiveURL)
+        try unzip(archive: archiveURL, into: stagingDirectory)
         #else
-        try extractWithBsdtar(from: info.resolvedURL, into: versionDirectory)
+        try extractWithBsdtar(from: info.resolvedURL, into: stagingDirectory)
         #endif
 
-        let locatedBinary = try locateExecutable(in: versionDirectory, preferredName: executableName)
-        let finalBinaryURL: URL
-        if locatedBinary.lastPathComponent == executableName {
-            finalBinaryURL = locatedBinary
-        } else {
-            finalBinaryURL = expectedExecutableURL
-            if fileManager.fileExists(atPath: finalBinaryURL.path) {
-                try fileManager.removeItem(at: finalBinaryURL)
+        // Normalise the binary to the expected name *inside* the staging directory so the
+        // published version directory is always structurally complete before it is renamed.
+        // Compare full paths, not just the last component: locateExecutable recurses, so a
+        // binary that already has the right name can still sit in a nested subdirectory
+        // (e.g. a versioned tarball folder). Relocating it to the top-level staged path
+        // unless it is already exactly there guarantees stagedExecutableURL exists before
+        // we set permissions and publish.
+        let locatedBinary = try locateExecutable(in: stagingDirectory, preferredName: executableName)
+        let stagedExecutableURL = stagingDirectory.appendingPathComponent(executableName, isDirectory: false)
+        if locatedBinary.standardizedFileURL != stagedExecutableURL.standardizedFileURL {
+            if fileManager.fileExists(atPath: stagedExecutableURL.path) {
+                try fileManager.removeItem(at: stagedExecutableURL)
             }
-            try fileManager.moveItem(at: locatedBinary, to: finalBinaryURL)
+            try fileManager.moveItem(at: locatedBinary, to: stagedExecutableURL)
+        }
+        try ensureExecutablePermissions(at: stagedExecutableURL)
+
+        try publishVersionDirectory(from: stagingDirectory, to: versionDirectory, expectedExecutableURL: expectedExecutableURL)
+        return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL)
+    }
+
+    /// Publishes a fully-prepared staging directory to its final version directory.
+    ///
+    /// Correctness does not depend on `moveItem`'s throw-on-existing-destination behaviour,
+    /// which differs across Foundation platforms (Darwin throws `fileWriteFileExists`; a
+    /// bare POSIX `rename(2)` silently replaces an empty destination). We check for the
+    /// destination explicitly: when it is absent the publish is a single atomic rename on
+    /// the shared cache filesystem (staging and version dir are both children of cacheRoot),
+    /// so concurrent builds never observe a half-formed version directory; when it is
+    /// present — another build won the race, or `forceDownload` is refreshing a stale copy —
+    /// a valid published binary is reused, otherwise the stale directory is replaced and the
+    /// rename retried once. The replace path is deliberate last-writer-wins and is *not*
+    /// atomic; it tolerates a peer removing or republishing the directory concurrently
+    /// rather than failing the build.
+    private func publishVersionDirectory(from stagingDirectory: URL, to versionDirectory: URL, expectedExecutableURL: URL) throws {
+        // Fast path: destination absent -> single atomic rename. A create race that briefly
+        // loses (destination appears between the check and the move) falls through to the
+        // shared "destination present" handling below rather than failing.
+        if !fileManager.fileExists(atPath: versionDirectory.path) {
+            do {
+                try fileManager.moveItem(at: stagingDirectory, to: versionDirectory)
+                return
+            } catch {
+                // Fall through.
+            }
+        }
+
+        // Destination present: reuse a valid binary unless a forced refresh was requested.
+        if !forceDownload, fileManager.isExecutableFile(atPath: expectedExecutableURL.path) {
+            return
         }
 
-        try ensureExecutablePermissions(at: finalBinaryURL)
-        return BrowserStackCLIArtifact(version: info.version, executableURL: finalBinaryURL)
+        // Stale/incomplete destination, or a forced refresh: replace and retry once.
+        // removeItem is best-effort so a peer deleting the directory first cannot turn into
+        // an ENOENT crash mid-race.
+        try? fileManager.removeItem(at: versionDirectory)
+        do {
+            try fileManager.moveItem(at: stagingDirectory, to: versionDirectory)
+        } catch {
+            // A peer republished the version directory between our remove and move. If it
+            // now holds a valid binary, treat that as success rather than failing a build
+            // that already has the artifact it needs.
+            if fileManager.isExecutableFile(atPath: expectedExecutableURL.path) {
+                return
+            }
+            throw error
+        }
     }
 
 #if !os(Windows)