Repository navigation
Fix/executor codex capability home - #2352
Conversation
ede30d0 to
7217238
Compare
![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
Handle stale plugins when migrating the Codex home
When upgrading a device whose manifest records a managed plugin under the old $CODEX_HOME, the backend's default replace sync may omit that plugin. remove_stale_managed_plugins then passes the legacy codex_link to remove_codex_plugin_runtime_path, which rejects it because it is outside this newly configured codex_plugins_dir; the entire sync consequently returns an error before saving the migrated manifest. The retained-plugin guard in install_plugin_runtime_metadata does not run for stale entries, so stale cleanup needs equivalent old-home migration handling.
ℹ️ 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".
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
🚧 Files skipped from review as they are similar to previous changes (7)
📝 WalkthroughWalkthroughThe PR makes ChangesCodex runtime integration
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@executor/src/local/capabilities.rs`:
- Around line 491-493: Update the path validation logic in the block where
previous_codex_link is compared against self.codex_plugins_dir to protect
against parent-directory traversal attacks. The current starts_with check does
not normalize .. components, allowing paths like <codex_plugins_dir>/../target
to bypass validation and reach remove_existing_path. Either strip the prefix
from previous_codex_link after the starts_with check and reject the path if it
contains ParentDir components, or canonicalize previous_codex_link before
performing the deletion check to ensure it actually resolves within the intended
directory bounds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3097f990-796b-48d5-8745-30db29757b32
📒 Files selected for processing (7)
executor/src/agents/codex.rsexecutor/src/agents/codex/home.rsexecutor/src/agents/git_auth.rsexecutor/src/agents/mod.rsexecutor/src/local/backend/capability.rsexecutor/src/local/capabilities.rsexecutor/tests/local_capability_sync_contract.rs
| if previous_codex_link != codex_link | ||
| && previous_codex_link.starts_with(&self.codex_plugins_dir) | ||
| { |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
According to the Rust standard-library documentation, does std::path::Path::starts_withnormalize.. parent-directory components before comparing paths?
💡 Result:
According to the Rust standard library documentation, the std::path::Path::starts_with method does not normalize .. (parent-directory) components before comparing paths [1][2][3]. The documentation specifies that while several methods in the std::path module perform basic normalization—such as ignoring repeated separators, non-leading . components, and trailing separators—they do not resolve .. components or symlinks [4][1][2][3]. Because starts_with performs a comparison based on path components, it treats paths containing .. literally rather than resolving them [4][3]. Consequently, paths like a/b/../c and a/c are considered distinct [4][3]. For full normalization that resolves .. components, the documentation recommends using Path::canonicalize, which accesses the filesystem [1][2][3].
Citations:
- 1: https://doc.rust-lang.org/nightly/src/std/path.rs.html
- 2: https://github.com/rust-lang/rust/blob/master/library/std/src/path.rs
- 3: https://doc.rust-lang.org/nightly/std/path/struct.Path.html
- 4: https://doc.rust-lang.org/std/path/struct.Path.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)capabilities\.rs$|codex_plugins_dir|codex_link|cleanup|Manifest' || true
echo
echo "Locate capabilities.rs:"
fd -a 'capabilities\.rs$' . || true
echo
echo "Context around lines 460-515 in executor/src/local/capabilities.rs:"
if [ -f executor/src/local/capabilities.rs ]; then
nl -ba executor/src/local/capabilities.rs | sed -n '440,525p'
fi
echo
echo "Search for codex_manifest / codex_link usage:"
rg -n "codex_link|codex_plugins_dir|previous_codex_link|cleanup|remove_dir_all|symlink" executor/src/local/capabilities.rs || true
echo
echo "Rust version/source docs if available:"
rustc --version 2>/dev/null || true
if [ -d "${CARGO_HOME:-$HOME/.cargo}/registry/src" ]; then
rg -n "fn starts_with|is_sub_path" "${CARGO_HOME:-$HOME/.cargo}/registry/src" | head -20 || true
fi
echo
echo "Standalone lexical path component comparison model for ParentDir behavior (read-only):"
python3 - <<'PY'
from pathlib import PurePosixPath, PureWindowsPath
cases = [
("/app/codex_plugins/plugins", "/app/codex_plugins/plugins/../target"),
("/app/codex_plugins/plugins", "/app/codex_plugins/plugins/../../target"),
("/app/codex_plugins/plugins", "/app/codex_plugins/plugins/runtime"),
]
for base, link in cases:
b = PurePosixPath(base)
l = PurePosixPath(link)
# starts_with compares component lists; it does not canonicalize `..`
print(f"base={base} link={link} lexically_startswith={str(l).startswith(str(base) + '/') or str(l) == str(base)} parts_startswith={l.parts[:len(b.parts)]==b.parts}")
PYRepository: wecode-ai/Wegent
Length of output: 1072
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Lines 440-525:"
sed -n '440,525p' executor/src/local/capabilities.rs | cat -n
echo
echo "Relevant searches:"
rg -n "codex_link|codex_plugins_dir|previous_codex_link|cleanup|remove_dir_all|symlink|Manifest|manifest" executor/src/local/capabilities.rs || true
echo
echo "Rust standard-library source/source-doc equivalent:"
printf '\n--- Rust std/std::path in rust-lang source repo (if network available) ---\n'
curl -fsSL 'https://raw.githubusercontent.com/rust-lang/rust/master/library/std/src/path.rs' | sed -n '/impl Path/,+220p' | rg -n "starts_with|is_parent|component|Canonicalize|..|ParentDir|is_symlink" | head -120 || true
echo
echo "Local Rust std source if present:"
root="${CARGO_HOME:-$HOME/.cargo}/registry/src"
(
cd "$root" 2>/dev/null || exit 0
files="$(find . -path '*/library/std/src/path.rs' -type f | head -5)"
if [ -n "$files" ]; then
echo "$files" | while read -r f; do
echo "--- $f ---"
sed -n '/impl Path/,+220p' "$f" | rg -n "starts_with|is_parent|component|Canonicalize|..|ParentDir|is_symlink" | head -120
done
fi
) || trueRepository: wecode-ai/Wegent
Length of output: 16557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "remove_codex_plugin_runtime_path implementation:"
sed -n '510,555p' executor/src/local/capabilities.rs | cat -n
echo
echo "remove_existing_path definition and callers:"
rg -n "fn remove_existing_path|remove_existing_path\(" executor/src -S
echo
echo "Cargo Rust standard deps:"
rg -n 'name = "std" \{|package = "\{\s*name\s*=\s*"std"|path\s*=.*std|registry-index' Cargo.lock .cargo Cargo.toml 2>/dev/null || true
echo
echo "Manifest entry shape around runtime/codex_link:"
sed -n '850,875p' executor/src/local/capabilities.rs | cat -nRepository: wecode-ai/Wegent
Length of output: 4748
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "remove_existing_path source:"
fd -a 'capabilities/support.rs|capability.rs' executor/src/local | while read -r file; do
if rg -q "pub(super) fn remove_existing_path|fn remove_existing_path" "$file"; then
echo "--- $file ---"
rg -n -A 40 "fn remove_existing_path|pub\(super\) fn remove_existing_path" "$file"
fi
done
echo
echo "Path::starts_with lexical behavior model from Rust std library source if vendored:"
python3 - <<'PY'
import pathlib, subprocess, sys, tempfile, os, shutil
cases = [
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/../target"),
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/../../target"),
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/runtime"),
]
for base, link in cases:
b_path = pathlib.Path(base)
l_path = pathlib.Path(link)
stripped = try_strip_prefix(l_path, b_path)
has_parent = has_component_named(stripped, ".." if os.name != "nt" else "." if b_path.drive or base.startswith("\\\\") else "ParentDir")
print({"link": link, "prefix": base, "starts_with_model": link.startswith(base + "/") or link == base, "stripped": str(stripped), "stripped_has_parent_component_model": has_parent})
def try_strip_prefix(l, b):
if b.parents and l.parts[:len(b.parts)] == b.parts:
return pathlib.Path(*l.parts[len(b.parts):])
return l
def has_component_named(p, name):
for part in p.parts:
if part == "":
continue
if os.name != "nt":
if part == "..":
return True
else:
if part == name:
return True
return False
PY
echo
echo "Check for available Rust compiler and Path::starts_with source from installed std source if any:"
if command -v rustc >/dev/null 2>&1; then
rustc -vV || true
fi
find "${CARGO_HOME:-$HOME/.cargo}/registry/src" "${CARGO_HOME:-$HOME/.cargo}/git/checkouts" "$HOME/.cargo/registry/src" /tmp 2>/dev/null \
-path '*library/std/src/path.rs' -type f | head -5 | while read -r f; do
echo "--- $f ---"
rg -n -A 35 "fn starts_with|component = Component::ParentDir|Component::ParentDir|Component::Prefix|component" "$f" | head -120
done || trueRepository: wecode-ai/Wegent
Length of output: 1856
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Support source with remove_existing_path:"
rg -n -A 120 "pub\(super\) fn remove_existing_path|fn remove_existing_path" executor/src/local/capabilities/support.rs
echo
echo "Lexical path-prefix model for Rust Path::starts_with semantics:"
python3 - <<'PY'
from pathlib import PurePosixPath
cases = [
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/../target"),
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/../../target"),
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/runtime"),
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/.%2e/target"),
]
for base, link in cases:
b = PurePosixPath(base)
l = PurePosixPath(link)
stripped = None
if b.parts and l.parts and len(l.parts) >= len(blk):
pass
print({"link": link, "base_parts": b.parts, "link_parts": l.parts, "lexical_startswith": str(l).startswith(str(b) + "/") or str(l) == str(b), "prefix_has_parent_after_strip": ".." in l.parts[len(b.parts):]})
PY
echo
echo "Rust std library Path::starts_with from rust-lang source:"
python3 - <<'PY'
import urllib.request
url = "https://raw.githubusercontent.com/rust-lang/rust/master/library/std/src/path.rs"
text = urllib.request.urlopen(url, timeout=20).read().decode("utf-8", "replace")
needle = "pub fn starts_with"
pos = text.find(needle)
print("found", pos != -1)
if pos != -1:
print(text[pos:pos+2500])
PYRepository: wecode-ai/Wegent
Length of output: 2223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Rust std library Path::starts_with from rust-lang source:"
python3 - <<'PY'
import urllib.request
url = "https://raw.githubusercontent.com/rust-lang/rust/master/library/std/src/path.rs"
text = urllib.request.urlopen(url, timeout=20).read().decode("utf-8", "replace")
needle = "pub fn starts_with"
pos = text.find(needle)
print("found", pos != -1)
if pos != -1:
end = text.find("\n\n", pos)
print(text[pos:end] if end != -1 else text[pos:pos+2500])
PY
echo
echo "Lexical path-prefix model for Rust Path::starts_with semantics:"
python3 - <<'PY'
from pathlib import PurePosixPath
cases = [
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/../target"),
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/../../target"),
("/tmp/codex_plugins/plugins", "/tmp/codex_plugins/plugins/runtime"),
]
for base, link in cases:
b = PurePosixPath(base)
l = PurePosixPath(link)
# Rust Path::starts_with uses component prefix: ["component1", "component2", ...].
# It does not treat ".." as resolving for prefix comparison.
starts_with_model = b.parts and l.parts and l.parts[:len(b.parts)] == b.parts and len(l.parts) >= len(b.parts)
after_prefix = l.parts[len(b.parts):]
has_parent_after_prefix = ".." in after_prefix
print({"link": link, "base_parts": b.parts, "link_parts": l.parts, "lexical_startswith_model": starts_with_model, "after_prefix": after_prefix, "has_parent_after_prefix": has_parent_after_prefix})
PYRepository: wecode-ai/Wegent
Length of output: 2498
Reject parent-directory traversal before Codex cleanup.
CodexPluginsDir is checked with path.starts_with(...), but Path::starts_with compares components lexically and does not normalize ... A persisted codex_link such as <codex_plugins_dir>/../target can pass both checks and reach remove_existing_path, which may delete target outside .codex/plugins.
Reject ParentDir components after strip_prefix, or canonicalize the existing path before deletion. Add a regression test for a manifest value containing ...
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@executor/src/local/capabilities.rs` around lines 491 - 493, Update the path
validation logic in the block where previous_codex_link is compared against
self.codex_plugins_dir to protect against parent-directory traversal attacks.
The current starts_with check does not normalize .. components, allowing paths
like <codex_plugins_dir>/../target to bypass validation and reach
remove_existing_path. Either strip the prefix from previous_codex_link after the
starts_with check and reject the path if it contains ParentDir components, or
canonicalize previous_codex_link before performing the deletion check to ensure
it actually resolves within the intended directory bounds.
Summary by CodeRabbit
Bug Fixes
Tests