diff --git a/.dev/features/tar-entry-names/GRILL.md b/.dev/features/tar-entry-names/GRILL.md new file mode 100644 index 0000000..d6ae791 --- /dev/null +++ b/.dev/features/tar-entry-names/GRILL.md @@ -0,0 +1,38 @@ +# GRILL — tar-entry-names + +Plan: `.dev/features/tar-entry-names/PLAN.md` · spec-hash `bca940a5…d729d3c4e` matches live +`ARCHITECTURE.md`. Registered grillers: `{"registered":0,"grillers":[]}` → inline axes only. + +## Findings + +```yaml +- type: FINDING + rule_id: 'P0' + severity: important + file: '.dev/features/tar-entry-names/PLAN.md:37' + problem: 'A `TextDecoder` built with only `{ fatal: true }` silently STRIPS a leading byte-order mark (measured: EF BB BF 61 decodes to "a"). A name whose field starts with U+FEFF, itself a Cf format character, would lose it before the check and be written under a different name than the archive holds: neither refused nor faithful. Build the decoder with `ignoreBOM: true` so the BOM stays in the string and the Cf check refuses it.' + evidence: '`readPathField` (strict `TextDecoder(''utf-8'', { fatal: true })`' +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/features/tar-entry-names/PLAN.md:41' + problem: 'The byte-exact claim should be asserted on BYTES: read the extracted directory with `readdirSync(dir, { encoding: ''buffer'' })` and compare against the archive bytes (e2 80 94 for the em dash), not the decoded string, which would pass even if both sides were decoded the same wrong way. Add a BOM-prefixed name to the refusal cases.' + evidence: 'valid UTF-8 `a—b.md` → written as exactly those bytes (FAILS on base)' +- type: FINDING + rule_id: 'P0' + severity: minor + file: '.dev/features/tar-entry-names/PLAN.md:62' + problem: '"on-disk name is its archive bytes decoded as UTF-8" holds for the name pharn passes to the filesystem; a filesystem that normalizes Unicode (HFS+ stores NFD) may store different bytes. Scope the claim to the name written, or label the on-disk half advisory for such filesystems.' + evidence: '"an extracted entry''s on-disk name is its archive bytes decoded as UTF-8' +``` + +## Summary + +The plan correctly moves the check onto the string that is written, which is the actual defect. +The one soundness gap is the decoder's default BOM handling: without `ignoreBOM: true` a leading U+FEFF +is removed before the check ever sees it, which recreates, for one character, exactly the check-vs-write +mismatch this increment exists to close. The pax budget and the escaped numeric message are sound as +planned (overlong forms and encoded surrogates are already refused by a fatal decoder — measured). + +ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 3 advisory — 1 important) — for the human to +weigh before /pharn-dev-build; all three fold into the build without changing the plan's Files. diff --git a/.dev/features/tar-entry-names/PLAN.md b/.dev/features/tar-entry-names/PLAN.md new file mode 100644 index 0000000..d24ea8a --- /dev/null +++ b/.dev/features/tar-entry-names/PLAN.md @@ -0,0 +1,80 @@ +# PLAN — tar-entry-names (judge the name that is written; bound pax work per archive, not per header) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: the extractor (1) decodes each entry's `prefix`/`name` fields ONCE, as strict UTF-8, + refuses a field that is not valid UTF-8, runs the control/format-character check on that decoded + string, and uses the SAME string for the path rules and the write; (2) bounds pax work per + ARCHIVE — a cumulative 64 KiB payload budget across every `g` header, and every `g` header counts + toward `maxEntries`; (3) renders a malformed numeric field without its raw bytes, so no newline or + control byte from a header reaches the fatal-error sink. +- layer(s): the CLI itself (`src/lib/tar-extract.ts`) +- constitution_refs: [P0, P1, P2, P5] + +## Discovery — verified this run (P6) + +- `readString` (`tar-extract.ts:100-104`) decodes header fields as latin1; the entry path is built + from it (`:442-445`) and written as that latin1 string (`:477-480`), which fs encodes as UTF-8. + `assertDisplayablePath` (`:206-217`, PHARN-17) checks a DIFFERENT string — the UTF-8 decoding of + the latin1 bytes. Reproduced by review (end-to-end with `extractTar`, then real `runStatus` / + `runUpdate` on a git-archive of pharn-oss): a raw `0x9B` byte decodes to U+FFFD for the check + (passes) and lands on disk as U+009B (bytes `c2 9b`, the C1 CSI); `status`'s MISSING list and + `update`'s SKIPPED list then print it raw. A legitimate UTF-8 name (`a—b.md`) lands as mojibake + (`a` U+00E2 U+0080 U+0094 `b`). The latin1 decoding predates the range (#153); PHARN-17's check + is the part that disagrees with the write. +- Every codeload entry in the live archive is ASCII (review: pharn-oss@b31e540 via `git archive`, + 2623 entries), so strict UTF-8 changes nothing for the real archive. +- pax (PHARN-18): `MAX_PAX_BYTES` (64 KiB) is checked per header (`:397`), `g` headers are unlimited + in number and skip the `entries`/`totalBytes` accounting (`:421-438` vs `:459-471`). Measured by + review on node 22: 2030 `g` headers × 64 KiB (a 288 KiB .tgz) → accepted after 8.9 s CPU / 375 MB + RSS, vs 11.5 s / 906 MB for the single-header archive PHARN-18 fixed. Real archives carry exactly + one `g` (`comment=`, ~52 bytes). `MAX_ENTRIES` is 20 000 (`repo.ts:21`). +- `readOctal` (`:112-126`) interpolates the raw field text; `logError` keeps `\n`, so a size field + `1\n Done` prints an attacker-chosen second line on the fatal path (reproduced by review). The + checksum field reaches the same `readOctal`. + +## Files + +- `src/lib/tar-extract.ts` — `readPathField` (strict `TextDecoder('utf-8', { fatal: true })`, refusal + message renders the bytes as printable ASCII + `\xNN` escapes, capped); `assertDisplayablePath` runs + on the decoded path with no re-decoding; the decoded path feeds `resolveEntryPath` and the write; + a per-archive `paxBytes` budget checked BEFORE `readPaxKeywords`; `g` headers increment `entries`; + `readOctal`'s message shows the field via the same escaped rendering — layer CLI/lib +- `tests/tar-extract.test.ts` — raw `0x9B` in `name` → refused, nothing written (FAILS on base); + invalid UTF-8 in `prefix` → refused; valid UTF-8 `a—b.md` → written as exactly those bytes (FAILS + on base); valid-UTF-8 U+009B and U+202E still refused; the refusal message holds no C0/C1 byte; + two `g` headers each under 64 KiB but over it together → refused (FAILS on base); `maxEntries`+1 + empty `g` headers → refused (FAILS on base); a numeric field containing `\n` → message has no + newline (FAILS on base); the codeload-shaped fixture still extracts +- `CHANGELOG.md` — `[Unreleased]` → `### Security` entry + +## Contracts satisfied + +- `THREAT-MODEL.md` §2 surface 7 / §3 "malformed / hostile archive entry" and PHARN-17's + "no such name is installed or printed" — now true of the bytes written (cited, P4). + +## Evals to write (P1) + +- listed under Files. + +## Guarantee audit (P0) + +- "an extracted entry's on-disk name is its archive bytes decoded as UTF-8 and contains no C0/C1/Cf + character" → floor: fatal UTF-8 decode + the `hasUnsafeChars` regex over the exact string written. +- "pax parsing costs O(64 KiB) per archive" → floor: cumulative size compare before any parse. +- "every header counts toward `maxEntries`" → floor: counter + compare. +- "no header byte reaches a message raw" → floor: the escaped rendering is total over bytes + (printable ASCII 0x20-0x7E kept, everything else `\xNN`). + +## Trust audit (P2) + +- Only narrows what an untrusted archive may contain; untrusted bytes reach messages only in the + escaped rendering. + +## Determinism audit (P5) + +- Byte/regex checks and integer compares; no fallback decoding (invalid UTF-8 is a refusal, not a + replacement character). + +## Open questions (HALT) + +- none diff --git a/.dev/features/tar-entry-names/REGRESSION.md b/.dev/features/tar-entry-names/REGRESSION.md new file mode 100644 index 0000000..d2941cd --- /dev/null +++ b/.dev/features/tar-entry-names/REGRESSION.md @@ -0,0 +1,36 @@ +# REGRESSION — tar-entry-names + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `431f02b59c39dbbcda028b4291fbc92d877162f6` (`HEAD` — `origin/main` after #217; the build is an + uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): `src/lib/tar-extract.ts`, `tests/tar-extract.test.ts`, + `CHANGELOG.md`. +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []`. `.pharn/` (hook scratch) and + this feature's own stage artifacts are not build output. +- **outside gates:** the 46 stdlib `*.test.mjs` / `*.test.cjs` files `scope` returned (754 tests) + + whole-repo `validate`; 0 committed eval pairs. +- **style-gate skip:** `inside` touches no shared style config, so `lint` / `format:check` / `lint:md` are + absent from both maps. +- **environment:** both sides ran with no proxy variables and as root **without** `CAP_DAC_OVERRIDE` / + `CAP_DAC_READ_SEARCH` / `CAP_FOWNER` (`setpriv`), the CI-equivalent of this root sandbox. + +## Per-gate exit codes + +| gate | base | head | flipped? | +| ---------- | ---- | ---- | -------- | +| `tests` | 0 | 0 | no | +| `validate` | 0 | 0 | no | + +- `regressions[]`: **empty** +- `pre_existing[]`: **empty** + +## Verdict + +**REGRESSIONS: none — no deterministically-detectable breakage outside the feature.** +(`regression-report.json` `.verdict` = `no-regressions`.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising `src/**` is +owned by `/pharn-dev-build`'s floor and `/pharn-dev-verify`. This certifies the comparison, never the increment. diff --git a/.dev/features/tar-entry-names/REVIEW.md b/.dev/features/tar-entry-names/REVIEW.md new file mode 100644 index 0000000..a0a159d --- /dev/null +++ b/.dev/features/tar-entry-names/REVIEW.md @@ -0,0 +1,50 @@ +# REVIEW — tar-entry-names + +Increment: `src/lib/tar-extract.ts` (`readPathField` — strict UTF-8 with `ignoreBOM`; the check, the path +rules and the write share the decoded string; a per-archive pax budget; global headers counted toward the +entry cap; `describeBytes` for header bytes in messages), `tests/tar-extract.test.ts` (nine cases), +`CHANGELOG.md` (`### Security`). Treated as `trust: untrusted`; nothing in it read as an instruction. + +## Floor first (P0) + +`node .dev/floor/validate.mjs .` → `FLOOR: GREEN` (exit 0). `/pharn-dev-build`'s `npm run check` exit 0 +(1507 tests), `/pharn-dev-regress` `no-regressions`, `/pharn-dev-verify` `PASS`. + +## Floor-gate findings (blocking) + +None. + +- L-floor (P0): "the name passed to the filesystem holds no C0/C1/Cf character and is the archive's bytes + decoded as UTF-8" → a fatal decode plus the `hasUnsafeChars` regex over the exact string written; the + on-disk half is scoped to the name written (a Unicode-normalizing filesystem is named in VERIFY.md). + "Pax parsing costs O(64 KiB) per archive" → a cumulative compare before any parse. "Every global header + counts toward the entry cap" → counter + compare. "No header byte reaches a message raw" → + `describeBytes` is total over bytes; every other message interpolates only a path that already passed + the check. +- L-eval (P1): eight of the nine new cases fail on the base `tar-extract.ts` (checked by stashing it); + the ninth (a leading byte-order mark) passes on both by design — it pins the decoder option the grill + found. The byte-exact case asserts on the BYTES `readdirSync` returns. A git-archive of pharn-oss + extracts to the same 2208 files / 414 directories as before. +- L-trust (P2): the increment only narrows what an untrusted archive may contain. +- L-axis (P3): everything stays in the fetch boundary's own module. + +## Advisory findings (warn — severity is this reviewer's judgment, fix #3) + +```yaml +- type: FINDING + rule_id: 'P3' + severity: minor + file: 'src/lib/tar-extract.ts:106' + problem: '`describeBytes` is a second display renderer beside terminal-safe.ts, which PHARN-17 called the one display sanitizer. The input differs (raw bytes that may not decode, vs a string), which justifies a byte-level form; if a second caller needs it, it belongs in terminal-safe.ts.' + evidence: 'function describeBytes(bytes: Buffer, max = 200): string {' +- type: FINDING + rule_id: 'P7' + severity: minor + file: 'src/lib/tar-extract.ts:137' + problem: 'One non-UTF-8 name refuses the whole archive — the extractor''s reject-don''t-skip posture, but upstream main is a live input to every deployed CLI (LIMITS.md §3e), so a single such file upstream would fail every install until reverted. pharn-oss ships only ASCII names today; named, not changed.' + evidence: 'tar entry path is not valid UTF-8' +``` + +## Verdict + +**GREEN — 0 floor-gate findings, 2 advisory (minor).** The standing decision is the human's (GATE 2). diff --git a/.dev/features/tar-entry-names/SHIP.md b/.dev/features/tar-entry-names/SHIP.md new file mode 100644 index 0000000..14729c3 --- /dev/null +++ b/.dev/features/tar-entry-names/SHIP.md @@ -0,0 +1,18 @@ +# SHIP — tar-entry-names + +Stages run, in order: `/pharn-dev-plan` → GATE 1 (human: all four plans of this batch accepted — "deliver +all of them one by one using pharn-dev-ship") → `/pharn-dev-grill` → `/pharn-dev-build` → +`/pharn-dev-regress` → `/pharn-dev-verify` → `/pharn-dev-review` → GATE 2. + +| stage | structural verdict (verbatim) | +| -------------------- | ------------------------------------------------------ | +| `/pharn-dev-build` | `node .dev/floor/validate.mjs .` exit `0` | +| `/pharn-dev-regress` | `regression-report.json` `.verdict` = `no-regressions` | +| `/pharn-dev-verify` | `verify-report.json` `.verdict` = `PASS` | + +- Review: [`REVIEW.md`](REVIEW.md) · Grill (advisory): [`GRILL.md`](GRILL.md) +- Run ended at **GATE 2**. The human's standing instruction for this batch: after each increment, open a + pull request and merge it once its checks are green, then start the next plan. + +chain ran; the named floor verdicts are as shown — this is NOT a judgment that the increment is good or +wise; that is the human's call at the post-review gate. diff --git a/.dev/features/tar-entry-names/VERIFY.md b/.dev/features/tar-entry-names/VERIFY.md new file mode 100644 index 0000000..2974975 --- /dev/null +++ b/.dev/features/tar-entry-names/VERIFY.md @@ -0,0 +1,33 @@ +# VERIFY — tar-entry-names + +## FLOOR layer (owns the verdict) + +Gates run over the whole repo with the feature present, on node 22 with the session proxy variables unset +and as root **without** `CAP_DAC_OVERRIDE` / `CAP_DAC_READ_SEARCH` / `CAP_FOWNER` (`setpriv`) — the +CI-equivalent of this root sandbox. + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `test:floor` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +`test` is vitest (1507 tests), which collects this increment's own tests (`tests/tar-extract.test.ts`). +`test:floor` is floor.yml's `node --test` run (754 tests). No `structural:*` gate — the increment ships no +eval-actual pair. Outside the verdict, a git-archive of pharn-oss@b31e540 (the review's real-archive +fixture) was extracted with the new code: 2208 files and 414 directories, the same as before. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}` — no verifiers registered, floor +gates only. + +Residual (P0/P7): verified = the named gates passed; this is NOT a guarantee of correctness beyond what those +gates check — verifier concerns are advisory help, not assurance. The byte-exact name holds for the name +pharn passes to the filesystem; a filesystem that normalizes Unicode (HFS+) may store other bytes. diff --git a/.dev/features/tar-entry-names/regression-report.json b/.dev/features/tar-entry-names/regression-report.json new file mode 100644 index 0000000..3c9107e --- /dev/null +++ b/.dev/features/tar-entry-names/regression-report.json @@ -0,0 +1,21 @@ +{ + "base": "431f02b59c39dbbcda028b4291fbc92d877162f6", + "inside": [ + "CHANGELOG.md", + "src/lib/tar-extract.ts", + "tests/tar-extract.test.ts" + ], + "outside_gates": { + "tests": { + "base": 0, + "head": 0 + }, + "validate": { + "base": 0, + "head": 0 + } + }, + "regressions": [], + "pre_existing": [], + "verdict": "no-regressions" +} diff --git a/.dev/features/tar-entry-names/verify-report.json b/.dev/features/tar-entry-names/verify-report.json new file mode 100644 index 0000000..be50e14 --- /dev/null +++ b/.dev/features/tar-entry-names/verify-report.json @@ -0,0 +1,18 @@ +{ + "feature": "tar-entry-names", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 0, + "test:floor": 0, + "typecheck": 0, + "validate": 0 + }, + "verdict": "PASS", + "failing_gates": [], + "verifiers": { + "registered": 0, + "findings": [] + } +} diff --git a/.pharn/writes-scope.json b/.pharn/writes-scope.json index bde7b08..9c0d566 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/update-frozen-recheck/SHIP.md" + ".dev/features/tar-entry-names/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-24T19:24:04.227Z" + "set_at": "2026-09-24T19:32:56.865Z" } diff --git a/CHANGELOG.md b/CHANGELOG.md index 23a94a7..d6c0c89 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -19,6 +19,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **A re-run `pharn init` dropped the `pharn.config.json` keys you added by hand.** Upstream PHARN reads top-level keys that users add themselves: `testResults` (without it `/pharn-loop` stops with `blocked: no-test-runner`) and `ship.requireAttestation`. `add`, `update` and `remove` kept them, but `init` rebuilt the config from its own fields alone. It now copies every top-level key pharn does not own across from the config it replaces, unchanged. It does so from any file that parses as a JSON object, including one the other commands refuse. Keys pharn owns are still written fresh. - **`pharn update` could stop re-checking a capability whose files were still out of date.** When pharn cannot read a capability upstream, `update` keeps it, skips its files, and moves the skills version on. Once pharn could read it again, the next run removed it from `frozenCapabilities` even if one of its files had to be skipped, for example because you edited it. Every later run then said "Already up to date", and that file stayed at the old version even after you resolved your edit. The capability now stays listed until none of its files are skipped, so each run checks it again. `update` also no longer writes a `pendingSkillsVersion` equal to the recorded `skillsVersion`. +### Security + +- **The extractor's name check now judges the name it writes.** It checked one decoding of a tar entry's name and wrote another. A raw C1 byte (such as `0x9B`, the terminal CSI) passed the control-character check and landed on disk as that control character, where `pharn status` and `pharn update` then printed it raw. Every non-ASCII name was also written garbled. Entry names are now decoded once, as strict UTF-8, and the check, the path rules and the write all use that one string. A name that is not valid UTF-8, or that starts with a byte-order mark, is refused. +- **Pax parsing is bounded per archive.** The 64 KiB cap applied to each global header, not to their number, so many headers each under the cap still cost seconds of CPU. The cap now covers all global headers in the archive together, and every header counts toward the entry limit. +- **A malformed numeric header field no longer reaches the error output raw.** It is shown with non-printable bytes escaped, so an archive cannot add a line of its own to the fatal message. + ## [0.5.0] - 2026-09-10 ### Changed — BREAKING diff --git a/src/lib/tar-extract.ts b/src/lib/tar-extract.ts index 38f9f28..16484d9 100644 --- a/src/lib/tar-extract.ts +++ b/src/lib/tar-extract.ts @@ -103,6 +103,45 @@ function readString(block: Buffer, offset: number, length: number): string { return raw.subarray(0, end === -1 ? raw.length : end).toString('latin1'); } +/** + * Header bytes for a message, and only for a message: printable ASCII is kept, + * every other byte becomes `\xNN`, and the result is capped. Total over bytes, + * so no header byte (a newline, an ESC, a C1 control) reaches a terminal raw. + */ +function describeBytes(bytes: Buffer, max = 200): string { + let out = ''; + for (const byte of bytes.subarray(0, max)) { + out += + byte >= 0x20 && byte <= 0x7e && byte !== 0x5c // printable, not `\` + ? String.fromCharCode(byte) + : `\\x${byte.toString(16).padStart(2, '0')}`; + } + return bytes.length > max ? `${out}…` : out; +} + +/** + * Entry-path fields are decoded ONCE, and the check and the write both use the + * result. `fatal`: bytes that are not valid UTF-8 are a refusal, never a + * replacement character. `ignoreBOM`: a leading U+FEFF stays IN the string — + * the default strips it silently — so the format-character check below sees, + * and refuses, exactly the name that would otherwise be written. + */ +const PATH_DECODER = new TextDecoder('utf-8', { fatal: true, ignoreBOM: true }); + +/** A NUL-terminated (or field-filling) entry-path field, as strict UTF-8. */ +function readPathField(block: Buffer, offset: number, length: number): string { + const raw = block.subarray(offset, offset + length); + const end = raw.indexOf(0); + const bytes = raw.subarray(0, end === -1 ? raw.length : end); + try { + return PATH_DECODER.decode(bytes); + } catch { + throw new TarExtractError( + `tar entry path is not valid UTF-8: "${describeBytes(bytes)}"`, + ); + } +} + /** * A ustar numeric field: octal digits, space- or NUL-terminated. GNU's base-256 * extension (high bit set on the first byte) is REJECTED rather than @@ -120,7 +159,10 @@ function readOctal(block: Buffer, offset: number, length: number): number { if (text === '') return 0; if (!/^[0-7]+$/.test(text)) { throw new TarExtractError( - `tar header has a non-octal numeric field: ${text}`, + `tar header has a non-octal numeric field: "${describeBytes( + Buffer.from(text, 'latin1'), + 32, + )}"`, ); } return parseInt(text, 8); @@ -196,20 +238,19 @@ function describeKeywords(keywords: string[] | null): string { /** * Refuse an entry path holding a control character or a Unicode format - * character (U+202E, U+200B, …). Such a name would be printed later — by an - * error here, or by `status`/`update` listing installed files — and a terminal - * interprets those characters. Names are read as latin1 (one char per byte), so - * the check runs on the UTF-8 decoding of the same bytes: that is where a - * multi-byte U+202E becomes visible, while an ordinary non-ASCII name (`é`, - * `—`) decodes to printable text and passes. pharn-oss ships no such name. + * character (U+202E, U+200B, U+FEFF, …). Such a name would be printed later — + * by an error here, or by `status`/`update` listing installed files — and a + * terminal interprets those characters. `fullPath` is the strict UTF-8 decoding + * (`readPathField`) that the path rules and the write also use, so what is + * judged here is exactly the name passed to the filesystem: an ordinary + * non-ASCII name (`é`, `—`) passes and is written as those same bytes, while a + * C1 byte never survives the decode. pharn-oss ships no non-ASCII name today. */ function assertDisplayablePath(fullPath: string): void { - if (hasUnsafeChars(Buffer.from(fullPath, 'latin1').toString('utf8'))) { + if (hasUnsafeChars(fullPath)) { throw new TarExtractError( `tar entry path contains a control or format character: ${JSON.stringify( - terminalSafe(Buffer.from(fullPath, 'latin1').toString('utf8'), { - max: 200, - }), + terminalSafe(fullPath, { max: 200 }), )}`, ); } @@ -355,6 +396,10 @@ export function extractTar( let offset = 0; let entries = 0; let totalBytes = 0; + // Pax payload bytes parsed so far, across EVERY global header. The per-header + // cap alone bounded one payload, not their number: 2030 headers of 64 KiB each + // still cost seconds of CPU. A real archive carries one, of ~52 bytes. + let paxBytes = 0; let expectedRoot: string | null = null; while (offset + BLOCK <= tar.length) { @@ -419,6 +464,20 @@ export function extractTar( // what is judged, because a global record is a default for every entry that // follows it. if (typeflag === 'g') { + // Every header counts toward the entry cap, a global one included, and the + // payload toward the per-archive pax budget — both before any parse. + entries += 1; + if (entries > limits.maxEntries) { + throw new TarExtractError( + `tar archive has more than ${limits.maxEntries} entries.`, + ); + } + paxBytes += size; + if (paxBytes > MAX_PAX_BYTES) { + throw new TarExtractError( + `tar archive's pax global headers total ${paxBytes} bytes, over the ${MAX_PAX_BYTES}-byte limit pharn accepts.`, + ); + } const keywords = readPaxKeywords(tar.subarray(dataStart, dataEnd)); if (keywords === null) { throw new TarExtractError( @@ -439,9 +498,10 @@ export function extractTar( } // The FULL path (prefix + name) is judged before any other entry rule, so - // no later message can interpolate a control or format character from it. - const name = readString(header, OFF_NAME, LEN_NAME); - const prefix = readString(header, OFF_PREFIX, LEN_PREFIX); + // no later message can interpolate a control or format character from it — + // and it is the same decoded string the path rules and the write use. + const name = readPathField(header, OFF_NAME, LEN_NAME); + const prefix = readPathField(header, OFF_PREFIX, LEN_PREFIX); const fullPath = prefix === '' ? name : `${prefix}/${name}`; assertDisplayablePath(fullPath); diff --git a/tests/tar-extract.test.ts b/tests/tar-extract.test.ts index 842f292..bf8cfc4 100644 --- a/tests/tar-extract.test.ts +++ b/tests/tar-extract.test.ts @@ -1,4 +1,4 @@ -import { existsSync, readFileSync } from 'node:fs'; +import { existsSync, readFileSync, readdirSync } from 'node:fs'; import { join } from 'node:path'; import { gzipSync } from 'node:zlib'; import { describe, expect, it } from 'vitest'; @@ -590,6 +590,137 @@ describe('extractTar', () => { ); }); + // tar-entry-names. The check used to judge a DIFFERENT string than the one + // written — the UTF-8 decoding of latin1-read bytes, against the latin1 string + // itself — so a raw C1 byte passed as U+FFFD and landed on disk as U+009B, and + // every non-ASCII name landed garbled. Names are now decoded once, strictly, + // and the check, the path rules and the write share that one string. Asserted + // on the BYTES the filesystem returns, not on a decoded string. + const onDisk = (dir: string): Buffer[] => + readdirSync(dir, { encoding: 'buffer' }); + + it('writes a non-ASCII name as exactly the archive bytes', () => { + const dest = tmp.path(); + extractTar( + githubArchive([ + { name: utf8AsLatin1('pharn-oss-abc1234/a—b.md'), data: 'ok' }, + ]), + dest, + LIMITS, + ); + expect(onDisk(dest)).toEqual([Buffer.from('a—b.md', 'utf8')]); + }); + + it.each([ + ['a raw C1 byte', 'pharn-oss-abc1234/evil-\x9b2J.md'], + ['a truncated UTF-8 sequence', 'pharn-oss-abc1234/bad-\xe2\x80.md'], + ['an overlong encoding of "/"', 'pharn-oss-abc1234/slash-\xc0\xaf.md'], + ])( + 'refuses a name holding %s — not valid UTF-8 — writing nothing', + (_label, name) => { + const dest = tmp.path(); + let message = ''; + try { + extractTar(githubArchive([{ name, data: 'x' }]), dest, LIMITS); + } catch (err) { + expect(err).toBeInstanceOf(TarExtractError); + message = (err as Error).message; + } + expect(message).toMatch(/not valid UTF-8/); + expect(hasUnsafeChars(message)).toBe(false); + expect(onDisk(dest)).toEqual([]); + }, + ); + + it('refuses invalid UTF-8 in the ustar prefix too', () => { + expect(() => + extractTar( + githubArchive([{ name: 'f.md', prefix: 'pharn-oss-abc1234/d\x9b' }]), + tmp.path(), + LIMITS, + ), + ).toThrow(/not valid UTF-8/); + }); + + // A decoder's DEFAULT drops a leading byte-order mark: the name would be + // written under a different name than the archive holds instead of refused. + it('refuses a name field that starts with a byte-order mark, never strips it', () => { + const dest = tmp.path(); + expect(() => + extractTar( + githubArchive([ + { + name: utf8AsLatin1('f.md'), + prefix: 'pharn-oss-abc1234', + data: 'x', + }, + ]), + dest, + LIMITS, + ), + ).toThrow(/control or format character/); + expect(onDisk(dest)).toEqual([]); + }); + + // PHARN-18 capped one pax payload, not their number: 2030 global headers of + // 64 KiB each still cost seconds of CPU. The budget is per ARCHIVE now, and + // every header counts toward the entry cap. + it('refuses global headers each under the pax cap but over it together', () => { + const g: EntryInit = { + name: 'pax_global_header', + type: 'g', + data: paxRecord('comment', 'x'.repeat(40 * 1024)), + }; + expect(() => + extractTar( + tar([g, g, { name: 'pharn-oss-abc1234/', type: '5' }]), + tmp.path(), + LIMITS, + ), + ).toThrow(/pax global headers total \d+ bytes, over the 65536-byte limit/); + }); + + it('counts every pax global header toward the entry cap', () => { + const empty: EntryInit = { name: 'pax_global_header', type: 'g', data: '' }; + expect(() => + extractTar( + tar([ + empty, + empty, + empty, + empty, + { name: 'pharn-oss-abc1234/', type: '5' }, + ]), + tmp.path(), + { maxEntries: 3, maxTotalBytes: 1024 }, + ), + ).toThrow(/more than 3 entries/); + }); + + it('names a malformed numeric field escaped — no raw newline reaches the message', () => { + const bad = header({ name: 'pharn-oss-abc1234/f.txt' }); + bad.fill(0, 124, 136); // the size field + bad.write('1\n Done', 124, 12, 'latin1'); + bad.write(' ', 148, 8, 'latin1'); // re-checksum over the forged field + let sum = 0; + for (const byte of bad) sum += byte; + bad.write(sum.toString(8).padStart(6, '0') + '\0 ', 148, 8, 'latin1'); + + let message = ''; + try { + extractTar( + Buffer.concat([bad, Buffer.alloc(BLOCK * 2)]), + tmp.path(), + LIMITS, + ); + } catch (err) { + message = (err as Error).message; + } + expect(message).toMatch(/non-octal numeric field/); + expect(message).not.toContain('\n'); + expect(message).toContain('1\\x0a Done'); + }); + // PHARN-18: strict framing and bounded pax parsing. Each case was ACCEPTED // by the base source (or, for the pax payload, parsed at full size). const refusal = (archive: Buffer): string => {