input validation: unchecked values reach destructive and authoritative paths

john commented

2026-08-13 15:43:57 +00:00

Owner

Three places take input that was never validated and act on it destructively or authoritatively.

1. A valueless flag becomes the integer 1

parseArgs (planning.mjs:2889-2890) gives a flag with nothing after it the boolean true, and Number(true) === 1. Three sites use bare Number(opts.x):

planning.mjs:483 (phase-done)
planning.mjs:673 (uat record)
planning.mjs:2471 (renumber)

lib/require-int.mjs's own header names this exact hazard - "or, worse, to a wrong-but-valid int like Number(true)===1" - and require-int.test.mjs:23 tests it. --phase, --total, --attempt, --tokens, --read, --step, --role and --criterion all route through it. These three skip it.

All three reproduced:

$ planning.mjs phase-done --dir fx --n
{"ok":true,...}      # phase 1 boxed complete, AUTH-01 flipped Pending -> Complete

$ planning.mjs uat record --dir fx4 --phase 3 --item --result pass
{"ok":true,"item":{"k":1,"status":"pass"}}   # item 1 marked pass, nobody walked it

$ planning.mjs renumber remove --dir fx2 --n --dry-run
{"ok":true,"ops":[{"git_mv":["phases/2","phases/1"]},{"rm":"phases/1"}]}

The trigger is ordinary: --n "$PHASE" unquoted with $PHASE unset drops the token. --item is worst because uat record's guard order puts Number() BEFORE the unknown-item check, so the wrong item is found rather than refused.

Fix: requirePhaseArg at :483 and :2471, requireInt at :673, plus the valueless-form test none of the three has.

2. criteria-coverage exempts a phase whose CONTEXT.md is unreadable

planning.mjs:118 read() collapses ENOENT with EACCES, EISDIR and EIO. :1247 then treats null as "absent CONTEXT is nothing to prove (D-10)". D-10 was written for ABSENT.

readable CONTEXT:  breaks:[{phase:1,id:"AC1",break:"uncovered"},{...AC2...}]
chmod 000:         {"ok":true,"phases":[],"counts":{criteria:0}}

Two uncovered breaks become a clean pass. self-verify.mjs:493-507 guards this exact class deliberately and says why; planning.mjs never adopted it.

Fix: discriminate ENOENT from every other errno, and report a non-ENOENT failure as a break rather than an exemption.

3. Path traversal through milestone-prune --label

planning.mjs:2812 trims the label and nothing else; :2847 builds join(dir, '_archive-' + label); :2854-2855 mkdir and rename into it.

join('/home/u/repo/.planning', '_archive-../../../../tmp/pwned') -> /home/u/tmp/pwned

Directories created and phase directories relocated outside the tree. Per workflows/milestone.md:87 the label is "the version on a release, else the milestone name from PROJECT.md" - an LLM-authored planning artifact.

Fix: validate against /^[A-Za-z0-9][A-Za-z0-9._-]*$/, the shape publish-decision.mjs:28 already uses for branch and remote names, and refuse otherwise.

Three places take input that was never validated and act on it destructively or authoritatively. ## 1. A valueless flag becomes the integer 1 `parseArgs` (`planning.mjs:2889-2890`) gives a flag with nothing after it the boolean `true`, and `Number(true) === 1`. Three sites use bare `Number(opts.x)`: - `planning.mjs:483` (`phase-done`) - `planning.mjs:673` (`uat record`) - `planning.mjs:2471` (`renumber`) `lib/require-int.mjs`'s own header names this exact hazard - "or, worse, to a wrong-but-valid int like `Number(true)===1`" - and `require-int.test.mjs:23` tests it. `--phase`, `--total`, `--attempt`, `--tokens`, `--read`, `--step`, `--role` and `--criterion` all route through it. These three skip it. All three reproduced: ``` $ planning.mjs phase-done --dir fx --n {"ok":true,...} # phase 1 boxed complete, AUTH-01 flipped Pending -> Complete $ planning.mjs uat record --dir fx4 --phase 3 --item --result pass {"ok":true,"item":{"k":1,"status":"pass"}} # item 1 marked pass, nobody walked it $ planning.mjs renumber remove --dir fx2 --n --dry-run {"ok":true,"ops":[{"git_mv":["phases/2","phases/1"]},{"rm":"phases/1"}]} ``` The trigger is ordinary: `--n "$PHASE"` unquoted with `$PHASE` unset drops the token. `--item` is worst because `uat record`'s guard order puts `Number()` BEFORE the `unknown-item` check, so the wrong item is found rather than refused. Fix: `requirePhaseArg` at `:483` and `:2471`, `requireInt` at `:673`, plus the valueless-form test none of the three has. ## 2. criteria-coverage exempts a phase whose CONTEXT.md is unreadable `planning.mjs:118` `read()` collapses ENOENT with EACCES, EISDIR and EIO. `:1247` then treats null as "absent CONTEXT is nothing to prove (D-10)". D-10 was written for ABSENT. ``` readable CONTEXT: breaks:[{phase:1,id:"AC1",break:"uncovered"},{...AC2...}] chmod 000: {"ok":true,"phases":[],"counts":{criteria:0}} ``` Two uncovered breaks become a clean pass. `self-verify.mjs:493-507` guards this exact class deliberately and says why; `planning.mjs` never adopted it. Fix: discriminate ENOENT from every other errno, and report a non-ENOENT failure as a break rather than an exemption. ## 3. Path traversal through milestone-prune --label `planning.mjs:2812` trims the label and nothing else; `:2847` builds `join(dir, '_archive-' + label)`; `:2854-2855` mkdir and rename into it. ``` join('/home/u/repo/.planning', '_archive-../../../../tmp/pwned') -> /home/u/tmp/pwned ``` Directories created and phase directories relocated outside the tree. Per `workflows/milestone.md:87` the label is "the version on a release, else the milestone name from PROJECT.md" - an LLM-authored planning artifact. Fix: validate against `/^[A-Za-z0-9][A-Za-z0-9._-]*$/`, the shape `publish-decision.mjs:28` already uses for branch and remote names, and refuse otherwise.

john added this to the v3.2.0 milestone

2026-08-13 15:43:57 +00:00

john added the

bug

security

labels

2026-08-13 15:43:57 +00:00

john referenced this issue

2026-08-13 16:06:30 +00:00

housekeeping: typecheck scope, dead code, drifted duplicates, and five tests that cannot fail #122

john closed this issue

2026-08-14 17:01:42 +00:00

john commented

2026-08-14 17:02:00 +00:00

Author

Owner

Fixed in v3.2.0 (PR #124). VAL-01, phase 2. A valueless flag can no longer become the integer 1 at phase-done, uat record or renumber, each with the valueless-form test it lacked. An unreadable CONTEXT.md is reported as a break rather than granted the exemption written for an absent one, and a --label that escapes the tree is refused before any mkdir or rename.

Verified through the phase UAT walk. The milestone audit traced 12/12 requirements with 0 broken and 36/36 acceptance criteria covered.

Fixed in v3.2.0 (PR #124). VAL-01, phase 2. A valueless flag can no longer become the integer 1 at phase-done, uat record or renumber, each with the valueless-form test it lacked. An unreadable CONTEXT.md is reported as a break rather than granted the exemption written for an absent one, and a --label that escapes the tree is refused before any mkdir or rename. Verified through the phase UAT walk. The milestone audit traced 12/12 requirements with 0 broken and 36/36 acceptance criteria covered.

Rows
Columns

input validation: unchecked values reach destructive and authoritative paths #117

1. A valueless flag becomes the integer 1

2. criteria-coverage exempts a phase whose CONTEXT.md is unreadable

3. Path traversal through milestone-prune --label