From 02413cc1a221d4e11acbc2568fe94d7e94cbb744 Mon Sep 17 00:00:00 2001 From: Crash0v3rrid3 Date: Wed, 17 Jun 2026 13:51:24 +0530 Subject: [PATCH] fix(spm): eliminate TOCTOU races in cache prep and spm.sh cleanup (DEVA11Y-482,483) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two confirmed time-of-check/time-of-use race conditions (APPSEC-415): F-013 (DEVA11Y-482) — Cache version-dir TOCTOU in the SPM plugin: prepareArtifact() did a non-atomic check -> removeItem -> createDirectory -> extract on the cache directory. Concurrent plugin invocations could corrupt each other's binary state. The download/extract now happens in a per-invocation staging dir (.tmp.) and is atomically published into place via FileManager.moveItem. If the move fails because another invocation already published the directory, the loser falls back to reading the existing executable. The non-concurrent happy path is unchanged; forceDownload swaps the existing dir aside atomically before publishing. F-014 (DEVA11Y-483) — Concurrent spm.sh share CWD; cleanup trap deletes a sibling's Package.swift: the synthetic manifest was written into the user's CWD and the EXIT trap ran `rm -f Package.swift Package.resolved`, so the first instance to exit deleted the manifest out from under a second concurrent instance. The synthetic manifest is now written into a per-invocation `mktemp -d` dir, passed to `swift package` via `--package-path`, and the cleanup trap removes only that temp dir (never files in the user's CWD). Include globs are rooted at the original working directory so the scan still targets the user's sources. Applied identically to the bash, zsh, and fish variants. The pre-existing user-supplied-Package.swift path is unchanged. Scoped to the cleanup-trap / temp-dir CWD fix only; the self-update and dependency-pinning changes (DEVA11Y-475/477/478) to the same spm.sh files are handled in a separate PR. Co-Authored-By: Claude Opus 4.8 --- .../BrowserStackAccessibilityLint.swift | 70 ++++++++++++++----- scripts/bash/spm.sh | 35 ++++++++-- scripts/fish/spm.sh | 35 ++++++++-- scripts/zsh/spm.sh | 35 ++++++++-- 4 files changed, 136 insertions(+), 39 deletions(-) diff --git a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift index c6705a1..54fd9ee 100644 --- a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift +++ b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift @@ -202,37 +202,71 @@ 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 per-invocation staging directory so that concurrent plugin + // invocations cannot corrupt each other's binary state. Once extraction + // completes we atomically move the staging directory into the final + // version directory. If the move fails because another invocation already + // published the version directory, we treat that instance as the winner and + // read the existing binary instead (see DEVA11Y-482 / F-013). + let stagingDirectory = cacheRoot.appendingPathComponent( + "\(info.version).tmp.\(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") + 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) + // Normalize the extracted layout inside the staging directory so the + // executable lives at /. + let stagedBinary = try locateExecutable(in: stagingDirectory, preferredName: executableName) + let stagedExecutableURL = stagingDirectory.appendingPathComponent(executableName, isDirectory: false) + if stagedBinary.path != stagedExecutableURL.path { + if fileManager.fileExists(atPath: stagedExecutableURL.path) { + try fileManager.removeItem(at: stagedExecutableURL) + } + try fileManager.moveItem(at: stagedBinary, to: stagedExecutableURL) + } + try ensureExecutablePermissions(at: stagedExecutableURL) + + // Atomically publish the staged directory into place. moveItem throws if the + // destination already exists, which means another concurrent invocation won + // the race — fall back to reading the published binary. + do { + if forceDownload, fileManager.fileExists(atPath: versionDirectory.path) { + // Caller explicitly requested a fresh download. Replace any existing + // version directory atomically via a swap through a temp location. + let obsoleteDirectory = cacheRoot.appendingPathComponent( + "\(info.version).old.\(UUID().uuidString)", + isDirectory: true + ) + try? fileManager.moveItem(at: versionDirectory, to: obsoleteDirectory) + defer { try? fileManager.removeItem(at: obsoleteDirectory) } + try fileManager.moveItem(at: stagingDirectory, to: versionDirectory) + } else { + try fileManager.moveItem(at: stagingDirectory, to: versionDirectory) } - try fileManager.moveItem(at: locatedBinary, to: finalBinaryURL) + } catch { + // Another invocation already populated the version directory. If its + // binary is present and executable, use it; otherwise surface the error. + if fileManager.isExecutableFile(atPath: expectedExecutableURL.path) { + return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL) + } + throw error } - try ensureExecutablePermissions(at: finalBinaryURL) - return BrowserStackCLIArtifact(version: info.version, executableURL: finalBinaryURL) + return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL) } #if !os(Windows) diff --git a/scripts/bash/spm.sh b/scripts/bash/spm.sh index 1202e11..e890af7 100644 --- a/scripts/bash/spm.sh +++ b/scripts/bash/spm.sh @@ -39,12 +39,21 @@ EOF } a11y_scan() { - # Ensure Package.swift is removed on exit (acts like a finally block) + # Scan target is always the directory the user invoked us from. + local scan_root="${PWD}" + + # Per-invocation staging directory for the synthetic manifest. Using a unique + # temp dir (instead of writing Package.swift into the user's CWD) prevents + # concurrent invocations from deleting each other's manifest on exit + # (DEVA11Y-483 / F-014). Only created when the user has no Package.swift. + local synthetic_pkg_dir="" + + # Ensure the per-invocation temp dir is removed on exit (acts like a finally + # block). We only ever delete our own temp dir, never files in the user's CWD. cleanup() { - if [ $PACKAGE_EXISTS -eq 0 ]; then - return + if [ -n "$synthetic_pkg_dir" ] && [ -d "$synthetic_pkg_dir" ]; then + rm -rf -- "$synthetic_pkg_dir" fi - rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved" } trap cleanup EXIT @@ -53,7 +62,9 @@ a11y_scan() { return fi - cat > Package.swift < "${synthetic_pkg_dir}/Package.swift" < Package.swift < "${synthetic_pkg_dir}/Package.swift" < Package.swift < "${synthetic_pkg_dir}/Package.swift" <