Skip to content

fix(text): enforce workspace-relative search paths - #543

Open
Hiro-Chiba wants to merge 4 commits into
ix-infrastructure:mainfrom
Hiro-Chiba:fix/text-workspace-boundary
Open

fix(text): enforce workspace-relative search paths#543
Hiro-Chiba wants to merge 4 commits into
ix-infrastructure:mainfrom
Hiro-Chiba:fix/text-workspace-boundary

Conversation

@Hiro-Chiba

@Hiro-Chiba Hiro-Chiba commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Fixes #542

Summary

Resolve ix text --path from the selected workspace root and reject paths that escape it.

Type

  • Bug fix
  • Feature
  • Refactor
  • Docs
  • Test
  • CI

Changes

  • Resolve relative search paths against --root instead of the process working directory.
  • Apply lexical and real-path containment checks before invoking ripgrep.
  • Return structured JSON and LLM errors with exit status 1 for path escapes.
  • Emit successful result paths relative to the selected workspace with POSIX separators on every platform. This changes the existing JSON path field from absolute to workspace-relative, including the default --path . case.
  • Document that --path is workspace-root-relative, including when the command is launched from a subdirectory.
  • Add command-level regression coverage for an accepted in-workspace path without depending on a system ripgrep binary.

Plugin compatibility

Audited the current ix-claude-plugin, ix-codex-plugin, ix-cursor-plugin, ix-gemini-plugin, ix-openclaw-plugin, and ix-opencode-plugin consumers. Their text-search consumers display, forward, or reduce the path to a basename; none requires an absolute path. Several fixtures already use workspace-relative paths.

Known limitation

A non-existent path below a symlinked workspace root can still be reported as path_outside_workspace because the existing containment helper falls back to a lexical candidate when realpath fails. This fails closed and is not a traversal bypass. It is intentionally left for the separate follow-up offered in review.

Validation

  • npm test (85 files, 1449 passed, 3 skipped; parser smoke passed)
  • npm run typecheck
  • npm run build
  • npm run knip

Checklist

  • Tests pass
  • Smoke tests pass
  • No raw errors introduced
  • CLI output follows Ix format

Release checklist (if merging to main)

  • ix-cli/package.json version bumped
  • After merge: tag pushed (git tag vX.Y.Z && git push origin vX.Y.Z)
  • If backend changes: ix-memory-layer tagged and released first
  • If docker-compose.standalone.yml changed: verified curl | sh install works

@KageBinary

Copy link
Copy Markdown
Collaborator

Review — sound fix, three things to weigh before merge

The containment logic is right and isPathInsideResolvedRoot is a good extraction: isReadablePath was doing exactly this inline, and hoisting it keeps one definition of "inside, after symlinks" rather than two that can drift. The symlink-escape test is the one that matters and it is there.

1. Machine output changed shape, and the PR body understates it

path: path.relative(root, absoluteFilePath),

Before this change the default --path . case already passed an absolute searchPath to rg, so --format json emitted absolute paths. They are now workspace-relative. That is a better contract and I would keep it — but it is a wire change for every existing ix text --format json consumer, not just a normalisation of the newly-resolved case. Worth a line in the body, and worth checking against the plugins that call ix text before this ships.

2. --path is now root-relative, which breaks running from a subdirectory

This is what #542 asked for, so I am not arguing against it — but the consequence is not written down anywhere. From /repo/src:

ix text TODO --path components      # was /repo/src/components, now /repo/components

and when /repo/components does not exist the user gets rg's failure surfaced as Error: ripgrep failed: ... No such file or directory, which does not explain what happened. Two options: mention the new resolution rule in the --path help text, or check existence and emit a structured error the way the escape path does. The second is more consistent with the rest of the PR.

3. A non-existent path under a symlinked root reports the wrong reason

real() falls back to a lexical resolve when realpathSync throws, which it does for any path that does not exist yet:

const real = (p: string) => { try { return realpathSync(p); } catch { return resolvePath(p); } };

On macOS, where /tmp/private/tmp and /var/private/var, that mixes a realpath'd root with a lexical candidate:

  • root = /tmp/wsreal(root) = /private/tmp/ws
  • searchPath = /tmp/ws/typo → throws → falls back to /tmp/ws/typo
  • isPathInside("/private/tmp/ws", "/tmp/ws/typo")falsepath_outside_workspace

So a typo'd subdirectory under a symlinked root is reported as an escape attempt. It fails closed, so it is not a security hole — but it is a confusing error, and ix text is much more likely to be handed a non-existent path than read is. It is pre-existing in isReadablePath; this PR is what exposes it to a new command. The standard fix is to realpath the longest existing ancestor and re-append the remainder, which stays closed against symlink escapes because every symlink in the existing prefix is still resolved. Happy to do it as a follow-up if you want it separate.

Smaller notes

  • The three-way json / llm / stderr switch here is hand-rolled. ui.ts already has reportFailure (llm + stderr + exit code) and reportUnresolvedTarget (json + llm + exit code) — neither covers all three, so this is not a straight reuse miss, but fix(map): canonicalize and validate workspace roots #545 and fix(read): report invalid and denied reads #553 in this same stack each hand-roll it again. Four copies is the point at which one reportError(code, message, format) in ui.ts pays for itself.
  • No positive test that a legitimate in-workspace --path is accepted. Both new containment tests assert rejection; the only acceptance check is the pure resolveTextSearchPath unit. Worth one case, especially given point 3 above.

Separately: the other untrusted input to this command

While reading this I found that term reaches ripgrep's argv unseparated — rgArgs.push(term, searchPath) — so a term beginning with - is parsed by rg as a flag. Reachable through ix mcp, where the term is a string a model chose. --files runs rg's file listing; --pre=<cmd> takes the path argument away and leaves rg reading stdin for ever, which blocks the serialised MCP runner and leaks the child.

That is independent of this PR and reproduces on main, so it is #564 rather than a request here. It touches only the rgArgs.push line, which is untouched context in this diff — the two merge cleanly in either order.

@Hiro-Chiba

Copy link
Copy Markdown
Contributor Author

Thanks! I addressed the items on my side and left the symlink follow-up untouched as suggested. The updated CI is passing.

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.

[bug] ix text resolves --path outside --root and can search outside the workspace

2 participants