Repository navigation
fix(skills): make the wire probe's leak check find a path under its temp dir - #575
Conversation
…emp dir
The launcher gives nvim its own HOME, XDG folders, TMPDIR and CLAUDE_CONFIG_DIR inside
its temp dir (/tmp/wire-probe.XXXXXX). The checker looked only for the home folder of the
password database, so a frame that held vim.fn.expand("~"), vim.env.HOME, a stdpath() path or
the lock file path passed with leaks=0.
The checker now rewrites the rest of the temp dir to $RUN, after the work folder ($CWD),
and counts $RUN as a leak. The temp dir is the parent of the work folder that the launcher
already passes, so the launcher gives no new argument. The refusal of a path with a double
quote, a backslash or a control character covers the temp dir path too, because it loops over
the same list.
The page says the check finds four things, says what lives in the temp dir, and says that a
home folder of "/" switches the home check off. AGENTS.md drops "to make that time shorter"
from the wire probe cleanup rule.
…whole path The checker rewrote each path as a text prefix. The work folder is the one path under the temp dir that a frame may hold, so a path that only starts with its text (the work folder plus "X" or "-backup") read as $CWD plus a suffix and passed. A placeholder now stands for a whole path. The rewrite replaces a path only where "/", the end of a JSON string (a quote, with the backslashes of a nested string before it) or the end of the text follows it. The work folder still goes first, so the work folder and the paths below it pass, and a sibling of it reads as a $RUN path and fails. The page says this where the rewrite is described, and fixes three sentences that were not true: the work folder is under the temp dir and may appear in a frame; a bad character in the temp dir path is refused through the work folder path, and a backslash makes nvim fail first; two of the stdpath() paths (config_dirs and data_dirs) are system folders.
…r only b64a341 rewrote every placeholder only as a whole path. Then a leak path followed by a space or a colon (the run dir plus " now", the home folder plus ": now") was not rewritten, and the check missed it. The text-prefix rewrite found it. Only the work folder, the one path that a frame may hold, is rewritten as a whole path: where "/", the end of a JSON string or the end of the text follows it. The run dir, the tree and the home path are rewritten wherever their text appears, as before: an over-match of a leak path is a leak. The work folder followed by other text reads as a run dir leak. The page and the comments say this.
…run dir The checker found the work folder as a text prefix of the run dir, and a text rule that tells where a path ends inside raw JSON drew one more encoding case per review round (the work folder followed by an escaped backslash at the end of a JSON string passed). The boundary rule is removed by construction. The launcher makes the work folder in a second temp dir, /tmp/wire-probe-cwd.XXXXXX, beside the run dir /tmp/wire-probe.XXXXXX and not in it. The two names differ after "wire-probe", and mktemp fills XXXXXX with letters and digits, so neither path is a prefix of the other. The launcher asserts it, and passes the run dir to the checker as a fourth argument. The checker rewrites every placeholder by plain text, as main does, with the work folder first. The cleanup removes both temp dirs in the same place. Its refusal of a path with a double quote, a backslash or a control character now covers the run dir path by itself. The page says where the work folder lives and that each rewrite is plain text, and drops the sentences about a whole-path boundary.
…KILL of its launcher
|
@codex review |
|
@codex security review |
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 601c406499
ℹ️ 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".
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
@codex security review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security ReviewSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
Why
The operator's goal is to land the code-shape refactor of the plugin on main in small PRs, fixes first. The
tools/callwire probe (#517) checks that no frame that the plugin server sends leaks a path. The launcher gives nvim its ownHOME, XDG folders,TMPDIRandCLAUDE_CONFIG_DIRinside its temp dir (/tmp/wire-probe.XXXXXX), and the lock file is there too. The checker looked only for the home folder that the password database gives. So a frame that holdsvim.fn.expand("~"),vim.env.HOME, astdpath()path or the lock file path passed withcases=44 failed=0 leaks=0on both Neovim versions. The page said that the probe checks for "the user's home folder" and "a path under the real home folder", and for "a Lua source path" where the rule looks for a Lua position (.lua:and a digit).What changed
proof-tools-call-wire.mjs) rewrites each of four paths to a placeholder by plain text, as main does for the tree and the home folder: the work folder to$CWD, the temp dir to$RUN, the tree to$TREEand the real home folder to$HOME, in that order.$RUNjoinsPLACEHOLDERS,PATHSandLEAK, so a frame that holds a path under the temp dir is a leak. The launcher passes the temp dir as a new fourth argument, after the work folder./tmp/wire-probe-cwd.XXXXXX(the folder isworkin it, as the case table names it), beside the temp dir and not in it. The two names differ afterwire-probe(.against-cwd.), andmktempfillsXXXXXXwith letters and digits, so neither path is a prefix of the other. The launcher also asserts it. So no path under the temp dir starts with the text of the work folder, and the checker needs no rule for where a path ends inside raw JSON text. Earlier versions of this PR had such a rule (a text prefix, a whole-path boundary on every path, then on the work folder only), and each drew one more case in review.AGENTS.md("Wire probe cleanup": nvim is a direct child of the script, checks its parent every half second and stops its server, so the lock file is gone within 5 seconds after the script ends) is unchanged and still true. The second dir adds no second cleanup mechanism.nvim wrote no lock file in 10 s). Both end with exit 2 before a case is sent.features/tools-call-wire.md), lines 3, 12, 23, 31, 37, 39, 40 and 41, says that the check finds four things: a Lua position, a path under the temp dir, the tree and the home folder. It says what lives in the temp dir (HOME, the XDG folders,TMPDIR,CLAUDE_CONFIG_DIRand the lock file), that each rewrite is plain text, where the work folder lives and why no path under the temp dir can read as the work folder, that the script removes both temp dirs, and that a home folder of/switches the home check off, because the checker keeps a path only when it is longer than one character. Only line 3 said "a Lua source path"; it now says "a Lua position (.lua:and a digit)". Thestdpath()paths are under the temp dir, exceptconfig_dirsanddata_dirs, which are system folders.AGENTS.md, the line "Wire probe cleanup": "Do not add a second cleanup mechanism to make that time shorter." becomes "Do not add a second cleanup mechanism." The old words limited the ban to one purpose, and the rule bans a second cleanup mechanism for any purpose. Nothing else in that file changes.features/tools-call-wire.mdline 39: the sentence "the temp dirs can stay, and neither holds a token" is false when a SIGKILL of the launcher lands after its cleanup has killed a blocked nvim and before the run dir is removed. A verifier had found the earlier form of the sentence false in another case. The last commit (c33e1d894d12e8a61463481d8c6550b8ca82919f) cuts the token claim instead of refining it: line 39 says only that the temp dirs can stay, and the next sentences of the same line say when the lock file keeps its token (the 5-second bound needs nvim's main loop to run, and a SIGKILL of the whole process group leaves the lock file with its token in the0700temp dir). Its parent commit (cecf52dbae3e59435e2e1f33da10d232f4c6e5fe) tried a longer sentence with a list of cases, and the last commit replaces it: the line now equals the line of601c4064without, and neither holds a token.How I proved it
Every run uses
PATHwith only the node dir,/usr/binand/bin, an absolute Neovim path (VCN_NVIM), a scratchHOME, and an empty environment otherwise (env -i). The launcher gives nvim its ownHOME, XDG folders andCLAUDE_CONFIG_DIR. Noclauderuns. The runs use the verifier'sprobe.shand mutant list, with these changes:probe.shreads the temp dir fromCLAUDE_CONFIG_DIR(it read it fromWIRE_CWD) and reports the work folder's temp dir too, and the builder takes the commit and the checkout of this head. The tenr_*mutants below are mine. The matrix has 68 runs across the two Neovim versions, and the host load was 23 to 28 (evidence/option-b-r4/matrix.txt).cases=44 failed=0 leaks=0, exit 0.-32602data of the entry check, in the spellings that earlier rounds used for the work folder: the temp dir itself, a path below it, plusX/file.txt, plus-backup/file.txt, plus: now, plusnow, plus one backslash, plus two backslashes, plus\/sub, and plus"X. All fail on both versions:cases=44 failed=31 leaks=31, with 31 lines that name$RUN.X/file.txt,-backup/file.txt,: now, one backslash, two backslashes,\/suband"X(cases=44 failed=0 leaks=0). The work folder lies beside the temp dir, so text that starts with its path is no path under the temp dir, and a frame may hold the work folder. The parent of the work folder plus" now"passes for the same reason. With the previous commit of this branch (8716d76a), the work folder plus one backslash and plus two backslashes passed although the work folder was under the temp dir there (the finding of the second verify round), and the temp dir plus one backslash failed.vim.fn.expand("~"),vim.env.HOME,vim.fn.stdpath("data")and the lock directory) fail on both versions: 31 of 44 cases, 31 lines name$RUN. A Lua position, the tree path plus": x", the tree path, the real home folder (also plus": now") and the token (<holds the auth token>in 31 lines) fail 31 of 44 cases on both versions./tmpheld nowire-probe*entry after the matrix.evidence/option-b-r4/kill-results.txt).nvim wrote no lock file in 10 s, because nvim fails first. My own run (evidence/option-b-r4/refusal.txt) started the checker alone, and there a backslash gives the temp dir message too.c33e1d894d12e8a61463481d8c6550b8ca82919f(each file ends with its exit code):mise run format-check,mise run check(CASES 5forproof-static.sh, 0 warnings / 0 errors in 185 files) andcheck.sh --hash-only(covered.lock matches the code) all pass.mise run testis not run: no line oflua/orplugin/changes.d649d11b) had one finding in the first verify round. The head8716d76ahad one finding in the second round, which this commit removes by construction.b64a3414had no verify round. In the third round, one fresh verifier passed387078bdwith one wording finding: the page said that both temp dirs stay after a SIGKILL of the launcher, but a kill during the cleanup leaves only the work folder's temp dir. The commit601c4064fixes that one page line. The first automated code review then raised one P2 on it (the last bullet of "What changed" says what it found). The two last commits change only that same page line, so no run above depends on them.What this PR does not cover
~as text. The page says so. This PR adds no detector for one./usr/share/...for example). The work folder's own temp dir (/tmp/wire-probe-cwd.XXXXXX, the parent of the work folder) is not looked for either.523ee200). The files of this PR are the same on main now, and a text merge into main is clean. This PR and part 1 of the wire probe's second follow-up (skills-wire-probe-table-1) change adjacent lines of the page (thecall-argumentstocall-no-leaklines): one text conflict, which the PR that lands second resolves by taking part 1's three lines and this PR'scall-no-leakline.Generated with
xum• Model:anthropic:claude-sonnet-5-5• Thinking:xhigh