From 28f7987812828fb6287dbf508880955fc9270d25 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 07:37:03 +0000 Subject: [PATCH] fix(remove): validate config capability names; delete only a strict child (PHARN-01) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A hand-edited `pharn.config.json` entry such as `{"name":"../..","role":"lens"}` made `pharn remove ../..` recursively delete the project root (including `.git`): capability names were never validated at ingest, and `safeJoin` accepts the base directory itself. - `readPharnConfig` now validates every `capabilities[]` entry: an object whose `name` matches `CAPABILITY_NAME_RE` (no control characters) and whose `role` is `griller|lens`, else the new named `CapabilityEntryError` (joins `isConfigValidationError`, so every command exits 1 with the offender named). - `remove` deletes through the new `safeChildJoin`, which only returns a single-segment child of the role subtree — a second, independent floor. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01TvcuVhk8hTeDskp5pAJhnc --- .../GRILL.md | 49 ++++++++ .../config-capability-name-validation/PLAN.md | 80 +++++++++++++ .../REGRESSION.md | 39 +++++++ .../REVIEW.md | 49 ++++++++ .../config-capability-name-validation/SHIP.md | 17 +++ .../VERIFY.md | 27 +++++ .../regression-report.json | 26 +++++ .../verify-report.json | 17 +++ .pharn/writes-scope.json | 4 +- CLAUDE.md | 6 +- SECURITY.md | 2 +- src/commands/remove.ts | 21 +++- src/lib/pharn-config.ts | 68 ++++++++++- src/lib/validate.ts | 18 ++- tests/pharn-config.test.ts | 107 +++++++++++++++++- tests/remove.test.ts | 106 ++++++++++++++++- tests/validate.test.ts | 27 +++++ 17 files changed, 643 insertions(+), 20 deletions(-) create mode 100644 .dev/features/config-capability-name-validation/GRILL.md create mode 100644 .dev/features/config-capability-name-validation/PLAN.md create mode 100644 .dev/features/config-capability-name-validation/REGRESSION.md create mode 100644 .dev/features/config-capability-name-validation/REVIEW.md create mode 100644 .dev/features/config-capability-name-validation/SHIP.md create mode 100644 .dev/features/config-capability-name-validation/VERIFY.md create mode 100644 .dev/features/config-capability-name-validation/regression-report.json create mode 100644 .dev/features/config-capability-name-validation/verify-report.json diff --git a/.dev/features/config-capability-name-validation/GRILL.md b/.dev/features/config-capability-name-validation/GRILL.md new file mode 100644 index 00000000..c6f670ee --- /dev/null +++ b/.dev/features/config-capability-name-validation/GRILL.md @@ -0,0 +1,49 @@ +# GRILL — config-capability-name-validation + +Plan: `.dev/features/config-capability-name-validation/PLAN.md` · spec-hash check: `bca940a5…d729d3c4e` matches live `ARCHITECTURE.md` (no drift). +Registered grillers: `{"registered":0,"grillers":[]}` → inline axes only. + +## Findings + +### P1 — eval coverage + +```yaml +- type: FINDING + rule_id: 'P1' + severity: important + file: '.dev/features/config-capability-name-validation/PLAN.md:47' + problem: 'The ingest check affects every command, but only remove/list are named in the end-to-end evals; update/status/add rely on the shared loader being exercised once.' + evidence: '`loadConfigOrExit` / `list --json` route the new error to the named message + exit 1' +``` + +Advisory: acceptable because all four go through `loadArchetypeConfigOrExit` → `loadConfigOrExit`; one loader test covers the shared path. + +### P4 — docs + +```yaml +- type: FINDING + rule_id: 'P4' + severity: minor + file: '.dev/features/config-capability-name-validation/PLAN.md:34' + problem: "SECURITY.md edit is conditional ('if SECURITY.md enumerates them'); it does — line 64 lists records/lock as local-but-user-editable sidecars and omits pharn.config.json." + evidence: '`SECURITY.md` — list `pharn.config.json` `capabilities[]` beside records/lock ..., if SECURITY.md enumerates them' +``` + +### P7 — scope + +```yaml +- type: FINDING + rule_id: 'P7' + severity: minor + file: '.dev/features/config-capability-name-validation/PLAN.md:31' + problem: 'Rejecting non-object entries is a new refusal beyond name/role; justified (consumers dereference .name) but should be stated in the error message so the hand-edit is fixable.' + evidence: 'each entry: plain object, `name` via `assertSafeString(…, CAPABILITY_NAME_RE)`' +``` + +## Summary + +The plan closes the reproduced defect at two independent floors (ingest regex/enum + strict-child +containment at the delete). Symlinked components are correctly deferred to PHARN-02. Concerns are +coverage/documentation completeness, not design. + +ADVISORY VERDICT: 3 concerns raised (0 blocking-severity, 3 advisory) — for the human to weigh before /pharn-dev-build. diff --git a/.dev/features/config-capability-name-validation/PLAN.md b/.dev/features/config-capability-name-validation/PLAN.md new file mode 100644 index 00000000..731e960f --- /dev/null +++ b/.dev/features/config-capability-name-validation/PLAN.md @@ -0,0 +1,80 @@ +# PLAN — config-capability-name-validation (PHARN-01: `remove` must never delete outside `/`) + +- spec_content_hash: bca940a5ad247c120e6d8a3acba119d0d8df51dca275964d0e54c48d729d3c4e # fix #4 +- increment: validate every `capabilities[]` entry's `name` (`CAPABILITY_NAME_RE`, no control chars) and + `role` (`{griller, lens}`) at config ingest with a new named `CapabilityEntryError`, and make + `remove`'s recursive delete accept only a STRICT child of the role subtree (`safeChildJoin`), so a + hand-edited `{"name":"../.."}` can no longer make `pharn remove` delete the project root. +- layer(s): the CLI itself (`src/lib/*`, `src/commands/remove.ts`) +- constitution_refs: [P0, P1, P2, P3, P5, P7] + +## Discovery — verified this run (P6) + +HEAD `2fffcb3` on `claude/bold-archimedes-5czyyn`, tree clean. Reproduced in the review +(`repro/remove-traversal`): config `{"name":"../..","role":"lens"}` + `pharn remove ../..` → +project root (incl. `.git`) deleted; `../../src` → `src/` deleted; picker path identical. + +- `src/lib/pharn-config.ts:22-24` — comment: "`name` and `role` are still passed through unvalidated"; + `:216` only `validateCapabilitySources` runs. +- `src/lib/validate.ts:115-124` — `safeJoin` accepts `target === root`, so `${subtree}/../..` = cwd passes. +- `src/commands/remove.ts:87-96` — `rmSync(safeJoin(cwd, \`${subtree}/${name}\`), {recursive, force})`. + +## Files + +- `src/lib/validate.ts` — add `safeChildJoin(base, name)`: returns `resolve(base, name)` only when its + `dirname` is exactly `resolve(base)` (a strict, single-segment child); throws `ManifestValidationError` + otherwise — layer CLI/lib +- `src/lib/pharn-config.ts` — `CapabilityEntryError` + `validateCapabilityEntries(raw.capabilities)` + (each entry: plain object, `name` via `assertSafeString(…, CAPABILITY_NAME_RE)`, `role` via + `ROLE_VALUES` membership); joins `isConfigValidationError`; replaces the "unvalidated" comment — layer CLI/lib +- `src/commands/remove.ts` — `deleteCapabilityDir` resolves `safeChildJoin(safeJoin(cwd, subtree), name)` — layer CLI/command +- `tests/validate.test.ts` — `safeChildJoin` cases +- `tests/pharn-config.test.ts` — ingest rejection cases + valid legacy config still loads +- `tests/remove.test.ts` — end-to-end: traversal names exit 1, tree snapshot unchanged; valid name still removes only its dir +- `CLAUDE.md` — the `lib/pharn-config.ts` paragraph ("`name`/`role` stay unvalidated") updated to the new truth (P4) +- `SECURITY.md` — list `pharn.config.json` `capabilities[]` beside records/lock as validated local input, if SECURITY.md enumerates them (P4) + +## Contracts satisfied + +- CLAUDE.md "`lib/validate.ts` is security-sensitive" — names validated against `CAPABILITY_NAME_RE`, `..` rejected, control chars rejected; `safeJoin` containment preserved. +- README "Safety model": `remove` "deletes only the selected capability directory" — now floor-backed. + +## Evals to write (P1) + +- `safeChildJoin(base,'a11y')` → `base/a11y`; `'..'`, `'.'`, `''`, `'a/b'`, `'../x'`, `'/abs'` → throw +- `readPharnConfig` with name `../..` / `../../src` / `..` / `.` / `""` / `a/b` / `"a\u001b[2J"` / non-string → `CapabilityEntryError` naming `capabilities[i].name` +- role `"bogus"` / missing → `CapabilityEntryError` naming `capabilities[i].role` +- non-object entry → `CapabilityEntryError` +- valid archetype config (with and without `source`) and a legacy module config (no `capabilities`) still load byte-for-byte as before (P7) +- `loadConfigOrExit` / `list --json` route the new error to the named message + exit 1 +- `runRemove('../..')` on a crafted config → exit 1, project tree snapshot (incl. `.git`, `src/`) unchanged +- `deleteCapabilityDir` defense-in-depth: even if validation were bypassed, `safeChildJoin` refuses (unit via `safeChildJoin`) +- `runRemove('a11y')` still deletes only `/a11y`, sibling `a11y-extended` intact + +## Guarantee audit (P0) + +- "a config capability name is a single kebab segment" → floor: enum-regex (`CAPABILITY_NAME_RE` + `CONTROL_CHARS_RE`) at ingest +- "role is griller|lens" → floor: enum membership (`ROLE_VALUES`) +- "`remove` deletes only a strict child of the role subtree" → floor: path-containment (`safeChildJoin`: `dirname(target) === base`) layered on `safeJoin` +- symlinked components (PHARN-02) → NOT claimed here; separate increment + +## Trust audit (P2) + +- `pharn.config.json` is committed, hand-editable → treated as untrusted local input; taint stops at + ingest (named error, exit 1, zero writes); downstream consumers (`remove`, `list`, `update`, `status`, + `add`) only ever see validated names, which also closes raw-ESC echo of names in `list`. + +## Determinism audit (P5) + +- All branches are regex/enum membership or an exact string compare (`dirname === base`); failure is a + named hard-fail, never a fallback. + +## P7 note + +Every `name` this CLI ever wrote came from `parseCapabilityIndex` (already `CAPABILITY_NAME_RE`-validated) +and every `role` from `assertRole`, so no legitimately written config is newly rejected. Pre-archetype +configs have no `capabilities` and are untouched. + +## Open questions (HALT) + +- none diff --git a/.dev/features/config-capability-name-validation/REGRESSION.md b/.dev/features/config-capability-name-validation/REGRESSION.md new file mode 100644 index 00000000..b4e65f5b --- /dev/null +++ b/.dev/features/config-capability-name-validation/REGRESSION.md @@ -0,0 +1,39 @@ +# REGRESSION — config-capability-name-validation + +The verdict below is computed by `.dev/floor/check-regress.mjs`, not by this stage's judgment. + +## Base and partition + +- **base:** `2fffcb3d5039b570ac8cdf0ac38d46336633fc3b` — passed explicitly (`--base 2fffcb3`, the + reviewed baseline; the build is an uncommitted working tree on top of it). +- **inside** (all declared in `PLAN.md` `## Files`): `CLAUDE.md`, `SECURITY.md`, + `src/commands/remove.ts`, `src/lib/pharn-config.ts`, `src/lib/validate.ts`, + `tests/pharn-config.test.ts`, `tests/remove.test.ts`, `tests/validate.test.ts`. +- **scope partition:** `check-regress.mjs scope` exited **0**, `escaped: []` — no fix #7 finding. + `.pharn/writes-scope.json` (hook scratch) and this feature's own `.dev/features/…` stage artifacts are + not build output. +- **outside gates:** 46 stdlib test files (`*.test.mjs` / `*.test.cjs`) + whole-repo `validate`; + 0 committed eval pairs. + +## Style-gate skip (P5/P7) + +`lint` / `format:check` / `lint:md` skipped at both sides: `inside` touches no shared style config. + +## 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`; helper exit **0**.) + +Residual (P0/P7): this catches exactly what its suite catches. The vitest suite exercising +`src/**` is inside scope here and is owned by `/pharn-dev-build`'s floor (`npm run check` GREEN, 1277/1277 +as a non-root user on node 22) and `/pharn-dev-verify`. This certifies the comparison, never the increment. diff --git a/.dev/features/config-capability-name-validation/REVIEW.md b/.dev/features/config-capability-name-validation/REVIEW.md new file mode 100644 index 00000000..87b3eb6e --- /dev/null +++ b/.dev/features/config-capability-name-validation/REVIEW.md @@ -0,0 +1,49 @@ +# REVIEW — config-capability-name-validation + +Floor first: `node .dev/floor/validate.mjs .` → exit 0 (GREEN). Everything below is **advisory**. + +## Floor-gate findings (blocking) + +None. + +- **L-floor (P0):** both new guarantees reduce to floor primitives — `CAPABILITY_NAME_RE` + + `CONTROL_CHARS_RE` + `ROLE_VALUES` membership at ingest (enum-regex), and `safeChildJoin`'s + `dirname(target) === resolve(base)` containment at the delete. Symlinked components are explicitly + not claimed (PHARN-02). +- **L-eval (P1):** every new behavior has a vitest test that fails on the base source (38 red before the + fix, green after): 21 ingest cases, 9 `safeChildJoin` cases, 10 `remove` end-to-end cases (real loader + - bypassed-ingest defense in depth + picker + valid-name/prefix-sibling). +- **L-trust (P2):** `pharn.config.json` is now treated as untrusted local input at ingest; the error + message quotes the offending name via `JSON.stringify` and control characters are reported without + echoing them (tested). No instruction-looking content was encountered in the diff. +- **L-axis (P3):** `validate.ts` gains one lexical primitive (its existing axis); `pharn-config.ts` + gains one ingest validator (its axis); `remove.ts` only consumes them. No leaf→leaf import. + +## Advisory findings + +```yaml +- type: FINDING + rule_id: 'P4' + severity: minor + file: 'CHANGELOG.md:8' + problem: "The user-visible behavior change (a hand-edited invalid capability name/role now exits 1 on every command) has no CHANGELOG `[Unreleased]` entry; CHANGELOG.md was not in the plan's `## Files`." + evidence: '## [Unreleased]' +- type: FINDING + rule_id: 'P5' + severity: minor + file: 'src/commands/remove.ts:94' + problem: 'The defense-in-depth refusal throws an uncaught ManifestValidationError (stack trace, no curated message); unreachable while ingest validation holds, so a crash-loud failure is acceptable for a should-never-happen path.' + evidence: 'const dir = safeChildJoin(' +- type: FINDING + rule_id: 'P1' + severity: minor + file: 'tests/pharn-config.test.ts' + problem: "`list --json`'s own error path is covered only through the shared `isConfigValidationError` union, not by a list-level test with an invalid name." + evidence: 'joins isConfigValidationError, so every loader reports it and exits 1' +``` + +## Verdict + +**GREEN** — 0 floor-gate findings, 3 advisory findings (minor). + +No lesson proposed for canon. diff --git a/.dev/features/config-capability-name-validation/SHIP.md b/.dev/features/config-capability-name-validation/SHIP.md new file mode 100644 index 00000000..65263de5 --- /dev/null +++ b/.dev/features/config-capability-name-validation/SHIP.md @@ -0,0 +1,17 @@ +# SHIP — config-capability-name-validation + +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/config-capability-name-validation/VERIFY.md b/.dev/features/config-capability-name-validation/VERIFY.md new file mode 100644 index 00000000..d08e03ba --- /dev/null +++ b/.dev/features/config-capability-name-validation/VERIFY.md @@ -0,0 +1,27 @@ +# VERIFY — config-capability-name-validation + +## FLOOR layer (owns the verdict) + +Gates run over the whole repo with the feature present, as a non-root user on node 22 with proxy +variables unset (as root with the session proxy set, 5 pre-existing tests in `init.test.ts` / `update.test.ts` fail for environmental reasons, identically at the baseline). + +| gate | exit | +| -------------- | ------------- | +| `test` | 0 (1277/1277) | +| `validate` | 0 | +| `lint` | 0 | +| `format:check` | 0 | +| `lint:md` | 0 | +| `typecheck` | 0 | + +No `structural:*` gate — the increment ships no eval-actual pair. + +**VERDICT: PASS** (`.dev/floor/check-verify.mjs` exit 0, `failing_gates: []`). + +## ADVISORY layer + +`node .dev/floor/count-verifiers.mjs .` → `{"registered":0,"verifiers":[]}` — no verifiers registered, +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/config-capability-name-validation/regression-report.json b/.dev/features/config-capability-name-validation/regression-report.json new file mode 100644 index 00000000..e1daff60 --- /dev/null +++ b/.dev/features/config-capability-name-validation/regression-report.json @@ -0,0 +1,26 @@ +{ + "base": "2fffcb3d5039b570ac8cdf0ac38d46336633fc3b", + "inside": [ + "CLAUDE.md", + "SECURITY.md", + "src/commands/remove.ts", + "src/lib/pharn-config.ts", + "src/lib/validate.ts", + "tests/pharn-config.test.ts", + "tests/remove.test.ts", + "tests/validate.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/config-capability-name-validation/verify-report.json b/.dev/features/config-capability-name-validation/verify-report.json new file mode 100644 index 00000000..178d6a31 --- /dev/null +++ b/.dev/features/config-capability-name-validation/verify-report.json @@ -0,0 +1,17 @@ +{ + "feature": "config-capability-name-validation", + "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 803f304b..b8609ad3 100644 --- a/.pharn/writes-scope.json +++ b/.pharn/writes-scope.json @@ -1,7 +1,7 @@ { "scope": [ - ".dev/features/features-readme-relocation/SHIP.md" + ".dev/features/config-capability-name-validation/SHIP.md" ], "set_by": ".claude/commands/pharn-dev-ship.md", - "set_at": "2026-09-10T13:59:50.442Z" + "set_at": "2026-09-24T07:36:53.788Z" } diff --git a/CLAUDE.md b/CLAUDE.md index 4d7e6752..3506823f 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,13 +55,13 @@ ESM-only (`"type": "module"`, NodeNext). **Relative imports must use `.js` exten **`lib/install-capabilities.ts`** is the shared capability copy core — `installCapabilityDirs` (`add`) and `installCapabilities` (`init`); `update` applies the manifest per file instead. `installCapabilityDirs(repoDir, projectRoot, capabilities, paths?)` pre-flights **every** selected capability source (validated name via `CAPABILITY_NAME_RE` + `safeJoin` + existence + symlink rejection) before any write — no partial installs — then copies each griller/lens dir into the mirrored layout. `installCapabilities` additionally copies the fixed product surfaces: product `pharn-*` commands (excluding `pharn-dev-*`), `.cjs` hooks (excluding `*.test.cjs`), `settings.json` (**never** overwritten), the trusted docs, the layout-resolved product-loop boundary contract (`resolveFeaturesReadme`) (layout-invariant; guarded by `findSymlinkComponent`, not just the leaf `isSymlink`, since it is the one root-relative copy with an intermediate directory), `pharn-contracts/`, and `.dev/floor/` minus test files and its `test-fixtures/` subtree (a floor-RELATIVE segment match, single-sourced as `FLOOR_TEST_FIXTURES_DIR` and anchored at the floor root on both sides — an unanchored absolute-path match would prune the whole floor copy under an ancestor of that name). Copying from the untrusted clone is symlink-guarded (`isSymlink` reject / `noSymlinks` filter) and `safeJoin`-contained; file contents are copied verbatim, never executed. The install set is resolved by `lib/capability-index.ts` (`parseCapabilityIndex` — the untrusted-frontmatter → typed `CapabilityIndex` fetch boundary, reading only `name`/`role`/`applies` via a strict field reader) + `lib/resolve-capabilities.ts` (select where `applies` is `universal` or intersects the detected archetypes). The commit SHA is threaded from `fetchRepo` (`repo.sha`) — no separate GitHub fetch (closes the resolve/fetch TOCTOU). -**`lib/validate.ts` is security-sensitive.** Untrusted names, versions, paths, and capability frontmatter are validated against strict regex/enum allowlists (`CAPABILITY_NAME_RE`, `VERSION_RE`, `COPY_FILENAME_RE`, `COMMIT_RE`, the `role`/`applies` enums), checked for `..`, and rejected on control chars. **`safeJoin` lives here** (relocated from the deleted `install-modules.ts`) — the lexical path-containment gate that `install-capabilities.ts`, `diff.ts`, `capability-index.ts`, `layout.ts`, `skills-version.ts`, and `remove.ts` all guard their fs access with, so nothing escapes its base dir (`install-capabilities.ts` adds a symlink-aware backstop at the write sites). `toPosix` (the other purely lexical primitive — separator normalization + trailing-slash strip, relocated here once `install-manifest.ts` and `symlink-guard.ts` both needed it) sits beside it. The **physical** counterpart lives in **`lib/symlink-guard.ts`** — `findSymlinkComponent(base, rel)`, the single component walk that `backup.ts` (read side), `apply-update.ts` (write side), and `install-manifest.ts` (twice, skip side) all reach for; it returns the first symlinked component below `base` or `null`, and each **caller owns its failure shape** (backup and apply throw their own `ManifestValidationError` messages, the manifest skips), which is why the core returns a value instead of throwing. Nonexistent components deliberately pass — `applyWrites` creates parents _after_ the walk. That lexical/physical split is the point: `safeJoin` contains the path _string_, `findSymlinkComponent` refuses the path _on disk_. Remote fetches (`skills-version.ts`) use `redirect: 'error'`, an 8s timeout, and a 256KB body cap. Preserve these invariants. +**`lib/validate.ts` is security-sensitive.** Untrusted names, versions, paths, and capability frontmatter are validated against strict regex/enum allowlists (`CAPABILITY_NAME_RE`, `VERSION_RE`, `COPY_FILENAME_RE`, `COMMIT_RE`, the `role`/`applies` enums), checked for `..`, and rejected on control chars. **`safeJoin` lives here** (relocated from the deleted `install-modules.ts`) — the lexical path-containment gate that `install-capabilities.ts`, `diff.ts`, `capability-index.ts`, `layout.ts`, `skills-version.ts`, and `remove.ts` all guard their fs access with, so nothing escapes its base dir (its strict-child sibling `safeChildJoin` — exactly one segment below the base, never the base — guards `remove`'s recursive delete) (`install-capabilities.ts` adds a symlink-aware backstop at the write sites). `toPosix` (the other purely lexical primitive — separator normalization + trailing-slash strip, relocated here once `install-manifest.ts` and `symlink-guard.ts` both needed it) sits beside it. The **physical** counterpart lives in **`lib/symlink-guard.ts`** — `findSymlinkComponent(base, rel)`, the single component walk that `backup.ts` (read side), `apply-update.ts` (write side), and `install-manifest.ts` (twice, skip side) all reach for; it returns the first symlinked component below `base` or `null`, and each **caller owns its failure shape** (backup and apply throw their own `ManifestValidationError` messages, the manifest skips), which is why the core returns a value instead of throwing. Nonexistent components deliberately pass — `applyWrites` creates parents _after_ the walk. That lexical/physical split is the point: `safeJoin` contains the path _string_, `findSymlinkComponent` refuses the path _on disk_. Remote fetches (`skills-version.ts`) use `redirect: 'error'`, an 8s timeout, and a 256KB body cap. Preserve these invariants. -**`lib/pharn-config.ts`** reads/writes `pharn.config.json` (`pharnVersion`, `skillsVersion`, `repo`, `commit`, `installedAt`, and — for an archetype install — `archetypes[]`, `capabilities[]` (`{name, role}`), `layout`, plus the `models`/`seam` blocks). A present-but-invalid `capabilities[].source` (anything outside `{auto, manual}`; ABSENT is legal, P7) is rejected at ingest by `CapabilitySourceError` — the first capabilities-entry check, `source` only (`name`/`role` stay unvalidated) — which joins `isConfigValidationError`'s union. A present-but-invalid `models`/`seam` block is caught by its own validator — `lib/model-routing.ts` (`validateModelRouting`/`ModelRoutingError`) and `lib/seam-config.ts` (`validateSeamConfig`/`SeamConfigError`) — and `readPharnConfig` lets that named error PROPAGATE (never collapsing a bad hand-edit into the "run init" path); `isConfigValidationError` + `loadConfigOrExit` catch and report it, and the validated/stripped blocks replace the raw ones. Schema is additive — a legacy config's now-unused `modules[]`/`constitution`/`stackAnswers`/`installedSkills[]` still load (P7). `isArchetypeConfig` (= `Array.isArray(config.capabilities)`) is the deterministic discriminator; **`loadArchetypeConfigOrExit`** is the shared load-or-reject surface for `add`/`update`/`status`/`remove` (a pre-archetype config → `LEGACY_CONFIG_MESSAGE` + exit(1), never a fetch; `list` keeps its own json-aware check so `--json` stderr stays clean). `add`/`update`/`remove` update the config in place. +**`lib/pharn-config.ts`** reads/writes `pharn.config.json` (`pharnVersion`, `skillsVersion`, `repo`, `commit`, `installedAt`, and — for an archetype install — `archetypes[]`, `capabilities[]` (`{name, role}`), `layout`, plus the `models`/`seam` blocks). Every `capabilities[]` entry is validated at ingest: it must be an object whose `name` matches `CAPABILITY_NAME_RE` (no control chars) and whose `role` is in `ROLE_VALUES`, else `CapabilityEntryError` — the config is committed and hand-editable, and `name` is path-joined by `remove`'s recursive delete (a `{"name":"../.."}` entry used to delete the project root). A present-but-invalid `capabilities[].source` (anything outside `{auto, manual}`; ABSENT is legal, P7) is rejected by `CapabilitySourceError`. Both join `isConfigValidationError`'s union. A present-but-invalid `models`/`seam` block is caught by its own validator — `lib/model-routing.ts` (`validateModelRouting`/`ModelRoutingError`) and `lib/seam-config.ts` (`validateSeamConfig`/`SeamConfigError`) — and `readPharnConfig` lets that named error PROPAGATE (never collapsing a bad hand-edit into the "run init" path); `isConfigValidationError` + `loadConfigOrExit` catch and report it, and the validated/stripped blocks replace the raw ones. Schema is additive — a legacy config's now-unused `modules[]`/`constitution`/`stackAnswers`/`installedSkills[]` still load (P7). `isArchetypeConfig` (= `Array.isArray(config.capabilities)`) is the deterministic discriminator; **`loadArchetypeConfigOrExit`** is the shared load-or-reject surface for `add`/`update`/`status`/`remove` (a pre-archetype config → `LEGACY_CONFIG_MESSAGE` + exit(1), never a fetch; `list` keeps its own json-aware check so `--json` stderr stays clean). `add`/`update`/`remove` update the config in place. **`pharn add` addressing** (`commands/add.ts` + `lib/capability-address.ts`). `add ` or `add :` (e.g. `add a11y`, `add lens:n-plus-one`) installs one capability into an archetype project — a manual override of archetype auto-selection. It clones pharn-oss (SHA-pinned), then applies **the version gate**: a local `versionGate` helper, called ONCE per command from INSIDE each path's existing `try` (so `readSkillsVersion`'s throw still reaches the `finally` that cleans up the clone), refuses when `readSkillsVersion(repo.dir) !== config.skillsVersion` — reusing the existing `{kind:'error'}` outcome → `exit(1)`. It fires on `!==` (never `<`, so a rollback reads the same), fires **gate-first** (before the already-installed no-op and before the picker's `all-installed`, and before `groupMultiselect` renders), and writes nothing. A sibling **layout gate** (`layoutGate`) sits immediately after it at both call sites, `??`-chained (`versionGate(…) ?? layoutGate(…)`) so the **version** refusal wins when both mismatch — by short-circuit evaluation, not statement order: it refuses when `detectLayout(repo.dir) !== configLayout(config)`, naming both resolved layouts and `pharn update --force`. `add` copies at the CLONE's layout (`installCapabilityDirs`' default) and records at it (`mergeCapabilityRecords`), while `remove`/`status`/`diff.ts` address the project at `configLayout` — so without the gate a mismatched add lands files nothing ever looks at, and the next `remove` drops the config entry reporting "its files were already gone", orphaning the dir. `add` must NEVER record the clone's layout the way `update` does: `update` may only because it rewrites the WHOLE tree at that layout, whereas `add` writes one capability, so stamping `layout` here would re-address every other already-placed file. Comparing `configLayout(config)` (not the raw `config.layout`) is the point — agreement with the readers is the invariant, and it makes a garbage hand-edited value resolve to `flat` and fail closed. This is what keeps `update`'s `config.skillsVersion === latest` early-return honest: `add` must never stamp a newer `skillsVersion` over unchanged old bytes. `add` therefore refreshes `commit` but NEVER `skillsVersion` (the legal same-version-different-commit case). Past the gate it resolves the arg against `parseCapabilityIndex`, and if it uniquely names a not-yet-installed capability, copies it via `installCapabilityDirs` and **appends** to `capabilities` with `source: 'manual'` (never touches `archetypes`). That tag is what makes the override survive `update`, and it is written at BOTH entry-construction sites — `resolveArchetypeAdd` and the picker's threaded `cfg` mirror, which the next pick spreads into its own config write. Already-installed → no-op; unknown/ambiguous → lists the valid `role:name` addresses. **Before the copy, `add` backs up destination drift**: `capabilityCloneFiles` (`lib/install-manifest.ts`) enumerates the capability dir in the CLONE, `collectDestDrift` (`lib/dest-drift.ts`) keeps the rels whose dest exists as a regular file and whose `sha256File` DIFFERS from the clone's, and a non-empty set goes to `createBackup` (`lib/backup.ts` — the same `.pharn-backup//` `update --force` writes) BEFORE the first `cpSync`, logged at creation so the pointer survives a later throw. `add` was the only write path with none of the product's three edit-protections (init's prompt, update's per-file skip, `--force`'s backup), and `update` itself manufactures the reachable sequence: a `dropped-unselected` capability's files are left on disk, the user edits them, and the later `add` is not a config no-op. Byte-identical is NOT drift (mirrors update's `identical → no-op`), so a drop-then-re-add of unedited files stays silent. The scan SKIPS an absent/symlinked/non-dir source rather than throwing, so `installCapabilityDirs`' curated `Capability "x" (griller) is missing at …` still wins — the same read-skips/write-throws split `install-manifest.ts`'s `addDir` already uses. `scanDest` returns a PARTITION, not just the drift set: a rel whose PROJECT-side path crosses a symlinked component is `unsafe`, and `add` **refuses the whole install** naming the component. That is measured, not defensive — on node v24.13.1 `cpSync` guards only the source, so a symlinked INTERMEDIATE dir under the capability dir is written straight THROUGH (bytes outside the project replaced), while a symlinked leaf is REPLACED and a symlinked capability root throws `ERR_FS_CP_DIR_TO_NON_DIR`. Skipping would be strictly worse than doing nothing (the copy writes through anyway while the backup omits the file), and backing up is impossible (`createBackup` refuses symlinked components; `copyFileSync` would save the link's TARGET). An ENOTDIR walk (a component below a regular file) is NOT unsafe — `cpSync` throws on that tree by itself — so it stays a skip, which is also what keeps a non-directory component out of the set `createBackup` consumes unwrapped. `add` also merges the capability's files into `pharn.records.json` (only extending an already-readable store; it never mints one), keyed by that SAME clone-derived list — never a walk of the destination, which used to sweep a user's own file inside a leftover capability dir into the store as pharn-written and let a later `update` read it as cleanly upgradeable instead of `modified`; the hashes are still taken at the DEST (`buildRecords`), so a record can never disagree with what landed. `CONSTITUTION.md` is **not** touched — `add` installs capability dirs only. `pharn update` re-resolves the **recorded archetypes** against the latest index and re-copies — drift-safely, see below. -**`pharn remove` addressing** (`commands/remove.ts`) is the inverse of `add`. `remove ` / `remove :` (no arg → an interactive picker over the installed capabilities) deletes that one isolated capability dir — addressed at the project's recorded `layout` (flat `pharn-review` / `pharn-pipeline/grillers/`, OR the same under `pharn/`, via `configLayout` + `layoutPaths`) — and drops its `capabilities` entry. **No clone, no network** — everything is derivable from `config.capabilities` + the filesystem, so `remove.ts` imports no repo module at all; `archetypes` is never touched; `CONSTITUTION.md`/`memory-bank/` are **never** touched (they are not capability dirs). Removing an entry whose stored `source` is **literally** `'auto'` warns that the next `update` will reinstall it; an **absent** `source` warns NOTHING (absence means provenance-unknown, and a false warning on a legacy manual add is worse than silence) — derived from the stored field only, so `remove` stays zero-network. Not-installed → benign no-op listing the removable capabilities; a name installed in both roles → hard-fail (ambiguous). `remove` has no `--yes`, and the reason is per-path: the NAMED remove deletes without asking (no prompt to skip), while the bare picker's ONE confirm — listing the picks, default No — IS the destructive gate, so nothing may skip that either. **`remove`'s `ALLOWED_FLAGS` row is empty, so `pharn remove --yes` now exits 1** naming the flag. This SUPERSEDES the previous contract, which read: _"`--yes` stays a declared minimist boolean only because it is `update`'s flag; that is what keeps `pharn remove --yes` a harmless parse rather than an unknown-option refusal, and turning it into a refusal belongs to a per-command allowlist, not to the dispatch."_ That allowlist now exists (see the dispatch paragraph above), so the deferral is spent — but its reasoning still holds and is why the row is empty rather than `['yes']`: there is no confirm on the named path to skip, and the picker's one confirm may not be skipped. `--yes` is still declared globally, because the declaration list is DERIVED from the table and `update`'s row grants it. Every delete path is `safeJoin`-contained. `remove` also **prunes that capability's entries from `pharn.records.json`** (`pruneCapabilityRecords`), so it is no longer the one write path leaving records that describe bytes that are gone — the converse of the invariant `update` is pinned to. The prune is a **key-prefix filter over the store**, never an fs enumeration: a walk of the DEST dir finds nothing after the delete AND nothing before it on the "already gone" path — which is exactly where the stale keys are the only thing left to clean, so the store is the only place the answer still exists. It mirrors `add`'s `mergeCapabilityRecords` guard verbatim (`recordsBaseline(...) === null → return`): absent/corrupt/stale-stamped → the file is left byte-identical, **never** minted and never blessed (the baseline `note` is deliberately not surfaced — `update` is where an unusable store changes an outcome, and it is the command that names the reason). The trailing slash in `relDir + '/'` is load-bearing (`lenses/a11y` must not eat `lenses/a11y-extended`); nothing matched → the write is skipped entirely; and the `skillsVersion`/`commit` stamp is re-written from the CONFIG pair unchanged, since `remove` advances neither. Order is delete → prune → config (mirroring `add`'s records-before-config) — a benignity argument, not atomicity: each write is individually atomic (`lib/atomic-write.ts` — temp-then-`rename`), but the PAIR is not a transaction, so a failed prune leaves the old status quo and a failed config write leaves an entry the next `update` restores. The role→dir ternary is single-sourced in `capabilityRelDir`, shared with `deleteCapabilityDir`, so the delete and the prune address the same directory by construction. Both call sites prune — the named path with `[target]`, the picker ONCE with the full selection (one store write matching the one config write). +**`pharn remove` addressing** (`commands/remove.ts`) is the inverse of `add`. `remove ` / `remove :` (no arg → an interactive picker over the installed capabilities) deletes that one isolated capability dir — addressed at the project's recorded `layout` (flat `pharn-review` / `pharn-pipeline/grillers/`, OR the same under `pharn/`, via `configLayout` + `layoutPaths`) — and drops its `capabilities` entry. **No clone, no network** — everything is derivable from `config.capabilities` + the filesystem, so `remove.ts` imports no repo module at all; `archetypes` is never touched; `CONSTITUTION.md`/`memory-bank/` are **never** touched (they are not capability dirs). Removing an entry whose stored `source` is **literally** `'auto'` warns that the next `update` will reinstall it; an **absent** `source` warns NOTHING (absence means provenance-unknown, and a false warning on a legacy manual add is worse than silence) — derived from the stored field only, so `remove` stays zero-network. Not-installed → benign no-op listing the removable capabilities; a name installed in both roles → hard-fail (ambiguous). `remove` has no `--yes`, and the reason is per-path: the NAMED remove deletes without asking (no prompt to skip), while the bare picker's ONE confirm — listing the picks, default No — IS the destructive gate, so nothing may skip that either. **`remove`'s `ALLOWED_FLAGS` row is empty, so `pharn remove --yes` now exits 1** naming the flag. This SUPERSEDES the previous contract, which read: _"`--yes` stays a declared minimist boolean only because it is `update`'s flag; that is what keeps `pharn remove --yes` a harmless parse rather than an unknown-option refusal, and turning it into a refusal belongs to a per-command allowlist, not to the dispatch."_ That allowlist now exists (see the dispatch paragraph above), so the deferral is spent — but its reasoning still holds and is why the row is empty rather than `['yes']`: there is no confirm on the named path to skip, and the picker's one confirm may not be skipped. `--yes` is still declared globally, because the declaration list is DERIVED from the table and `update`'s row grants it. Every delete is a **strict child** of its role subtree — `safeChildJoin(safeJoin(cwd, subtree), name)` (`lib/validate.ts`), which, unlike `safeJoin`, never returns the base itself nor a deeper path — a second floor under the ingest-time name validation. `remove` also **prunes that capability's entries from `pharn.records.json`** (`pruneCapabilityRecords`), so it is no longer the one write path leaving records that describe bytes that are gone — the converse of the invariant `update` is pinned to. The prune is a **key-prefix filter over the store**, never an fs enumeration: a walk of the DEST dir finds nothing after the delete AND nothing before it on the "already gone" path — which is exactly where the stale keys are the only thing left to clean, so the store is the only place the answer still exists. It mirrors `add`'s `mergeCapabilityRecords` guard verbatim (`recordsBaseline(...) === null → return`): absent/corrupt/stale-stamped → the file is left byte-identical, **never** minted and never blessed (the baseline `note` is deliberately not surfaced — `update` is where an unusable store changes an outcome, and it is the command that names the reason). The trailing slash in `relDir + '/'` is load-bearing (`lenses/a11y` must not eat `lenses/a11y-extended`); nothing matched → the write is skipped entirely; and the `skillsVersion`/`commit` stamp is re-written from the CONFIG pair unchanged, since `remove` advances neither. Order is delete → prune → config (mirroring `add`'s records-before-config) — a benignity argument, not atomicity: each write is individually atomic (`lib/atomic-write.ts` — temp-then-`rename`), but the PAIR is not a transaction, so a failed prune leaves the old status quo and a failed config write leaves an entry the next `update` restores. The role→dir ternary is single-sourced in `capabilityRelDir`, shared with `deleteCapabilityDir`, so the delete and the prune address the same directory by construction. Both call sites prune — the named path with `[target]`, the picker ONCE with the full selection (one store write matching the one config write). **`pharn update` (`commands/update.ts`) is drift-safe by default.** It re-resolves the recorded archetypes and **unions** the result with the user's manual adds (`lib/merge-capabilities.ts` — the pure 9-row membership table: `next = resolve(archetypes) ∪ manual`; sticky manual; a manual entry gone from the index is dropped, its files left alone; a `source`-less legacy entry is inferred ONCE at merge time — in the resolved set → `auto`, outside it → `manual` — which is the ONLY place absence may be resolved). Every membership change is NAMED in a `CAPABILITIES` note (`added` / `dropped-unselected` / `dropped-gone` / `kept-manual`); zero changes print nothing. Resurrection of a removed capability is **reported, not prevented** (no tombstones). It then decides **per file** instead of re-copying wholesale: `lib/install-records.ts` holds `pharn.records.json` (a sha256 of every file an install wrote, hashed at the DEST, stamped with the config's `skillsVersion`/`commit` so a store left by another tool is detected and ignored); `lib/update-decision.ts` is the PURE 6-row table (`decideFileAction` + `planUpdate` — missing→restore, identical→no-op, equals-record→upgrade, else SKIP `modified`/`unrecorded`/`unverifiable`); `lib/apply-update.ts` executes the writes (dest-symlink refusal, parent `mkdir`, and an `ApplyError` carrying what was already written so those files are still recorded on a partial failure); `lib/backup.ts` copies every `--force` casualty to `.pharn-backup//` BEFORE any original is touched. Records are written BEFORE the config, and a run that skipped anything **withholds** the `skillsVersion`/`commit` bump so the recorded version stays true and the next run still has work. `--force` overwrites the skip buckets and bypasses the same-version early-return. Update **never deletes** and never touches `.claude/settings.json`. `CONSTITUTION.md` (from `paths.docs` in `lib/install-manifest.ts` — flat: root; pharn layout: `pharn/CONSTITUTION.md`) is in the expected trusted-doc set and follows the same per-file table: missing→restore, still-at-recorded-hash→upgrade, locally modified→skip (`modified`); `add`/`remove` never touch it. It records the layout detected in the CLONE (closing the latent drift where bytes landed at `pharn/` paths while the config still said `flat`). diff --git a/SECURITY.md b/SECURITY.md index 3f347d75..10f7feb3 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -61,7 +61,7 @@ Scope follows the surface described above: everything that touches remote input - **Untrusted-content injection** — a poisoned pharn-oss tree (malicious capability frontmatter, symlink-planted paths, or unexpected directory entries) that leads the CLI to write outside the project or copy unintended content. - Logic in the archetype install pipeline (`src/commands/init.ts`, `src/steps/archetype-summary.ts`, `src/steps/overwrite-check.ts`, `src/steps/install-archetype.ts`, and the other commands) that could be abused to skip a consent prompt or overwrite files without confirmation. - **The forward-compatibility gate** in `src/lib/min-cli-gate.ts` and `readMinCli` (`src/lib/skills-version.ts`) — an optional `MIN_CLI` file read out of the **untrusted clone** and version-parsed to decide whether to refuse execution. It fails open by design (absent or malformed means "no constraint"); report ways to make a well-formed newer value fail to refuse, or a malformed one to crash rather than warn. -- **Local-but-user-editable sidecar parsing** — `src/lib/install-records.ts` (whose own header declares the store untrusted input: a defensive parser, a key-segment rule, and the invariant that a record key is **never** path-joined) and `src/lib/project-lock.ts` (which parses a JSON payload written by another process and feeds its `pid` to `process.kill`). Report ways to get a crafted store or lock file to write outside the project, signal an unintended process, or defeat the fail-closed stamp check. +- **Local-but-user-editable sidecar parsing** — `src/lib/pharn-config.ts` (every `capabilities[]` entry's `name` is validated against `CAPABILITY_NAME_RE` and its `role` against the role enum at ingest, because `pharn remove` path-joins that name into a recursive delete — which additionally accepts only a strict child of the role subtree via `safeChildJoin`), `src/lib/install-records.ts` (whose own header declares the store untrusted input: a defensive parser, a key-segment rule, and the invariant that a record key is **never** path-joined) and `src/lib/project-lock.ts` (which parses a JSON payload written by another process and feeds its `pid` to `process.kill`). Report ways to get a crafted store or lock file to write outside the project, signal an unintended process, or defeat the fail-closed stamp check. - **Control-character or escape-sequence injection** into terminal output from upstream names — `src/lib/unknown-capabilities.ts` strips them before display; report ways past it. ### Out of scope diff --git a/src/commands/remove.ts b/src/commands/remove.ts index 50d446ac..77b328a8 100644 --- a/src/commands/remove.ts +++ b/src/commands/remove.ts @@ -16,7 +16,7 @@ import { buildRemoveSelection, interactiveAllowed, } from '../lib/capability-picker.js'; -import { safeJoin } from '../lib/validate.js'; +import { safeChildJoin, safeJoin } from '../lib/validate.js'; import { ProjectLockedError, withProjectLock } from '../lib/project-lock.js'; import { loadArchetypeConfigOrExit, @@ -83,13 +83,18 @@ async function removeCapability( // Delete one capability's isolated dir at the project's recorded layout (flat OR // pharn/), returning whether the dir existed. The shared per-name delete path — // the named remove and the picker loop both call it, so the two never diverge. -// Every delete is safeJoin-contained. +// The target must be a STRICT child of the role subtree (safeChildJoin), not +// merely inside the project (safeJoin): names are already validated at config +// ingest, and this is the second, independent floor under a recursive delete. function deleteCapabilityDir( cwd: string, paths: LayoutPaths, target: InstalledCapability, ): boolean { - const dir = safeJoin(cwd, capabilityRelDir(paths, target)); + const dir = safeChildJoin( + safeJoin(cwd, capabilitySubtree(paths, target)), + target.name, + ); const existed = existsSync(dir); if (existed) rmSync(dir, { recursive: true, force: true }); return existed; @@ -103,8 +108,14 @@ function capabilityRelDir( paths: LayoutPaths, capability: InstalledCapability, ): string { - const subtree = capability.role === 'griller' ? paths.grillers : paths.lenses; - return `${subtree}/${capability.name}`; + return `${capabilitySubtree(paths, capability)}/${capability.name}`; +} + +function capabilitySubtree( + paths: LayoutPaths, + capability: InstalledCapability, +): string { + return capability.role === 'griller' ? paths.grillers : paths.lenses; } // Drop the removed capabilities' entries from `pharn.records.json`, so `remove` diff --git a/src/lib/pharn-config.ts b/src/lib/pharn-config.ts index 2336f538..8baf6f28 100644 --- a/src/lib/pharn-config.ts +++ b/src/lib/pharn-config.ts @@ -2,7 +2,12 @@ import { existsSync, readFileSync } from 'node:fs'; import { writeJsonAtomic } from './atomic-write.js'; import { resolve } from 'node:path'; import { errorMessage, logError } from './report-error.js'; -import { isPlainObject } from './validate.js'; +import { + assertSafeString, + CAPABILITY_NAME_RE, + isPlainObject, + ROLE_VALUES, +} from './validate.js'; import { validateModelRouting, ModelRoutingError } from './model-routing.js'; import { validateSeamConfig, SeamConfigError } from './seam-config.js'; import type { PharnConfig } from '../types.js'; @@ -19,9 +24,8 @@ const CAPABILITY_SOURCES = ['auto', 'manual']; * `ModelRoutingError`/`SeamConfigError` pattern, so a hand-edit is reported as * the hand-edit it is and never collapsed into the "run `pharn init`" lie. * - * Deliberately narrow (P7): it validates `source` ONLY. `name` and `role` are - * still passed through unvalidated — hardening those is a separate axis, and - * widening it here would change what a legacy config is allowed to hold. + * It validates `source` ONLY; an entry's `name` and `role` are the separate + * `CapabilityEntryError` below. */ export class CapabilitySourceError extends Error { constructor(message: string) { @@ -30,6 +34,53 @@ export class CapabilitySourceError extends Error { } } +/** + * A `capabilities[]` entry whose `name` or `role` is not something this CLI + * could ever have written. The config is committed and hand-editable, so these + * are untrusted local input (P2) — and `name` is path-joined: `pharn remove` + * deletes `/` recursively, so `{"name": "../.."}` used to delete + * the project root. Every name this CLI writes comes from the index, already + * `CAPABILITY_NAME_RE`-validated, and every role from `assertRole`, so no config + * it wrote is newly rejected (P7); pre-archetype configs carry no + * `capabilities` and never reach this check. + */ +export class CapabilityEntryError extends Error { + constructor(message: string) { + super(message); + this.name = 'CapabilityEntryError'; + } +} + +/** + * Reject any `capabilities[]` entry that is not an object with a + * `CAPABILITY_NAME_RE` `name` (no control characters) and a `role` in + * `ROLE_VALUES` (enum membership, P5). A non-array `capabilities` is left to the + * existing shape handling (`isArchetypeConfig`). + */ +function validateCapabilityEntries(raw: unknown): void { + if (!Array.isArray(raw)) return; + raw.forEach((entry, i) => { + const at = `${CONFIG_FILENAME}: capabilities[${i}]`; + if (!isPlainObject(entry)) { + throw new CapabilityEntryError( + `${at} must be an object with "name" and "role" (found ${JSON.stringify(entry)}). Fix it by hand.`, + ); + } + try { + assertSafeString(entry.name, `${at}.name`, CAPABILITY_NAME_RE); + } catch (err) { + throw new CapabilityEntryError( + `${errorMessage(err)}. A capability name is one lowercase, hyphen-separated segment (e.g. "a11y", "n-plus-one"). Fix it by hand.`, + ); + } + if (!ROLE_VALUES.some((r) => r === entry.role)) { + throw new CapabilityEntryError( + `${at}.role must be ${ROLE_VALUES.map((r) => `"${r}"`).join(' or ')} (found ${JSON.stringify(entry.role)}). Fix it by hand.`, + ); + } + }); +} + /** * Reject a present-but-invalid `capabilities[].source`. ABSENT is legal (P7 — * additive: every config written before the field existed omits it), and so is a @@ -155,7 +206,8 @@ export function configPath(cwd: string): string { * something this CLI will not act on, and each throws its own NAMED error that * PROPAGATES rather than collapsing into the "run init" lie: `ConfigParseError` * (not JSON at all), and — validated OUTSIDE the null-returning try — - * `ModelRoutingError`/`SeamConfigError`/`CapabilitySourceError` (BUG 1). + * `ModelRoutingError`/`SeamConfigError`/`CapabilityEntryError`/ + * `CapabilitySourceError` (BUG 1). * * The parse split is the point. "File absent" and "file corrupt" used to be the * same `null`, so a stray comma was reported as a MISSING file and answered with @@ -213,6 +265,7 @@ export function readPharnConfig(cwd: string): PharnConfig | null { // throws NAMED here rather than flowing into the merge. Entries are otherwise // passed through the spread unreconstructed, so `source` round-trips with no // load-mechanics change at all. + validateCapabilityEntries(raw.capabilities); validateCapabilitySources(raw.capabilities); const config: PharnConfig = { ...(raw as unknown as PharnConfig), @@ -263,7 +316,8 @@ export function loadConfigOrExit(cwd: string): PharnConfig { /** * Is `err` a present-but-invalid-config error — a file that IS there and that * the user has to fix (unparseable JSON, a hand-edited `models`/`seam` block, or - * a `capabilities[].source` outside its enum) — as opposed to a programming bug? + * a `capabilities[]` entry with an invalid `name`/`role`/`source`) — as opposed + * to a programming bug? * * The single definition of "config error" — used by `loadConfigOrExit` and by * `list`'s own `--json`-aware error path, so neither re-encodes the class @@ -277,11 +331,13 @@ export function isConfigValidationError( | ConfigParseError | ModelRoutingError | SeamConfigError + | CapabilityEntryError | CapabilitySourceError { return ( err instanceof ConfigParseError || err instanceof ModelRoutingError || err instanceof SeamConfigError || + err instanceof CapabilityEntryError || err instanceof CapabilitySourceError ); } diff --git a/src/lib/validate.ts b/src/lib/validate.ts index 263a6532..c62da547 100644 --- a/src/lib/validate.ts +++ b/src/lib/validate.ts @@ -1,4 +1,4 @@ -import { resolve, sep } from 'node:path'; +import { dirname, resolve, sep } from 'node:path'; export class ManifestValidationError extends Error { constructor(message: string) { @@ -123,6 +123,22 @@ export function safeJoin(base: string, rel: string): string { return target; } +// The STRICT-child form of safeJoin, for a destructive op addressed by one +// name: the result is always exactly one segment below `base` — never `base` +// itself (which safeJoin allows, so `${subtree}/../..` resolved to the project +// root and `pharn remove` deleted it) and never a grandchild (`a/b`). Lexical, +// like safeJoin; the symlink-aware walk stays a separate, physical check. +export function safeChildJoin(base: string, name: string): string { + const root = resolve(base); + const target = resolve(root, name); + if (target === root || dirname(target) !== root) { + throw new ManifestValidationError( + `Refusing path: ${JSON.stringify(name)} is not a single directory entry directly under ${base}`, + ); + } + return target; +} + // Normalize a dest/relpath to a posix, no-trailing-slash key. Purely LEXICAL — // it rewrites the string and touches no filesystem — which is why it lives here // beside safeJoin rather than in a consumer (one lexical-path axis, P3). Shared diff --git a/tests/pharn-config.test.ts b/tests/pharn-config.test.ts index 6a56585d..3c23804a 100644 --- a/tests/pharn-config.test.ts +++ b/tests/pharn-config.test.ts @@ -12,6 +12,7 @@ import { isArchetypeConfig, configPath, LEGACY_CONFIG_MESSAGE, + CapabilityEntryError, CapabilitySourceError, ConfigParseError, } from '../src/lib/pharn-config.js'; @@ -198,7 +199,7 @@ describe('pharn-config', () => { // ------------------------------------------------------------------------- // capabilities[].source — the FIRST capabilities-entry check this config has. - // Validates `source` ONLY; name/role stay unvalidated (a separate axis, P7). + // Validates `source` ONLY; name/role are the `CapabilityEntryError` block below. // ------------------------------------------------------------------------- describe('capabilities[].source ingest', () => { stubProcessExit(); @@ -270,6 +271,110 @@ describe('pharn-config', () => { }); }); + // ------------------------------------------------------------------------- + // capabilities[].name / .role (PHARN-01). `name` is path-joined by `remove`'s + // recursive delete, so `{"name":"../.."}` used to delete the project root. + // ------------------------------------------------------------------------- + describe('capabilities[].name / .role ingest', () => { + stubProcessExit(); + const withCaps = (caps: unknown[]) => ({ + pharnVersion: '0.4.0', + skillsVersion: '1.0.0', + repo: 'pharn-dev/pharn-oss', + commit: null, + modules: [], + installedAt: '2026-08-07T00:00:00.000Z', + archetypes: ['ssr'], + capabilities: caps, + }); + const writeRaw = (value: unknown): void => { + writeFileSync( + join(tmp.path(), 'pharn.config.json'), + JSON.stringify(value, null, 2), + ); + }; + + it.each([ + ['../..'], + ['../../src'], + ['..'], + ['.'], + [''], + ['a/b'], + ['a\\b'], + ['/abs'], + ['A11y'], + ['a\u001b[2J'], + [7], + [null], + ])('THROWS a named error for name %j, naming the offender', (name) => { + writeRaw( + withCaps([ + { name: 'a11y', role: 'griller' }, + { name, role: 'lens' }, + ]), + ); + expect(() => readPharnConfig(tmp.path())).toThrow(CapabilityEntryError); + expect(() => readPharnConfig(tmp.path())).toThrow( + /capabilities\[1\]\.name/, + ); + }); + + it('never echoes a control character from the offending name', () => { + writeRaw(withCaps([{ name: 'x\u001b]0;pwned\u0007', role: 'lens' }])); + let message = ''; + try { + readPharnConfig(tmp.path()); + } catch (err) { + message = (err as Error).message; + } + expect(message).toMatch(/control characters/); + // eslint-disable-next-line no-control-regex + expect(message).not.toMatch(/[\x00-\x1f]/); + }); + + it.each([['bogus'], [undefined], [1]])( + 'THROWS a named error for role %j', + (role) => { + writeRaw(withCaps([{ name: 'a11y', role }])); + expect(() => readPharnConfig(tmp.path())).toThrow(CapabilityEntryError); + expect(() => readPharnConfig(tmp.path())).toThrow( + /capabilities\[0\]\.role/, + ); + }, + ); + + it.each([['a11y'], [null], [['a11y']]])( + 'THROWS a named error for a non-object entry %j', + (entry) => { + writeRaw(withCaps([entry])); + expect(() => readPharnConfig(tmp.path())).toThrow(CapabilityEntryError); + expect(() => readPharnConfig(tmp.path())).toThrow(/capabilities\[0\]/); + }, + ); + + it('still loads every config shape this CLI writes (P7)', () => { + const caps = [ + { name: 'a11y', role: 'griller', source: 'auto' }, + { name: 'n-plus-one', role: 'lens' }, + ]; + writeRaw(withCaps(caps)); + expect(readPharnConfig(tmp.path())?.capabilities).toEqual(caps); + writeRaw(withCaps([])); + expect(readPharnConfig(tmp.path())?.capabilities).toEqual([]); + }); + + it('joins isConfigValidationError, so every loader reports it and exits 1', () => { + expect(isConfigValidationError(new CapabilityEntryError('x'))).toBe(true); + writeRaw(withCaps([{ name: '../..', role: 'lens' }])); + expect(() => loadConfigOrExit(tmp.path())).toThrow(ProcessExit); + expect(() => loadArchetypeConfigOrExit(tmp.path())).toThrow(ProcessExit); + expect(vi.mocked(log.error).mock.calls.map(String).join()).toContain( + 'capabilities[0].name', + ); + }); + }); + it('still loads a config carrying a removed vendorSkills key (P7 additive)', async () => { // A config written by an older CLI may carry the now-removed vendorSkills // field; the passthrough loader ignores unknown keys and still reads it. diff --git a/tests/remove.test.ts b/tests/remove.test.ts index 325e74cb..df45ec70 100644 --- a/tests/remove.test.ts +++ b/tests/remove.test.ts @@ -1,11 +1,12 @@ import { existsSync, mkdirSync, + readdirSync, readFileSync, rmSync, writeFileSync, } from 'node:fs'; -import { dirname, join } from 'node:path'; +import { dirname, join, relative } from 'node:path'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { CANCEL, @@ -189,6 +190,109 @@ describe('runRemove (archetype)', () => { expect(writePharnConfig).not.toHaveBeenCalled(); }); + // --- PHARN-01: a hand-edited capability name must never steer the delete ------ + + // Every path under the project, so "nothing was deleted" is a whole-tree claim. + function snapshot(root: string): string[] { + const out: string[] = []; + const walk = (dir: string): void => { + for (const e of readdirSync(dir, { withFileTypes: true })) { + const p = join(dir, e.name); + out.push(relative(root, p)); + if (e.isDirectory()) walk(p); + } + }; + walk(root); + return out.sort(); + } + + function seedProject(): void { + write(join(proj, '.git/HEAD'), 'ref: refs/heads/main'); + write(join(proj, 'src/index.ts'), 'code'); + write(join(proj, 'pharn/pharn-review/a11y/a11y.md'), 'A'); + write(join(proj, 'pharn/pharn-review/a11y-extended/x.md'), 'X'); + } + + it.each([['../..'], ['../../src'], ['..'], ['.']])( + 'the REAL config loader refuses name %j: exit 1, tree unchanged, no writes', + async (name) => { + const actual = await vi.importActual< + typeof import('../src/lib/pharn-config.js') + >('../src/lib/pharn-config.js'); + loadArchetypeConfigOrExit.mockImplementationOnce( + actual.loadArchetypeConfigOrExit, + ); + seedProject(); + writeFileSync( + join(proj, 'pharn.config.json'), + JSON.stringify( + archConfig([{ name, role: 'lens' }], { layout: 'pharn' }), + ), + ); + const before = snapshot(proj); + + await expect(runRemove(name)).rejects.toMatchObject(new ProcessExit(1)); + + expect(snapshot(proj)).toEqual(before); + expect(writePharnConfig).not.toHaveBeenCalled(); + }, + ); + + it.each([['../..'], ['../../src'], ['..'], ['.']])( + 'the delete itself refuses name %j even if ingest were bypassed (defense in depth)', + async (name) => { + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name, role: 'lens' }], { layout: 'pharn' }), + ); + seedProject(); + const before = snapshot(proj); + + await expect(runRemove(name)).rejects.toThrow(/Refusing path/); + + expect(snapshot(proj)).toEqual(before); + expect(writePharnConfig).not.toHaveBeenCalled(); + }, + ); + + it('the defense-in-depth refusal also holds on the picker path', async () => { + setTTY(true, true); + loadArchetypeConfigOrExit.mockReturnValue( + archConfig([{ name: '../..', role: 'lens' }], { layout: 'pharn' }), + ); + seedProject(); + const before = snapshot(proj); + vi.mocked(prompts.groupMultiselect).mockResolvedValueOnce(['lens:../..']); + vi.mocked(prompts.confirm).mockResolvedValueOnce(true); + + await expect(runRemove(undefined)).rejects.toThrow(/Refusing path/); + + expect(prompts.confirm).toHaveBeenCalledTimes(1); + expect(snapshot(proj)).toEqual(before); + expect(writePharnConfig).not.toHaveBeenCalled(); + }); + + it('a valid name still deletes only / (prefix sibling intact)', async () => { + loadArchetypeConfigOrExit.mockReturnValue( + archConfig( + [ + { name: 'a11y', role: 'lens' }, + { name: 'a11y-extended', role: 'lens' }, + ], + { layout: 'pharn' }, + ), + ); + seedProject(); + + await runRemove('a11y'); + + expect(existsSync(join(proj, 'pharn/pharn-review/a11y'))).toBe(false); + expect( + existsSync(join(proj, 'pharn/pharn-review/a11y-extended/x.md')), + ).toBe(true); + expect(existsSync(join(proj, 'src/index.ts'))).toBe(true); + expect(existsSync(join(proj, '.git/HEAD'))).toBe(true); + }); + // --- bare `pharn remove` (no arg): multi-select picker / non-TTY guard ------- it('no-arg in a TTY multi-selects, confirms, deletes each + one config write', async () => { diff --git a/tests/validate.test.ts b/tests/validate.test.ts index 5791a59d..6620c552 100644 --- a/tests/validate.test.ts +++ b/tests/validate.test.ts @@ -10,6 +10,7 @@ import { assertRole, assertAppliesToken, isPlainObject, + safeChildJoin, safeJoin, toPosix, } from '../src/lib/validate.js'; @@ -196,6 +197,32 @@ describe('safeJoin (path containment)', () => { }); }); +// The strict-child form `remove` deletes through (PHARN-01): safeJoin allows the +// base itself, which is how `${subtree}/../..` resolved to the project root. +describe('safeChildJoin (strict single-segment child)', () => { + const base = resolve('/tmp/pharn-base/pharn-review'); + + it('returns base/ for one plain segment', () => { + expect(safeChildJoin(base, 'a11y')).toBe(resolve(base, 'a11y')); + }); + + it.each([ + ['..'], + ['.'], + [''], + ['../..'], + ['../x'], + ['a/b'], + ['a/..'], + ['/abs'], + ])( + 'refuses %j (the base itself, a parent, a grandchild, or elsewhere)', + (name) => { + expect(() => safeChildJoin(base, name)).toThrow(ManifestValidationError); + }, + ); +}); + // toPosix is the other LEXICAL primitive here (relocated from install-manifest.ts // when symlink-guard.ts began sharing it — two consumers, so it belongs beside // safeJoin rather than inside one of them). Its two branches are pinned directly