Skip to content

fix(cli): stop unbounded runtime archive downloads - #23418

Open
saphid wants to merge 4 commits into
pingdotgg:mainfrom
saphid:fix/npm-launcher-download-byte-limit-20261022
Open

saphid wants to merge 4 commits into
pingdotgg:mainfrom
saphid:fix/npm-launcher-download-byte-limit-20261022

Conversation

@saphid

@saphid saphid commented Oct 22, 2026

Copy link
Copy Markdown
Contributor

Problem

The npm launcher's native-runtime downloader writes response chunks until the response ends. A missing or incorrect Content-Length can therefore let one archive response consume unbounded disk space before the checksum is checked.

Change

Bound each compressed native archive download to 128 MiB (134,217,728 bytes). Reject an oversized declared Content-Length before opening the destination, and check each actual chunk against the remaining allowance before writing it. The fixed ceiling is the smallest power-of-two MiB ceiling above the five supported CLI archives in release 0.1.2-nightly.20261021.3499; the largest archive there is Linux x64 at 94,048,738 bytes. A larger future archive requires an explicit limit change; no configuration option is added.

Complete short writes and count bytes actually written. Failed downloads abort the request, close the descriptor and remove the partial archive only when a nonfollowing BigInt device/inode check still identifies the file created by this download. Existing archive paths are not truncated. Missing, replaced, symlinked or unidentifiable paths are left alone, and identity-probe failure preserves the original transfer error.

The identity check and unlink are not atomic. This retains the existing requirement for a private, exclusively owned staging directory; it does not make cleanup safe in an adversarial shared directory. The outer installer's staging cleanup is unchanged. Existing fetch timeout/redirect defaults, HTTP errors and the install caller's checksum gate are retained.

Scope and approval

Submitted under the small, focused obvious-bug exception in CONTRIBUTING.md: this fixes the existing downloader's lack of a finite write bound, within one launcher file. It adds no settings, release pipeline or installer framework.

This bounds downloaded archive-file bytes only. It does not bound expanded archives, npm/cache/scratch/log output, retained versions, concurrent installers or forward/rollback whole-transaction costs. This PR does not deploy an update or enable pruning.

Verification

Author and fresh independent review both passed 22 focused tests on Node v24.12.0/macOS. The external component harness evaluates the actual source function bodies in VM with stub transport/filesystem boundaries and an eight-byte lexical test limit; it does not import top-level main(), fetch real payloads, extract archives or execute native binaries. The production 128 MiB constant is checked separately.

Cases cover under-limit/exact-boundary success, declared and cumulative overflow, missing/malformed/lying length, short/zero writes, network/disk failure cleanup and abort, existing archive/runtime preservation, HTTP errors, truncated-archive checksum rejection, replaced/missing/symlinked paths, identity-probe/capture failure and distinct 64-bit inode identities. The earlier replacement-path cleanup regression was reproduced and closed in the independently reviewed revision.

Sanitized commands (SOURCE, test paths and PATCH_FILE denote the external component packet, not repository-provided fixtures):

LAUNCHER_SOURCE="$SOURCE" EXPECT_DOWNLOAD_POLICY=1 node --test --test-reporter=tap "$DOWNLOAD_TEST" "$OWNED_CLEANUP_TEST"
node --check "$SOURCE"
git apply --check --whitespace=error-all "$PATCH_FILE"
Exact original preimage: 15 tests, 5 pass, 10 expected failures; exit 1
Earlier cleanup revision: 6 ownership cases, 6 expected failures; exit 1
Final author run:       22 tests, 22 pass, 0 fail; exit 0
Fresh independent run: 22 tests, 22 pass, 0 fail; exit 0
Syntax / strict patch / exact afterimage and mode: pass

Reviewed source SHA-256: 3261f86a9638bb10a977e51e2c46813b1202fffc843ad068489bc8551a2c778f; Git blob 51178c5eaa5ec53616b3655c30b07e1ab32fdf6e, mode 100644.

The external harness is not committed in this one-file contribution. These are component checks, not repository-wide or hosted CI results. No Windows or minimum-supported-Node execution is claimed. This draft awaits normal repository/upstream validation, including test-suite integration as maintainers require.

Implemented and independently reviewed with GPT-6.1 Sol (high reasoning, priority/Fast), through the Codex harness.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 22, 2026
@saphid
saphid marked this pull request as ready for review October 22, 2026 07:25
macroscopeapp[bot]
macroscopeapp Bot previously approved these changes Oct 22, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at bdc1dfb

Macroscope's review found this PR approvable — The launcher now bounds runtime archive downloads and cleans up failed transfers while retaining checksum verification and staged installation. Existing complete installations are unaffected.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review.

Recent review info
Run configuration
Commits

Reviewing files that changed from the base of the PR and between f5c3f75 and bdc1dfb.

Files selected for processing: 2
  • scripts/build-npm-launcher.test.ts
  • scripts/npm-launcher/t3.mjs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.

Walkthrough

The npm launcher passes request options to fetch, limits downloads to 128 MiB, creates destination files exclusively, handles partial writes, and removes incomplete files only when their identity matches the file created. A test verifies that an oversized response fails without installing a version.

Changes: npm launcher downloads

Layer / File(s) Summary
Bounded download and cleanup
scripts/npm-launcher/t3.mjs, scripts/build-npm-launcher.test.ts
get passes optional request options to fetch. download checks declared and received data against the 128 MiB limit, creates the destination exclusively, handles partial writes, and returns the SHA-256 digest on success. On failure, it aborts the request and removes the destination only if its identity still matches the file created. The test verifies that an oversized response exits with code 1 and leaves the versions directory empty.

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: Minimal · up to bdc1d

No actionable issue is established; the change is mergeable after normal repository validation.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: preventing unbounded runtime archive downloads. It follows the required conventional commit style.
Description check Passed The description includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the failure mode, implementation, scope limits, approval exception, focused test cove…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description includes all required sections: Problem, Change, Scope and approval, and Verification. It explains the failure mode, implementation, scope limits, approval exception, focused test coverage, observed results, and unverified environments.

Finishing Touches
Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

@macroscopeapp
macroscopeapp Bot dismissed their stale review October 31, 2026 08:09

Dismissing prior approval to re-evaluate bdc1dfb

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

Verified bdc1dfb8e2906d84dc22c2aa534a0d9bda059bc3 on Linux with the actual npm launcher entry point and real release archive, plus the running dev web client and Codex gpt-6.1-sol.

  • Reproduced on github/main (575579d68a): a chunked HTTP response containing two copies of the actual v0.1.6 Linux x64 archive transferred 246,305,096 bytes before checksum rejection.
  • The PR rejects both oversized declared length and chunked overflow with download exceeds 134217728-byte limit. Syscall tracing recorded 134,133,632 archive bytes written for chunked overflow, below the ceiling. The partial archive was unlinked and neither failure published a runtime.
  • The normal 123,152,548-byte GitHub release passed its published checksum, extracted, and executed the real native binary: t3 v0.1.6, exit 0. Cached startup worked with the release endpoint unavailable. A fresh install retained the missing-release error.
  • Started the exact PR source with explicit isolated dev state. Used repository Playwright and /usr/bin/google-chrome to add a real git project, choose gpt-6.1-sol, and send a real provider turn. The client displayed the provider reading the exact PR HEAD and completing successfully.
  • Blacksmith passed the existing launcher test and check:changed against the PR base: formatting, lint, knip, affected typecheck and launcher test. Hosted checks were green at this head. No source changes or pushes were needed.

The overflow input was fault injection through a real HTTP release relay serving the actual archive bytes, with unchanged production limits and launcher source. Normal download verification used the real release and executable. Source dev startup and npm download verification are separate paths; starting dev alone would not exercise this fix.

command transcript

real isolated dev web client showing completed codex turn and exact pr head

Not verified

Native macOS, Windows, ARM64, desktop shell and iOS/Android clients were not exercised on this Linux x64 VM. Short filesystem writes, replacement-path races and hostile shared staging directories were not runtime-injected. No UI behavior changed, so the downloader proof is the command transcript rather than visual before/after captures.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants