Reject special files and cap restore size (#2, #3) #9

Merged

stephen merged 1 commit from feat/verified-confined-restore into main

2026-06-17 18:25:44 +00:00

stephen commented

2026-06-17 18:18:49 +00:00

Owner

What changed

Restore already confines archive members to the requested path: roots (#3) and verifies the sha256 sidecar before extracting (#2). This closes the remaining gaps in the same pre-extraction pass, so a poisoned cache object cannot slip past it through a payload that is in-bounds but still hostile.

Refuse any special-file entry: character/block device, fifo, or socket. A dependency or build cache never contains one; a poisoned object could carry one to plant a node in the cache tree.
Cap the restore by declared total size and entry count, both read from the tar -tvP listing before any byte is written, so a single object cannot exhaust the runner's disk (decompression bomb) or inodes (entry flood). Defaults are generous sanity ceilings, tunable with RASTER_CACHE_MAX_BYTES and RASTER_CACHE_MAX_ENTRIES.
The listing parser now also captures each member's size.

Digest behavior is unchanged and already matches the intended policy: a present-but-wrong digest always fails the restore, while a legacy archive with no sidecar restores with a warning unless RASTER_CACHE_REQUIRE_DIGEST is set. Flipping missing-digest to fail-by-default would brick existing digest-less caches, so it stays opt-in.

How it was tested

make lint clean (actionlint/yq/shellcheck absent locally and skipped)
make test green (84 tests, up from 78)
new behavior has a test: a real fifo and an over-cap archive are each refused with nothing written, plus unit coverage for the special-file check, both caps, and device-size parsing; existing poisoned-path, escaping-link, and digest tests still pass
e2e workflow green on a runner

CHANGELOG updated? yes

Closes #2
Closes #3

**What changed** Restore already confines archive members to the requested `path:` roots (#3) and verifies the sha256 sidecar before extracting (#2). This closes the remaining gaps in the same pre-extraction pass, so a poisoned cache object cannot slip past it through a payload that is in-bounds but still hostile. - Refuse any special-file entry: character/block device, fifo, or socket. A dependency or build cache never contains one; a poisoned object could carry one to plant a node in the cache tree. - Cap the restore by declared total size and entry count, both read from the `tar -tvP` listing before any byte is written, so a single object cannot exhaust the runner's disk (decompression bomb) or inodes (entry flood). Defaults are generous sanity ceilings, tunable with `RASTER_CACHE_MAX_BYTES` and `RASTER_CACHE_MAX_ENTRIES`. - The listing parser now also captures each member's size. Digest behavior is unchanged and already matches the intended policy: a present-but-wrong digest always fails the restore, while a legacy archive with no sidecar restores with a warning unless `RASTER_CACHE_REQUIRE_DIGEST` is set. Flipping missing-digest to fail-by-default would brick existing digest-less caches, so it stays opt-in. **How it was tested** - [x] `make lint` clean (actionlint/yq/shellcheck absent locally and skipped) - [x] `make test` green (84 tests, up from 78) - [x] new behavior has a test: a real fifo and an over-cap archive are each refused with nothing written, plus unit coverage for the special-file check, both caps, and device-size parsing; existing poisoned-path, escaping-link, and digest tests still pass - [ ] e2e workflow green on a runner **CHANGELOG updated?** yes Closes #2 Closes #3

stephen added 1 commit

2026-06-17 18:18:49 +00:00

Reject special files and cap restore size (#2 , #3 )

test / unit (pull_request) Successful in 27s

Details

test / e2e (pull_request) Successful in 6s

Details

test / e2e-post-save (pull_request) Successful in 16s

Details

601b7d764d

Restore already confines archive members to the requested path roots and
verifies the sha256 sidecar before extracting. This closes the remaining
gaps in the same pre-extraction pass so a poisoned object cannot get past
it another way:

- Refuse any special-file entry (character/block device, fifo, socket).
  A dependency or build cache never contains one; a poisoned object could
  carry one to plant a node in the cache tree.
- Cap the restore by declared size and entry count, both read from the
  archive listing before any byte is written, so a single object cannot
  exhaust the runner's disk (decompression bomb) or inodes (entry flood).
  Defaults are generous sanity ceilings, tunable with RASTER_CACHE_MAX_BYTES
  and RASTER_CACHE_MAX_ENTRIES.

The listing parser now also captures each member's size. Digest behavior is
unchanged and already matches the intended policy: a present-but-wrong digest
always fails the restore, while a legacy archive with no sidecar restores with
a warning unless RASTER_CACHE_REQUIRE_DIGEST is set.

Tests cover a special file (real fifo) and an over-cap archive being refused
with nothing written, plus unit coverage for the new checks; the existing
poisoned-path, escaping-link, and digest tests still pass.

stephen commented

2026-06-17 18:23:33 +00:00

Author

Owner

Non-author review: APPROVE (do not self-merge)

Reviewed feat/verified-confined-restore @ 601b7d7. CI is green (test/unit, test/e2e, test/e2e-post-save all success). Real diff is src/lib/tar.js + tests/restore-confine.test.js (plus CHANGELOG/README/SECURITY). I ran the suite locally: 84/84 pass.

1. Special-file reject + size/count caps confine and refuse hostile archives; benign still restores

All four vetting passes run on the tar -tvP listing in extractArchive (src/lib/tar.js:262-265) before a single byte is written, first failure aborts:

assertNoSpecialFiles (tar.js:192-200) rejects char/block device, fifo, socket (c/b/p/s); files, dirs, symlinks, hardlinks pass.
assertWithinCaps (tar.js:236-245) caps entry count and summed declared size, both read from the listing pre-extraction, so an inode flood or a decompression bomb is refused before writing. Defaults are generous (1e6 entries / 100 GiB), overridable via RASTER_CACHE_MAX_ENTRIES / RASTER_CACHE_MAX_BYTES; an unset/invalid value falls back to the safe default (capFromEnv, tar.js:213-217).
assertConfined (absolute-path / .. / escaping-link refusal from #3/#6) is still invoked.

The size column is parsed by widening ENTRY_RE with a capture group and parseSize (tar.js:101-105), which folds a device node's major,minor to 0 (those are rejected anyway). Tests prove refusal AND that nothing is written: fifo end-to-end (restore-confine.test.js:167), over-cap via extractArchive asserting !existsSync (:218), plus the kept poisoned-path/escaping-link tests (:74/:88/:106/:145). Benign path verified by normal archive restores its files under the requested root (:61).

2. (CRITICAL) Digest policy is correct and was NOT flipped

This PR touches no digest/restore source (only tests/restore-confine.test.js among that set), so the #7 policy is inherited unchanged. Confirmed in verifyDigest (src/lib/restore-impl.js:134-158):

Sidecar present and mismatched -> hard throw "Refusing to extract a corrupted or tampered cache object" (:138-143).
Sidecar missing -> core.warning and restore proceeds (:153-157), so legacy digest-less caches still restore.
Strict-on-missing only when opted in: requireDigest throws (:147-152), and the default is false (src/lib/config.js:145, bool(RASTER_CACHE_REQUIRE_DIGEST, false)).
Missing-digest is still warn-by-default, not fail-by-default. Covered by tests: tampered refused + nothing extracted (restore-digest.test.js:116), missing warns by default (:145), missing + RASTER_CACHE_REQUIRE_DIGEST refuses (:172).

3. No regression to normal save/restore

The new checks only reject hostile archives; a normal archive passes all four passes and the round-trip + benign tests are green (84/84 local, CI green). extractArchive keeps its signature with caps defaulted, so existing callers are unaffected.

4. Tests are real (poisoned / over-cap / mismatch / missing)

Behavioral, not happy-path: they build real archives (a real mkfifo, a real over-cap tar) and assert both the refusal and that nothing landed on disk; the digest tests cover tamper, missing-warn, and missing-strict.

Non-blocking note

The byte/inode caps bound what a restore can write; listing a poisoned object still streams the full decompression through tar -tv to enumerate (CPU/time, no disk). That is the pre-existing listing approach from #3/#7, not introduced here, and the new caps correctly protect the write side (disk + inodes). Worth a future ceiling on listing work if compressed-bomb CPU ever matters, but not in scope for this PR.

Verdict: APPROVE. All four properties verified against the code and the test suite. Leaving the merge to you.

## Non-author review: APPROVE (do not self-merge) Reviewed `feat/verified-confined-restore` @ 601b7d7. CI is green (test/unit, test/e2e, test/e2e-post-save all success). Real diff is `src/lib/tar.js` + `tests/restore-confine.test.js` (plus CHANGELOG/README/SECURITY). I ran the suite locally: 84/84 pass. ### 1. Special-file reject + size/count caps confine and refuse hostile archives; benign still restores All four vetting passes run on the `tar -tvP` listing in `extractArchive` (`src/lib/tar.js:262-265`) before a single byte is written, first failure aborts: - `assertNoSpecialFiles` (`tar.js:192-200`) rejects char/block device, fifo, socket (`c/b/p/s`); files, dirs, symlinks, hardlinks pass. - `assertWithinCaps` (`tar.js:236-245`) caps entry count and summed declared size, both read from the listing pre-extraction, so an inode flood or a decompression bomb is refused before writing. Defaults are generous (1e6 entries / 100 GiB), overridable via `RASTER_CACHE_MAX_ENTRIES` / `RASTER_CACHE_MAX_BYTES`; an unset/invalid value falls back to the safe default (`capFromEnv`, `tar.js:213-217`). - `assertConfined` (absolute-path / `..` / escaping-link refusal from #3/#6) is still invoked. The size column is parsed by widening `ENTRY_RE` with a capture group and `parseSize` (`tar.js:101-105`), which folds a device node's `major,minor` to 0 (those are rejected anyway). Tests prove refusal AND that nothing is written: fifo end-to-end (`restore-confine.test.js:167`), over-cap via `extractArchive` asserting `!existsSync` (`:218`), plus the kept poisoned-path/escaping-link tests (`:74/:88/:106/:145`). Benign path verified by `normal archive restores its files under the requested root` (`:61`). ### 2. (CRITICAL) Digest policy is correct and was NOT flipped This PR touches no digest/restore source (only `tests/restore-confine.test.js` among that set), so the #7 policy is inherited unchanged. Confirmed in `verifyDigest` (`src/lib/restore-impl.js:134-158`): - Sidecar present and mismatched -> hard throw "Refusing to extract a corrupted or tampered cache object" (`:138-143`). - Sidecar missing -> `core.warning` and restore proceeds (`:153-157`), so legacy digest-less caches still restore. - Strict-on-missing only when opted in: `requireDigest` throws (`:147-152`), and the default is `false` (`src/lib/config.js:145`, `bool(RASTER_CACHE_REQUIRE_DIGEST, false)`). Missing-digest is still warn-by-default, not fail-by-default. Covered by tests: tampered refused + nothing extracted (`restore-digest.test.js:116`), missing warns by default (`:145`), missing + `RASTER_CACHE_REQUIRE_DIGEST` refuses (`:172`). ### 3. No regression to normal save/restore The new checks only reject hostile archives; a normal archive passes all four passes and the round-trip + benign tests are green (84/84 local, CI green). `extractArchive` keeps its signature with `caps` defaulted, so existing callers are unaffected. ### 4. Tests are real (poisoned / over-cap / mismatch / missing) Behavioral, not happy-path: they build real archives (a real `mkfifo`, a real over-cap tar) and assert both the refusal and that nothing landed on disk; the digest tests cover tamper, missing-warn, and missing-strict. ### Non-blocking note The byte/inode caps bound what a restore can write; listing a poisoned object still streams the full decompression through `tar -tv` to enumerate (CPU/time, no disk). That is the pre-existing listing approach from #3/#7, not introduced here, and the new caps correctly protect the write side (disk + inodes). Worth a future ceiling on listing work if compressed-bomb CPU ever matters, but not in scope for this PR. Verdict: APPROVE. All four properties verified against the code and the test suite. Leaving the merge to you.

stephen merged commit 949a48a2bf into main

2026-06-17 18:25:44 +00:00

stephen referenced this pull request from a commit

2026-06-17 18:25:45 +00:00

Reject special files and cap restore size (#2, #3) (#9)