Canonicalize paths across all humanize hooks and scripts#93
Conversation
is_cancel_authorized previously compared the user-provided mv source and destination (lowercased command text) against a lowercase-only prefix derived from active_loop_dir. Now that resolve_project_root canonicalizes via realpath, a user command that uses the non-canonical form of the prefix (e.g. a symlinked ancestor) would fail authorization even when every other check passes. Canonicalize both the loop dir and the parsed src/dest through canonicalize_path, then re-lowercase, so the comparison is symmetric across symlinked prefixes. Extend the symlink-rejection check to use the canonical loop dir so the on-disk probe targets the real path. Also guard the merged-config load in loop-common.sh: when the library is sourced from .bashrc/.zshrc in a non-repo directory, project root resolution yields an empty string and load_merged_config would emit its usage error to stderr on every shell startup. Skip the call when the resolved root is empty and leave the cached config empty. Add a regression test that points a symlinked prefix at the real loop dir and confirms the helper still authorizes the cancel mv.
|
@codex review this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e2c6a0afa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| dest_canonical="$(canonicalize_path "$dest")" | ||
| dest_canonical="${dest_canonical:-$dest}" | ||
| dest_canonical=$(echo "$dest_canonical" | tr '[:upper:]' '[:lower:]') | ||
| local expected_dest="${loop_dir_lower}cancel-state.md" | ||
| if [[ "$dest" != "$expected_dest" ]]; then | ||
| if [[ "$dest_canonical" != "$expected_dest" ]]; then |
There was a problem hiding this comment.
Canonicalizing dest with realpath before comparison allows a destination symlink outside the loop (for example, /tmp/link -> <loop>/cancel-state.md) to pass authorization as if it were the real cancel-state.md. In that case mv <loop>/state.md /tmp/link is allowed, but mv replaces the symlink path itself, so state.md is removed without creating <loop>/cancel-state.md, corrupting loop state and bypassing the intended path restriction.
Useful? React with 👍 / 👎.
1 participant
Summary
resolve_project_roothelper inhooks/lib/project-root.shand route every hook and script through it. The resolver prefersCLAUDE_PROJECT_DIR, falls back togit rev-parse --show-toplevel, and refuses the legacypwdfallback (which drifts withcdduring a session and silently caused state lookups to miss the active loop directory).realpathso symlinked ancestors (e.g./var/...vs/private/var/...on macOS, or a symlinked repo checkout on Linux) do not diverge between setup time and hook time.PROJECT_ROOT:loop-read-validator.sh,loop-write-validator.sh, andis_cancel_authorizedinhooks/lib/loop-common.sh.is_cancel_authorizednow canonicalizes both the loop dir and the parsedmvsource/destination, then re-lowercases so the comparison stays symmetric on case-insensitive filesystems.loop-common.shso sourcinghumanize.shfrom.bashrc/.zshrcin a non-repo directory no longer emitsError: Usage: load_merged_config <plugin_root> <project_root>to stderr on every shell startup.Test plan
bash tests/run-all-tests.sh-> 1723 passed / 0 failedHELPER TEST 8intests/test-cancel-signal-file.shcovers a symlinked-prefix cancel command against a canonical loop dirtests/test-stop-gate.shstageshooks/lib/project-root.shalongside the mock hook so the wrapper resolves a project rootscripts/humanize.shfrom$HOME(non-repo, noCLAUDE_PROJECT_DIR) and confirm no usage error is printed