diff --git a/crates/skilllite-commands/src/evolution_desktop.rs b/crates/skilllite-commands/src/evolution_desktop.rs index 48a93d6..3893cff 100644 --- a/crates/skilllite-commands/src/evolution_desktop.rs +++ b/crates/skilllite-commands/src/evolution_desktop.rs @@ -149,6 +149,7 @@ pub fn list_pending_skills(workspace: &str) -> Result> } pub fn read_pending_skill_md(workspace: &str, skill_name: &str) -> Result { + let skill_name = skilllite_evolution::skill_synth::validate_pending_skill_name(skill_name)?; let skills_root = resolve_skills_root(workspace)?; let path = skills_root .join("_evolved") diff --git a/crates/skilllite-evolution/src/skill_synth/mod.rs b/crates/skilllite-evolution/src/skill_synth/mod.rs index 3567318..019bba9 100644 --- a/crates/skilllite-evolution/src/skill_synth/mod.rs +++ b/crates/skilllite-evolution/src/skill_synth/mod.rs @@ -26,7 +26,7 @@ mod scan; mod validate; use std::collections::HashSet; -use std::path::Path; +use std::path::{Component, Path}; use rusqlite::Connection; @@ -242,7 +242,20 @@ pub fn list_pending_skills_with_review(skills_root: &Path) -> Vec<(String, bool) .collect() } +pub fn validate_pending_skill_name(skill_name: &str) -> Result<&str> { + if skill_name.trim().is_empty() || skill_name.chars().any(|c| matches!(c, '/' | '\\' | '\0')) { + bail!("invalid pending skill name: {}", skill_name); + } + + let mut components = Path::new(skill_name).components(); + match (components.next(), components.next()) { + (Some(Component::Normal(_)), None) => Ok(skill_name), + _ => bail!("invalid pending skill name: {}", skill_name), + } +} + pub fn confirm_pending_skill(skills_root: &Path, skill_name: &str) -> Result<()> { + let skill_name = validate_pending_skill_name(skill_name)?; let pending_dir = skills_root.join("_evolved").join("_pending"); let evolved_dir = skills_root.join("_evolved"); let src = pending_dir.join(skill_name); @@ -261,6 +274,7 @@ pub fn confirm_pending_skill(skills_root: &Path, skill_name: &str) -> Result<()> } pub fn reject_pending_skill(skills_root: &Path, skill_name: &str) -> Result<()> { + let skill_name = validate_pending_skill_name(skill_name)?; let pending_dir = skills_root.join("_evolved").join("_pending"); let src = pending_dir.join(skill_name); @@ -278,3 +292,64 @@ pub fn reject_pending_skill(skills_root: &Path, skill_name: &str) -> Result<()> pub use repair::{repair_one_skill, repair_skills}; pub use scan::track_skill_usage; pub use validate::{validate_skills, SkillValidation}; + +#[cfg(test)] +mod tests { + use super::*; + + fn create_pending_skill(skills_root: &Path, name: &str) { + let skill_dir = skills_root.join("_evolved").join("_pending").join(name); + std::fs::create_dir_all(&skill_dir).unwrap(); + std::fs::write(skill_dir.join("SKILL.md"), "# Test skill\n").unwrap(); + } + + #[test] + fn reject_pending_skill_rejects_absolute_path_without_deleting_escape_target() { + let temp = tempfile::tempdir().unwrap(); + let skills_root = temp.path().join("skills"); + let escape_target = temp.path().join("keep-me"); + std::fs::create_dir_all(skills_root.join("_evolved").join("_pending")).unwrap(); + std::fs::create_dir_all(&escape_target).unwrap(); + std::fs::write(escape_target.join("sentinel.txt"), "still here").unwrap(); + + let err = reject_pending_skill(&skills_root, escape_target.to_str().unwrap()).unwrap_err(); + + assert!(err.to_string().contains("invalid pending skill name")); + assert!(escape_target.join("sentinel.txt").is_file()); + } + + #[test] + fn confirm_pending_skill_rejects_path_traversal_without_moving_escape_target() { + let temp = tempfile::tempdir().unwrap(); + let skills_root = temp.path().join("skills"); + std::fs::create_dir_all(skills_root.join("_evolved").join("_pending")).unwrap(); + let escape_source = skills_root.join("external-skill"); + let escape_destination = temp.path().join("external-skill"); + std::fs::create_dir_all(&escape_source).unwrap(); + + let err = confirm_pending_skill(&skills_root, "../../external-skill").unwrap_err(); + + assert!(err.to_string().contains("invalid pending skill name")); + assert!(escape_source.is_dir()); + assert!(!escape_destination.exists()); + } + + #[test] + fn confirm_and_reject_pending_skill_accept_safe_names() { + let temp = tempfile::tempdir().unwrap(); + let skills_root = temp.path().join("skills"); + std::fs::create_dir_all(skills_root.join("_evolved").join("_pending")).unwrap(); + create_pending_skill(&skills_root, "safe-skill"); + create_pending_skill(&skills_root, "other-safe-skill"); + + confirm_pending_skill(&skills_root, "safe-skill").unwrap(); + reject_pending_skill(&skills_root, "other-safe-skill").unwrap(); + + assert!(skills_root.join("_evolved").join("safe-skill").is_dir()); + assert!(!skills_root + .join("_evolved") + .join("_pending") + .join("other-safe-skill") + .exists()); + } +} diff --git a/tasks/TASK-2026-067-pending-skill-path-safety/CONTEXT.md b/tasks/TASK-2026-067-pending-skill-path-safety/CONTEXT.md new file mode 100644 index 0000000..60aa161 --- /dev/null +++ b/tasks/TASK-2026-067-pending-skill-path-safety/CONTEXT.md @@ -0,0 +1,40 @@ +# Technical Context + +## Current State + +- Relevant crates/files: `crates/skilllite-evolution/src/skill_synth/mod.rs` + owns pending skill confirm/reject; `crates/skilllite-commands/src/evolution_desktop.rs` + owns desktop pending skill reads and delegates confirm/reject; `skilllite/src/dispatch/mod.rs` + exposes CLI dispatch. +- Current behavior: callers provide a `skill_name` string that is joined onto + `_evolved/_pending` without checking whether it is a single safe path segment. + +## Architecture Fit + +- Layer boundaries involved: entry CLI -> commands -> evolution library. The + invariant belongs in `skilllite-evolution` for mutating operations and in + `skilllite-commands` for the read helper that constructs its own path. +- Interfaces to preserve: command names, JSON output shape, and successful + operation semantics for safe pending skill directory names. + +## Dependency and Compatibility + +- New dependencies: none. +- Backward compatibility notes: names containing path separators were never + valid entries returned by pending skill listing, so rejecting them closes an + unsafe input path without changing listed pending skill behavior. + +## Design Decisions + +- Decision: validate identifiers as single normal path components before any + pending skill filesystem operation. + - Rationale: this directly addresses absolute-path and `..` traversal while + keeping the accepted identifier model aligned with `read_dir` listing. + - Alternatives considered: canonicalize after join and check ancestors. + - Why rejected: canonicalization requires the path to exist and still benefits + from segment validation; a single-component invariant is simpler and + sufficient for pending skill IDs. + +## Open Questions + +- [x] No open questions for the minimal security fix. diff --git a/tasks/TASK-2026-067-pending-skill-path-safety/PRD.md b/tasks/TASK-2026-067-pending-skill-path-safety/PRD.md new file mode 100644 index 0000000..22d5cda --- /dev/null +++ b/tasks/TASK-2026-067-pending-skill-path-safety/PRD.md @@ -0,0 +1,51 @@ +# PRD + +## Background + +Recent evolution L2 CLI work exposed pending skill confirmation and rejection +through desktop-facing commands. Those commands pass `skill_name` through to +filesystem operations that currently accept path separators and absolute paths. +This creates an arbitrary directory delete/move risk in a trusted local CLI and +desktop process. + +## Objective + +Constrain pending skill operations so the identifier can only name one pending +skill directory under `_evolved/_pending`, and preserve the existing valid +confirm/reject workflow. + +## Functional Requirements + +- FR-1: Reject pending skill identifiers containing path separators, absolute + path semantics, `.` or `..` segments, or empty/whitespace-only names. +- FR-2: Apply the same validation to pending skill read, confirm, and reject + paths. +- FR-3: Keep successful confirm/reject behavior unchanged for safe skill names. + +## Non-Functional Requirements + +- Security: pending skill operations must remain confined to the pending skill + directory and must not delete or move arbitrary filesystem paths. +- Performance: validation should be constant-time over a short identifier and + add no meaningful overhead. +- Compatibility: generated/listed pending skill names that are normal directory + names remain accepted. + +## Constraints + +- Technical: avoid new dependencies and keep the fix in shared lower-level code + where CLI and desktop callers both benefit. +- Timeline: immediate P0 bug fix; avoid broad refactors. + +## Success Metrics + +- Metric: traversal regression tests. +- Baseline: unsafe names can be joined outside `_pending`. +- Target: unsafe names return validation errors and out-of-scope directories are + left intact. + +## Rollout + +- Rollout plan: ship as a patch release / normal merge once tests pass. +- Rollback plan: revert the small validation change if it blocks legitimate + generated skill names; no data migration is involved. diff --git a/tasks/TASK-2026-067-pending-skill-path-safety/REVIEW.md b/tasks/TASK-2026-067-pending-skill-path-safety/REVIEW.md new file mode 100644 index 0000000..2c04bcf --- /dev/null +++ b/tasks/TASK-2026-067-pending-skill-path-safety/REVIEW.md @@ -0,0 +1,45 @@ +# Review Report + +## Scope Reviewed + +- Files/modules: `crates/skilllite-evolution/src/skill_synth/mod.rs`, + `crates/skilllite-commands/src/evolution_desktop.rs`, task artifacts. +- Commits/changes: recent evolution L2 CLI bridge paths for pending skill + read/confirm/reject. + +## Findings + +- Critical: `skill_name` was previously joined directly onto + `_evolved/_pending`; absolute paths could make `reject_pending_skill` delete + arbitrary directories and `..` paths could make `confirm_pending_skill` move + non-pending directories. +- Major: None. +- Minor: None. + +## Quality Gates + +- Architecture boundary checks: `pass` +- Security invariants: `pass` +- Required tests executed: `pass` +- Docs sync (EN/ZH): `pass` (no user-facing command, env, or policy wording + change; validation closes unsafe inputs only) + +## Test Evidence + +- Commands run: `cargo test -p skilllite-evolution pending_skill -- --nocapture`; + `cargo test -p skilllite-evolution`; `cargo test -p skilllite-commands`; + `cargo fmt --check`; `cargo clippy --all-targets -- -D warnings`; + `cargo test`; `python3 scripts/validate_tasks.py`. +- Key outputs: focused tests passed with + `3 passed; 0 failed; 94 filtered out`; `skilllite-evolution` passed + `97 passed; 0 failed`; `skilllite-commands` passed `23 passed; 0 failed`; + full workspace `cargo test` completed successfully; task validation passed + `67 task directories checked`. +- Environment note: the first test attempt failed before compilation because + Cargo 1.83 does not support edition 2024 dependencies; stable was updated to + Rust/Cargo 1.96 per repository instructions, then validation passed. + +## Decision + +- Merge readiness: `ready` +- Follow-up actions: None required for this PR. diff --git a/tasks/TASK-2026-067-pending-skill-path-safety/STATUS.md b/tasks/TASK-2026-067-pending-skill-path-safety/STATUS.md new file mode 100644 index 0000000..3ffb609 --- /dev/null +++ b/tasks/TASK-2026-067-pending-skill-path-safety/STATUS.md @@ -0,0 +1,32 @@ +# Status Journal + +## Timeline + +- 2026-06-03: + - Progress: Created task artifacts for a P0 pending skill path traversal + fix. Confirmed the vulnerable operations and defined validation/test plan. + - Blockers: None. + - Next step: Implement shared pending skill name validation and regression + tests. +- 2026-06-03: + - Progress: Implemented shared pending skill name validation in + `skilllite-evolution`, applied it to desktop pending skill reads, and added + regression tests for absolute-path deletion, `..` movement, and safe-name + compatibility. + - Blockers: Initial validation hit Rust 1.83 / Cargo edition-2024 support; + updated stable toolchain to Rust 1.96 per `AGENTS.md`. + - Next step: Open PR after final artifact/board verification. +- 2026-06-03: + - Progress: Completed final task artifact updates and board transition to + Done. + - Blockers: None. + - Next step: Open PR. + +## Checkpoints + +- [x] PRD drafted before implementation (or `N/A` recorded) +- [x] Context drafted before implementation (or `N/A` recorded) +- [x] Implementation complete +- [x] Tests passed +- [x] Review complete +- [x] Board updated diff --git a/tasks/TASK-2026-067-pending-skill-path-safety/TASK.md b/tasks/TASK-2026-067-pending-skill-path-safety/TASK.md new file mode 100644 index 0000000..6c01434 --- /dev/null +++ b/tasks/TASK-2026-067-pending-skill-path-safety/TASK.md @@ -0,0 +1,68 @@ +# TASK Card + +## Metadata + +- Task ID: `TASK-2026-067` +- Title: Constrain pending skill path operations +- Status: `done` +- Priority: `P0` +- Owner: `agent` +- Contributors: Cursor automation +- Created: `2026-06-03` +- Target milestone: critical bug fix + +## Problem + +Pending skill operations accept `skill_name` values and join them directly under +`/_evolved/_pending`. Absolute paths and `..` segments can escape that +directory; `reject_pending_skill` then calls `remove_dir_all` on the escaped +path, and `confirm_pending_skill` can move directories outside the pending +queue. The CLI and desktop bridge expose these operations. + +## Scope + +- In scope: validate pending skill names as safe single path segments before + read, confirm, or reject filesystem operations. +- In scope: add regression tests that prove path traversal inputs are rejected + without deleting or moving out-of-scope directories. +- Out of scope: redesign pending skill storage, UI contract changes, or broader + evolution policy changes. + +## Acceptance Criteria + +- [x] `confirm_pending_skill`, `reject_pending_skill`, and desktop pending skill + reads reject absolute paths and `..` escapes. +- [x] Valid pending skill names continue to confirm and reject normally. +- [x] Regression tests cover rejection behavior and prove out-of-scope + directories survive. + +## Risks + +- Risk: Overly strict validation could reject valid skill directories. + - Impact: Users may be unable to confirm or reject a generated pending skill. + - Mitigation: Match the safety boundary to existing listing behavior: only + single directory names are valid pending skill identifiers. + +## Validation Plan + +- Required tests: focused `skilllite-evolution` unit tests plus workspace Rust + format, clippy, and test commands required by repository policy. +- Commands to run: `cargo fmt --check`; `cargo test -p skilllite-evolution`; + `cargo test -p skilllite-commands`; `cargo clippy --all-targets -- -D warnings`; + `cargo test`; `python3 scripts/validate_tasks.py`. +- Manual checks: inspect the patched filesystem paths and task artifacts after + edits. + +## Regression Scope + +- Areas likely affected: evolution pending skill read/confirm/reject paths, + desktop evolution UI commands, CLI `skilllite evolution confirm|reject`. +- Explicit non-goals: changing generated skill naming rules beyond path safety, + adding new dependencies, or changing user-facing command names. + +## Links + +- Source TODO section: N/A; discovered during critical bug automation. +- Related PRs/issues: recent evolution L2 CLI bridge commits. +- Related docs: `spec/security-nonnegotiables.md`, + `spec/rust-conventions.md`, `spec/testing-policy.md`. diff --git a/tasks/board.md b/tasks/board.md index 43428f7..3d3d0dc 100644 --- a/tasks/board.md +++ b/tasks/board.md @@ -1,6 +1,6 @@ # Task Board -Last updated: 2026-05-29 (TASK-2026-066 utf8 evolution log truncate done) +Last updated: 2026-06-03 (TASK-2026-067 pending skill path safety done) ## In Progress @@ -17,6 +17,7 @@ Last updated: 2026-05-29 (TASK-2026-066 utf8 evolution log truncate done) ## Done +- `TASK-2026-067-pending-skill-path-safety` - Status: `done` - Owner: `agent` - `TASK-2026-066-utf8-evolution-log-truncate` - Status: `done` - Owner: `agent` - `TASK-2026-064-env-keys-single-source` - Status: `done` - Owner: `agent` - `TASK-2026-063-extension-tool-metadata-dispatch` - Status: `done` - Owner: `agent`