From 7154e11f68a7cf524eae786e00962dd932d93a4a Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 10:59:46 +0000 Subject: [PATCH] fix(output): upstream-derived text never reaches the terminal raw (PHARN-17) Extractor errors interpolated raw entry names and typeflag bytes, which reportFatal printed to stderr (fake "OK", line erase, OSC 52), and the unknown-capability list let Unicode format characters (U+202E) through. - tar-extract refuses an entry path holding a C0/C1 or \p{Cf} character, judged first, and renders names/typeflags safely in its messages - new lib/terminal-safe.ts: the one display sanitizer, now also used by unknown-capabilities - logError (the fatal sink) strips the same set, keeping \n and \t Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TvcuVhk8hTeDskp5pAJhnc --- .dev/features/terminal-safe-text/GRILL.md | 29 ++++++++ .dev/features/terminal-safe-text/PLAN.md | 60 ++++++++++++++++ .../features/terminal-safe-text/REGRESSION.md | 30 ++++++++ .dev/features/terminal-safe-text/REVIEW.md | 49 +++++++++++++ .dev/features/terminal-safe-text/SHIP.md | 17 +++++ .dev/features/terminal-safe-text/VERIFY.md | 27 +++++++ .../terminal-safe-text/regression-report.json | 26 +++++++ .../terminal-safe-text/verify-report.json | 17 +++++ .pharn/writes-scope.json | 4 +- src/lib/report-error.ts | 12 +++- src/lib/tar-extract.ts | 41 +++++++++-- src/lib/terminal-safe.ts | 46 ++++++++++++ src/lib/unknown-capabilities.ts | 10 ++- tests/report-error.test.ts | 14 ++++ tests/tar-extract.test.ts | 72 +++++++++++++++++++ tests/terminal-safe.test.ts | 49 +++++++++++++ tests/unknown-capabilities.test.ts | 11 +++ 17 files changed, 499 insertions(+), 15 deletions(-) create mode 100644 .dev/features/terminal-safe-text/GRILL.md create mode 100644 .dev/features/terminal-safe-text/PLAN.md create mode 100644 .dev/features/terminal-safe-text/REGRESSION.md create mode 100644 .dev/features/terminal-safe-text/REVIEW.md create mode 100644 .dev/features/terminal-safe-text/SHIP.md create mode 100644 .dev/features/terminal-safe-text/VERIFY.md create mode 100644 .dev/features/terminal-safe-text/regression-report.json create mode 100644 .dev/features/terminal-safe-text/verify-report.json create mode 100644 src/lib/terminal-safe.ts create mode 100644 tests/terminal-safe.test.ts diff --git a/.dev/features/terminal-safe-text/GRILL.md b/.dev/features/terminal-safe-text/GRILL.md new file mode 100644 index 00000000..792b57ac --- /dev/null +++ b/.dev/features/terminal-safe-text/GRILL.md @@ -0,0 +1,29 @@ +# GRILL — terminal-safe-text + +Plan: `.dev/features/terminal-safe-text/PLAN.md` · spec-hash `bca940a5…d729d3c4e` matches live +`ARCHITECTURE.md`. Registered grillers: `{"registered":0,"grillers":[]}` → inline axes only. + +## Findings + +```yaml +- type: FINDING + rule_id: 'P2' + severity: important + file: '.dev/features/terminal-safe-text/PLAN.md:8' + problem: 'Keeping `\n` in the sink still lets a hostile name start a new line that LOOKS like a separate pharn message; acceptable (no terminal control), but the extractor refusal must itself render the name with newlines stripped so the refusal line is one line.' + evidence: '`sanitizes its whole message (keeping \n/\t)`' +- type: FINDING + rule_id: 'P5' + severity: minor + file: '.dev/features/terminal-safe-text/PLAN.md:12' + problem: 'Check the FULL reconstructed path (prefix + name) before any other rule, so a control byte in the ustar `prefix` field is refused too, and before the typeflag check so an unsupported-type entry is refused by path first when both apply — or render both safely; either way no raw byte.' + evidence: '`REFUSES a tar entry whose path contains …`' +- type: FINDING + rule_id: 'P1' + severity: minor + file: '.dev/features/terminal-safe-text/PLAN.md:31' + problem: 'The sink test must spy on the real stream write (stderr), not on the argument to logError, or it passes by construction.' + evidence: '`an ESC sequence in a fatal message never reaches stderr raw`' +``` + +ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 3 advisory) — all folded into the build. diff --git a/.dev/features/terminal-safe-text/PLAN.md b/.dev/features/terminal-safe-text/PLAN.md new file mode 100644 index 00000000..a466418e --- /dev/null +++ b/.dev/features/terminal-safe-text/PLAN.md @@ -0,0 +1,60 @@ +# PLAN — terminal-safe-text (PHARN-17: upstream-derived text never reaches the terminal raw) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: three layers. (1) The extractor REFUSES a tar entry whose path contains a C0/C1 control + character or a Unicode format character (`\p{Cf}`: U+202E, U+200B, …) — upstream has none (2161 paths + checked in the review), so nothing with such a name is ever installed or later printed by + `status`/`update`. (2) One shared sanitizer, `lib/terminal-safe.ts`, strips C0/C1 + `\p{Cf}` and caps + length; `unknown-capabilities.ts` uses it (gaining `\p{Cf}`), and every extractor message that + interpolates an entry name or typeflag renders it through it. (3) The fatal sink `logError` + (`report-error.ts`) sanitizes its whole message (keeping `\n`/`\t`), so no fatal path — present or + future — can print a raw escape sequence. +- layer(s): the CLI itself (`src/lib/*`) +- constitution_refs: [P1, P2, P3, P5] + +## Discovery — verified this run (P6) + +Review repro: `unsupported type '2': pharn-oss-…/n^[[2K^M^[[32mOK` printed by `reportFatal` in +`init`/`status` (a faked "OK", line erase, OSC 52 clipboard write possible). `unknown-capabilities.ts` +strips `[\x00-\x1f\x7f-\x9f]` but not U+202E/U+200B. `resolveEntryPath` (`tar-extract.ts`) checks `..`, +absolute, root only. `logError` passes messages verbatim; no caller passes colored text (grep: 31 call +sites, none with `pc.`/ANSI). + +## Files + +- `src/lib/terminal-safe.ts` — NEW: `terminalSafe(value, opts)` (strip C0/C1 + `\p{Cf}`, optional keep + `\n`/`\t`, optional length cap) + `hasUnsafeChars(value)` — layer CLI/lib +- `src/lib/unknown-capabilities.ts` — use the shared sanitizer — layer CLI/lib +- `src/lib/report-error.ts` — `logError` sanitizes (keeps newlines/tabs) — layer CLI/lib +- `src/lib/tar-extract.ts` — reject unsafe entry paths; render interpolated names/typeflags safely — layer CLI/lib +- `tests/terminal-safe.test.ts` — NEW: ESC/CSI, OSC, C1, U+202E, U+200B stripped; newline policy; cap +- `tests/unknown-capabilities.test.ts` — a U+202E name is neutralised +- `tests/report-error.test.ts` — an ESC sequence in a fatal message never reaches stderr raw +- `tests/tar-extract.test.ts` — entry paths with ESC / U+202E refused; the unsupported-type message + carries no raw control byte + +## Contracts satisfied + +- `unknown-capabilities.ts` header: "control characters are stripped … BEFORE it reaches the terminal" — + now including format characters, and extended to the fatal path. + +## Evals to write (P1) + +- listed above; the extractor, sink and U+202E cases fail on the base source. + +## Guarantee audit (P0) + +- "no upstream-derived byte in {C0, C1, Cf} reaches stderr through the fatal sink" → floor: regex strip + at `logError` + refusal at extraction. + +## Trust audit (P2) + +- Narrows accepted archive names; sanitizes display only (never used for paths). + +## Determinism audit (P5) + +- Pure string functions. + +## Open questions (HALT) + +- none diff --git a/.dev/features/terminal-safe-text/REGRESSION.md b/.dev/features/terminal-safe-text/REGRESSION.md new file mode 100644 index 00000000..14e4e21c --- /dev/null +++ b/.dev/features/terminal-safe-text/REGRESSION.md @@ -0,0 +1,30 @@ +# REGRESSION — terminal-safe-text + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `1c4b10737729faddaf7deb5ef4d1c06471d3d94d` (`origin/main` at build time; the build is an uncommitted working tree on top of it). +- **inside** (each declared in `PLAN.md` `## Files`): `src/lib/report-error.ts`, `src/lib/tar-extract.ts`, `src/lib/terminal-safe.ts`, `src/lib/unknown-capabilities.ts`, `tests/report-error.test.ts`, `tests/tar-extract.test.ts`, `tests/terminal-safe.test.ts`, `tests/unknown-capabilities.test.ts`. +- **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 stdlib `*.test.mjs` / `*.test.cjs` files + 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. + +## 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/terminal-safe-text/REVIEW.md b/.dev/features/terminal-safe-text/REVIEW.md new file mode 100644 index 00000000..8912f7a2 --- /dev/null +++ b/.dev/features/terminal-safe-text/REVIEW.md @@ -0,0 +1,49 @@ +# REVIEW — terminal-safe-text + +Floor first: `node .dev/floor/validate.mjs .` → exit 0 (GREEN). Everything below is **advisory**. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0):** (1) the extractor refuses any entry whose full path (prefix + name, UTF-8 decoded) + holds a C0/C1/`\p{Cf}` character — judged before every other entry rule (grill #2), and the refusal + renders the name sanitized on one line (grill #1); (2) `unknown-capabilities.ts` now uses the shared + sanitizer, gaining `\p{Cf}`; (3) `logError` strips the same set (keeping `\n`/`\t`) for every fatal. +- **L-eval (P1):** 9 cases fail on the base source (extractor ×6, sink ×2, U+202E in the unknown + list); `terminal-safe.test.ts` pins the sanitizer. An ordinary non-ASCII name still extracts. +- **L-trust (P2):** narrows accepted archive names; sanitizing is display-only. +- **L-axis (P3):** one sanitizer module; the extractor owns the refusal, the sink owns display. + +## Advisory findings + +```yaml +- type: FINDING + rule_id: 'P1' + severity: minor + file: 'tests/report-error.test.ts' + problem: 'The sink test asserts the argument handed to the mocked clack `log.error` (the last hop inside pharn), not bytes on the real stderr stream (grill #3); clack adds only its own prefix.' + evidence: "const sent = vi.mocked(log.error).mock.calls[0]![0];" +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'THREAT-MODEL.md' + problem: 'THREAT-MODEL.md §2 lists what the extractor rejects; the new control/format-character refusal should be added by a maintainer (write-protected, human-only).' + evidence: 'see `THREAT-MODEL.md` §2 for what the extractor rejects' +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/lib/tar-extract.ts' + problem: 'Non-fatal warnings (log.warn outside unknown-capabilities) are not routed through the sanitizer; none of them interpolates archive-derived text today.' + evidence: 'logError sanitizes (the fatal sink only)' +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'CHANGELOG.md:8' + problem: "No CHANGELOG `[Unreleased]` entry (not in the plan's `## Files`)." + evidence: '## [Unreleased]' +``` + +## Verdict + +**GREEN** — 0 floor-gate findings, 4 advisory findings. No lesson proposed for canon. diff --git a/.dev/features/terminal-safe-text/SHIP.md b/.dev/features/terminal-safe-text/SHIP.md new file mode 100644 index 00000000..60a5024e --- /dev/null +++ b/.dev/features/terminal-safe-text/SHIP.md @@ -0,0 +1,17 @@ +# SHIP — terminal-safe-text + +Stages run, in order: `/pharn-dev-plan` → GATE 1 (human: **Approve as written**) → `/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: open a PR and merge it only if + its CI checks are green. + +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/terminal-safe-text/VERIFY.md b/.dev/features/terminal-safe-text/VERIFY.md new file mode 100644 index 00000000..615d49d5 --- /dev/null +++ b/.dev/features/terminal-safe-text/VERIFY.md @@ -0,0 +1,27 @@ +# VERIFY — terminal-safe-text + +## FLOOR layer (owns the verdict) + +Gates run over the whole repo with the feature present, as a non-root user on node 22 with the session +proxy variables unset (as root with the proxy set, 5 pre-existing tests in `init.test.ts` / +`update.test.ts` fail for environmental reasons, identically at the baseline). + +| gate | exit | +| -------------- | ---- | +| `format:check` | 0 | +| `lint` | 0 | +| `lint:md` | 0 | +| `test` | 0 | +| `typecheck` | 0 | +| `validate` | 0 | + +No `structural:*` gate — the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs`, `failing_gates: []`). + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}` — floor gates only. + +Residual (P0/P7): "verified" means the named gates passed — not that the feature is correct in any sense +the suite does not encode. diff --git a/.dev/features/terminal-safe-text/regression-report.json b/.dev/features/terminal-safe-text/regression-report.json new file mode 100644 index 00000000..458f032a --- /dev/null +++ b/.dev/features/terminal-safe-text/regression-report.json @@ -0,0 +1,26 @@ +{ + "base": "1c4b10737729faddaf7deb5ef4d1c06471d3d94d", + "inside": [ + "src/lib/report-error.ts", + "src/lib/tar-extract.ts", + "src/lib/terminal-safe.ts", + "src/lib/unknown-capabilities.ts", + "tests/report-error.test.ts", + "tests/tar-extract.test.ts", + "tests/terminal-safe.test.ts", + "tests/unknown-capabilities.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/terminal-safe-text/verify-report.json b/.dev/features/terminal-safe-text/verify-report.json new file mode 100644 index 00000000..e4690912 --- /dev/null +++ b/.dev/features/terminal-safe-text/verify-report.json @@ -0,0 +1,17 @@ +{ + "feature": "terminal-safe-text", + "gates": { + "format:check": 0, + "lint": 0, + "lint:md": 0, + "test": 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 207a5045..dba58e5f 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/init-dest-type-preflight/SHIP.md" + ".dev/features/terminal-safe-text/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-24T10:53:06.579Z" + "set_at": "2026-09-24T10:59:45.280Z" } diff --git a/src/lib/report-error.ts b/src/lib/report-error.ts index 0f383af3..24996816 100644 --- a/src/lib/report-error.ts +++ b/src/lib/report-error.ts @@ -1,4 +1,5 @@ import { log } from '@clack/prompts'; +import { terminalSafe } from './terminal-safe.js'; // The ONE place the CLI turns a failure into user-visible output. Two axes, one // file (P3): WHICH STREAM the line goes to, and WHETHER the failure came from an @@ -48,11 +49,16 @@ export function errorMessage(err: unknown): string { * exited 1 with 0 bytes on stderr, leaving the operator grepping an empty file. * Exit codes were always correct — this is the stream contract, not correctness. * - * The message is passed through verbatim. Callers own their wording; this owns - * the stream. + * Callers own their wording; this owns the stream — and what the terminal is + * allowed to INTERPRET. A fatal message can carry text from the fetched archive + * (an extractor refusal naming an entry), so control and Unicode format + * characters are stripped here, at the one sink every fatal path shares; `\n` + * and `\t` are kept for multi-line messages. No caller passes styled text. */ export function logError(message: string): void { - log.error(message, { output: process.stderr }); + log.error(terminalSafe(message, { keepNewlines: true }), { + output: process.stderr, + }); } /** diff --git a/src/lib/tar-extract.ts b/src/lib/tar-extract.ts index b263082a..a1b4421f 100644 --- a/src/lib/tar-extract.ts +++ b/src/lib/tar-extract.ts @@ -1,6 +1,7 @@ import { gunzipSync } from 'node:zlib'; import { mkdirSync, writeFileSync } from 'node:fs'; import { dirname } from 'node:path'; +import { hasUnsafeChars, terminalSafe } from './terminal-safe.js'; import { safeJoin } from './validate.js'; /** @@ -192,6 +193,34 @@ function describeKeywords(keywords: string[] | null): string { return `records ${shown.join(', ')}${more > 0 ? ` (+${more} more)` : ''}`; } +/** + * 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. + */ +function assertDisplayablePath(fullPath: string): void { + if (hasUnsafeChars(Buffer.from(fullPath, 'latin1').toString('utf8'))) { + throw new TarExtractError( + `tar entry path contains a control or format character: ${JSON.stringify( + terminalSafe(Buffer.from(fullPath, 'latin1').toString('utf8'), { + max: 200, + }), + )}`, + ); + } +} + +/** A typeflag for a message: the character if printable, else its code. */ +function describeTypeflag(typeflag: string): string { + return hasUnsafeChars(typeflag) + ? `0x${typeflag.charCodeAt(0).toString(16).padStart(2, '0')}` + : `'${typeflag}'`; +} + /** True when the block is 512 NUL bytes — the end-of-archive marker. */ function isZeroBlock(block: Buffer): boolean { for (const byte of block) if (byte !== 0) return false; @@ -364,17 +393,21 @@ export function extractTar( continue; } + // 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); + const fullPath = prefix === '' ? name : `${prefix}/${name}`; + assertDisplayablePath(fullPath); + const isFile = typeflag === '0' || typeflag === '\0'; const isDirectory = typeflag === '5'; if (!isFile && !isDirectory) { throw new TarExtractError( - `tar entry has an unsupported type '${typeflag}': ${readString(header, OFF_NAME, LEN_NAME)}`, + `tar entry has an unsupported type ${describeTypeflag(typeflag)}: ${fullPath}`, ); } - const name = readString(header, OFF_NAME, LEN_NAME); - const prefix = readString(header, OFF_PREFIX, LEN_PREFIX); - const fullPath = prefix === '' ? name : `${prefix}/${name}`; const { rel, root } = resolveEntryPath(fullPath, isDirectory, expectedRoot); expectedRoot = root; diff --git a/src/lib/terminal-safe.ts b/src/lib/terminal-safe.ts new file mode 100644 index 00000000..e9132046 --- /dev/null +++ b/src/lib/terminal-safe.ts @@ -0,0 +1,46 @@ +// --------------------------------------------------------------------------- +// The ONE display sanitizer for text that may carry untrusted bytes (names and +// messages derived from the fetched archive). A terminal INTERPRETS what it is +// given: an ESC/CSI sequence can erase a line or fake an "OK", an OSC sequence +// can write the clipboard, and a Unicode format character (U+202E RIGHT-TO-LEFT +// OVERRIDE, U+200B ZERO WIDTH SPACE, …) can make a printed name read as +// something it is not. So every such character is removed before display. +// +// Display only (P2): the result is never used as a path or compared against +// anything — refusing an unsafe NAME is the extractor's job (`hasUnsafeChars`). +// --------------------------------------------------------------------------- + +// C0 controls, DEL, C1 controls, and every Unicode format character (Cf). +// eslint-disable-next-line no-control-regex +const UNSAFE_RE = /[\x00-\x1f\x7f-\x9f\p{Cf}]/gu; +// The same set minus `\t` and `\n`, for multi-line messages. +// eslint-disable-next-line no-control-regex +const UNSAFE_KEEP_WS_RE = /[\x00-\x08\x0b-\x1f\x7f-\x9f\p{Cf}]/gu; + +export interface TerminalSafeOptions { + /** Keep `\n` and `\t` (a multi-line message). Default: strip them too. */ + keepNewlines?: boolean; + /** Truncate to this many characters (after stripping), marked with `…`. */ + max?: number; +} + +export function terminalSafe( + value: string, + options: TerminalSafeOptions = {}, +): string { + const stripped = value.replace( + options.keepNewlines ? UNSAFE_KEEP_WS_RE : UNSAFE_RE, + '', + ); + const { max } = options; + return max !== undefined && stripped.length > max + ? `${stripped.slice(0, max)}…` + : stripped; +} + +/** True when `value` holds any character `terminalSafe` would strip. */ +export function hasUnsafeChars(value: string): boolean { + // A fresh, non-global test: a /g regex carries lastIndex between calls. + // eslint-disable-next-line no-control-regex + return /[\x00-\x1f\x7f-\x9f\p{Cf}]/u.test(value); +} diff --git a/src/lib/unknown-capabilities.ts b/src/lib/unknown-capabilities.ts index ed04c569..207bb539 100644 --- a/src/lib/unknown-capabilities.ts +++ b/src/lib/unknown-capabilities.ts @@ -1,3 +1,4 @@ +import { terminalSafe } from './terminal-safe.js'; import type { UnknownCapability } from '../types.js'; // --------------------------------------------------------------------------- @@ -18,8 +19,6 @@ import type { UnknownCapability } from '../types.js'; // construction. // --------------------------------------------------------------------------- -// eslint-disable-next-line no-control-regex -const CONTROL_CHARS_RE = /[\x00-\x1f\x7f-\x9f]/g; // Long enough that a real validation message survives intact (the longest one // this repo produces is ~120 chars), short enough that a hostile 5MB dir name // cannot flood a terminal. @@ -27,11 +26,10 @@ const MAX_FIELD = 200; // A hard cap on how many are listed; the count line always states the true total. const MAX_LISTED = 10; +// Control AND Unicode format characters (U+202E, U+200B, …) are stripped — the +// shared display sanitizer (lib/terminal-safe.ts). function safe(value: string): string { - const stripped = value.replace(CONTROL_CHARS_RE, ''); - return stripped.length > MAX_FIELD - ? `${stripped.slice(0, MAX_FIELD)}…` - : stripped; + return terminalSafe(value, { max: MAX_FIELD }); } /** diff --git a/tests/report-error.test.ts b/tests/report-error.test.ts index 16cbc488..d22384b5 100644 --- a/tests/report-error.test.ts +++ b/tests/report-error.test.ts @@ -57,6 +57,20 @@ describe('report-error', () => { ); }); + // PHARN-17: a fatal message can carry archive-derived text (an extractor + // refusal naming an entry). The sink is the one place every fatal path shares, + // so nothing a terminal would interpret gets past it — newlines stay. + it('strips control and format characters before the message leaves', () => { + logError('bad entry: x\u001b[2K\r\u001b[32mOK\u202e\nsecond line'); + const sent = vi.mocked(log.error).mock.calls[0]![0]; + expect(sent).toBe('bad entry: x[2K[32mOK\nsecond line'); + }); + + it('reportFatal goes through the same sink', () => { + reportFatal('x\u009b31my', { err: new Error('e') }); + expect(vi.mocked(log.error).mock.calls[0]![0]).toBe('⚠ x31my'); + }); + // --- the hint axis (FABLE 4.6) -------------------------------------------- // // Passing the error is the SINGLE axis that marks "this came from an diff --git a/tests/tar-extract.test.ts b/tests/tar-extract.test.ts index 40b627d5..43b51186 100644 --- a/tests/tar-extract.test.ts +++ b/tests/tar-extract.test.ts @@ -8,6 +8,7 @@ import { extractTarGz, TarExtractError, } from '../src/lib/tar-extract.js'; +import { hasUnsafeChars } from '../src/lib/terminal-safe.js'; // Fixtures are built here, byte by byte, rather than shelled out to `tar`. // That is the whole point: the entries worth testing are the ones a system tar @@ -518,6 +519,77 @@ describe('extractTar', () => { ).toThrow(/unsupported type/); }); + // PHARN-17: a name a terminal would interpret is refused before any other + // entry rule, so it is neither installed (and later listed by status/update) + // nor echoed raw by a refusal message. Names are latin1 in the header, so a + // UTF-8 U+202E is written as its three bytes. + const utf8AsLatin1 = (s: string): string => + Buffer.from(s, 'utf8').toString('latin1'); + + it.each([ + [ + 'an ESC sequence in the name', + { name: `pharn-oss-abc1234/n${ESC}[2K\r${ESC}[32mOK` }, + ], + [ + 'a right-to-left override', + { name: utf8AsLatin1('pharn-oss-abc1234/evil\u202egnp.md') }, + ], + [ + 'a zero-width space', + { name: utf8AsLatin1('pharn-oss-abc1234/se\u200bcurity.md') }, + ], + [ + 'a control byte in the ustar prefix', + { name: 'f.md', prefix: `pharn-oss-abc1234/d${ESC}x` }, + ], + ])('refuses %s, naming it without the raw character', (_label, entry) => { + let message = ''; + try { + extractTar(githubArchive([entry]), tmp.path(), LIMITS); + } catch (err) { + expect(err).toBeInstanceOf(TarExtractError); + message = (err as Error).message; + } + expect(message).toMatch(/control or format character/); + expect(hasUnsafeChars(message)).toBe(false); + }); + + it('refuses an unsafe name even on an unsupported-type entry (path judged first)', () => { + expect(() => + extractTar( + githubArchive([{ name: `pharn-oss-abc1234/n${ESC}[32mOK`, type: '2' }]), + tmp.path(), + LIMITS, + ), + ).toThrow(/control or format character/); + }); + + it('renders an unprintable typeflag as its code, not the raw byte', () => { + let message = ''; + try { + extractTar( + githubArchive([{ name: 'pharn-oss-abc1234/x', type: ESC }]), + tmp.path(), + LIMITS, + ); + } catch (err) { + message = (err as Error).message; + } + expect(message).toContain('unsupported type 0x1b'); + expect(message).not.toContain(ESC); + }); + + it('still extracts an ordinary non-ASCII name', () => { + extractTar( + githubArchive([ + { name: utf8AsLatin1('pharn-oss-abc1234/café.md'), data: 'ok' }, + ]), + tmp.path(), + LIMITS, + ); + }); + it('enforces the entry-count cap', () => { const many = Array.from({ length: 5 }, (_, i) => ({ name: `pharn-oss-abc1234/f${i}.txt`, diff --git a/tests/terminal-safe.test.ts b/tests/terminal-safe.test.ts new file mode 100644 index 00000000..382fc93d --- /dev/null +++ b/tests/terminal-safe.test.ts @@ -0,0 +1,49 @@ +import { describe, expect, it } from 'vitest'; +import { hasUnsafeChars, terminalSafe } from '../src/lib/terminal-safe.js'; + +// Code points, not raw characters, so no control byte sits in this source. +const ESC = '\u001b'; +const BEL = '\u0007'; +const CSI_C1 = '\u009b'; +const RLO = '‮'; +const ZWSP = '​'; + +describe('terminalSafe (PHARN-17)', () => { + it.each([ + ['an ESC/CSI sequence', `a${ESC}[2K${ESC}[32mOK`, 'a[2K[32mOK'], + [ + 'an OSC 52 clipboard write', + `x${ESC}]52;c;ZXZpbA==${BEL}y`, + 'x]52;c;ZXZpbA==y', + ], + ['a C1 CSI', `a${CSI_C1}31mb`, 'a31mb'], + ['a right-to-left override', `evil${RLO}gnp.exe`, 'evilgnp.exe'], + ['a zero-width space', `se${ZWSP}curity`, 'security'], + ['a carriage return and newline', 'a\rb\nc\td', 'abcd'], + ])('strips %s', (_label, input, expected) => { + expect(terminalSafe(input)).toBe(expected); + expect(hasUnsafeChars(terminalSafe(input))).toBe(false); + }); + + it('keeps newlines and tabs on request, still stripping the rest', () => { + expect(terminalSafe(`a\nb\tc\r${ESC}d`, { keepNewlines: true })).toBe( + 'a\nb\tcd', + ); + }); + + it('caps the length after stripping', () => { + expect(terminalSafe('x'.repeat(10), { max: 4 })).toBe('xxxx…'); + expect(terminalSafe('xxxx', { max: 4 })).toBe('xxxx'); + }); + + it('leaves ordinary non-ASCII text alone', () => { + expect(terminalSafe('café — naïve')).toBe('café — naïve'); + expect(hasUnsafeChars('café — naïve')).toBe(false); + }); + + it('hasUnsafeChars is stable across calls (no /g lastIndex carry-over)', () => { + expect(hasUnsafeChars(`a${ESC}`)).toBe(true); + expect(hasUnsafeChars(`a${ESC}`)).toBe(true); + expect(hasUnsafeChars(RLO)).toBe(true); + }); +}); diff --git a/tests/unknown-capabilities.test.ts b/tests/unknown-capabilities.test.ts index f964a897..c12a2a44 100644 --- a/tests/unknown-capabilities.test.ts +++ b/tests/unknown-capabilities.test.ts @@ -70,6 +70,17 @@ describe('unknownCapabilitiesWarning', () => { expect(out).toContain('badreason'); }); + // PHARN-17: Unicode format characters were not in the stripped set, so a + // right-to-left override could make a listed name read as something else. + it('strips Unicode format characters (U+202E, U+200B) too', () => { + const out = unknownCapabilitiesWarning([ + unk({ name: 'evil\u202egnp', reason: 'r\u200beason' }), + ])!; + expect(out).not.toMatch(/[\u202e\u200b]/); + expect(out).toContain('evilgnp'); + expect(out).toContain('reason'); + }); + it('caps each rendered field so one huge upstream string cannot flood the terminal', () => { const out = unknownCapabilitiesWarning([ unk({ reason: 'x'.repeat(5000) }),