diff --git a/.github/workflows/extraction-guard-tests.yml b/.github/workflows/extraction-guard-tests.yml new file mode 100644 index 0000000..036dd32 --- /dev/null +++ b/.github/workflows/extraction-guard-tests.yml @@ -0,0 +1,75 @@ +# Regression tests for the DEVA11Y-484 decompression-bomb extraction guard. +# +# Runs the shell-launcher suite: the real download_binary() from +# scripts/{bash,zsh,fish}/cli.sh against locally generated archives, through a +# curl shim that redirects only the hardcoded download URL. No network egress to +# BrowserStack, no credentials, no mocks of bsdtar/head/curl. +# +# macos-latest ships bsdtar (libarchive), curl, python3 and awk, which is the +# full dependency set. Ubuntu is not used: `tar` there is GNU tar, and the guard +# under test is bsdtar-specific. +name: Extraction Guard Tests + +on: + pull_request: + branches: ["master", "main"] + paths: + - "scripts/bash/cli.sh" + - "scripts/zsh/cli.sh" + - "scripts/fish/cli.sh" + - "tests/extraction-guard/**" + - ".github/workflows/extraction-guard-tests.yml" + push: + branches: ["master", "main"] + # Same filter as the PR trigger. Without it every push to main runs a macOS job + # that generates ~106 MB of fixtures, billed at 10x, regardless of whether + # anything it tests changed — verify-selfupdate-checksums.yml filters both + # triggers for the same reason. + paths: + - "scripts/bash/cli.sh" + - "scripts/zsh/cli.sh" + - "scripts/fish/cli.sh" + - "tests/extraction-guard/**" + - ".github/workflows/extraction-guard-tests.yml" + +permissions: + contents: read + +# Three quick pushes to a PR would otherwise run three concurrent macOS jobs, each +# generating its own 106 MB of fixtures. Matches spm-smoke-test.yml. +concurrency: + group: extraction-guard-${{ github.ref }} + cancel-in-progress: true + +jobs: + extraction-guard: + name: extraction-guard / shell launchers + # Pinned, not macos-latest: the suite depends on bsdtar, python3, BSD `head -c` + # and BSD `stat -f` all being present, so an unannounced image roll can break it + # for reasons unrelated to the guard. spm-smoke-test.yml pins the same image. + runs-on: macos-14 + # timeout-minutes is the ONLY hang backstop here — there is no per-case timeout + # and cli.sh's download has no --max-time. The suite completes in ~30s; the + # fixture lock waits at most 300s. 20 minutes is headroom for that role. + timeout-minutes: 20 + steps: + # v4.2.2, matching the repo's three newest workflows. v3.5.3 is a Node16 + # action; those emit deprecation annotations and are being removed from + # runner images, which would red out this job at checkout. + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 + + - name: Show toolchain + run: | + bsdtar --version + curl --version | head -1 + python3 --version + bash --version | head -1 + + - name: Syntax-check the launchers + run: | + for f in scripts/bash/cli.sh scripts/zsh/cli.sh scripts/fish/cli.sh; do + bash -n "$f" && echo "ok $f" + done + + - name: Run DEVA11Y-484 shell regression suite + run: bash tests/extraction-guard/run_tests.sh diff --git a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift index 34c74dc..ab63afa 100644 --- a/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift +++ b/Plugins/BrowserStackAccessibilityLint/BrowserStackAccessibilityLint.swift @@ -170,6 +170,12 @@ private struct BrowserStackCLIDownloader { private var fileManager: FileManager { .default } + // Decompression-bomb guards (DEVA11Y-484). The CLI binary is a few tens of MB; these + // ceilings leave generous headroom while bounding a malicious archive's footprint. + private static let maxCompressedBytes: Int64 = 100 * 1024 * 1024 // 100 MB on the wire + private static let maxDecompressedBytes: Int64 = 200 * 1024 * 1024 // 200 MB on disk + private static let maxArchiveEntries = 10_000 + func ensureArtifact() async throws -> BrowserStackCLIArtifact { if let overrideURL { let info = try await resolveOverrideArtifact(from: overrideURL) @@ -193,11 +199,16 @@ private struct BrowserStackCLIDownloader { } /// 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. + /// behind when a previous extraction was interrupted. + /// + /// This no longer describes the bsdtar path: `extractLocalArchive` now throws instead + /// of calling `forwardExit()`, precisely so `prepareArtifact`'s `defer` cleanup DOES + /// run (DEVA11Y-484). Two routes still bypass those defers and keep this sweep + /// necessary — the Windows `unzip` path, which still reaches `forwardExit()` via + /// `run(process:errorDescription:)`, and SIGKILL, which can land at any point. + /// + /// 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() @@ -447,16 +458,114 @@ private struct BrowserStackCLIDownloader { let errorPipe = Pipe() process.standardError = errorPipe + // Drain stderr on a separate queue BEFORE waiting. bsdtar's stderr is a 64 KB pipe; + // if it fills, bsdtar blocks writing and waitUntilExit() never returns. Reading only + // after the wait (the previous shape) is a deadlock, and it is reachable from an + // archive that stays UNDER both ceilings, so the watchdog does not save us — it spins + // on `process.isRunning` forever alongside the hang. Measured: a 4,000-entry archive + // whose members all contain `..` decompresses to 0 bytes / 4,000 entries yet emits + // ~227 KB of "Path contains '..'" warnings and wedges extraction indefinitely + // (DEVA11Y-484 review). Capped so a chatty archive cannot balloon memory either. + let stderrLimit = 64 * 1024 + var stderrData = Data() + let stderrQueue = DispatchQueue(label: "com.browserstack.a11y.bsdtar-stderr") + let stderrDrained = DispatchSemaphore(value: 0) + let stderrHandle = errorPipe.fileHandleForReading + + let limitState: ExtractionLimitState do { try process.run() + stderrQueue.async { + while let chunk = try? stderrHandle.read(upToCount: 4096), !chunk.isEmpty { + if stderrData.count < stderrLimit { + stderrData.append(chunk.prefix(stderrLimit - stderrData.count)) + } + // Keep draining past the cap — discarding is what stops bsdtar blocking. + } + stderrDrained.signal() + } + // Decompressed-size/entry guard (DEVA11Y-484); see the EXTRACTION GUARD block below. + limitState = startExtractionWatchdog(on: process, directory: directory, maxBytes: Self.maxDecompressedBytes, maxEntries: Self.maxArchiveEntries) process.waitUntilExit() + stderrDrained.wait() } catch { throw PluginError("Failed to launch bsdtar: \(error.localizedDescription)") } + // Catch a bomb that completed within a single watchdog poll interval (fast disk). + if !limitState.exceeded, let reason = footprintExceeded(at: directory, maxBytes: Self.maxDecompressedBytes, maxEntries: Self.maxArchiveEntries) { + limitState.markExceeded(reason) + } + if limitState.exceeded { + try? fileManager.removeItem(at: directory) + // THROW, do not forwardExit. forwardExit calls exit(), which skips every `defer` + // — including prepareArtifact's cleanup of the downloaded archive. Exiting here + // therefore left a <=100 MB archive in the cache on every guard trip, inside the + // control whose job is to prevent disk exhaustion (DEVA11Y-484 review). Throwing + // unwinds normally, both defers fire, and it matches locateExecutable's entry cap. + throw PluginError("BrowserStack CLI archive rejected: \(limitState.reason). Aborting to prevent disk exhaustion.") + } + if process.terminationReason != .exit || process.terminationStatus != 0 { // Fall back to copying the file directly if it's already an executable. - let message = String(data: errorPipe.fileHandleForReading.readDataToEndOfFile(), encoding: .utf8)?.trimmingCharacters(in: .whitespacesAndNewlines) ?? "" + // Surface a BOUNDED excerpt. Draining stays unbounded — that is what stops bsdtar + // blocking — but what we SHOW must not be. The drain already caps retention at + // 64 KB, so this bounds what we RENDER: even 64 KB of near-identical warnings is + // unusable in Xcode's Issue Navigator. (For scale, 4,000 `..` entries emit ~205 KB + // across ~4,001 lines before the retention cap trims it.) + // + // Three properties are load-bearing, each earned from a measured failure: + // + // 1. Decode LOSSILY. `String(data:encoding:.utf8)` returns nil — not a partial + // string — on any invalid sequence, and `?? ""` then discarded the WHOLE + // diagnostic. Two paths reach that: a non-UTF-8 member name (tar names are + // arbitrary bytes and bsdtar echoes them verbatim), and the drain's byte-wise + // cap slicing a multi-byte scalar (14 of 129 cut points, measured). + // 2. Keep the HEAD *and* the TAIL. bsdtar streams per-entry warnings first and puts + // the decisive cause last ("Truncated tar archive", then "Error exit delayed from + // previous errors"). Head-only truncation dropped exactly the line support needs. + // 3. Bound BYTES as well as LINES, and budget each END separately. A line cap + // alone is bypassable: one pax entry with a 60,000-character `..` path emits + // just 2 lines totalling ~60 KB, sailing through a 20-line cap untouched. + // But a single byte cap over the *joined* head+notice+tail is also wrong: when + // the head lines are individually large, the cut lands inside the head and + // discards the notice AND the tail — silently undoing property 2 (measured: + // 25 head lines of ~500 B each dropped the "Truncated tar archive" cause). + // So head and tail get half the budget each, clamped BEFORE they are joined. + // Clamping is on the `utf8` view because String's `prefix` counts CHARACTERS, + // which would let ~4x the cap through on multi-byte input. + // + // Empty input must stay empty: the throw below relies on `message.isEmpty` to + // choose the generic fallback text (DEVA11Y-484 review). + let headLines = 10 + let tailLines = 10 + // DERIVED, not an independent 20. `omitted` below is + // `count - headLines - tailLines`, and it stays >= 1 only because the + // threshold equals headLines + tailLines. Writing 20 here as its own + // literal made that a coincidence of three constants: bumping headLines + // alone would silently produce a negative "at least -3 further messages" + // in a user-facing error (DEVA11Y-484 review). + let maxMessageLines = headLines + tailLines + let maxMessageBytes = 4096 + var message = String(decoding: stderrData, as: UTF8.self) + .trimmingCharacters(in: .whitespacesAndNewlines) + let messageLines = message.split(separator: "\n", omittingEmptySubsequences: false) + if messageLines.count > maxMessageLines { + let halfBudget = maxMessageBytes / 2 + let omitted = messageLines.count - headLines - tailLines + let head = clampToUTF8Bytes( + messageLines.prefix(headLines).joined(separator: "\n"), halfBudget) + // keepingEnd: the cause is the LAST line — clamp the tail from its end. + let tail = clampToUTF8Bytes( + messageLines.suffix(tailLines).joined(separator: "\n"), halfBudget, + keepingEnd: true) + message = head + + "\n… at least \(omitted) further bsdtar message(s) omitted" + + " (stderr retention is capped at 64 KB, so the true count may be higher).\n" + + tail + } else { + message = clampToUTF8Bytes(message, maxMessageBytes) + } if fileManager.isExecutableFile(atPath: archiveURL.path) { let destination = directory.appendingPathComponent(archiveURL.lastPathComponent) if fileManager.fileExists(atPath: destination.path) { @@ -464,7 +573,10 @@ private struct BrowserStackCLIDownloader { } try fileManager.copyItem(at: archiveURL, to: destination) } else { - forwardExit(code: process.terminationStatus, message: message.isEmpty ? "bsdtar failed to extract BrowserStack CLI." : message) + // THROW for the same reason as the guard branch above: forwardExit calls exit() + // and skips prepareArtifact's defers, leaking the archive and the staging dir. + // SwiftPM flattens the exit code anyway, so nothing is lost (DEVA11Y-484 review). + throw PluginError(message.isEmpty ? "bsdtar failed to extract BrowserStack CLI." : message) } } } @@ -582,8 +694,42 @@ private struct BrowserStackCLIDownloader { let (tempURL, response) = try await URLSession.shared.download(from: url) if let httpResponse = response as? HTTPURLResponse, !(200..<300).contains(httpResponse.statusCode) { + try? fileManager.removeItem(at: tempURL) throw PluginError("Failed to download BrowserStack CLI (HTTP \(httpResponse.statusCode)).") } + + // Compressed-size cap (DEVA11Y-484 review). Without it a multi-GB *compressed* + // payload from an attacker-controlled URL (BROWSERSTACK_A11Y_CLI_DOWNLOAD_URL) is + // checksummed and handed to the extraction guard, which only ever bounds the + // *decompressed* footprint — so the archive itself is an unbounded surface. + // + // LIMITATION, stated plainly: URLSession.download(from:) has no byte-level hook, so + // these checks reject the archive *after* the transfer rather than aborting it + // mid-stream. They therefore prevent an oversized archive from being verified, + // extracted, published or executed, but they do NOT bound peak temporary disk during + // the transfer itself. Bounding that needs a URLSessionDownloadDelegate that cancels + // in didWriteData — deliberately left as a separate change (DEVA11Y-761) rather than + // rewriting this shared download path here. The shell launchers do abort pre-transfer, + // via curl --max-filesize. + if response.expectedContentLength > Self.maxCompressedBytes { + try? fileManager.removeItem(at: tempURL) + throw PluginError("BrowserStack CLI archive declares \(response.expectedContentLength) bytes, above the \(Self.maxCompressedBytes)-byte limit; refusing to download it.") + } + // Fail CLOSED on an unreadable size. This is the load-bearing half of the compressed + // cap: expectedContentLength is -1 for chunked/unknown-length responses, so an + // attacker-controlled URL that omits Content-Length is caught only here. Reading via + // resourceValues(.fileSizeKey) — the same idiom extractionFootprint uses — rather than + // attributesOfItem[.size] as? Int64, which could yield nil and skip the cap silently + // (DEVA11Y-484 review). + guard let downloadedBytes = (try? tempURL.resourceValues(forKeys: [.fileSizeKey]).fileSize).map(Int64.init) else { + try? fileManager.removeItem(at: tempURL) + throw PluginError("Could not determine the downloaded BrowserStack CLI archive's size; refusing to use it.") + } + if downloadedBytes > Self.maxCompressedBytes { + try? fileManager.removeItem(at: tempURL) + throw PluginError("BrowserStack CLI archive is \(downloadedBytes) bytes, above the \(Self.maxCompressedBytes)-byte limit; refusing to use it.") + } + if fileManager.fileExists(atPath: destination.path) { try fileManager.removeItem(at: destination) } @@ -616,8 +762,16 @@ private struct BrowserStackCLIDownloader { ) var fallback: URL? + var scanned = 0 while let element = enumerator?.nextObject() as? URL { + scanned += 1 + if scanned > Self.maxArchiveEntries { + // Bound enumeration so an archive packed with millions of entries can't turn + // locateExecutable into a CPU/IO drain (DEVA11Y-484). + throw PluginError("Extracted archive contains more than \(Self.maxArchiveEntries) entries; refusing to continue.") + } + var isDirectory: ObjCBool = false guard fileManager.fileExists(atPath: element.path, isDirectory: &isDirectory), !isDirectory.boolValue else { continue @@ -787,6 +941,153 @@ private let browserstackCLIPermissionDeniedExitCode: Int32 = 4 // MARK: - Error +// === DEVA11Y-484 EXTRACTION GUARD === +// +// Rationale: bsdtar writes decompressed bytes straight to disk, so bounding the +// archive's *compressed* size says nothing about how much it expands to — useless +// against a decompression bomb. Instead we poll the destination directory while +// bsdtar runs and terminate it if the decompressed footprint crosses a byte OR +// entry ceiling (the entry ceiling stops a "millions of tiny files" bomb that stays +// small on disk). +// +// Containment assumption (load-bearing): `bsdtar -x` WITHOUT `-P` neutralises `..`, +// absolute paths and symlink-through, so every write lands inside the `-C` directory we +// poll. Adding `-P` would let writes escape that directory and the footprint poll would +// measure nothing — do not add it (DEVA11Y-484 review). +// +// Applies to extractLocalArchive, which since #37 (DEVA11Y-473/474) is the single +// non-Windows extraction path: the archive is downloaded to a file and checksum- +// verified first, then extracted. Windows' unzip path has no streaming guard. + +/// Thread-safe flag shared between the extraction watchdog and the main flow. +private final class ExtractionLimitState { + private let lock = NSLock() + private var didExceed = false + private var why = "" + + func markExceeded(_ reason: String) { + lock.lock() + if !didExceed { + didExceed = true + why = reason + } + lock.unlock() + } + + var exceeded: Bool { + lock.lock() + defer { lock.unlock() } + return didExceed + } + + var reason: String { + lock.lock() + defer { lock.unlock() } + return why + } +} + +/// Total bytes and entry count of all regular files under `url`. +/// Truncates `text` to at most `limit` UTF-8 BYTES, not characters. +/// +/// `keepingEnd` selects WHICH end survives, and it is not cosmetic. When clamping the +/// tail of a bsdtar excerpt the decisive cause is the LAST line, so keeping the tail's +/// beginning discards exactly what the tail was retained for — measured: 8 large +/// warnings ahead of "Truncated tar archive" pushed the cause out of a prefix-clamped +/// tail (DEVA11Y-484 review). +/// +/// `String.prefix` counts Characters, so using it as a byte cap lets roughly 4x the +/// limit through on multi-byte input; the `utf8` view is the correct one. Decoding is +/// lossy so a cut landing mid-scalar yields U+FFFD rather than nil — +/// `String(data:encoding:.utf8)` would return nil and discard the whole diagnostic. +private func clampToUTF8Bytes(_ text: String, _ limit: Int, keepingEnd: Bool = false) -> String { + guard text.utf8.count > limit else { return text } + let bytes = Array(text.utf8) + if keepingEnd { + return "… (truncated)\n" + String(decoding: bytes.suffix(limit), as: UTF8.self) + } + return String(decoding: bytes.prefix(limit), as: UTF8.self) + "… (truncated)" +} + +private func extractionFootprint(at url: URL) -> (bytes: Int64, entries: Int, measured: Bool) { + let fm = FileManager.default + // `.skipsHiddenFiles` is deliberately NOT set, so the entry count here matches what + // bsdtar actually wrote — including dotfiles. locateExecutable skips hidden files + // because it is searching for a binary, not measuring a footprint; the two use the + // same ceiling but count deliberately different things (DEVA11Y-484 review). + // Fail CLOSED when the tree cannot be read. The `guard let ... else` below is NOT + // sufficient on its own: FileManager.enumerator(at:includingPropertiesForKeys:) does not + // return nil for a missing or unreadable directory — it routes errors to an errorHandler + // whose default is "skip and continue", so such a directory yields a valid enumerator + // that produces zero elements and the function returned (0, 0), i.e. "not exceeded", the + // exact silent guard-disable this was meant to prevent (DEVA11Y-484 review). Supplying + // the handler and reporting the ceiling makes the failure closed for real. + var enumerationFailed = false + guard let enumerator = fm.enumerator( + at: url, + includingPropertiesForKeys: [.isRegularFileKey, .fileSizeKey], + options: [], + errorHandler: { _, _ in enumerationFailed = true; return false } + ) else { + return (0, 0, false) + } + var total: Int64 = 0 + var count = 0 + for case let element as URL in enumerator { + count += 1 + let values = try? element.resourceValues(forKeys: [.isRegularFileKey, .fileSizeKey]) + if values?.isRegularFile == true, let size = values?.fileSize { + total += Int64(size) + } + } + // Any enumeration error, at any depth, means the measurement is incomplete and must not + // be reported as "under the ceiling". Signalled with measured = false rather than an + // infinite footprint, so callers can say "could not be measured" instead of reporting an + // I/O or permission failure as a size violation (DEVA11Y-484 review). + return (total, count, !enumerationFailed) +} + +/// Returns a rejection reason if the footprint under `directory` exceeds either ceiling. +private func footprintExceeded(at directory: URL, maxBytes: Int64, maxEntries: Int) -> String? { + let footprint = extractionFootprint(at: directory) + if !footprint.measured { + return "extraction directory could not be measured" + } + if footprint.bytes > maxBytes { + return "decompressed size exceeds \(maxBytes / (1024 * 1024)) MB" + } + if footprint.entries > maxEntries { + return "archive contains more than \(maxEntries) entries" + } + return nil +} + +/// Starts a background watchdog that terminates `process` (bsdtar) if the decompressed +/// footprint in `directory` exceeds the byte or entry ceiling. +/// +/// This is a SOFT ceiling: bsdtar can write up to one poll interval's worth of data past +/// the limit before it is killed, so peak disk use is roughly `maxBytes + (50 ms × disk +/// write rate)` — the poll interval below is 50 ms. The goal is to prevent disk +/// *exhaustion* by a multi-GB/TB bomb, not to enforce an exact byte count. +/// Callers MUST also run `footprintExceeded` once the process exits, to catch a fast bomb +/// that finished within a single poll interval. +private func startExtractionWatchdog(on process: Process, directory: URL, maxBytes: Int64, maxEntries: Int) -> ExtractionLimitState { + let state = ExtractionLimitState() + let watchdog = Thread { + while process.isRunning { + if let reason = footprintExceeded(at: directory, maxBytes: maxBytes, maxEntries: maxEntries) { + state.markExceeded(reason) + process.terminate() + break + } + Thread.sleep(forTimeInterval: 0.05) + } + } + watchdog.start() + return state +} +// === END DEVA11Y-484 EXTRACTION GUARD === + private struct PluginError: Error, CustomStringConvertible { let message: String diff --git a/scripts/bash/cli.sh b/scripts/bash/cli.sh index 5912a57..5a9cd31 100644 --- a/scripts/bash/cli.sh +++ b/scripts/bash/cli.sh @@ -241,21 +241,98 @@ verify_binary_integrity() { } download_binary() { - local resolved_url - resolved_url=$(curl -fR -z "$BINARY_ZIP_PATH" -L "https://api.browserstack.com/sdk/v1/download_cli?os=${OS}&os_arch=${ARCH}" -o "$BINARY_ZIP_PATH" -w '%{url_effective}') || { - echo "CLI download failed." >&2 + local max_compressed=104857600 # 100 MB cap on the compressed download + local max_decompressed=209715200 # 200 MB cap on the decompressed binary + + # --max-filesize aborts the transfer once the declared size is known to exceed the cap. + # Measured against this endpoint (which 302s to sdk-assets), curl bails with a non-zero + # exit and nothing written to disk. curl documents the flag as a no-op when the length is + # unknown (chunked responses), so the explicit size check below backstops that case — + # otherwise an attacker-controlled endpoint could exhaust the disk during download, before + # the checksum and the decompression guard ever run (DEVA11Y-484 review). + local resolved_url curl_status=0 + resolved_url=$(curl -fR --max-filesize "$max_compressed" -z "$BINARY_ZIP_PATH" -L "https://api.browserstack.com/sdk/v1/download_cli?os=${OS}&os_arch=${ARCH}" -o "$BINARY_ZIP_PATH" -w '%{url_effective}') || curl_status=$? + if [[ $curl_status -ne 0 ]]; then + # Distinguish a size abort from a network failure by what landed on disk rather than by + # curl's exit code: --max-filesize is documented to exit 63, but measured against this + # endpoint (which 302s to sdk-assets) curl aborts during receive and exits 56 instead, so + # branching on 63 alone would misreport the common case (DEVA11Y-484 review). + local partial_size + partial_size=$(wc -c < "$BINARY_ZIP_PATH" 2>/dev/null || echo 0) + if [[ $partial_size -ge $max_compressed ]]; then + echo "BrowserStack CLI archive exceeds the maximum allowed download size (100 MB). Aborting." >&2 + else + echo "BrowserStack CLI download failed (curl exited $curl_status)." >&2 + fi + # Remove unconditionally, including on a transient network error. This deliberately gives + # up the -z If-Modified-Since fast path on the next run: a partial write carries a fresh + # mtime, so keeping it risks the next -z revalidation getting a 304 and handing a + # truncated archive to verify_binary_integrity. Losing a 304 is cheaper than trusting a + # truncated payload. + rm -f "$BINARY_ZIP_PATH" return 1 - } + fi + + local compressed_size + compressed_size=$(wc -c < "$BINARY_ZIP_PATH" 2>/dev/null || echo 0) + if [[ $compressed_size -gt $max_compressed ]]; then + echo "BrowserStack CLI archive exceeds the maximum allowed download size (100 MB). Aborting." >&2 + rm -f "$BINARY_ZIP_PATH" + return 1 + fi + verify_binary_integrity "$BINARY_ZIP_PATH" "$resolved_url" || return $? + # Extract to a temp path and atomically publish it. `> "$BINARY_PATH"` truncates the # destination before bsdtar is known to have succeeded, so a corrupt payload — the live # case today, since no sidecars are published yet and verification fails open — would # zero out a previously-good cached binary. Stage + mv keeps the cached binary intact # unless a fresh, extractable payload is in hand (DEVA11Y-473/474 review). - bsdtar -xvf "$BINARY_ZIP_PATH" -O > "${BINARY_PATH}.tmp" \ - && chmod 0755 "${BINARY_PATH}.tmp" \ - && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH" \ - && strip_quarantine + # + # The decompression-bomb guard (DEVA11Y-484) sits on that same staged path: head -c stops + # bsdtar via SIGPIPE once the decompressed output reaches the cap, and pipefail surfaces + # that as a failure. Because the cap applies to ${BINARY_PATH}.tmp and publication is a + # later mv, a rejected bomb leaves any previously-cached binary untouched. + # Save and restore pipefail rather than clearing it: these scripts do not enable it + # globally today, but unconditionally turning it off would silently disable it for + # everything after download_binary if they ever do (DEVA11Y-484 review). + local pipefail_was_set=0 + case "$(set +o)" in *"-o pipefail"*) pipefail_was_set=1 ;; esac + set -o pipefail + # `|| extract_status=$?` rather than a bare pipeline: the shebang is `bash -il`, so the + # user's rc files are sourced, and if one of them sets `-e` a bare failing pipeline aborts + # the script here — skipping the diagnostics below and leaving ${BINARY_PATH}.tmp behind, + # the exact residue the publish-failure cleanup was added to prevent (DEVA11Y-484 review). + local extract_status=0 + bsdtar -xvf "$BINARY_ZIP_PATH" -O | head -c "$max_decompressed" > "${BINARY_PATH}.tmp" || extract_status=$? + [[ $pipefail_was_set -eq 1 ]] || set +o pipefail + + local extracted_size + extracted_size=$(wc -c < "${BINARY_PATH}.tmp" 2>/dev/null || echo 0) + + # Size first, and `-ge` not `-gt`: head -c caps output at exactly $max_decompressed, so a + # file of exactly that size is indistinguishable from a truncated overflow and must be + # rejected. A bomb trips both this and extract_status (SIGPIPE), and the size message is + # the accurate one, so it is checked first. + if [[ $extracted_size -ge $max_decompressed ]]; then + echo "BrowserStack CLI archive exceeds the maximum allowed decompressed size (200 MB). Aborting." >&2 + rm -f "${BINARY_PATH}.tmp" + return 1 + fi + if [[ $extract_status -ne 0 ]]; then + echo "BrowserStack CLI archive could not be extracted (bsdtar exited $extract_status). Aborting." >&2 + rm -f "${BINARY_PATH}.tmp" + return 1 + fi + + # Clean the staged file up on *any* failure below, not just the size rejection above, + # so a failed chmod/mv never leaves a stray ${BINARY_PATH}.tmp in the cache. + if ! { chmod 0755 "${BINARY_PATH}.tmp" && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH"; }; then + echo "BrowserStack CLI: failed to publish the downloaded binary." >&2 + rm -f "${BINARY_PATH}.tmp" + return 1 + fi + strip_quarantine } # Self-update is opt-in (DEVA11Y-475): it runs only via the explicit `self-update` diff --git a/scripts/bash/cli.sh.sha256 b/scripts/bash/cli.sh.sha256 index 089b5d5..59f2384 100644 --- a/scripts/bash/cli.sh.sha256 +++ b/scripts/bash/cli.sh.sha256 @@ -1 +1 @@ -14b7e853e5cbd233aa402a6be434cd860ee7cc4037f0e653752d6867c99bb7f2 cli.sh +1652f1cd582110d30832f48fc931681ee46b3cae0afad156fa64e4031ddf7804 cli.sh diff --git a/scripts/fish/cli.sh b/scripts/fish/cli.sh index 5ad931f..97165cc 100644 --- a/scripts/fish/cli.sh +++ b/scripts/fish/cli.sh @@ -253,21 +253,98 @@ verify_binary_integrity() { } download_binary() { - local resolved_url - resolved_url=$(curl -fR -z "$BINARY_ZIP_PATH" -L "https://api.browserstack.com/sdk/v1/download_cli?os=${OS}&os_arch=${ARCH}" -o "$BINARY_ZIP_PATH" -w '%{url_effective}') || { - echo "CLI download failed." >&2 + local max_compressed=104857600 # 100 MB cap on the compressed download + local max_decompressed=209715200 # 200 MB cap on the decompressed binary + + # --max-filesize aborts the transfer once the declared size is known to exceed the cap. + # Measured against this endpoint (which 302s to sdk-assets), curl bails with a non-zero + # exit and nothing written to disk. curl documents the flag as a no-op when the length is + # unknown (chunked responses), so the explicit size check below backstops that case — + # otherwise an attacker-controlled endpoint could exhaust the disk during download, before + # the checksum and the decompression guard ever run (DEVA11Y-484 review). + local resolved_url curl_status=0 + resolved_url=$(curl -fR --max-filesize "$max_compressed" -z "$BINARY_ZIP_PATH" -L "https://api.browserstack.com/sdk/v1/download_cli?os=${OS}&os_arch=${ARCH}" -o "$BINARY_ZIP_PATH" -w '%{url_effective}') || curl_status=$? + if [[ $curl_status -ne 0 ]]; then + # Distinguish a size abort from a network failure by what landed on disk rather than by + # curl's exit code: --max-filesize is documented to exit 63, but measured against this + # endpoint (which 302s to sdk-assets) curl aborts during receive and exits 56 instead, so + # branching on 63 alone would misreport the common case (DEVA11Y-484 review). + local partial_size + partial_size=$(wc -c < "$BINARY_ZIP_PATH" 2>/dev/null || echo 0) + if [[ $partial_size -ge $max_compressed ]]; then + echo "BrowserStack CLI archive exceeds the maximum allowed download size (100 MB). Aborting." >&2 + else + echo "BrowserStack CLI download failed (curl exited $curl_status)." >&2 + fi + # Remove unconditionally, including on a transient network error. This deliberately gives + # up the -z If-Modified-Since fast path on the next run: a partial write carries a fresh + # mtime, so keeping it risks the next -z revalidation getting a 304 and handing a + # truncated archive to verify_binary_integrity. Losing a 304 is cheaper than trusting a + # truncated payload. + rm -f "$BINARY_ZIP_PATH" return 1 - } + fi + + local compressed_size + compressed_size=$(wc -c < "$BINARY_ZIP_PATH" 2>/dev/null || echo 0) + if [[ $compressed_size -gt $max_compressed ]]; then + echo "BrowserStack CLI archive exceeds the maximum allowed download size (100 MB). Aborting." >&2 + rm -f "$BINARY_ZIP_PATH" + return 1 + fi + verify_binary_integrity "$BINARY_ZIP_PATH" "$resolved_url" || return $? + # Extract to a temp path and atomically publish it. `> "$BINARY_PATH"` truncates the # destination before bsdtar is known to have succeeded, so a corrupt payload — the live # case today, since no sidecars are published yet and verification fails open — would # zero out a previously-good cached binary. Stage + mv keeps the cached binary intact # unless a fresh, extractable payload is in hand (DEVA11Y-473/474 review). - bsdtar -xvf "$BINARY_ZIP_PATH" -O > "${BINARY_PATH}.tmp" \ - && chmod 0755 "${BINARY_PATH}.tmp" \ - && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH" \ - && strip_quarantine + # + # The decompression-bomb guard (DEVA11Y-484) sits on that same staged path: head -c stops + # bsdtar via SIGPIPE once the decompressed output reaches the cap, and pipefail surfaces + # that as a failure. Because the cap applies to ${BINARY_PATH}.tmp and publication is a + # later mv, a rejected bomb leaves any previously-cached binary untouched. + # Save and restore pipefail rather than clearing it: these scripts do not enable it + # globally today, but unconditionally turning it off would silently disable it for + # everything after download_binary if they ever do (DEVA11Y-484 review). + local pipefail_was_set=0 + case "$(set +o)" in *"-o pipefail"*) pipefail_was_set=1 ;; esac + set -o pipefail + # `|| extract_status=$?` rather than a bare pipeline: the shebang is `bash -il`, so the + # user's rc files are sourced, and if one of them sets `-e` a bare failing pipeline aborts + # the script here — skipping the diagnostics below and leaving ${BINARY_PATH}.tmp behind, + # the exact residue the publish-failure cleanup was added to prevent (DEVA11Y-484 review). + local extract_status=0 + bsdtar -xvf "$BINARY_ZIP_PATH" -O | head -c "$max_decompressed" > "${BINARY_PATH}.tmp" || extract_status=$? + [[ $pipefail_was_set -eq 1 ]] || set +o pipefail + + local extracted_size + extracted_size=$(wc -c < "${BINARY_PATH}.tmp" 2>/dev/null || echo 0) + + # Size first, and `-ge` not `-gt`: head -c caps output at exactly $max_decompressed, so a + # file of exactly that size is indistinguishable from a truncated overflow and must be + # rejected. A bomb trips both this and extract_status (SIGPIPE), and the size message is + # the accurate one, so it is checked first. + if [[ $extracted_size -ge $max_decompressed ]]; then + echo "BrowserStack CLI archive exceeds the maximum allowed decompressed size (200 MB). Aborting." >&2 + rm -f "${BINARY_PATH}.tmp" + return 1 + fi + if [[ $extract_status -ne 0 ]]; then + echo "BrowserStack CLI archive could not be extracted (bsdtar exited $extract_status). Aborting." >&2 + rm -f "${BINARY_PATH}.tmp" + return 1 + fi + + # Clean the staged file up on *any* failure below, not just the size rejection above, + # so a failed chmod/mv never leaves a stray ${BINARY_PATH}.tmp in the cache. + if ! { chmod 0755 "${BINARY_PATH}.tmp" && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH"; }; then + echo "BrowserStack CLI: failed to publish the downloaded binary." >&2 + rm -f "${BINARY_PATH}.tmp" + return 1 + fi + strip_quarantine } # Self-update is opt-in (DEVA11Y-475): it runs only via the explicit `self-update` diff --git a/scripts/fish/cli.sh.sha256 b/scripts/fish/cli.sh.sha256 index bad1cac..6d29f3f 100644 --- a/scripts/fish/cli.sh.sha256 +++ b/scripts/fish/cli.sh.sha256 @@ -1 +1 @@ -0d2ca5760c849521d4d5a74fc418c168f76ed129af4a6c1f763003b50a3a4f51 cli.sh +eb31e82f018e80c969e3c2b3103e79c8e490135aecfd52e0e78abe7a6c46816d cli.sh diff --git a/scripts/zsh/cli.sh b/scripts/zsh/cli.sh index 1d554d8..8b92862 100644 --- a/scripts/zsh/cli.sh +++ b/scripts/zsh/cli.sh @@ -252,21 +252,98 @@ verify_binary_integrity() { } download_binary() { - local resolved_url - resolved_url=$(curl -fR -z "$BINARY_ZIP_PATH" -L "https://api.browserstack.com/sdk/v1/download_cli?os=${OS}&os_arch=${ARCH}" -o "$BINARY_ZIP_PATH" -w '%{url_effective}') || { - echo "CLI download failed." >&2 + local max_compressed=104857600 # 100 MB cap on the compressed download + local max_decompressed=209715200 # 200 MB cap on the decompressed binary + + # --max-filesize aborts the transfer once the declared size is known to exceed the cap. + # Measured against this endpoint (which 302s to sdk-assets), curl bails with a non-zero + # exit and nothing written to disk. curl documents the flag as a no-op when the length is + # unknown (chunked responses), so the explicit size check below backstops that case — + # otherwise an attacker-controlled endpoint could exhaust the disk during download, before + # the checksum and the decompression guard ever run (DEVA11Y-484 review). + local resolved_url curl_status=0 + resolved_url=$(curl -fR --max-filesize "$max_compressed" -z "$BINARY_ZIP_PATH" -L "https://api.browserstack.com/sdk/v1/download_cli?os=${OS}&os_arch=${ARCH}" -o "$BINARY_ZIP_PATH" -w '%{url_effective}') || curl_status=$? + if [[ $curl_status -ne 0 ]]; then + # Distinguish a size abort from a network failure by what landed on disk rather than by + # curl's exit code: --max-filesize is documented to exit 63, but measured against this + # endpoint (which 302s to sdk-assets) curl aborts during receive and exits 56 instead, so + # branching on 63 alone would misreport the common case (DEVA11Y-484 review). + local partial_size + partial_size=$(wc -c < "$BINARY_ZIP_PATH" 2>/dev/null || echo 0) + if [[ $partial_size -ge $max_compressed ]]; then + echo "BrowserStack CLI archive exceeds the maximum allowed download size (100 MB). Aborting." >&2 + else + echo "BrowserStack CLI download failed (curl exited $curl_status)." >&2 + fi + # Remove unconditionally, including on a transient network error. This deliberately gives + # up the -z If-Modified-Since fast path on the next run: a partial write carries a fresh + # mtime, so keeping it risks the next -z revalidation getting a 304 and handing a + # truncated archive to verify_binary_integrity. Losing a 304 is cheaper than trusting a + # truncated payload. + rm -f "$BINARY_ZIP_PATH" return 1 - } + fi + + local compressed_size + compressed_size=$(wc -c < "$BINARY_ZIP_PATH" 2>/dev/null || echo 0) + if [[ $compressed_size -gt $max_compressed ]]; then + echo "BrowserStack CLI archive exceeds the maximum allowed download size (100 MB). Aborting." >&2 + rm -f "$BINARY_ZIP_PATH" + return 1 + fi + verify_binary_integrity "$BINARY_ZIP_PATH" "$resolved_url" || return $? + # Extract to a temp path and atomically publish it. `> "$BINARY_PATH"` truncates the # destination before bsdtar is known to have succeeded, so a corrupt payload — the live # case today, since no sidecars are published yet and verification fails open — would # zero out a previously-good cached binary. Stage + mv keeps the cached binary intact # unless a fresh, extractable payload is in hand (DEVA11Y-473/474 review). - bsdtar -xvf "$BINARY_ZIP_PATH" -O > "${BINARY_PATH}.tmp" \ - && chmod 0755 "${BINARY_PATH}.tmp" \ - && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH" \ - && strip_quarantine + # + # The decompression-bomb guard (DEVA11Y-484) sits on that same staged path: head -c stops + # bsdtar via SIGPIPE once the decompressed output reaches the cap, and pipefail surfaces + # that as a failure. Because the cap applies to ${BINARY_PATH}.tmp and publication is a + # later mv, a rejected bomb leaves any previously-cached binary untouched. + # Save and restore pipefail rather than clearing it: these scripts do not enable it + # globally today, but unconditionally turning it off would silently disable it for + # everything after download_binary if they ever do (DEVA11Y-484 review). + local pipefail_was_set=0 + case "$(set +o)" in *"-o pipefail"*) pipefail_was_set=1 ;; esac + set -o pipefail + # `|| extract_status=$?` rather than a bare pipeline: the shebang is `bash -il`, so the + # user's rc files are sourced, and if one of them sets `-e` a bare failing pipeline aborts + # the script here — skipping the diagnostics below and leaving ${BINARY_PATH}.tmp behind, + # the exact residue the publish-failure cleanup was added to prevent (DEVA11Y-484 review). + local extract_status=0 + bsdtar -xvf "$BINARY_ZIP_PATH" -O | head -c "$max_decompressed" > "${BINARY_PATH}.tmp" || extract_status=$? + [[ $pipefail_was_set -eq 1 ]] || set +o pipefail + + local extracted_size + extracted_size=$(wc -c < "${BINARY_PATH}.tmp" 2>/dev/null || echo 0) + + # Size first, and `-ge` not `-gt`: head -c caps output at exactly $max_decompressed, so a + # file of exactly that size is indistinguishable from a truncated overflow and must be + # rejected. A bomb trips both this and extract_status (SIGPIPE), and the size message is + # the accurate one, so it is checked first. + if [[ $extracted_size -ge $max_decompressed ]]; then + echo "BrowserStack CLI archive exceeds the maximum allowed decompressed size (200 MB). Aborting." >&2 + rm -f "${BINARY_PATH}.tmp" + return 1 + fi + if [[ $extract_status -ne 0 ]]; then + echo "BrowserStack CLI archive could not be extracted (bsdtar exited $extract_status). Aborting." >&2 + rm -f "${BINARY_PATH}.tmp" + return 1 + fi + + # Clean the staged file up on *any* failure below, not just the size rejection above, + # so a failed chmod/mv never leaves a stray ${BINARY_PATH}.tmp in the cache. + if ! { chmod 0755 "${BINARY_PATH}.tmp" && mv -f "${BINARY_PATH}.tmp" "$BINARY_PATH"; }; then + echo "BrowserStack CLI: failed to publish the downloaded binary." >&2 + rm -f "${BINARY_PATH}.tmp" + return 1 + fi + strip_quarantine } # Self-update is opt-in (DEVA11Y-475): it runs only via the explicit `self-update` diff --git a/scripts/zsh/cli.sh.sha256 b/scripts/zsh/cli.sh.sha256 index c161ba4..0b00980 100644 --- a/scripts/zsh/cli.sh.sha256 +++ b/scripts/zsh/cli.sh.sha256 @@ -1 +1 @@ -aeb2333296f5b2b25c48a420abd0e89834fc3bf1b2e73acf782f223041dc3edf cli.sh +42c2f590cab5a24594e9f0db5d7ae25dffd95795838184a78d4c302d31945d4a cli.sh diff --git a/tests/README.md b/tests/README.md index 798e449..d4732f7 100644 --- a/tests/README.md +++ b/tests/README.md @@ -1,16 +1,22 @@ -# Integration test harnesses +# Test harnesses -End-to-end harnesses that integrate the `a11y-scan` command plugin from this -repository into real consumer projects and run accessibility scans against -sample sources with intentional issues. Each harness uses a **path dependency** -on the repo root (`../..`), so it always exercises the local plugin sources. +Two different kinds of suite live here. -| Folder | Consumer type | How the plugin is integrated | +**Integration harnesses** (`spm/`, `xcode-app/`) integrate the `a11y-scan` command +plugin from this repository into real consumer projects and run accessibility scans +against sample sources with intentional issues. Each uses a **path dependency** on +the repo root (`../..`), so it always exercises the local plugin sources. + +**Regression suites** (`extraction-guard/`) test one hardened code path directly — +no consumer project, no credentials, no network. + +| Folder | Kind | What it exercises | |---|---|---| -| [`spm/`](./spm) | SwiftPM package | Package dependency on `AccessibilityDevTools`; the command plugin is invoked with `swift package plugin … scan`. | -| [`xcode-app/`](./xcode-app) | Xcode iOS app (XcodeGen) | A pre-compile build phase runs the scan on every build — the official Xcode integration. | +| [`spm/`](./spm) | Integration | SwiftPM consumer: package dependency on `AccessibilityDevTools`; the command plugin is invoked with `swift package plugin … scan`. | +| [`xcode-app/`](./xcode-app) | Integration | Xcode iOS app (XcodeGen): a pre-compile build phase runs the scan on every build — the official Xcode integration. | +| [`extraction-guard/`](./extraction-guard) | Regression | The DEVA11Y-484 decompression-bomb guard in the shell launchers (`scripts/{bash,zsh,fish}/cli.sh`). Run with `bash tests/extraction-guard/run_tests.sh`. | -## Why two harnesses +## Why two integration harnesses The plugin supports both project types the product targets — SwiftPM packages and Xcode apps — and they integrate the **command** plugin differently: @@ -25,8 +31,10 @@ and Xcode apps — and they integrate the **command** plugin differently: ## Authentication -Both harnesses need BrowserStack credentials to actually run a scan (the plugin -downloads the CLI and makes authenticated calls): +Both **integration** harnesses need BrowserStack credentials to actually run a scan +(the plugin downloads the CLI and makes authenticated calls). The +`extraction-guard/` regression suite needs none — it serves its own fixtures over +localhost: ```bash export BROWSERSTACK_USERNAME= diff --git a/tests/extraction-guard/.gitignore b/tests/extraction-guard/.gitignore new file mode 100644 index 0000000..07ce746 --- /dev/null +++ b/tests/extraction-guard/.gitignore @@ -0,0 +1,7 @@ +# Generated by make_fixtures.sh — ~106 MB of archives, never commit. +# The bomb and oversized-download fixtures alone exceed GitHub's limits and have +# been rejected by the pre-receive hook before; keep this ignore in place. +# No trailing slash on purpose: `fixtures/` matches only a directory, so a +# SYMLINK named `fixtures` (easy to create when reusing fixtures across branches) +# slips past it and gets staged. This form matches both. +fixtures diff --git a/tests/extraction-guard/README.md b/tests/extraction-guard/README.md new file mode 100644 index 0000000..7145422 --- /dev/null +++ b/tests/extraction-guard/README.md @@ -0,0 +1,124 @@ +# DEVA11Y-484 extraction-guard regression suite + +Regression tests for the decompression-bomb guard on the CLI download path. + +```bash +bash tests/extraction-guard/run_tests.sh +``` + +First run generates ~106 MB of fixtures into `fixtures/` (gitignored). Requires +`bsdtar`, `curl`, `python3`, `awk` — all present on GitHub's `macos-latest`. + +## What it covers + +The **shell launchers**: the real `download_binary()` from +`scripts/{bash,zsh,fish}/cli.sh`, 20 assertions per variant, 60 total. + +| Case | Asserts | +|---|---| +| legit `.tar.gz` | exits 0, binary present, mode matches that variant's `cli.sh`, extracted binary runs | +| legit `.zip` | same — this is the format production actually serves | +| 400 MB bomb | rejected **by the decompressed-size cap specifically**, partial file cleaned up | +| 110 MB download | rejected *before extraction*, no binary written | +| corrupt archive | rejected, and *not* misreported as a size rejection | +| missing URL | rejected, no hang | +| 20,000-entry archive | succeeds — `-O` streams to one file, so nothing lands per-entry | +| multi-file archive | succeeds (pre-existing `-O` concatenation) | +| bomb after a good download | rejected, and the already-cached good binary survives | + +Note on the 110 MB row: it asserts "rejected before extraction", **not** "the +`--max-filesize` flag is present". Those are different claims — see the mutation +table below. + +Note on the 20,000-entry row: it asserts no per-entry disk amplification, **not** +that entry counts are capped. The shell path has no entry-count ceiling at all, +unlike the Swift path's `maxArchiveEntries = 10_000`; and because `-O` concatenates, +that archive publishes a 0-byte `browserstack-cli`. Pre-existing and outside this +ticket's remediation, but a real gap rather than covered behaviour. + +## How it avoids testing the wrong thing + +**Functions are extracted verbatim.** `load_download_binary` lifts +`_self_update_sha256`, `strip_quarantine`, `verify_binary_integrity` and +`download_binary` straight out of `cli.sh` and sources them. Loading only +`download_binary` leaves the others undefined, the function dies with exit 127, +and *every abort assertion passes for the wrong reason* — so all four are loaded. +Faithfulness greps then assert the extracted code still contains the guarded +`bsdtar … | head -c` pipeline and both cap constants; if `cli.sh` is refactored +past those, the suite fails loudly instead of quietly testing nothing. + +**Only curl's CLI boundary is shimmed.** `_shim/curl` rewrites the hardcoded +`api.browserstack.com` URL to a local `python3 -m http.server` and passes every +other argument through, so `--max-filesize`, `-L`, `-z` and the +`bsdtar | head -c` pipeline all execute for real. No network, no credentials. + +**The local server must be ours.** The port is derived from the PID, so an +unrelated local service can already hold it. A bare "does anything answer?" probe +would then succeed against *that* server, every fixture request would 404, and the +run would fail with a dozen confusing per-case errors instead of "port busy". +`start_server` therefore serves a random token and requires the responding server to +return it, trying other ports otherwise. + +**The expected mode is read per variant.** The three `cli.sh` files are +byte-identical in that region today, but reading bash's value for all three would +check a zsh- or fish-only change against the wrong source. + +**Exit status alone is not trusted.** A bomb trips both the size check *and* +`extract_status` (bsdtar takes SIGPIPE when `head -c` closes the pipe), so +asserting "exit 1" still passes with the size cap deleted — confirmed by mutation +test. The suite asserts on the *message*, which differs per path. + +**The expected file mode is read from `cli.sh`**, not hardcoded: main tightened it +from `0775` to `0755` and a hardcoded expectation had already rotted. + +**Fixture generation is locked and marker-gated.** Generation is not atomic, and +the first version gated on "does `legit.tar.gz` exist?" — which `make_fixtures.sh` +creates *first*. A second run starting behind a generating one therefore saw the +gate satisfied and read `bomb`/`manyfiles`/`multifile` while they were still being +written, failing 5 of 51 assertions. This was not theoretical: a reviewer running +the suite alongside another run hit it (1 run in 7). Generation now takes an +atomic `mkdir` lock and writes a `.complete` marker last; `run_tests.sh` gates on +that marker. Validated with 4 concurrent cold starts, a staggered cold start, and +6 warm serial runs — all green. + +## Mutation-validated + +Baseline green; each guard removal below flips it red: + +| Mutation | Caught by | +|---|---| +| disable the decompressed-size rejection | behavioural assertion | +| raise the decompressed cap to 4 GB | faithfulness grep | +| drop the `head -c` truncation | faithfulness grep | +| remove **both** compressed-cap layers | behavioural assertion | +| remove `--max-filesize` only | faithfulness grep | + +That last row is the subtle one, and it is why the greps exist. The compressed cap +has **two** layers: `--max-filesize` aborts the transfer pre-emptively, and an +explicit `compressed_size > max_compressed` check backstops responses with no +declared length. Removing the flag alone leaves the backstop, which still rejects — +measured: the full 105 MB downloads, then the backstop fires and emits the same +"maximum allowed download size" message. So **no behavioural assertion can detect +that mutation**; every one stays green. Only the grep catches it, and it matters, +because without the flag a chunked or undeclared-length response can write unbounded +bytes to disk before any check runs. + +An earlier revision of this suite asserted the opposite in a code comment ("with the +cap removed the failure moves to bsdtar"), which was false and contradicted this +table. Fixed. + +## Not covered + +The **Swift** half — `extractLocalArchive`, the extraction watchdog, and the +stderr excerpt bounding — has no automated coverage here. It is `private` on a +`private struct` in a plugin-only package with no library target, so no test can +import it. Making it testable requires the library extraction tracked in +**DEVA11Y-761**; the `refactor/DEVA11Y-484-testable-extraction` branch carries a +working prototype of that split plus Swift unit tests. + +## Location + +Under `tests/` rather than `scripts/` deliberately: the +`verify-selfupdate-checksums` workflow globs `scripts/**/*.sh` and requires a +committed `.sha256` sidecar for every match. Test scripts are not self-updated +and must not enter that glob. diff --git a/tests/extraction-guard/_shim/curl b/tests/extraction-guard/_shim/curl new file mode 100755 index 0000000..09a8294 --- /dev/null +++ b/tests/extraction-guard/_shim/curl @@ -0,0 +1,18 @@ +#!/usr/bin/env bash +# Test-only curl shim. Lets the real, unmodified download_binary() run while +# redirecting ONLY the hardcoded api.browserstack.com download URL to the local +# test server. Every other argument (--max-filesize, -L, -o, -z, ...) is preserved, +# so the security-relevant pipeline is exercised verbatim. Interception happens at +# curl's CLI boundary; the shell function itself is never edited. +set -u +: "${REAL_CURL:?REAL_CURL must point at the real curl}" +: "${TEST_DOWNLOAD_URL:?TEST_DOWNLOAD_URL must be set}" + +args=() +for a in "$@"; do + case "$a" in + https://api.browserstack.com/sdk/v1/download_cli*) a="$TEST_DOWNLOAD_URL" ;; + esac + args+=("$a") +done +exec "$REAL_CURL" "${args[@]}" diff --git a/tests/extraction-guard/lib/assert.sh b/tests/extraction-guard/lib/assert.sh new file mode 100644 index 0000000..d3bf654 --- /dev/null +++ b/tests/extraction-guard/lib/assert.sh @@ -0,0 +1,113 @@ +# Shared assertion + helper functions for the DEVA11Y-484 extraction tests. +# Sourced by the test suites; not executed directly. + +PASS=0 +FAIL=0 + +_green() { printf '\033[32m%s\033[0m' "$1"; } +_red() { printf '\033[31m%s\033[0m' "$1"; } + +ok() { PASS=$((PASS+1)); printf ' %s %s\n' "$(_green 'PASS')" "$1"; } +bad() { FAIL=$((FAIL+1)); printf ' %s %s\n' "$(_red 'FAIL')" "$1"; } + +# assert_eq +assert_eq() { + if [ "$1" = "$2" ]; then ok "$3 (= $2)"; else bad "$3 (expected '$2', got '$1')"; fi +} + +# assert_true <0-or-1 cmd-status> (call as: cmd; assert_status $? 0 "...") +assert_status() { + if [ "$1" = "$2" ]; then ok "$3 (exit $2)"; else bad "$3 (expected exit $2, got $1)"; fi +} + +# assert_le +assert_le() { + if [ "$1" -le "$2" ]; then ok "$3 ($1 <= $2)"; else bad "$3 ($1 > $2)"; fi +} + +# assert_absent +assert_absent() { + if [ ! -e "$1" ]; then ok "$2 (removed)"; else bad "$2 (still present: $1)"; fi +} + +# assert_contains +assert_contains() { + case "$1" in *"$2"*) ok "$3" ;; *) bad "$3 (missing '$2' in: $1)" ;; esac +} + +summary() { + echo + if [ "$FAIL" -eq 0 ]; then + printf '%s %d passed, 0 failed\n' "$(_green 'ALL GREEN')" "$PASS" + return 0 + fi + printf '%s %d passed, %d failed\n' "$(_red 'FAILURES')" "$PASS" "$FAIL" + return 1 +} + +# ---- local static file server (python3) ---- +SERVER_PID="" +SERVER_PORT="" +SERVER_TOKEN_FILE="" + +start_server() { + local root="$1" + # A bare readiness probe ("does anything answer on this port?") is not enough. If an + # unrelated local service already holds the PID-derived port, the probe succeeds + # against IT, start_server returns 0, and every fixture request 404s — surfacing as a + # dozen confusing per-case failures rather than "port busy". The old comment here + # promised a fallback that was never implemented (DEVA11Y-484 review). So: serve a + # token, require the responding server to be OURS, and try other ports if not. + # Per-run filename, not a fixed one. A shared name is itself a concurrency bug: + # four simultaneous runs overwrite each other's token, every probe then reads a + # foreign value, and start_server exhausts all its ports and fails with no tests + # run at all. Measured — 2 of 4 concurrent cold starts died that way. + local token_file=".eg-token.$$.${RANDOM:-0}" + local token="eg-$$-${RANDOM:-0}" + if ! printf '%s' "$token" > "${root}/${token_file}" 2>/dev/null; then + echo "ERROR: cannot write probe token into $root" >&2 + return 1 + fi + SERVER_TOKEN_FILE="${root}/${token_file}" + + local base=$(( 18000 + ($$ % 2000) )) + local attempt port got pid i + for attempt in 0 1 2 3 4 5 6 7 8 9; do + port=$(( base + attempt * 37 )) + [ "$port" -gt 64000 ] && port=$(( 18000 + attempt * 37 )) + ( cd "$root" && exec python3 -m http.server "$port" --bind 127.0.0.1 ) >/dev/null 2>&1 & + pid=$! + got="" + for i in $(seq 1 40); do + got=$(curl -fsS "http://127.0.0.1:${port}/${token_file}" 2>/dev/null || true) + [ -n "$got" ] && break + kill -0 "$pid" 2>/dev/null || break + sleep 0.1 + done + if [ "$got" = "$token" ]; then + SERVER_PID="$pid" + SERVER_PORT="$port" + return 0 + fi + kill "$pid" 2>/dev/null || true + wait "$pid" 2>/dev/null || true + done + + echo "ERROR: could not start a local server we own (tried 10 ports from ${base})" >&2 + return 1 +} + +stop_server() { + if [ -n "$SERVER_PID" ]; then + kill "$SERVER_PID" 2>/dev/null || true + wait "$SERVER_PID" 2>/dev/null || true + fi + SERVER_PID="" + # Remove our own probe token so repeated runs do not litter the fixtures dir. + if [ -n "$SERVER_TOKEN_FILE" ]; then + rm -f "$SERVER_TOKEN_FILE" 2>/dev/null || true + SERVER_TOKEN_FILE="" + fi +} + +url_for() { echo "http://127.0.0.1:${SERVER_PORT}/$1"; } diff --git a/tests/extraction-guard/make_fixtures.sh b/tests/extraction-guard/make_fixtures.sh new file mode 100755 index 0000000..dc37598 --- /dev/null +++ b/tests/extraction-guard/make_fixtures.sh @@ -0,0 +1,127 @@ +#!/usr/bin/env bash +# Generates real .tar.gz fixtures for the DEVA11Y-484 extraction-guard tests. +# Everything is bounded: even if a guard regressed and failed to abort, no fixture +# decompresses beyond ~400 MB, so a test run can never exhaust the disk. +# +# Generation is LOCKED and gated by a `.complete` marker written last, because it is +# not atomic and a concurrent reader will otherwise consume half-written files. +# Measured: gating on "does legit.tar.gz exist?" let a second run — started 3s behind +# a generating one — fail 5 of 51 assertions, because legit.tar.gz is created FIRST +# and the later fixtures were still being written. That is a real intermittent +# failure, first observed by a reviewer running the suite alongside another run. +set -euo pipefail + +DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/fixtures" +mkdir -p "$DIR" + +MARKER="$DIR/.complete" +LOCK="$DIR/.lock" + +# mkdir is atomic on every POSIX filesystem, so it serves as a lock without flock, +# which macOS does not ship as a CLI. +if ! mkdir "$LOCK" 2>/dev/null; then + echo " another run is generating fixtures; waiting..." + for _ in $(seq 1 300); do + [ -f "$MARKER" ] && { echo " fixtures ready (generated by the other run)."; exit 0; } + sleep 1 + done + echo "ERROR: timed out waiting for fixtures. Remove $LOCK if it is stale." >&2 + exit 1 +fi +trap 'rmdir "$LOCK" 2>/dev/null || true' EXIT + +if [ -f "$MARKER" ]; then + echo " fixtures already complete." + exit 0 +fi + +cd "$DIR" + +log() { printf ' %s\n' "$*"; } + +# A real, small, runnable host binary to stand in for browserstack-cli. +REAL_BIN="/usr/bin/true" +[ -x "$REAL_BIN" ] || REAL_BIN="/bin/echo" + +# --- 1. legit: a single real Mach-O binary (extracted artifact actually runs) --- +make_legit() { + rm -rf _legit && mkdir _legit + cp "$REAL_BIN" _legit/browserstack-cli + bsdtar -czf legit.tar.gz -C _legit browserstack-cli + rm -rf _legit + log "legit.tar.gz ($(wc -c < legit.tar.gz) bytes compressed)" +} + +# --- 2. bomb: small compressed, ~400 MB decompressed single file (bounded) --- +make_bomb() { + rm -rf _bomb && mkdir _bomb + # 400 MB of zeros compresses to a few hundred KB. + dd if=/dev/zero bs=1048576 count=400 of=_bomb/browserstack-cli 2>/dev/null + bsdtar -czf bomb.tar.gz -C _bomb browserstack-cli + rm -rf _bomb + log "bomb.tar.gz ($(wc -c < bomb.tar.gz) bytes compressed -> 400 MB decompressed)" +} + +# --- 3. many-files: lots of tiny entries, small total bytes (entry-count bomb) --- +make_manyfiles() { + rm -rf _many && mkdir _many + # 20k empty files: trivial bytes, entry count well past the 10k ceiling. + ( cd _many && touch $(seq -f 'f%.0f' 1 20000) ) + bsdtar -czf manyfiles.tar.gz -C _many . + rm -rf _many + log "manyfiles.tar.gz (20,000 entries, ~0 bytes each)" +} + +# --- 4. multi-file: a binary plus an extra file (structure, not a bomb) --- +make_multifile() { + rm -rf _multi && mkdir _multi + cp "$REAL_BIN" _multi/browserstack-cli + printf 'license text\n' > _multi/LICENSE + bsdtar -czf multifile.tar.gz -C _multi browserstack-cli LICENSE + rm -rf _multi + log "multifile.tar.gz (binary + LICENSE)" +} + +# --- 5. oversized-download: a payload larger than the 100 MB curl --max-filesize cap --- +# curl aborts on download size before bsdtar ever runs, so the bytes need not form a +# valid archive; 105 MB of zeros is enough and is cheap to produce. +make_oversized_download() { + dd if=/dev/zero bs=1048576 count=105 of=oversized-download.bin 2>/dev/null + log "oversized-download.bin ($(wc -c < oversized-download.bin) bytes > 100 MB cap)" +} + +# --- 6b. legit ZIP: the format production actually serves --- +# api.browserstack.com 302s to sdk-assets…/binary-macos-arm64-.zip, and cli.sh +# names the path BINARY_ZIP_PATH — yet every other fixture here is .tar.gz. The guard +# itself is format-independent (`head -c` plus a post-hoc `wc -c`), so this is fidelity +# rather than a correctness hole, but a suite that never sees the real format is not +# testing the real path (DEVA11Y-484 review). +make_legit_zip() { + rm -rf _zip && mkdir _zip + cp "$REAL_BIN" _zip/browserstack-cli + bsdtar -a -cf legit.zip -C _zip browserstack-cli + rm -rf _zip + log "legit.zip ($(wc -c < legit.zip) bytes, real production format)" +} + +# --- 6. corrupt: not a valid archive (bsdtar should fail cleanly) --- +# /dev/urandom, not /dev/zero: an all-zero file is a VALID EMPTY tar archive (tar +# terminates on two zero blocks), so zeros would make bsdtar exit 0 and the "corrupt" +# assertions would pass for the wrong reason — measured while building this suite. +make_corrupt() { + head -c 4096 /dev/urandom > corrupt.tar.gz + log "corrupt.tar.gz (random bytes, not a real archive)" +} + +echo "Generating fixtures in $DIR ..." +make_legit +make_legit_zip +make_bomb +make_manyfiles +make_multifile +make_oversized_download +make_corrupt + +# Marker LAST. A reader gated on this can never observe a partially written fixture. +touch "$MARKER" +echo "Done." diff --git a/tests/extraction-guard/run_tests.sh b/tests/extraction-guard/run_tests.sh new file mode 100755 index 0000000..41cef08 --- /dev/null +++ b/tests/extraction-guard/run_tests.sh @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# DEVA11Y-484 decompression-bomb guard — regression suite (shell launchers). +# +# Covers the shell half of the guard: the real download_binary() in +# scripts/{bash,zsh,fish}/cli.sh, exercised against locally generated archives +# through a curl shim. No network, no credentials, no mocks of bsdtar/head/curl. +# +# The Swift half (extractLocalArchive + the extraction watchdog) is NOT covered +# here. It is private on a private struct inside a plugin-only package with no +# library target, so it cannot be imported by a test. Making it testable requires +# the library extraction tracked in DEVA11Y-761. +set -uo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +rc=0 + +echo "════════════════════════════════════════════════════════" +echo " DEVA11Y-484 extraction guard — shell regression suite" +echo "════════════════════════════════════════════════════════" + +for tool in bsdtar curl python3 awk; do + command -v "$tool" >/dev/null 2>&1 || { + echo "FATAL: required tool '$tool' not found on PATH." >&2 + exit 1 + } +done + +# Gate on the completion marker, NOT on any individual fixture. legit.tar.gz is +# written first, so gating on it lets a concurrent run start reading while the later +# fixtures are still being written — measured: 5 of 51 assertions failed that way. +if [ ! -f "$HERE/fixtures/.complete" ]; then + echo + echo "▶ Generating fixtures (~106 MB, gitignored)" + bash "$HERE/make_fixtures.sh" || exit 1 +fi + +if [ ! -f "$HERE/fixtures/.complete" ]; then + echo "FATAL: fixtures incomplete after generation." >&2 + exit 1 +fi + +echo +echo "▶ Shell launcher tests (bash / zsh / fish)" +bash "$HERE/test_shell_extraction.sh" || rc=1 + +echo +if [ "$rc" -eq 0 ]; then + echo "DEVA11Y-484 shell suite: ALL GREEN" +else + echo "DEVA11Y-484 shell suite: FAILURES (see above)" +fi +exit "$rc" diff --git a/tests/extraction-guard/test_shell_extraction.sh b/tests/extraction-guard/test_shell_extraction.sh new file mode 100755 index 0000000..650156f --- /dev/null +++ b/tests/extraction-guard/test_shell_extraction.sh @@ -0,0 +1,222 @@ +#!/usr/bin/env bash +# Integration tests for the REAL download_binary() in scripts/{bash,zsh,fish}/cli.sh. +# +# The functions are extracted VERBATIM from the repo and run against a local +# server; only the hardcoded api.browserstack.com URL is redirected, via the curl +# shim. bsdtar, head, curl and the guarded pipeline are never mocked — if the +# shipped guard regresses, these tests fail. +# +# Lives under tests/ rather than scripts/ on purpose: the +# verify-selfupdate-checksums workflow globs scripts/**/*.sh and requires a +# committed .sha256 sidecar for every match. Test scripts are not self-updated, +# so they must not enter that glob. +set -uo pipefail + +HERE="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +REPO="$(cd "$HERE/../.." && pwd)" +FIXTURES="$HERE/fixtures" +# shellcheck source=lib/assert.sh +source "$HERE/lib/assert.sh" + +REAL_CURL="$(command -v curl)" +export REAL_CURL +SHIM_DIR="$HERE/_shim" +chmod +x "$SHIM_DIR/curl" + +WORK="$(mktemp -d)" +trap 'stop_server; rm -rf "$WORK"' EXIT + +start_server "$FIXTURES" || exit 1 + +# download_binary() calls three sibling functions. Extracting only download_binary +# leaves them undefined, and the function then dies with exit 127 on the first call +# — which looks exactly like a guard rejection and would make every abort assertion +# pass for the wrong reason. Load the whole dependency set. +DEPS=(_self_update_sha256 strip_quarantine verify_binary_integrity download_binary) + +load_download_binary() { + local variant="$1" + local src="$REPO/scripts/$variant/cli.sh" + local fn="$WORK/functions.$variant.sh" + : > "$fn" + + local dep + for dep in "${DEPS[@]}"; do + awk -v name="$dep" ' + $0 ~ "^" name "\\(\\) \\{" { p = 1 } + p { print } + /^\}/ { if (p) { p = 0 } } + ' "$src" >> "$fn" + grep -q "^${dep}() {" "$fn" || { bad "$variant: could not extract ${dep}()"; return 1; } + done + + # Faithfulness guards: the extracted code must still contain the security-relevant + # pipeline and both caps. If cli.sh is refactored so these no longer match, the + # suite must fail loudly rather than silently testing something else. + grep -q 'bsdtar -xvf "\$BINARY_ZIP_PATH" -O | head -c "\$max_decompressed"' "$fn" \ + || { bad "$variant: extracted code lost the guarded bsdtar|head pipeline"; return 1; } + grep -q 'max_compressed=104857600' "$fn" \ + || { bad "$variant: extracted code lost the 100 MB compressed cap"; return 1; } + # The pre-emptive transfer abort. Behavioural assertions CANNOT cover this: the + # explicit compressed_size backstop still rejects an oversized download without it, + # so removing the flag leaves every assertion green. Only this grep catches it — + # and without the flag a chunked/undeclared-length response can write unbounded + # bytes to disk before any check runs, which is the whole point of having it. + grep -q -- '--max-filesize "\$max_compressed"' "$fn" \ + || { bad "$variant: extracted code lost the pre-emptive --max-filesize abort"; return 1; } + grep -q 'max_decompressed=209715200' "$fn" \ + || { bad "$variant: extracted code lost the 200 MB decompressed cap"; return 1; } + + # shellcheck disable=SC1090 + source "$fn" +} + +# run_case -> sets LAST_STATUS / LAST_CACHE / LAST_STDERR +# +# stderr is captured, not discarded, because exit status alone cannot tell WHICH +# guard fired. A decompression bomb trips both the size check and extract_status +# (bsdtar takes SIGPIPE when `head -c` closes the pipe), so asserting only +# "exit 1" still passes with the size guard deleted — verified by mutation test. +# The two paths emit different messages, so the message is the discriminator. +run_case() { + local fixture="$1" + local cache + cache="$(mktemp -d "$WORK/cache.XXXX")" + export OS=macos ARCH=arm64 + export BINARY_ZIP_PATH="$cache/browserstack-cli.zip" + export BINARY_PATH="$cache/browserstack-cli" + TEST_DOWNLOAD_URL="$(url_for "$fixture")" + export TEST_DOWNLOAD_URL + local errfile="$cache/stderr.txt" + ( PATH="$SHIM_DIR:$PATH"; download_binary ) >/dev/null 2>"$errfile" + LAST_STATUS=$? + LAST_CACHE="$cache" + LAST_STDERR="$(cat "$errfile" 2>/dev/null)" +} + +# The mode download_binary applies before publishing the binary. Read it from the +# shipped script instead of hardcoding: main tightened this from 0775 to 0755, and +# a hardcoded expectation silently rotted. +# +# Read PER VARIANT, not once from bash: the three scripts are byte-identical in this +# region today, but reading bash's value for all three would check a zsh- or +# fish-only mode change against the wrong source (DEVA11Y-484 review). +read_expected_mode() { + local variant="$1" mode + mode="$(sed -n 's/.*chmod \(0[0-7][0-7][0-7]\) "${BINARY_PATH}.tmp".*/\1/p' \ + "$REPO/scripts/${variant}/cli.sh" | head -1)" + mode="${mode#0}" + [ -n "$mode" ] || { echo "FATAL: could not read expected chmod from ${variant}/cli.sh" >&2; exit 1; } + printf '%s' "$mode" +} + +for variant in bash zsh fish; do + echo "── variant: $variant ───────────────────────────────" + load_download_binary "$variant" || continue + EXPECTED_MODE="$(read_expected_mode "$variant")" + echo " expected published mode, read from ${variant}/cli.sh: 0${EXPECTED_MODE}" + + # 1. Legit binary: succeeds, runnable, correct mode, bytes intact. + run_case "legit.tar.gz" + assert_status "$LAST_STATUS" 0 "$variant legit: exits 0" + if [ -f "$LAST_CACHE/browserstack-cli" ]; then + ok "$variant legit: binary present" + else + bad "$variant legit: binary missing" + fi + perms=$(stat -f '%Lp' "$LAST_CACHE/browserstack-cli" 2>/dev/null \ + || stat -c '%a' "$LAST_CACHE/browserstack-cli" 2>/dev/null) + assert_eq "$perms" "$EXPECTED_MODE" "$variant legit: chmod 0${EXPECTED_MODE} applied" + "$LAST_CACHE/browserstack-cli" >/dev/null 2>&1 + assert_status $? 0 "$variant legit: extracted binary runs" + + # 2. Decompression bomb (400 MB): aborts via the SIZE cap specifically, partial + # binary removed. The message assertion is load-bearing — see run_case. + run_case "bomb.tar.gz" + assert_status "$LAST_STATUS" 1 "$variant bomb: aborts with exit 1" + assert_contains "$LAST_STDERR" "maximum allowed decompressed size" \ + "$variant bomb: rejected by the DECOMPRESSED-SIZE cap (not merely SIGPIPE)" + assert_absent "$LAST_CACHE/browserstack-cli" "$variant bomb: partial binary cleaned up" + + # 3. Oversized download (>100 MB): rejected before bsdtar ever runs. + # + # Be precise about what this proves, because an earlier version of this comment + # overclaimed. The compressed cap has TWO layers: `--max-filesize` aborts the + # transfer pre-emptively, and an explicit `compressed_size > max_compressed` + # check (cli.sh) backstops it for responses with no declared length. Removing + # the flag alone therefore does NOT move the failure to bsdtar — the backstop + # fires and emits "maximum allowed download size", which is the second pattern + # accepted below. So this assertion proves "rejected before extraction", NOT + # "the flag is present". + # + # Coverage for the flag itself is the faithfulness grep in + # load_download_binary, which fails loudly if `--max-filesize` disappears. + # Against a local server the Content-Length is known up front, so curl aborts at + # ~0 bytes and the specific size wording is unreachable here — cli.sh branches on + # bytes-on-disk, not curl's exit code, because against the real endpoint curl + # aborts mid-receive with 56 rather than the documented 63. + run_case "oversized-download.bin" + assert_status "$LAST_STATUS" 1 "$variant oversized-download: aborts with exit 1" + case "$LAST_STDERR" in + *"curl exited"*|*"maximum allowed download size"*) + ok "$variant oversized-download: rejected at the DOWNLOAD stage by --max-filesize" ;; + *) + bad "$variant oversized-download: not rejected during download (stderr: ${LAST_STDERR})" ;; + esac + assert_absent "$LAST_CACHE/browserstack-cli" "$variant oversized-download: no binary written" + + # 4. Corrupt archive: bsdtar fails, abort. Must NOT be misreported as a size + # rejection — that would mean the size branch is swallowing unrelated failures. + run_case "corrupt.tar.gz" + assert_status "$LAST_STATUS" 1 "$variant corrupt: aborts with exit 1" + case "$LAST_STDERR" in + *"maximum allowed decompressed size"*) + bad "$variant corrupt: misreported as a size rejection" ;; + *) ok "$variant corrupt: not misreported as a size rejection" ;; + esac + + # 5. 404 / network failure: abort, no hang. + run_case "does-not-exist.tar.gz" + assert_status "$LAST_STATUS" 1 "$variant missing-url: aborts with exit 1" + + # 6. Many-files archive via -O: the shell path concatenates entries into one stream, + # so the entry-count vector writes nothing per-entry; bounded by head -c. + # + # Read this as "no per-entry disk amplification", NOT "entry counts are capped". + # The shell path has NO entry-count ceiling at all, unlike the Swift path's + # maxArchiveEntries = 10_000; and because -O concatenates, a 20,000-entry archive + # of empty files publishes a 0-byte browserstack-cli chmod'd 0755. That asymmetry + # is pre-existing and outside DEVA11Y-484's remediation, but it is a real gap — + # do not let this passing assertion read as coverage of it. + run_case "manyfiles.tar.gz" + assert_status "$LAST_STATUS" 0 "$variant many-files: succeeds (-O stream, nothing per-entry on disk)" + + # 7. Multi-file archive via -O: pre-existing concatenation behaviour, unchanged. + run_case "multifile.tar.gz" + assert_status "$LAST_STATUS" 0 "$variant multi-file: succeeds (pre-existing -O concatenation)" + + # 7b. The format production ACTUALLY serves: a .zip. Every other fixture is + # .tar.gz, so without this the suite never exercises the real archive format. + run_case "legit.zip" + assert_status "$LAST_STATUS" 0 "$variant legit zip: exits 0 (real production format)" + perms=$(stat -f '%Lp' "$LAST_CACHE/browserstack-cli" 2>/dev/null \ + || stat -c '%a' "$LAST_CACHE/browserstack-cli" 2>/dev/null) + assert_eq "$perms" "$EXPECTED_MODE" "$variant legit zip: chmod 0${EXPECTED_MODE} applied" + "$LAST_CACHE/browserstack-cli" >/dev/null 2>&1 + assert_status $? 0 "$variant legit zip: extracted binary runs" + + # 8. A rejected download must not destroy an already-good cached binary. + run_case "legit.tar.gz" + printf 'sentinel' > "$LAST_CACHE/browserstack-cli" + GOOD_CACHE="$LAST_CACHE" + export BINARY_ZIP_PATH="$GOOD_CACHE/browserstack-cli.zip" + export BINARY_PATH="$GOOD_CACHE/browserstack-cli" + TEST_DOWNLOAD_URL="$(url_for bomb.tar.gz)" + export TEST_DOWNLOAD_URL + ( PATH="$SHIM_DIR:$PATH"; download_binary ) >/dev/null 2>&1 + assert_status $? 1 "$variant cached-binary: bomb still rejected" + assert_eq "$(cat "$GOOD_CACHE/browserstack-cli" 2>/dev/null)" "sentinel" \ + "$variant cached-binary: existing good binary left intact" +done + +summary