Repository navigation
fix(verify): resolve the sandbox config dir from the sandbox root - #112
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 36 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
kridaydave
left a comment
kridaydave
left a comment
There was a problem hiding this comment.
Review of PR #112
Scope: one file changed, .opencode/skills/verify-file-organizer/scripts/control-file-organizer.mjs. No server code, schemas, or docs touched.
Blockers
None. The Linux derivation matches getConfigDirectory() in src/core/config/paths.ts: both resolve to <sandbox>/.config/file-organizer-mcp, and sandboxEnv() sets XDG_CONFIG_HOME=<root>/.config on the child, so write and read can no longer drift. The guard is separator-anchored (target !== parent && target.startsWith(parent + path.sep)), so a root prefix cannot false-match a sibling like /tmp/other. The guard also runs before the --fresh fs.rm, so nothing is deleted while the config path is still unknown. This genuinely fixes the reported "Access Denied" failure.
Suggestions
- Fix Windows too instead of always failing.
sandboxEnv()already setsAPPDATA=<root>\AppData\Roamingon Windows, soconfigRootFor()could derive fromrootthere the same way it does on Linux and macOS. As written, the guard rejects every Windows invocation and makes the harness unusable on that platform. - Correct the macOS note. It says macOS "still trusts the child to honor HOME," but
configRootFor()already derives fromrootthere and the guard passes. Only Node honoringHOMEcould break it, which should be stated plainly. - Canonicalize
rootbefore the guard.isInside()is lexical; a symlinked--sandboxroot could slip past it. Low risk for tmpdir throwaways, butpath.resolveplusfs.realpathwould close it. - Add a test that locks this in. Nothing currently exercises the derivation or the guard refusal. A Node smoke that runs
doctorwithXDG_CONFIG_HOMEpointed at a sentinel real config dir and asserts nothing lands there would have caught the original bug. - Remove the stale leading
/** Where this platform reads config.json from... */comment block that now sits above the new JSDoc.
Contract / security
No API or tool-contract changes. Improves safety by turning a previously silent data-loss path (writing sandbox config into the developer config dir) into a hard error. The organizer and path validator are untouched.
bd53886 to
01603db
Compare
configRootFor() read XDG_CONFIG_HOME from the harness's own environment, so the harness wrote the sandbox's config.json into the developer's real config directory while the child server, spawned with its own XDG_CONFIG_HOME, read a directory that never received it. The sandbox data dir was never in the server's allow-list, so every tool call failed with "Access Denied: Path is outside allowed directories" and doctor's read-only check went red. The Linux branch now derives the path from the sandbox root, using one shared definition with sandboxEnv() so the write and the read cannot drift apart. A guard proves the resolved config dir is inside the sandbox root before the first write and refuses otherwise, the same shape as the --fresh refusal, which turns the failure mode into a hard error instead of a silent write into the developer's config dir. Windows keeps reading the harness's APPDATA and macOS still trusts the child to honor HOME. That is out of scope here and the comment on configRootFor says so, but the guard now makes both fail loudly rather than write somewhere wrong. Commit made by Step 5 Preview Free in OpenCode on behalf of Kriday.
01603db to
a3b023a
Compare
1 participant
Harness derives sandbox config dir from sandbox root on Linux to avoid writing into developer config. Guard refuses escape.