From cbe3de0e3839773eb594a675ce397f695ba8bf95 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 3 Jun 2026 11:10:51 +0000 Subject: [PATCH 1/3] fix(evolution): constrain pending skill paths Co-authored-by: EXboy --- .../src/evolution_desktop.rs | 1 + .../src/skill_synth/mod.rs | 76 ++++++++++++++++++- .../CONTEXT.md | 40 ++++++++++ .../PRD.md | 51 +++++++++++++ .../REVIEW.md | 29 +++++++ .../STATUS.md | 19 +++++ .../TASK.md | 68 +++++++++++++++++ tasks/board.md | 4 +- 8 files changed, 285 insertions(+), 3 deletions(-) create mode 100644 tasks/TASK-2026-067-pending-skill-path-safety/CONTEXT.md create mode 100644 tasks/TASK-2026-067-pending-skill-path-safety/PRD.md create mode 100644 tasks/TASK-2026-067-pending-skill-path-safety/REVIEW.md create mode 100644 tasks/TASK-2026-067-pending-skill-path-safety/STATUS.md create mode 100644 tasks/TASK-2026-067-pending-skill-path-safety/TASK.md diff --git a/crates/skilllite-commands/src/evolution_desktop.rs b/crates/skilllite-commands/src/evolution_desktop.rs index 48a93d69..3893cff9 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 3567318f..82328e31 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,63 @@ 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_path_traversal_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, "../../keep-me").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_absolute_paths_without_moving_escape_target() { + let temp = tempfile::tempdir().unwrap(); + let skills_root = temp.path().join("skills"); + let escape_target = temp.path().join("external-skill"); + std::fs::create_dir_all(skills_root.join("_evolved").join("_pending")).unwrap(); + std::fs::create_dir_all(&escape_target).unwrap(); + + let err = confirm_pending_skill(&skills_root, escape_target.to_str().unwrap()).unwrap_err(); + + assert!(err.to_string().contains("invalid pending skill name")); + assert!(escape_target.is_dir()); + assert!(!skills_root.join("_evolved").join("external-skill").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 00000000..60aa1617 --- /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 00000000..22d5cda0 --- /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 00000000..e5fd29ef --- /dev/null +++ b/tasks/TASK-2026-067-pending-skill-path-safety/REVIEW.md @@ -0,0 +1,29 @@ +# Review Report + +## Scope Reviewed + +- Files/modules: +- Commits/changes: + +## Findings + +- Critical: +- Major: +- Minor: + +## Quality Gates + +- Architecture boundary checks: `pass | fail` +- Security invariants: `pass | fail` +- Required tests executed: `pass | fail` +- Docs sync (EN/ZH): `pass | fail` + +## Test Evidence + +- Commands run: +- Key outputs: + +## Decision + +- Merge readiness: `ready | not ready` +- Follow-up actions: 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 00000000..079e05a3 --- /dev/null +++ b/tasks/TASK-2026-067-pending-skill-path-safety/STATUS.md @@ -0,0 +1,19 @@ +# 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. + +## Checkpoints + +- [x] PRD drafted before implementation (or `N/A` recorded) +- [x] Context drafted before implementation (or `N/A` recorded) +- [ ] Implementation complete +- [ ] Tests passed +- [ ] Review complete +- [ ] 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 00000000..ce0fce6f --- /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: `in_progress` +- 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 + +- [ ] `confirm_pending_skill`, `reject_pending_skill`, and desktop pending skill + reads reject absolute paths and `..` escapes. +- [ ] Valid pending skill names continue to confirm and reject normally. +- [ ] 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 43428f7b..8ad851d8 100644 --- a/tasks/board.md +++ b/tasks/board.md @@ -1,10 +1,10 @@ # 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 in progress) ## In Progress -- None. +- `TASK-2026-067-pending-skill-path-safety` - Status: `in_progress` - Owner: `agent` ## Ready From e5527721388b08a61508b7db339a132b828f7f70 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 3 Jun 2026 11:11:28 +0000 Subject: [PATCH 2/3] test(evolution): cover pending skill traversal Co-authored-by: EXboy --- .../skilllite-evolution/src/skill_synth/mod.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/crates/skilllite-evolution/src/skill_synth/mod.rs b/crates/skilllite-evolution/src/skill_synth/mod.rs index 82328e31..019bba94 100644 --- a/crates/skilllite-evolution/src/skill_synth/mod.rs +++ b/crates/skilllite-evolution/src/skill_synth/mod.rs @@ -304,7 +304,7 @@ mod tests { } #[test] - fn reject_pending_skill_rejects_path_traversal_without_deleting_escape_target() { + 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"); @@ -312,25 +312,26 @@ mod tests { 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, "../../keep-me").unwrap_err(); + 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_absolute_paths_without_moving_escape_target() { + fn confirm_pending_skill_rejects_path_traversal_without_moving_escape_target() { let temp = tempfile::tempdir().unwrap(); let skills_root = temp.path().join("skills"); - let escape_target = temp.path().join("external-skill"); std::fs::create_dir_all(skills_root.join("_evolved").join("_pending")).unwrap(); - std::fs::create_dir_all(&escape_target).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, escape_target.to_str().unwrap()).unwrap_err(); + let err = confirm_pending_skill(&skills_root, "../../external-skill").unwrap_err(); assert!(err.to_string().contains("invalid pending skill name")); - assert!(escape_target.is_dir()); - assert!(!skills_root.join("_evolved").join("external-skill").exists()); + assert!(escape_source.is_dir()); + assert!(!escape_destination.exists()); } #[test] From a02798c964393c37ebb81524a8fb6a2cd6760689 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Wed, 3 Jun 2026 11:15:29 +0000 Subject: [PATCH 3/3] docs(task): record pending skill path safety validation Co-authored-by: EXboy --- .../REVIEW.md | 42 +++++++++++++------ .../STATUS.md | 21 ++++++++-- .../TASK.md | 8 ++-- tasks/board.md | 5 ++- 4 files changed, 53 insertions(+), 23 deletions(-) diff --git a/tasks/TASK-2026-067-pending-skill-path-safety/REVIEW.md b/tasks/TASK-2026-067-pending-skill-path-safety/REVIEW.md index e5fd29ef..2c04bcfe 100644 --- a/tasks/TASK-2026-067-pending-skill-path-safety/REVIEW.md +++ b/tasks/TASK-2026-067-pending-skill-path-safety/REVIEW.md @@ -2,28 +2,44 @@ ## Scope Reviewed -- Files/modules: -- Commits/changes: +- 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: -- Major: -- Minor: +- 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 | fail` -- Security invariants: `pass | fail` -- Required tests executed: `pass | fail` -- Docs sync (EN/ZH): `pass | fail` +- 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: -- Key outputs: +- 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 | not ready` -- Follow-up actions: +- 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 index 079e05a3..3ffb6092 100644 --- a/tasks/TASK-2026-067-pending-skill-path-safety/STATUS.md +++ b/tasks/TASK-2026-067-pending-skill-path-safety/STATUS.md @@ -8,12 +8,25 @@ - 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) -- [ ] Implementation complete -- [ ] Tests passed -- [ ] Review complete -- [ ] Board updated +- [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 index ce0fce6f..6c014347 100644 --- a/tasks/TASK-2026-067-pending-skill-path-safety/TASK.md +++ b/tasks/TASK-2026-067-pending-skill-path-safety/TASK.md @@ -4,7 +4,7 @@ - Task ID: `TASK-2026-067` - Title: Constrain pending skill path operations -- Status: `in_progress` +- Status: `done` - Priority: `P0` - Owner: `agent` - Contributors: Cursor automation @@ -30,10 +30,10 @@ queue. The CLI and desktop bridge expose these operations. ## Acceptance Criteria -- [ ] `confirm_pending_skill`, `reject_pending_skill`, and desktop pending skill +- [x] `confirm_pending_skill`, `reject_pending_skill`, and desktop pending skill reads reject absolute paths and `..` escapes. -- [ ] Valid pending skill names continue to confirm and reject normally. -- [ ] Regression tests cover rejection behavior and prove out-of-scope +- [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 diff --git a/tasks/board.md b/tasks/board.md index 8ad851d8..3d3d0dcd 100644 --- a/tasks/board.md +++ b/tasks/board.md @@ -1,10 +1,10 @@ # Task Board -Last updated: 2026-06-03 (TASK-2026-067 pending skill path safety in progress) +Last updated: 2026-06-03 (TASK-2026-067 pending skill path safety done) ## In Progress -- `TASK-2026-067-pending-skill-path-safety` - Status: `in_progress` - Owner: `agent` +- None. ## Ready @@ -17,6 +17,7 @@ Last updated: 2026-06-03 (TASK-2026-067 pending skill path safety in progress) ## 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`