fix(api): prevent arbitrary deletion in DELETE /skills/by-path by validating and canonicalizing paths

[AkaraChen]

Motivation

• The DELETE /skills/by-path route accepted a caller-supplied source_path and agents and performed remove_dir_all on the derived directory, allowing deletion outside intended skill directories when agents was empty or when paths used lexical traversal (..).
• The endpoint treated nonexistent source_path values by deriving and deleting parent directories, which could recursively delete large parts of the filesystem.

Description

• Reject requests with an empty agents list early to avoid bypassing validation using if req.agents.is_empty() and return a clear error via a helper delete_by_path_response.
• Resolve the deletion candidate and allowed skill directories using canonicalization: added canonical_skill_dir, canonical_allowed_skills_paths and path_is_in_allowed_skill_dir helpers to perform containment checks on canonical paths rather than lexical starts_with.
• Compute the deletion target conservatively with source_skill_dir (use the directory itself or its parent for file targets) and only attempt deletion on the verified target directory (delete_dir), while treating nonexistent source_path as an idempotent no-op (return success without deleting a parent directory).
• Preserve plugin-managed protection by running detect_plugin_for_path against the resolved source directory and invoke remove_dir_all only on the validated delete_dir.
• Added unit tests for the new helpers and validation behaviors, covering missing paths, file-parent resolution, traversal rejection, skills-root rejection, and valid child paths.

Testing

• Ran cargo fmt -p aghub-api --check which passed.
• Ran repository checks (git diff --check) which passed.
• Attempted cargo test -p aghub-api and RUSTC_WRAPPER= cargo test -p aghub-api --offline, but full test execution was blocked by the environment (missing sccache wrapper binary and blocked network access / uncached dependencies), so unit tests added to the crate could not be executed here.
• The change includes focused unit tests in crates/api/src/routes/skills.rs for the new path validation helpers.

---

Codex Task

[coderabbitai]

Warning

Review limit reached

@AkaraChen, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 27 minutes and 19 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3f840d13-6048-4e78-a47f-d93e4d9f57b1

📥 Commits

Reviewing files that changed from the base of the PR and between 88dc33e and 6262643.

📒 Files selected for processing (1)

• crates/api/src/routes/skills.rs

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/fix-arbitrary-directory-deletion-vulnerability

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6262643061

ℹ️ 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".

[chatgpt-codex-connector]

Allow deleting symlinked skills by validating the link path

When a discovered skill is a symlink inside an agent's skills directory that points to a real directory elsewhere, this canonicalizes delete_dir and compares the target (skill_dir) against the allowed skills roots, so the request is rejected even though the link being deleted is under the valid skills path. Symlinked skills are supported elsewhere in the repo (discovery records canonical_path for symlink entries and the manager removes only the symlink), and the UI sends the displayed sourcePath, so users can no longer remove those valid symlinked skill installations via this endpoint.

Useful? React with 👍 / 👎.