Skip to content

Commit 02413cc

Browse files
Crash0v3rrid3claude
Crash0v3rrid3
and
claude
committed
fix(spm): eliminate TOCTOU races in cache prep and spm.sh cleanup (DEVA11Y-482,483)
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 <version> directory. Concurrent plugin invocations could corrupt each other's binary state. The download/extract now happens in a per-invocation staging dir (<version>.tmp.<UUID>) 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 <noreply@anthropic.com>
1 parent db817c3 commit 02413cc

4 files changed

Lines changed: 136 additions & 39 deletions

File tree

‎Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift‎

Lines changed: 52 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -202,37 +202,71 @@ private struct BrowserStackCLIDownloader {
202202
return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL)
203203
}
204204

205-
if fileManager.fileExists(atPath: versionDirectory.path) {
206-
try fileManager.removeItem(at: versionDirectory)
207-
}
208-
try fileManager.createDirectory(at: versionDirectory, withIntermediateDirectories: true)
205+
// Extract into a per-invocation staging directory so that concurrent plugin
206+
// invocations cannot corrupt each other's binary state. Once extraction
207+
// completes we atomically move the staging directory into the final
208+
// version directory. If the move fails because another invocation already
209+
// published the version directory, we treat that instance as the winner and
210+
// read the existing binary instead (see DEVA11Y-482 / F-013).
211+
let stagingDirectory = cacheRoot.appendingPathComponent(
212+
"\(info.version).tmp.\(UUID().uuidString)",
213+
isDirectory: true
214+
)
215+
defer { try? fileManager.removeItem(at: stagingDirectory) }
216+
217+
try fileManager.createDirectory(at: stagingDirectory, withIntermediateDirectories: true)
209218

210219
Diagnostics.remark("BrowserStackAccessibilityLint: Downloading CLI \(info.version)...")
211220

212221
#if os(Windows)
213-
let archiveURL = versionDirectory.appendingPathComponent("browserstack-cli.zip")
222+
let archiveURL = stagingDirectory.appendingPathComponent("browserstack-cli.zip")
214223
try await download(from: info.resolvedURL, to: archiveURL)
215224
Diagnostics.remark("BrowserStackAccessibilityLint: Extracting CLI \(info.version)...")
216-
try unzip(archive: archiveURL, into: versionDirectory)
225+
try unzip(archive: archiveURL, into: stagingDirectory)
217226
try? fileManager.removeItem(at: archiveURL)
218227
#else
219-
try extractWithBsdtar(from: info.resolvedURL, into: versionDirectory)
228+
try extractWithBsdtar(from: info.resolvedURL, into: stagingDirectory)
220229
#endif
221230

222-
let locatedBinary = try locateExecutable(in: versionDirectory, preferredName: executableName)
223-
let finalBinaryURL: URL
224-
if locatedBinary.lastPathComponent == executableName {
225-
finalBinaryURL = locatedBinary
226-
} else {
227-
finalBinaryURL = expectedExecutableURL
228-
if fileManager.fileExists(atPath: finalBinaryURL.path) {
229-
try fileManager.removeItem(at: finalBinaryURL)
231+
// Normalize the extracted layout inside the staging directory so the
232+
// executable lives at <staging>/<executableName>.
233+
let stagedBinary = try locateExecutable(in: stagingDirectory, preferredName: executableName)
234+
let stagedExecutableURL = stagingDirectory.appendingPathComponent(executableName, isDirectory: false)
235+
if stagedBinary.path != stagedExecutableURL.path {
236+
if fileManager.fileExists(atPath: stagedExecutableURL.path) {
237+
try fileManager.removeItem(at: stagedExecutableURL)
238+
}
239+
try fileManager.moveItem(at: stagedBinary, to: stagedExecutableURL)
240+
}
241+
try ensureExecutablePermissions(at: stagedExecutableURL)
242+
243+
// Atomically publish the staged directory into place. moveItem throws if the
244+
// destination already exists, which means another concurrent invocation won
245+
// the race — fall back to reading the published binary.
246+
do {
247+
if forceDownload, fileManager.fileExists(atPath: versionDirectory.path) {
248+
// Caller explicitly requested a fresh download. Replace any existing
249+
// version directory atomically via a swap through a temp location.
250+
let obsoleteDirectory = cacheRoot.appendingPathComponent(
251+
"\(info.version).old.\(UUID().uuidString)",
252+
isDirectory: true
253+
)
254+
try? fileManager.moveItem(at: versionDirectory, to: obsoleteDirectory)
255+
defer { try? fileManager.removeItem(at: obsoleteDirectory) }
256+
try fileManager.moveItem(at: stagingDirectory, to: versionDirectory)
257+
} else {
258+
try fileManager.moveItem(at: stagingDirectory, to: versionDirectory)
230259
}
231-
try fileManager.moveItem(at: locatedBinary, to: finalBinaryURL)
260+
} catch {
261+
// Another invocation already populated the version directory. If its
262+
// binary is present and executable, use it; otherwise surface the error.
263+
if fileManager.isExecutableFile(atPath: expectedExecutableURL.path) {
264+
return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL)
265+
}
266+
throw error
232267
}
233268

234-
try ensureExecutablePermissions(at: finalBinaryURL)
235-
return BrowserStackCLIArtifact(version: info.version, executableURL: finalBinaryURL)
269+
return BrowserStackCLIArtifact(version: info.version, executableURL: expectedExecutableURL)
236270
}
237271

238272
#if !os(Windows)

‎scripts/bash/spm.sh‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,21 @@ EOF
3939
}
4040

4141
a11y_scan() {
42-
# Ensure Package.swift is removed on exit (acts like a finally block)
42+
# Scan target is always the directory the user invoked us from.
43+
local scan_root="${PWD}"
44+
45+
# Per-invocation staging directory for the synthetic manifest. Using a unique
46+
# temp dir (instead of writing Package.swift into the user's CWD) prevents
47+
# concurrent invocations from deleting each other's manifest on exit
48+
# (DEVA11Y-483 / F-014). Only created when the user has no Package.swift.
49+
local synthetic_pkg_dir=""
50+
51+
# Ensure the per-invocation temp dir is removed on exit (acts like a finally
52+
# block). We only ever delete our own temp dir, never files in the user's CWD.
4353
cleanup() {
44-
if [ $PACKAGE_EXISTS -eq 0 ]; then
45-
return
54+
if [ -n "$synthetic_pkg_dir" ] && [ -d "$synthetic_pkg_dir" ]; then
55+
rm -rf -- "$synthetic_pkg_dir"
4656
fi
47-
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
4857
}
4958
trap cleanup EXIT
5059

@@ -53,7 +62,9 @@ a11y_scan() {
5362
return
5463
fi
5564

56-
cat > Package.swift <<EOF
65+
synthetic_pkg_dir="$(mktemp -d "${TMPDIR:-/tmp}/bstack-a11y-spm.XXXXXX")"
66+
67+
cat > "${synthetic_pkg_dir}/Package.swift" <<EOF
5768
// swift-tools-version: 5.9
5869
import PackageDescription
5970
@@ -69,14 +80,24 @@ EOF
6980

7081
setup
7182
if [[ -z "$EXTRA_ARGS" ]]; then
72-
EXTRA_ARGS="--include **/*.swift --include **/*.xib --include **/*.storyboard"
83+
EXTRA_ARGS="--include ${scan_root}/**/*.swift --include ${scan_root}/**/*.xib --include ${scan_root}/**/*.storyboard"
84+
fi
85+
86+
# When we synthesized a manifest, run the plugin against that temp package
87+
# directory (--package-path). The scan still targets the user's sources because
88+
# the include globs are rooted at the original working directory. When the user
89+
# already has a Package.swift, scan it in place exactly as before.
90+
local package_path_args=()
91+
if [ -n "$synthetic_pkg_dir" ]; then
92+
package_path_args=(--package-path "$synthetic_pkg_dir")
7393
fi
94+
7495
env -i HOME="$HOME" \
7596
XCODE_VERSION_ACTUAL="$XCODE_VERSION_ACTUAL"\
7697
BROWSERSTACK_USERNAME="$BROWSERSTACK_USERNAME"\
7798
BROWSERSTACK_ACCESS_KEY="$BROWSERSTACK_ACCESS_KEY"\
7899
PATH="$PATH" \
79-
swift package plugin \
100+
swift package "${package_path_args[@]}" plugin \
80101
--allow-writing-to-directory ~/.cache\
81102
--allow-writing-to-package-directory\
82103
--allow-network-connections 'all(ports: [])'\

‎scripts/fish/spm.sh‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,21 @@ EOF
5252
}
5353

5454
a11y_scan() {
55-
# Ensure Package.swift is removed on exit (acts like a finally block)
55+
# Scan target is always the directory the user invoked us from.
56+
local scan_root="${PWD}"
57+
58+
# Per-invocation staging directory for the synthetic manifest. Using a unique
59+
# temp dir (instead of writing Package.swift into the user's CWD) prevents
60+
# concurrent invocations from deleting each other's manifest on exit
61+
# (DEVA11Y-483 / F-014). Only created when the user has no Package.swift.
62+
local synthetic_pkg_dir=""
63+
64+
# Ensure the per-invocation temp dir is removed on exit (acts like a finally
65+
# block). We only ever delete our own temp dir, never files in the user's CWD.
5666
cleanup() {
57-
if [ $PACKAGE_EXISTS -eq 0 ]; then
58-
return
67+
if [ -n "$synthetic_pkg_dir" ] && [ -d "$synthetic_pkg_dir" ]; then
68+
rm -rf -- "$synthetic_pkg_dir"
5969
fi
60-
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
6170
}
6271
trap cleanup EXIT
6372

@@ -66,7 +75,9 @@ a11y_scan() {
6675
return
6776
fi
6877

69-
cat > Package.swift <<EOF
78+
synthetic_pkg_dir="$(mktemp -d "${TMPDIR:-/tmp}/bstack-a11y-spm.XXXXXX")"
79+
80+
cat > "${synthetic_pkg_dir}/Package.swift" <<EOF
7081
// swift-tools-version: 5.9
7182
import PackageDescription
7283
@@ -82,14 +93,24 @@ EOF
8293

8394
setup
8495
if [[ -z "$EXTRA_ARGS" ]]; then
85-
EXTRA_ARGS="--include **/*.swift --include **/*.xib --include **/*.storyboard"
96+
EXTRA_ARGS="--include ${scan_root}/**/*.swift --include ${scan_root}/**/*.xib --include ${scan_root}/**/*.storyboard"
97+
fi
98+
99+
# When we synthesized a manifest, run the plugin against that temp package
100+
# directory (--package-path). The scan still targets the user's sources because
101+
# the include globs are rooted at the original working directory. When the user
102+
# already has a Package.swift, scan it in place exactly as before.
103+
local package_path_args=()
104+
if [ -n "$synthetic_pkg_dir" ]; then
105+
package_path_args=(--package-path "$synthetic_pkg_dir")
86106
fi
107+
87108
env -i HOME="$HOME" \
88109
XCODE_VERSION_ACTUAL="$XCODE_VERSION_ACTUAL"\
89110
BROWSERSTACK_USERNAME="$BROWSERSTACK_USERNAME"\
90111
BROWSERSTACK_ACCESS_KEY="$BROWSERSTACK_ACCESS_KEY"\
91112
PATH="$PATH" \
92-
swift package plugin \
113+
swift package "${package_path_args[@]}" plugin \
93114
--allow-writing-to-directory ~/.cache\
94115
--allow-writing-to-package-directory\
95116
--allow-network-connections 'all(ports: [])'\

‎scripts/zsh/spm.sh‎

Lines changed: 28 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -51,12 +51,21 @@ EOF
5151
}
5252

5353
a11y_scan() {
54-
# Ensure Package.swift is removed on exit (acts like a finally block)
54+
# Scan target is always the directory the user invoked us from.
55+
local scan_root="${PWD}"
56+
57+
# Per-invocation staging directory for the synthetic manifest. Using a unique
58+
# temp dir (instead of writing Package.swift into the user's CWD) prevents
59+
# concurrent invocations from deleting each other's manifest on exit
60+
# (DEVA11Y-483 / F-014). Only created when the user has no Package.swift.
61+
local synthetic_pkg_dir=""
62+
63+
# Ensure the per-invocation temp dir is removed on exit (acts like a finally
64+
# block). We only ever delete our own temp dir, never files in the user's CWD.
5565
cleanup() {
56-
if [ $PACKAGE_EXISTS -eq 0 ]; then
57-
return
66+
if [ -n "$synthetic_pkg_dir" ] && [ -d "$synthetic_pkg_dir" ]; then
67+
rm -rf -- "$synthetic_pkg_dir"
5868
fi
59-
rm -f -- "${PWD}/Package.swift" "${PWD}/Package.resolved"
6069
}
6170
trap cleanup EXIT
6271

@@ -65,7 +74,9 @@ a11y_scan() {
6574
return
6675
fi
6776

68-
cat > Package.swift <<EOF
77+
synthetic_pkg_dir="$(mktemp -d "${TMPDIR:-/tmp}/bstack-a11y-spm.XXXXXX")"
78+
79+
cat > "${synthetic_pkg_dir}/Package.swift" <<EOF
6980
// swift-tools-version: 5.9
7081
import PackageDescription
7182
@@ -81,14 +92,24 @@ EOF
8192

8293
setup
8394
if [[ -z "$EXTRA_ARGS" ]]; then
84-
EXTRA_ARGS="--include **/*.swift --include **/*.xib --include **/*.storyboard"
95+
EXTRA_ARGS="--include ${scan_root}/**/*.swift --include ${scan_root}/**/*.xib --include ${scan_root}/**/*.storyboard"
96+
fi
97+
98+
# When we synthesized a manifest, run the plugin against that temp package
99+
# directory (--package-path). The scan still targets the user's sources because
100+
# the include globs are rooted at the original working directory. When the user
101+
# already has a Package.swift, scan it in place exactly as before.
102+
local package_path_args=()
103+
if [ -n "$synthetic_pkg_dir" ]; then
104+
package_path_args=(--package-path "$synthetic_pkg_dir")
85105
fi
106+
86107
env -i HOME="$HOME" \
87108
XCODE_VERSION_ACTUAL="$XCODE_VERSION_ACTUAL"\
88109
BROWSERSTACK_USERNAME="$BROWSERSTACK_USERNAME"\
89110
BROWSERSTACK_ACCESS_KEY="$BROWSERSTACK_ACCESS_KEY"\
90111
PATH="$PATH" \
91-
swift package plugin \
112+
swift package "${package_path_args[@]}" plugin \
92113
--allow-writing-to-directory ~/.cache\
93114
--allow-writing-to-package-directory\
94115
--allow-network-connections 'all(ports: [])'\

0 commit comments

Comments
 (0)