Skip to content

fix: harden artifact ingestion boundaries - #1692

Merged
thymikee merged 3 commits into
mainfrom
agent/security-hardening
Aug 9, 2026
Merged

fix: harden artifact ingestion boundaries#1692
thymikee merged 3 commits into
mainfrom
agent/security-hardening

Conversation

@thymikee

@thymikee thymikee commented Aug 8, 2026

Copy link
Copy Markdown
Member

Summary

  • pin and verify the MCP registry publisher before authentication and publication
  • add bounded, in-process archive handling and approved network destination enforcement for install sources
  • serialize resumable upload lifecycle operations and enforce byte, range, rollback, and cleanup contracts

This is separate from #1678, which changes MCP protocol behavior and does not overlap these files.

Validation

  • VITEST_MAX_WORKERS=2 pnpm check:affected --run — all runnable checks passed
  • affected coverage: 1,788 tests across 222 files; 82.64% changed-line coverage
  • pnpm audit --prod --audit-level high — no known vulnerabilities
  • clean published-package install and all 12 public entry-point imports verified

Closes #1685
Closes #1687
Closes #1688

@thymikee

thymikee commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

Review findings

  • [P1] Enforce archive budgets while inspecting, not after. extractTarArchive fully decompresses and drains every entry in inspectTar before reserveArchiveManifest sees the byte or entry totals; extractZipArchive likewise collects its complete central-directory entry list before reserving. A compressed tar.gz expansion bomb can therefore consume unbounded decompression work before the 4 GiB/100,000-entry limit rejects it, contrary to Harden install-source materialization boundaries #1687. Charge/check the budget during metadata inspection (and stop reading immediately at the limit), then add extractor-level low-limit regressions that prove bytes and entry enumeration do not pass the limit.

  • [P2] Do not refresh expiry for an ignored upload chunk. receiveResumableTransfer returns the current offset without appending for an out-of-order range or an un-ranged retry, but receiveResumableUploadChunk refreshes the ticket timer for every returned result. Repeated no-op requests can now keep a partially uploaded ticket and its temporary data alive indefinitely, violating the five-minute last-received-chunk lifecycle contract in Harden resumable upload validation and lifecycle #1688. Return an accepted/no-op outcome and refresh only after an append; cover it with a fake-timer regression.

Separately, GitHub currently reports this draft as conflicting/dirty and one commit behind main; rebase before re-review.

@thymikee
thymikee force-pushed the agent/security-hardening branch from 659fe06 to 989831d Compare August 8, 2026 11:19
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-09 07:56 UTC

@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 2.00 MB 2.01 MB +14.2 kB
JS gzip 646.5 kB 651.0 kB +4.5 kB
npm tarball 785.0 kB 789.6 kB +4.6 kB
npm unpacked 2.74 MB 2.76 MB +14.4 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.4 ms 25.9 ms -0.5 ms
CLI --help 63.9 ms 64.2 ms +0.3 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/internal/daemon.js +1.7 kB +374 B

@thymikee

thymikee commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 989831d06392566525bc3de90a50f88b991bf879. This rebase resolves the merge conflict only; the relevant remediation blobs are byte-identical to the reviewed 659fe06 head: archive-extraction-tar.ts (2ca4667), archive-extraction-zip.ts (d17429b), archive-safety.ts (519e337), resumable-upload-transfer.ts (1b7417), and resumable-upload.ts (229cd4b).

The P1 archive-budget issue remains: TAR inspection drains/decompresses before byte/entry reservation, and ZIP enumerates all entries before reservation. The P2 lifecycle issue remains: a stale/no-op receive returns without appending but still refreshes ticket expiry. The associated test blobs are also unchanged, so required extractor-level bounded-read and fake-timer no-op-expiry regressions are still absent.

The branch is now mergeable, but these findings still block readiness.

@thymikee

thymikee commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

Addressed both blocking findings in a1b352027:

  • Archive inspection now checks cumulative entry and expanded-byte budgets while metadata is enumerated, before TAR bodies are drained or ZIP enumeration advances past the limit. Added extractor-level adversarial regressions for truncated TGZ and bounded ZIP enumeration.
  • Ignored out-of-order and un-ranged retry chunks now report appended: false internally and no longer refresh ticket expiry. Added fake-timer regressions for both no-op paths.

Validation: pnpm check:affected --run passed (1,801 tests; 83.25% changed-line coverage), including normal TAR/ZIP install flows and resumable-upload integration.

@thymikee

thymikee commented Aug 8, 2026

Copy link
Copy Markdown
Member Author

[P2] Do not refresh an existing ticket on idempotent preflight

The ignored-chunk paths now correctly avoid refreshing expiry, but beginResumableUpload still finds a same-key existing ticket and unconditionally calls refreshResumableUploadTimer (src/daemon/resumable-upload.ts:62-65). A client can repeat POST /upload/preflight with the same attempt metadata before each deadline, retaining a partial upload and its temp data indefinitely despite receiving no chunk. That still violates the five-minute-after-last-received-chunk lifecycle contract.

Refresh only when creating a ticket (or otherwise never for idempotent preflight reuse), and add a fake-timer regression that begins, appends partial data, advances near expiry, repeats preflight, then proves the original deadline removes the ticket.

@thymikee

thymikee commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

Addressed the new preflight-expiry finding in 565ea5452.

  • Reusing an existing upload ticket is now a pure lookup and no longer refreshes expiry. Timer initialization moved into new-ticket creation, while successful appended chunks remain the only later refresh point.
  • Added a public-seam fake-timer regression that starts a partial upload, advances four minutes, repeats the identical preflight, and proves the ticket still expires at the original five-minute deadline. The test was red on the previous implementation (Upload is incomplete after five minutes) and is green after the fix.
  • Tightened the implementation without weakening boundaries: removed unused approval/transport fields and proxy parameters, removed redundant archive-budget state accessors, simplified the TAR mode mask, and dropped a response-body guard already guaranteed by the transport type. I kept the archive inspection/extraction passes and upload lifecycle states because they enforce TOCTOU validation, bounded inspection, serialization, stale-timer rejection, active receive abort, and terminal cleanup.

Validation: pnpm check:affected --run passed (1,802 tests; 83.20% changed-line coverage), and the branch is rebased/up to date with origin/main.

@thymikee

thymikee commented Aug 9, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 565ea5452: the preflight expiry bypass is fixed. Repeating an idempotent preflight no longer refreshes the ticket; timer initialization occurs only at entry creation and accepted chunks remain the only refresh path. The new fake-timer public-seam regression is non-vacuous and would fail on the prior head. The remaining delta is behavior-preserving cleanup. No actionable code finding remains; code review is clean / ready for human review.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 9, 2026
@thymikee
thymikee marked this pull request as ready for review August 9, 2026 07:52
@thymikee
thymikee merged commit e18a183 into main Aug 9, 2026
32 checks passed
@thymikee
thymikee deleted the agent/security-hardening branch August 9, 2026 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Harden resumable upload validation and lifecycle Harden install-source materialization boundaries Harden MCP registry publishing

1 participant