Skip to content

fix(delivery): validate project_path in set instead of mkdir-ing whatever it is given (#493) - #508

Open
Masashi-Ono0611 wants to merge 1 commit into
fujibee:mainfrom
Masashi-Ono0611:fix/493-project-path-validation
Open

fix(delivery): validate project_path in set instead of mkdir-ing whatever it is given (#493)#508
Masashi-Ono0611 wants to merge 1 commit into
fujibee:mainfrom
Masashi-Ono0611:fix/493-project-path-validation

Conversation

@Masashi-Ono0611

Copy link
Copy Markdown
Contributor

Part of #493.

delivery.sh set didn't validate <project_path> at all, so resolve_hooks_file concatenated whatever it was given and agmsg_delivery_apply mkdir -p'd the result — which is how a directory literally named myproject\n ended up next to the real one, containing only .codex/hooks.json with the newline baked into the hook commands' project path.

agmsg_validate_project_path() now runs once in do_set, before any type-specific apply logic.

The policy, and why

Reject, don't correct. A trailing newline is detected by trimming, then the value is refused because the trimmed form differs from what was passed. Silently "fixing" it would make a malformed command work this one time and hide the bug in whatever generated it. As the issue notes, agmsg's callers are largely LLM agents composing commands from SKILL.md — for that audience a loud error naming the exact value is worth more than a lucky save.

Never create implicitly. A path that doesn't already exist as a directory is refused rather than created. That is the actual defect from the report: a directory nobody asked for.

Also rejected: empty or whitespace-only, an embedded newline or carriage return, and a directory that exists but can't be entered (-d passes for a directory with no execute bit, and every apply implementation then tries to write inside it — better to fail here with a clear message than later with a confusing mkdir error).

Two things I changed after review

  • The validator was failing open. It ended with printf '%s' "$(cd "$raw" && pwd)". printf returns 0 regardless, so a permission failure or race inside the substitution would have produced a successful validation of an empty path. The status is now checked explicitly, and cd -- "$raw" keeps a real directory named like an option (-P, -L) from being parsed as one. A validator that fails open is worse than no validator.
  • Dropped the canonicalization. The first version returned cd && pwd, mirroring spawn.sh:188. But that's a second, unrequested behavioral change — it rewrites relative paths to absolute and collapses ./.., so anything downstream comparing or persisting this value would start seeing a different string than the caller passed. delivery.sh set silently mkdirs a bogus project dir when passed a malformed path (no existence check / trim) #493 is about refusing malformed input, not normalizing well-formed input. The function now echoes the caller's own spelling back.

Verification

  • bats tests/test_delivery.bats148 ok, 0 not ok, exit 0.
  • 7 new tests: the exact myproject\n repro (asserting no directory is created), nonexistent path, empty, whitespace-only, leading/trailing spaces where the trimmed path does exist (proving it doesn't quietly fall back), embedded newline, and the un-enterable directory (skipped when running as root, where permission bits don't restrict traversal).
  • Mutation-tested, twice. Making the validator a no-op fails 5 of the new tests; neutralising the traversability check fails the un-enterable test. Both restored and re-confirmed green.
  • Reviewed with codex exec (gpt-5.6-sol, high). The fail-open printf and the cd -- hardening are its findings.

Scope notes

  • Validation lives in do_set rather than in each apply implementation. That covers the default JSON-hooks path and rulefile_apply as far as I traced, but I have not done a full call-graph audit, so I'm not claiming categorically that no other entry point can reach the same mkdir. If you know of one, say so and I'll extend it.
  • On MSYS, pwd-style normalization can rewrite C:/repo to /c/repo; dropping the canonicalization sidesteps that question entirely rather than answering it.
  • The trim detects space, tab, CR and LF specifically — not every Unicode whitespace character.
  • Deliberately not touching send.sh: add a stdin/file input mode so message bodies survive shell quoting #378 (stdin input) or send.sh accepts nonexistent from/to agents without validation #355 (recipient validation), though they're the same front-door family.

…ever it is given (fujibee#493)

set built a hooks-file path straight from an unvalidated project_path and
mkdir -p'd it, so a malformed literal - the report's case is a trailing
newline from an agent-composed command - created a bogus sibling directory
and installed hooks into it.

agmsg_validate_project_path rejects rather than corrects: trimming is used to
DETECT surrounding whitespace, not to absorb it, so a caller that generated a
bad command gets a loud error naming the value instead of a lucky save that
hides the bug upstream. A path that does not already exist is refused, never
created.

The status of the traversability check is tested explicitly rather than folded
into a command substitution - printf returns 0 regardless, which would let a
permission failure pass as a successful validation of an empty path. The
caller's own path spelling is echoed back; canonicalizing would be a second,
unrequested behavioral change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Masashi-Ono0611
Masashi-Ono0611 marked this pull request as ready for review July 27, 2026 23:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant