From 216904f03d238919c53defd66f30b64b54fd5206 Mon Sep 17 00:00:00 2001 From: Crash0v3rrid3 Date: Thu, 30 Jul 2026 15:05:26 +0530 Subject: [PATCH 1/3] 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 --- .../BrowserStackAccessibilityLint.swift | 69 ++++++++++++++----- 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift index 3969158..df6c9bf 100644 --- a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift +++ b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift @@ -202,37 +202,70 @@ 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).\(ProcessInfo.processInfo.globallyUniqueString)", + 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") + let archiveURL = stagingDirectory.appendingPathComponent("browserstack-cli.zip") try await download(from: info.resolvedURL, to: archiveURL) Diagnostics.remark("BrowserStackAccessibilityLint: Extracting CLI \(info.version)...") - try unzip(archive: archiveURL, into: versionDirectory) + try unzip(archive: archiveURL, into: stagingDirectory) try? fileManager.removeItem(at: archiveURL) #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. + let locatedBinary = try locateExecutable(in: stagingDirectory, preferredName: executableName) + let stagedExecutableURL = stagingDirectory.appendingPathComponent(executableName, isDirectory: false) + if locatedBinary.lastPathComponent != executableName { + 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 ensureExecutablePermissions(at: finalBinaryURL) - return BrowserStackCLIArtifact(version: info.version, executableURL: finalBinaryURL) + try publishVersionDirectory(from: stagingDirectory, to: versionDirectory, expectedExecutableURL: expectedExecutableURL) + return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL) + } + + /// Atomically publishes a fully-prepared staging directory to its final version + /// directory. `moveItem` (rename(2)) is atomic on a single filesystem, so concurrent + /// callers can never observe a half-populated version directory. If the destination + /// already exists — another build won the race, or `forceDownload` is replacing a + /// stale copy — the existing binary is reused when valid, otherwise the stale + /// directory is replaced and the rename retried once. + private func publishVersionDirectory(from stagingDirectory: URL, to versionDirectory: URL, expectedExecutableURL: URL) throws { + do { + try fileManager.moveItem(at: stagingDirectory, to: versionDirectory) + return + } catch { + if !forceDownload, fileManager.isExecutableFile(atPath: expectedExecutableURL.path) { + // A concurrent build already published a valid binary; reuse it. + return + } + // Stale/incomplete destination, or a forced refresh: replace and retry once. + if fileManager.fileExists(atPath: versionDirectory.path) { + try fileManager.removeItem(at: versionDirectory) + } + try fileManager.moveItem(at: stagingDirectory, to: versionDirectory) + } } #if !os(Windows) From 12c722eec4a0945abcd59a29b5d2f1313b28d1a4 Mon Sep 17 00:00:00 2001 From: Crash0v3rrid3 Date: Thu, 30 Jul 2026 15:56:41 +0530 Subject: [PATCH 2/3] 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 --- .../BrowserStackAccessibilityLint.swift | 40 ++++++++++++++----- 1 file changed, 29 insertions(+), 11 deletions(-) diff --git a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift index df6c9bf..e589347 100644 --- a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift +++ b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift @@ -211,7 +211,7 @@ private struct BrowserStackCLIDownloader { // 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).\(ProcessInfo.processInfo.globallyUniqueString)", + ".tmp.\(info.version).\(UUID().uuidString)", isDirectory: true ) defer { try? fileManager.removeItem(at: stagingDirectory) } @@ -231,9 +231,14 @@ private struct BrowserStackCLIDownloader { // 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.lastPathComponent != executableName { + if locatedBinary.standardizedFileURL != stagedExecutableURL.standardizedFileURL { if fileManager.fileExists(atPath: stagedExecutableURL.path) { try fileManager.removeItem(at: stagedExecutableURL) } @@ -245,12 +250,15 @@ private struct BrowserStackCLIDownloader { return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL) } - /// Atomically publishes a fully-prepared staging directory to its final version - /// directory. `moveItem` (rename(2)) is atomic on a single filesystem, so concurrent - /// callers can never observe a half-populated version directory. If the destination - /// already exists — another build won the race, or `forceDownload` is replacing a - /// stale copy — the existing binary is reused when valid, otherwise the stale - /// directory is replaced and the rename retried once. + /// Publishes a fully-prepared staging directory to its final version directory. When + /// the destination does not yet exist the move 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 already + /// exists — 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 { do { try fileManager.moveItem(at: stagingDirectory, to: versionDirectory) @@ -261,10 +269,20 @@ private struct BrowserStackCLIDownloader { return } // Stale/incomplete destination, or a forced refresh: replace and retry once. - if fileManager.fileExists(atPath: versionDirectory.path) { - try fileManager.removeItem(at: versionDirectory) + // removeItem is best-effort so a peer deleting the directory first does not + // 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 } - try fileManager.moveItem(at: stagingDirectory, to: versionDirectory) } } From 833eff8f4742285c9c5bacc01a9650d58cf9a40e Mon Sep 17 00:00:00 2001 From: Crash0v3rrid3 Date: Thu, 30 Jul 2026 16:01:18 +0530 Subject: [PATCH 3/3] fix(review): resolve residual cache-publish findings (DEVA11Y-482) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../BrowserStackAccessibilityLint.swift | 92 ++++++++++++++----- 1 file changed, 67 insertions(+), 25 deletions(-) diff --git a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift index e589347..506e223 100644 --- a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift +++ b/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) @@ -220,11 +246,15 @@ private struct BrowserStackCLIDownloader { Diagnostics.remark("BrowserStackAccessibilityLint: Downloading CLI \(info.version)...") #if os(Windows) - let archiveURL = stagingDirectory.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: stagingDirectory) - try? fileManager.removeItem(at: archiveURL) #else try extractWithBsdtar(from: info.resolvedURL, into: stagingDirectory) #endif @@ -250,39 +280,51 @@ private struct BrowserStackCLIDownloader { return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL) } - /// Publishes a fully-prepared staging directory to its final version directory. When - /// the destination does not yet exist the move 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 already - /// exists — another build won the race, or `forceDownload` is refreshing a stale copy — + /// 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 + } + + // 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) - return } catch { - if !forceDownload, fileManager.isExecutableFile(atPath: expectedExecutableURL.path) { - // A concurrent build already published a valid binary; reuse it. + // 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 } - // Stale/incomplete destination, or a forced refresh: replace and retry once. - // removeItem is best-effort so a peer deleting the directory first does not - // 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 - } + throw error } }