diff --git a/docs/API.md b/docs/API.md index 3d6ea0b..2727443 100644 --- a/docs/API.md +++ b/docs/API.md @@ -344,7 +344,7 @@ Implemented functions: Install options use the same concepts across languages: -- `appId` / `AppID` / `app_id`: owner id written to `.kitup.json` +- `appId` / `AppID` / `app_id`: non-empty owner id written to `.kitup.json` - `skillBundle` / `SkillBundle` / `skill_bundle`: local directory, embedded files, or public GitHub bundle - `scope`: `user` or `project` - `agents`: `"auto"`, `"*"`, or explicit host ids diff --git a/docs/host-adapter-contract.md b/docs/host-adapter-contract.md index 17472c4..99d50dc 100644 --- a/docs/host-adapter-contract.md +++ b/docs/host-adapter-contract.md @@ -11,6 +11,7 @@ Each host entry describes where a local Agent Skill can be installed and how the The first path is the canonical install target for that host. Later paths are compatible discovery roots that the host also scans. SDKs should install to the first path unless a caller explicitly requests another supported path. Project paths must be relative paths. User paths must be home-relative paths beginning with `~/`. +All adapter paths use `/` separators and non-empty segments; `..`, backslashes, colons, and NUL bytes are invalid. If multiple selected hosts resolve to the same target directory, SDKs must copy once and associate that installed target with every matching host. Shared roots such as `.agents/skills` are common and should not produce duplicate writes. diff --git a/go/kitup.go b/go/kitup.go index c0d09e9..89ccd9e 100644 --- a/go/kitup.go +++ b/go/kitup.go @@ -401,6 +401,10 @@ type normalizedSkillBundle struct { var skillNamePattern = regexp.MustCompile(`^[a-z0-9]+(-[a-z0-9]+)*$`) +func isValidSkillName(skillName string) bool { + return skillNamePattern.MatchString(skillName) +} + func LoadHostSpec(hostsFile string) ([]Host, error) { data := []byte(defaultHostsSpecJSON) if hostsFile != "" { @@ -414,9 +418,53 @@ func LoadHostSpec(hostsFile string) ([]Host, error) { if err := json.Unmarshal(data, &spec); err != nil { return nil, err } + if err := validateHostSpec(spec.Hosts); err != nil { + return nil, err + } return spec.Hosts, nil } +func validateHostSpec(hosts []Host) error { + for _, host := range hosts { + for _, path := range host.ProjectSkillsDir { + if !isProjectHostPath(path) { + return fmt.Errorf("invalid project path %q for host %q", path, host.ID) + } + } + for _, path := range host.UserSkillsDir { + if !isHomeHostPath(path) { + return fmt.Errorf("invalid user path %q for host %q", path, host.ID) + } + } + for _, path := range host.Detect { + if !isHomeHostPath(path) && !isProjectHostPath(path) { + return fmt.Errorf("invalid detect path %q for host %q", path, host.ID) + } + } + } + return nil +} + +func isProjectHostPath(path string) bool { + return path != "" && !strings.HasPrefix(path, "/") && !strings.HasPrefix(path, "~") && isSafeHostPath(path) +} + +func isHomeHostPath(path string) bool { + return strings.HasPrefix(path, "~/") && isSafeHostPath(path[2:]) +} + +func isSafeHostPath(path string) bool { + if strings.ContainsAny(path, "\x00\\:") { + return false + } + for _, segment := range strings.Split(path, "/") { + if segment == ".." || segment == "" { + return false + } + } + return true +} + func ResolveHosts(agents AgentSelector, hosts []Host) ([]Host, []map[string]any) { if agents.Kind == "*" { return hosts, []map[string]any{} @@ -522,6 +570,12 @@ func ResolveInstallSelection(opts InstallSelectionOptions) (InstallSelection, er } func ResolveInstallTargets(opts BaseOptions, agents AgentSelector, scope Scope, skillName string) ([]TargetGroup, []map[string]any, []string, error) { + if !isValidSkillName(skillName) { + return nil, []map[string]any{{ + "skillName": skillName, + "reason": "invalid-skill-name", + }}, nil, nil + } hosts, err := LoadHostSpec(opts.HostsFile) if err != nil { return nil, nil, nil, err @@ -644,7 +698,7 @@ func validateNormalizedSkill(bundle normalizedSkillBundle) SkillInfo { fields := parseFrontmatter(text[4 : 4+end]) name := fields["name"] description := fields["description"] - if !skillNamePattern.MatchString(name) || len(description) < 1 || len(description) > 1024 { + if !isValidSkillName(name) || len(description) < 1 || len(description) > 1024 { return SkillInfo{Valid: false, ErrorCode: "invalid-frontmatter"} } return SkillInfo{Valid: true, SkillName: name, Description: description} @@ -783,6 +837,9 @@ func UpdateBundledSkill(opts InstallOptions) (InstallReport, error) { } func UninstallBundledSkill(opts UninstallOptions) (UninstallReport, error) { + if opts.AppID == "" { + return emptyUninstallReport([]map[string]any{{"reason": "invalid-app-id"}}), nil + } targets, errs, _, err := ResolveInstallTargets(opts.BaseOptions, opts.Agents, opts.Scope, opts.SkillName) if err != nil { return UninstallReport{}, err @@ -794,7 +851,7 @@ func UninstallBundledSkill(opts UninstallOptions) (UninstallReport, error) { switch { case !present: report.Skipped = append(report.Skipped, withReason(result, "missing")) - case !managed: + case !managed || meta.SkillName != opts.SkillName: report.Conflicts = append(report.Conflicts, withReason(result, "unmanaged")) case meta.AppID != opts.AppID: report.Conflicts = append(report.Conflicts, withReason(result, "owner-mismatch")) @@ -809,6 +866,9 @@ func UninstallBundledSkill(opts UninstallOptions) (UninstallReport, error) { } func installOrPlan(opts InstallOptions, write bool) (InstallReport, error) { + if opts.AppID == "" { + return emptyInstallReport([]map[string]any{{"reason": "invalid-app-id"}}), nil + } bundle, bundleMeta, err := resolveSkillBundle(opts.SkillBundle) if err != nil { reason := "invalid-skill-bundle" @@ -838,7 +898,7 @@ func installOrPlan(opts InstallOptions, write bool) (InstallReport, error) { } } report.Installed = append(report.Installed, result) - case !managed: + case !managed || meta.SkillName != skill.SkillName: if opts.Force { if write { if err := replaceManagedSkill(bundle, target.TargetDir, opts.AppID, skill.SkillName, hash, bundleMeta); err != nil { @@ -888,21 +948,33 @@ func installOrPlan(opts InstallOptions, write bool) (InstallReport, error) { } func copyManagedSkill(bundle normalizedSkillBundle, targetDir, appID, skillName, hash string, bundleMeta bundleMetadata) error { - if err := os.RemoveAll(targetDir); err != nil { + tmp, err := makeStagingDir(targetDir) + if err != nil { return err } - if err := copySkillBundle(bundle, targetDir); err != nil { + if err := copySkillBundle(bundle, tmp); err != nil { + _ = os.RemoveAll(tmp) + return err + } + if err := writeMetadata(tmp, appID, skillName, hash, bundleMeta); err != nil { + _ = os.RemoveAll(tmp) + return err + } + if err := os.Rename(tmp, targetDir); err != nil { + _ = os.RemoveAll(tmp) return err } - return writeMetadata(targetDir, appID, skillName, hash, bundleMeta) + return nil } func replaceManagedSkill(bundle normalizedSkillBundle, targetDir, appID, skillName, hash string, bundleMeta bundleMetadata) error { - suffix := ".kitup-" + time.Now().Format("20060102150405.000000000") - tmp := targetDir + suffix - backup := targetDir + suffix + "-backup" - _ = os.RemoveAll(tmp) + tmp, err := makeStagingDir(targetDir) + if err != nil { + return err + } + backup := tmp + "-backup" if err := copySkillBundle(bundle, tmp); err != nil { + _ = os.RemoveAll(tmp) return err } if err := writeMetadata(tmp, appID, skillName, hash, bundleMeta); err != nil { @@ -923,6 +995,22 @@ func replaceManagedSkill(bundle normalizedSkillBundle, targetDir, appID, skillNa return os.RemoveAll(backup) } +func makeStagingDir(targetDir string) (string, error) { + parent := filepath.Dir(targetDir) + if err := os.MkdirAll(parent, 0o755); err != nil { + return "", err + } + tmp, err := os.MkdirTemp(parent, "."+filepath.Base(targetDir)+".kitup-") + if err != nil { + return "", err + } + if err := os.Chmod(tmp, 0o755); err != nil { + _ = os.RemoveAll(tmp) + return "", err + } + return tmp, nil +} + func copySkillBundle(bundle normalizedSkillBundle, dest string) error { if err := os.MkdirAll(dest, 0o755); err != nil { return err @@ -999,9 +1087,20 @@ func readMetadata(targetDir string) (metadata, bool, bool) { if err := json.Unmarshal(data, &meta); err != nil { return metadata{}, true, false } + if !isOwnedMetadata(meta) { + return metadata{}, true, false + } return meta, true, true } +func isOwnedMetadata(meta metadata) bool { + return meta.SchemaVersion == 1 && + meta.AppID != "" && + isValidSkillName(meta.SkillName) && + (meta.Source == "bundled" || meta.Source == "github") && + meta.Hash != "" +} + func targetResult(target TargetGroup) TargetResult { result := TargetResult{SkillName: target.SkillName, TargetDir: target.TargetDir} if len(target.HostIDs) == 1 { diff --git a/go/kitup_test.go b/go/kitup_test.go index e10033b..23bb6c3 100644 --- a/go/kitup_test.go +++ b/go/kitup_test.go @@ -97,7 +97,7 @@ func runCase(t *testing.T, tc goldenCase, home, workspace string) { }), tc.Expected["parsed"].(map[string]any)) case "resolve-install-selection": selection, err := ResolveInstallSelection(InstallSelectionOptions{ - BaseOptions: baseOptions(home, workspace), + BaseOptions: caseBaseOptions(tc, home, workspace), Scope: Scope(opts["scope"].(string)), Agents: agentSelector(opts["agents"]), Yes: boolValue(opts["yes"]), @@ -109,7 +109,7 @@ func runCase(t *testing.T, tc goldenCase, home, workspace string) { var out bytes.Buffer report, err := RunBundledSkillInstall(InstallWorkflowOptions{ InstallOptions: InstallOptions{ - BaseOptions: baseOptions(home, workspace), + BaseOptions: caseBaseOptions(tc, home, workspace), AppID: opts["appId"].(string), SkillBundle: skillBundleFromOptions(opts), Scope: Scope(stringValue(opts["scope"])), @@ -139,12 +139,21 @@ func runCase(t *testing.T, tc goldenCase, home, workspace string) { assertExpectedFileModes(t, tc, home, workspace) assertExpectedMetadata(t, tc, home, workspace) default: + base := caseBaseOptions(tc, home, workspace) if expected, ok := tc.Expected["detectedHosts"]; ok { - hosts, err := DetectHosts(BaseOptions{Home: home, CWD: workspace, HostsFile: repoPathFromCase("spec/hosts.json")}, Scope(opts["scope"].(string))) + hosts, err := DetectHosts(base, Scope(opts["scope"].(string))) must(t, err) equal(t, hostIDs(hosts), expected) } - report := runReportCase(t, tc, opts, home, workspace) + report, err := runReportCase(t, tc, opts, base) + if throws, ok := tc.Expected["throws"].(bool); ok && throws { + if err == nil { + t.Fatal("expected operation to throw") + } + assertExpectedFiles(t, tc, home, workspace) + return + } + must(t, err) if expected, ok := tc.Expected["report"]; ok { equal(t, report, expandValue(expected, home, workspace)) } @@ -155,19 +164,16 @@ func runCase(t *testing.T, tc goldenCase, home, workspace string) { } } -func runReportCase(t *testing.T, tc goldenCase, opts map[string]any, home, workspace string) any { - base := baseOptions(home, workspace) +func runReportCase(t *testing.T, tc goldenCase, opts map[string]any, base BaseOptions) (any, error) { switch tc.Operation { case "uninstall": - report, err := UninstallBundledSkill(UninstallOptions{ + return UninstallBundledSkill(UninstallOptions{ BaseOptions: base, AppID: opts["appId"].(string), SkillName: opts["skillName"].(string), Scope: Scope(opts["scope"].(string)), Agents: agentSelector(opts["agents"]), }) - must(t, err) - return report case "install", "update", "plan": fn := InstallBundledSkill if tc.Operation == "update" { @@ -176,18 +182,17 @@ func runReportCase(t *testing.T, tc goldenCase, opts map[string]any, home, works if tc.Operation == "plan" { fn = PlanBundledSkill } - report, err := fn(InstallOptions{ + return fn(InstallOptions{ BaseOptions: base, AppID: opts["appId"].(string), SkillBundle: skillBundleFromOptions(opts), Scope: Scope(opts["scope"].(string)), Agents: agentSelector(opts["agents"]), + Force: boolValue(opts["force"]), }) - must(t, err) - return report default: t.Fatalf("unsupported operation: %s", tc.Operation) - return nil + return nil, nil } } @@ -195,6 +200,19 @@ func baseOptions(home, workspace string) BaseOptions { return BaseOptions{Home: home, CWD: workspace, HostsFile: repoPathFromCase("spec/hosts.json")} } +func caseBaseOptions(tc goldenCase, home, workspace string) BaseOptions { + base := baseOptions(home, workspace) + if hostsFile, ok := tc.Options["hostsFile"].(string); ok && hostsFile != "" { + expanded := expandString(hostsFile, home, workspace) + if filepath.IsAbs(expanded) { + base.HostsFile = expanded + } else { + base.HostsFile = repoPathFromCase(expanded) + } + } + return base +} + func setupGiven(t *testing.T, tc goldenCase, home, workspace string) { for _, dir := range stringSlice(tc.Given["dirs"]) { must(t, os.MkdirAll(expandString(dir, home, workspace), 0o755)) diff --git a/python/src/kitup/_metadata.py b/python/src/kitup/_metadata.py index 207a823..d9e6436 100644 --- a/python/src/kitup/_metadata.py +++ b/python/src/kitup/_metadata.py @@ -1,8 +1,15 @@ from __future__ import annotations import json +import re from pathlib import Path +_SKILL_NAME_RE = re.compile(r"^[a-z0-9]+(-[a-z0-9]+)*$") + + +def is_valid_skill_name(skill_name: str) -> bool: + return bool(_SKILL_NAME_RE.fullmatch(skill_name)) + def write_install_metadata( target_dir: Path, @@ -43,4 +50,24 @@ def read_install_metadata(target_dir: Path) -> dict[str, object] | None: return None if not isinstance(payload, dict): return None + if not is_owned_metadata(payload): + return None return payload + + +def is_owned_metadata(payload: dict[str, object]) -> bool: + if payload.get("schemaVersion") != 1: + return False + app_id = payload.get("appId") + skill_name = payload.get("skillName") + source = payload.get("source") + digest = payload.get("hash") + return ( + isinstance(app_id, str) + and bool(app_id) + and isinstance(skill_name, str) + and is_valid_skill_name(skill_name) + and source in ("bundled", "github") + and isinstance(digest, str) + and bool(digest) + ) diff --git a/python/src/kitup/bundle.py b/python/src/kitup/bundle.py index 0faf397..b0cae0a 100644 --- a/python/src/kitup/bundle.py +++ b/python/src/kitup/bundle.py @@ -12,6 +12,7 @@ from importlib.abc import Traversable from ._github import fetch_github_directory +from ._metadata import is_valid_skill_name from ._paths import normalize_bundle_path, resolve_path, skip_name from .types import ( BundleFile, @@ -106,7 +107,11 @@ def validate_normalized_skill_bundle(normalized: NormalizedSkillBundle) -> Skill fields = _parse_frontmatter(match.group(1)) skill_name = fields.get("name", "") description = fields.get("description", "") - if not _valid_skill_name(skill_name) or not description or len(description) > 1024: + if ( + not is_valid_skill_name(skill_name) + or not description + or len(description) > 1024 + ): return SkillInfo(valid=False, error_code="invalid-frontmatter") return SkillInfo(valid=True, skill_name=skill_name, description=description) @@ -214,7 +219,3 @@ def _parse_frontmatter(content: str) -> dict[str, str]: key, value = line.split(":", 1) fields[key] = value.strip() return fields - - -def _valid_skill_name(name: str) -> bool: - return re.fullmatch(r"[a-z0-9]+(?:-[a-z0-9]+)*", name) is not None diff --git a/python/src/kitup/hosts.py b/python/src/kitup/hosts.py index 1726fb4..046b697 100644 --- a/python/src/kitup/hosts.py +++ b/python/src/kitup/hosts.py @@ -2,7 +2,7 @@ from pathlib import Path from ._hosts_generated import DEFAULT_HOSTS_SPEC_JSON -from .types import BaseOptions, Host, HostSpec, Scope +from .types import BaseOptions, Host, HostSpec, KitupError, Scope _GENERIC_DETECT_PATHS = {"~/.agents", "~/.agents/skills", "~/.config/agents"} @@ -11,21 +11,55 @@ def load_host_spec(hosts_file: str | None = None) -> HostSpec: raw = json.loads( Path(hosts_file).read_text() if hosts_file else DEFAULT_HOSTS_SPEC_JSON ) + hosts = [ + Host( + id=item["id"], + display_name=item["displayName"], + aliases=item.get("aliases", []), + project_skills_dirs=item["projectSkillsDirs"], + user_skills_dirs=item["userSkillsDirs"], + detect=item["detect"], + status=item["status"], + notes=item.get("notes", []), + ) + for item in raw["hosts"] + ] + _validate_host_spec(hosts) return HostSpec( schema_version=raw["schemaVersion"], - hosts=[ - Host( - id=item["id"], - display_name=item["displayName"], - aliases=item.get("aliases", []), - project_skills_dirs=item["projectSkillsDirs"], - user_skills_dirs=item["userSkillsDirs"], - detect=item["detect"], - status=item["status"], - notes=item.get("notes", []), - ) - for item in raw["hosts"] - ], + hosts=hosts, + ) + + +def _validate_host_spec(hosts: list[Host]) -> None: + for host in hosts: + for path in host.project_skills_dirs: + if not _is_project_host_path(path): + raise KitupError(f"invalid project path {path!r} for host {host.id}") + for path in host.user_skills_dirs: + if not _is_home_host_path(path): + raise KitupError(f"invalid user path {path!r} for host {host.id}") + for path in host.detect: + if not _is_home_host_path(path) and not _is_project_host_path(path): + raise KitupError(f"invalid detect path {path!r} for host {host.id}") + + +def _is_project_host_path(path: str) -> bool: + return ( + bool(path) + and not path.startswith("/") + and not path.startswith("~") + and _is_safe_host_path(path) + ) + + +def _is_home_host_path(path: str) -> bool: + return path.startswith("~/") and _is_safe_host_path(path[2:]) + + +def _is_safe_host_path(path: str) -> bool: + return not any(character in path for character in "\0\\:") and not any( + segment in ("..", "") for segment in path.split("/") ) diff --git a/python/src/kitup/install.py b/python/src/kitup/install.py index e62dbcc..6267181 100644 --- a/python/src/kitup/install.py +++ b/python/src/kitup/install.py @@ -6,7 +6,11 @@ from pathlib import Path from ._github import fetch_github_directory_with_metadata -from ._metadata import read_install_metadata, write_install_metadata +from ._metadata import ( + is_valid_skill_name, + read_install_metadata, + write_install_metadata, +) from .bundle import ( DirectoryBundle, FilesBundle, @@ -120,17 +124,6 @@ def write_managed_bundle( metadata: dict[str, object], replace: bool, ) -> None: - if not replace: - copy_normalized_bundle(files, target_dir) - write_bundle_metadata( - target_dir, - app_id=app_id, - skill_name=skill_name, - digest=digest, - metadata=metadata, - ) - return - target_dir.parent.mkdir(parents=True, exist_ok=True) staged_dir = Path( tempfile.mkdtemp( @@ -140,6 +133,7 @@ def write_managed_bundle( ) backup_dir: Path | None = None try: + staged_dir.chmod(0o755) copy_normalized_bundle(files, staged_dir) write_bundle_metadata( staged_dir, @@ -148,16 +142,19 @@ def write_managed_bundle( digest=digest, metadata=metadata, ) - backup_dir = Path( - tempfile.mkdtemp( - prefix=f".{target_dir.name}.kitup-old-", - dir=target_dir.parent, + if replace and target_dir.exists(): + backup_dir = Path( + tempfile.mkdtemp( + prefix=f".{target_dir.name}.kitup-old-", + dir=target_dir.parent, + ) ) - ) - backup_dir.rmdir() - target_dir.replace(backup_dir) + backup_dir.rmdir() + target_dir.replace(backup_dir) + staged_dir.replace(target_dir) + shutil.rmtree(backup_dir) + return staged_dir.replace(target_dir) - shutil.rmtree(backup_dir) except Exception: if backup_dir is not None and backup_dir.exists() and not target_dir.exists(): backup_dir.replace(target_dir) @@ -166,6 +163,9 @@ def write_managed_bundle( def install_or_plan(options: InstallOptions, *, write: bool) -> InstallReport: + if not options.app_id: + return empty_install_report([TargetError(reason="invalid-app-id")]) + try: normalized, bundle_metadata = _resolve_bundle_and_metadata( options.skill_bundle, cwd=options.base.cwd @@ -195,7 +195,6 @@ def install_or_plan(options: InstallOptions, *, write: bool) -> InstallReport: for target in targets: result = target_result(target) target_dir = Path(target.target_dir) - metadata_file = target_dir / ".kitup.json" metadata = read_install_metadata(target_dir) if not target_dir.exists(): @@ -212,23 +211,7 @@ def install_or_plan(options: InstallOptions, *, write: bool) -> InstallReport: report.installed.append(result) continue - if metadata is None and metadata_file.exists(): - if options.force: - if write: - write_managed_bundle( - target_dir, - app_id=options.app_id, - skill_name=info.skill_name, - digest=digest, - metadata=bundle_metadata, - files=normalized.files, - replace=True, - ) - report.updated.append(result) - else: - report.conflicts.append(target_status(target, "unmanaged")) - continue - if metadata is None: + if metadata is None or metadata.get("skillName") != info.skill_name: if options.force: if write: write_managed_bundle( @@ -328,6 +311,9 @@ def write_bundle_metadata( def uninstall_bundled_skill(options: UninstallOptions) -> UninstallReport: + if not options.app_id: + return empty_uninstall_report([TargetError(reason="invalid-app-id")]) + targets, errors = _resolve_install_targets_with_errors( options.base, options.agents, @@ -338,16 +324,12 @@ def uninstall_bundled_skill(options: UninstallOptions) -> UninstallReport: for target in targets: result = target_result(target) target_dir = Path(target.target_dir) - metadata_file = target_dir / ".kitup.json" metadata = read_install_metadata(target_dir) if not target_dir.exists(): report.skipped.append(target_status(target, "missing")) continue - if metadata is None and metadata_file.exists(): - report.conflicts.append(target_status(target, "unmanaged")) - continue - if metadata is None: + if metadata is None or metadata.get("skillName") != options.skill_name: report.conflicts.append(target_status(target, "unmanaged")) continue if metadata.get("appId") != options.app_id: @@ -381,6 +363,9 @@ def _resolve_install_targets_with_errors( scope: Scope, skill_name: str, ) -> tuple[list[TargetGroup], list[TargetError]]: + if not is_valid_skill_name(skill_name): + return [], [TargetError(reason="invalid-skill-name", skill_name=skill_name)] + spec = load_host_spec(options.hosts_file) home = Path(options.home).expanduser() if options.home else Path.home() cwd = Path(options.cwd) if options.cwd else Path.cwd() diff --git a/python/tests/golden_test.py b/python/tests/golden_test.py index e0cbbd3..13460ef 100644 --- a/python/tests/golden_test.py +++ b/python/tests/golden_test.py @@ -139,6 +139,14 @@ def run_case(case, home: Path, workspace: Path) -> None: ) assert [host.id for host in hosts] == case["expected"]["detectedHosts"] + if case["expected"].get("throws"): + try: + run_report_case(case, home, workspace) + except Exception: + assert_expected_files(case, home, workspace) + return + raise AssertionError("expected operation to throw") + report = run_report_case(case, home, workspace) if "report" in case["expected"]: assert normalize_value(report) == camel_to_snake_dict( @@ -371,12 +379,19 @@ def restore_github_env(env_backup) -> None: os.environ[key] = value +def case_hosts_file(case, home: Path, workspace: Path) -> str: + raw = case["options"].get("hostsFile", "spec/hosts.json") + if "$HOME" in raw or "$WORKSPACE" in raw: + return str(expand_path(raw, home, workspace)) + return str(repo_path(raw)) + + def install_options_from_case(case, home: Path, workspace: Path) -> InstallOptions: return InstallOptions( base=BaseOptions( home=str(home), cwd=str(workspace), - hosts_file=repo_path("spec/hosts.json"), + hosts_file=case_hosts_file(case, home, workspace), ), app_id=case["options"]["appId"], skill_bundle=skill_bundle_from_case(case), @@ -391,7 +406,7 @@ def uninstall_options_from_case(case, home: Path, workspace: Path) -> UninstallO base=BaseOptions( home=str(home), cwd=str(workspace), - hosts_file=repo_path("spec/hosts.json"), + hosts_file=case_hosts_file(case, home, workspace), ), app_id=case["options"]["appId"], skill_name=case["options"]["skillName"], @@ -407,7 +422,7 @@ def selection_options_from_case( base=BaseOptions( home=str(home), cwd=str(workspace), - hosts_file=repo_path("spec/hosts.json"), + hosts_file=case_hosts_file(case, home, workspace), ), scope=case["options"].get("scope", "user"), agents=case["options"].get("agents", "auto"), diff --git a/rust/src/lib.rs b/rust/src/lib.rs index 11c560e..7465e7c 100644 --- a/rust/src/lib.rs +++ b/rust/src/lib.rs @@ -7,7 +7,8 @@ use std::collections::{BTreeMap, HashMap, HashSet}; use std::fs; use std::io::{self, BufRead, Write}; use std::path::{Path, PathBuf}; -use std::time::{Duration, SystemTime, UNIX_EPOCH}; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::time::Duration; #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum Scope { @@ -326,8 +327,13 @@ pub struct InstallWorkflowExit { #[derive(Deserialize)] struct Metadata { + #[serde(rename = "schemaVersion")] + schema_version: u32, #[serde(rename = "appId")] app_id: String, + #[serde(rename = "skillName")] + skill_name: String, + source: String, hash: String, } @@ -460,9 +466,57 @@ pub fn load_host_spec(hosts_file: Option<&Path>) -> io::Result> { Some(path) => serde_json::from_slice(&fs::read(path)?)?, None => serde_json::from_str(hosts_generated::DEFAULT_HOSTS_SPEC_JSON)?, }; + validate_host_spec(&spec.hosts)?; Ok(spec.hosts) } +fn validate_host_spec(hosts: &[Host]) -> io::Result<()> { + for host in hosts { + for path in &host.project_skills_dirs { + if !is_project_host_path(path) { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("invalid project path {path:?} for host {}", host.id), + )); + } + } + for path in &host.user_skills_dirs { + if !is_home_host_path(path) { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("invalid user path {path:?} for host {}", host.id), + )); + } + } + for path in &host.detect { + if !is_home_host_path(path) && !is_project_host_path(path) { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!("invalid detect path {path:?} for host {}", host.id), + )); + } + } + } + Ok(()) +} + +fn is_project_host_path(path: &str) -> bool { + !path.is_empty() && !path.starts_with('/') && !path.starts_with('~') && is_safe_host_path(path) +} + +fn is_home_host_path(path: &str) -> bool { + path.starts_with("~/") && is_safe_host_path(&path[2..]) +} + +fn is_safe_host_path(path: &str) -> bool { + !path + .chars() + .any(|character| matches!(character, '\0' | '\\' | ':')) + && !path + .split('/') + .any(|segment| segment == ".." || segment.is_empty()) +} + pub fn resolve_hosts(agents: &AgentSelector, hosts: &[Host]) -> (Vec, Vec) { match agents { AgentSelector::All => (hosts.to_vec(), vec![]), @@ -608,6 +662,16 @@ pub fn resolve_install_targets( scope: Scope, skill_name: &str, ) -> io::Result<(Vec, Vec, Vec)> { + if !valid_skill_name(skill_name) { + return Ok(( + vec![], + vec![json!({ + "skillName": skill_name, + "reason": "invalid-skill-name" + })], + vec![], + )); + } let hosts = load_host_spec(options.hosts_file.as_deref())?; let (home, cwd) = defaults(options)?; let (selected, mut errors) = match agents { @@ -920,6 +984,11 @@ pub fn update_bundled_skill(options: &InstallOptions) -> io::Result io::Result { + if options.app_id.is_empty() { + return Ok(uninstall_report(vec![json!({ + "reason": "invalid-app-id" + })])); + } let (targets, errors, _) = resolve_install_targets( &options.base, &options.agents, @@ -932,6 +1001,9 @@ pub fn uninstall_bundled_skill(options: &UninstallOptions) -> io::Result report.skipped.push(with_reason(result, "missing")), MetadataState::Unmanaged => report.conflicts.push(with_reason(result, "unmanaged")), + MetadataState::Managed(meta) if meta.skill_name != options.skill_name => { + report.conflicts.push(with_reason(result, "unmanaged")) + } MetadataState::Managed(meta) if meta.app_id != options.app_id => { report.conflicts.push(with_reason(result, "owner-mismatch")) } @@ -945,6 +1017,11 @@ pub fn uninstall_bundled_skill(options: &UninstallOptions) -> io::Result io::Result { + if options.app_id.is_empty() { + return Ok(install_report(vec![json!({ + "reason": "invalid-app-id" + })])); + } let (bundle, bundle_metadata) = match resolve_skill_bundle(&options.skill_bundle) { Ok(value) => value, Err(_) => { @@ -1002,6 +1079,23 @@ fn install_or_plan(options: &InstallOptions, write: bool) -> io::Result { + if options.force { + if write { + replace_managed_skill( + &bundle, + &target.target_dir, + &options.app_id, + &skill_name, + &hash, + &bundle_metadata, + )?; + } + report.updated.push(result); + } else { + report.conflicts.push(with_reason(result, "unmanaged")); + } + } MetadataState::Managed(meta) if meta.app_id != options.app_id => { if options.force { if write { @@ -1067,9 +1161,17 @@ fn copy_managed_skill( hash: &str, bundle_metadata: &BundleMetadata, ) -> io::Result<()> { - let _ = fs::remove_dir_all(target_dir); - copy_skill_bundle(bundle, target_dir)?; - write_metadata(target_dir, app_id, skill_name, hash, bundle_metadata) + let tmp = make_staging_dir(target_dir)?; + if let Err(error) = (|| -> io::Result<()> { + copy_skill_bundle(bundle, &tmp)?; + write_metadata(&tmp, app_id, skill_name, hash, bundle_metadata)?; + fs::rename(&tmp, target_dir)?; + Ok(()) + })() { + let _ = fs::remove_dir_all(&tmp); + return Err(error); + } + Ok(()) } fn replace_managed_skill( @@ -1080,19 +1182,20 @@ fn replace_managed_skill( hash: &str, bundle_metadata: &BundleMetadata, ) -> io::Result<()> { - let suffix = format!( - ".kitup-{}", - SystemTime::now() - .duration_since(UNIX_EPOCH) - .unwrap() - .as_nanos() - ); - let tmp = PathBuf::from(format!("{}{}", target_dir.display(), suffix)); - let backup = PathBuf::from(format!("{}{}-backup", target_dir.display(), suffix)); - let _ = fs::remove_dir_all(&tmp); - copy_skill_bundle(bundle, &tmp)?; - write_metadata(&tmp, app_id, skill_name, hash, bundle_metadata)?; - fs::rename(target_dir, &backup)?; + let tmp = make_staging_dir(target_dir)?; + let backup = PathBuf::from(format!("{}-backup", tmp.display())); + if let Err(error) = copy_skill_bundle(bundle, &tmp) { + let _ = fs::remove_dir_all(&tmp); + return Err(error); + } + if let Err(error) = write_metadata(&tmp, app_id, skill_name, hash, bundle_metadata) { + let _ = fs::remove_dir_all(&tmp); + return Err(error); + } + if let Err(error) = fs::rename(target_dir, &backup) { + let _ = fs::remove_dir_all(&tmp); + return Err(error); + } if let Err(error) = fs::rename(&tmp, target_dir) { let _ = fs::remove_dir_all(&tmp); if !target_dir.exists() && backup.exists() { @@ -1103,6 +1206,37 @@ fn replace_managed_skill( fs::remove_dir_all(backup) } +static STAGING_COUNTER: AtomicU64 = AtomicU64::new(0); + +fn make_staging_dir(target_dir: &Path) -> io::Result { + let parent = target_dir.parent().ok_or_else(|| { + io::Error::new(io::ErrorKind::InvalidInput, "install target has no parent") + })?; + fs::create_dir_all(parent)?; + let name = target_dir + .file_name() + .ok_or_else(|| io::Error::new(io::ErrorKind::InvalidInput, "install target has no name"))? + .to_string_lossy(); + loop { + let candidate = parent.join(format!( + ".{name}.kitup-{}-{}", + std::process::id(), + STAGING_COUNTER.fetch_add(1, Ordering::Relaxed) + )); + match fs::create_dir(&candidate) { + Ok(()) => { + if let Err(error) = set_mode(&candidate, 0o755) { + let _ = fs::remove_dir_all(&candidate); + return Err(error); + } + return Ok(candidate); + } + Err(error) if error.kind() == io::ErrorKind::AlreadyExists => continue, + Err(error) => return Err(error), + } + } +} + fn copy_skill_bundle(bundle: &NormalizedSkillBundle, dest: &Path) -> io::Result<()> { fs::create_dir_all(dest)?; for file in &bundle.files { @@ -1179,12 +1313,20 @@ fn read_metadata(target_dir: &Path) -> MetadataState { let Ok(data) = fs::read(target_dir.join(".kitup.json")) else { return MetadataState::Unmanaged; }; - match serde_json::from_slice(&data) { - Ok(meta) => MetadataState::Managed(meta), - Err(_) => MetadataState::Unmanaged, + match serde_json::from_slice::(&data) { + Ok(meta) if is_owned_metadata(&meta) => MetadataState::Managed(meta), + _ => MetadataState::Unmanaged, } } +fn is_owned_metadata(meta: &Metadata) -> bool { + meta.schema_version == 1 + && !meta.app_id.is_empty() + && valid_skill_name(&meta.skill_name) + && (meta.source == "bundled" || meta.source == "github") + && !meta.hash.is_empty() +} + fn target_result(target: &TargetGroup) -> TargetResult { if target.host_ids.len() == 1 { TargetResult { diff --git a/rust/tests/golden.rs b/rust/tests/golden.rs index 747bfed..6783ce8 100644 --- a/rust/tests/golden.rs +++ b/rust/tests/golden.rs @@ -266,19 +266,19 @@ fn run_case(case: &GoldenCase, home: &Path, workspace: &Path) { assert_expected_metadata(case, home, workspace); } _ => { + let base = case_base_options(options, home, workspace); if let Some(expected) = case.expected.get("detectedHosts") { - let hosts = detect_hosts( - &BaseOptions { - home: Some(home.to_path_buf()), - cwd: Some(workspace.to_path_buf()), - hosts_file: Some(repo_path("spec/hosts.json")), - }, - Some(scope(options["scope"].as_str().unwrap())), - ) - .unwrap(); + let hosts = + detect_hosts(&base, Some(scope(options["scope"].as_str().unwrap()))).unwrap(); assert_json_eq(&json!(host_ids(&hosts)), expected.clone()); } - let report = run_report_case(case, options, home, workspace); + let result = run_report_case(case, options, base); + if case.expected.get("throws").and_then(Value::as_bool) == Some(true) { + assert!(result.is_err(), "expected operation to throw"); + assert_expected_files(case, home, workspace); + return; + } + let report = result.unwrap(); if let Some(expected) = case.expected.get("report") { assert_json_eq(&report, expand_value(expected, home, workspace)); } @@ -290,29 +290,41 @@ fn run_case(case: &GoldenCase, home: &Path, workspace: &Path) { } } +fn case_base_options(options: &Map, home: &Path, workspace: &Path) -> BaseOptions { + let hosts_file = match options.get("hostsFile").and_then(Value::as_str) { + Some(path) => { + let expanded = expand_value(&Value::String(path.to_string()), home, workspace); + let expanded = expanded.as_str().unwrap(); + if Path::new(expanded).is_absolute() { + PathBuf::from(expanded) + } else { + repo_path(expanded) + } + } + None => repo_path("spec/hosts.json"), + }; + BaseOptions { + home: Some(home.to_path_buf()), + cwd: Some(workspace.to_path_buf()), + hosts_file: Some(hosts_file), + } +} + fn run_report_case( case: &GoldenCase, options: &Map, - home: &Path, - workspace: &Path, -) -> Value { - let base = BaseOptions { - home: Some(home.to_path_buf()), - cwd: Some(workspace.to_path_buf()), - hosts_file: Some(repo_path("spec/hosts.json")), - }; + base: BaseOptions, +) -> Result> { match case.operation.as_str() { - "uninstall" => serde_json::to_value( - uninstall_bundled_skill(&UninstallOptions { + "uninstall" => Ok(serde_json::to_value(uninstall_bundled_skill( + &UninstallOptions { base, app_id: options["appId"].as_str().unwrap().to_string(), skill_name: options["skillName"].as_str().unwrap().to_string(), scope: scope(options["scope"].as_str().unwrap()), agents: agent_selector(&options["agents"]), - }) - .unwrap(), - ) - .unwrap(), + }, + )?)?), "install" | "update" | "plan" => { let options = InstallOptions { base, @@ -326,9 +338,9 @@ fn run_report_case( .unwrap_or(false), }; match case.operation.as_str() { - "update" => serde_json::to_value(update_bundled_skill(&options).unwrap()).unwrap(), - "plan" => serde_json::to_value(plan_bundled_skill(&options).unwrap()).unwrap(), - _ => serde_json::to_value(install_bundled_skill(&options).unwrap()).unwrap(), + "update" => Ok(serde_json::to_value(update_bundled_skill(&options)?)?), + "plan" => Ok(serde_json::to_value(plan_bundled_skill(&options)?)?), + _ => Ok(serde_json::to_value(install_bundled_skill(&options)?)?), } } other => panic!("unsupported operation: {other}"), diff --git a/scripts/check.mjs b/scripts/check.mjs index d5fd7f0..c67bbff 100755 --- a/scripts/check.mjs +++ b/scripts/check.mjs @@ -33,13 +33,31 @@ function shouldRun(group) { return selectedGroups.size === 0 || selectedGroups.has(group); } -function validateHosts(spec) { +function validateHosts(spec, schema) { assert(spec.schemaVersion === 1, "hosts schemaVersion must be 1"); assert(Array.isArray(spec.hosts), "hosts must be an array"); const idPattern = /^[a-z0-9]+(-[a-z0-9]+)*$/; - const projectPattern = /^(?!\/)(?!~)(?!.*(^|\/)\.\.(\/|$))[^\0]+$/; - const homePattern = /^~\/[^\0]+$/; + const projectPattern = new RegExp(schema.$defs.projectPath.pattern); + const homePattern = new RegExp(schema.$defs.homePath.pattern); + for (const path of [ + "../outside", + "..\\outside", + "C:/outside", + "a//b", + "a/", + ]) { + assert(!projectPattern.test(path), `unsafe project path accepted: ${path}`); + } + for (const path of [ + "~/../outside", + "~/..\\outside", + "~/C:/outside", + "~//a", + "~/a/", + ]) { + assert(!homePattern.test(path), `unsafe user path accepted: ${path}`); + } const statuses = new Set([ "verified", "documented", @@ -280,10 +298,10 @@ function validateReleaseWorkflow() { const hostsSpec = readJson("spec/hosts.json"); const cases = readJson("testdata/cases/bundled-skill-install.json"); -readJson("spec/hosts.schema.json"); +const hostsSchema = readJson("spec/hosts.schema.json"); readJson("testdata/cases.schema.json"); -const { ids, aliases } = validateHosts(hostsSpec); +const { ids, aliases } = validateHosts(hostsSpec, hostsSchema); validateCases(cases, hostsSpec.hosts); validateFixtures(); validateVersions(); @@ -332,7 +350,13 @@ for (const [group, name, command, args, cwd, env] of [ rootPath, ], ["typescript", "typescript", "pnpm", ["--dir", "ts", "test"], rootPath], - ["go", "go", "go", ["test", "./..."], new URL("../go/", import.meta.url)], + [ + "go", + "go", + "go", + ["test", "-count=1", "./..."], + new URL("../go/", import.meta.url), + ], [ "go", "go-cobra", diff --git a/spec/hosts.schema.json b/spec/hosts.schema.json index 407b1dc..67ed375 100644 --- a/spec/hosts.schema.json +++ b/spec/hosts.schema.json @@ -115,12 +115,12 @@ "projectPath": { "type": "string", "minLength": 1, - "pattern": "^(?!/)(?!~)(?!.*(^|/)\\.\\.(/|$))[^\\u0000]+$" + "pattern": "^(?!/)(?!~)(?!.*(^|/)\\.\\.(/|$))[^/\\u0000:\\\\]+(?:/[^/\\u0000:\\\\]+)*$" }, "homePath": { "type": "string", "minLength": 3, - "pattern": "^~/[^\\u0000]+$" + "pattern": "^(?!.*(^|/)\\.\\.(/|$))~/[^/\\u0000:\\\\]+(?:/[^/\\u0000:\\\\]+)*$" }, "detectPath": { "anyOf": [ diff --git a/testdata/cases/bundled-skill-install.json b/testdata/cases/bundled-skill-install.json index d7f73ad..fcc5407 100644 --- a/testdata/cases/bundled-skill-install.json +++ b/testdata/cases/bundled-skill-install.json @@ -476,7 +476,10 @@ "filesPresent": [ "$WORKSPACE/.agents/skills/basic/SKILL.md", "$WORKSPACE/.agents/skills/basic/.kitup.json" - ] + ], + "fileModes": { + "$WORKSPACE/.agents/skills/basic": "755" + } } }, { @@ -1252,6 +1255,156 @@ } } }, + { + "id": "install-incomplete-metadata-conflict", + "operation": "install", + "description": "Treats incomplete .kitup.json as unmanaged and refuses overwrite.", + "options": { + "appId": "example-cli", + "skillBundleDir": "testdata/skills/basic", + "scope": "user", + "agents": [ + "codex" + ], + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [ + "$HOME/.agents/skills/basic" + ], + "files": { + "$HOME/.agents/skills/basic/.kitup.json": { + "appId": "example-cli" + } + } + }, + "expected": { + "report": { + "installed": [], + "updated": [], + "skipped": [], + "conflicts": [ + { + "hostId": "codex", + "skillName": "basic", + "targetDir": "$HOME/.agents/skills/basic", + "reason": "unmanaged" + } + ], + "errors": [] + }, + "filesPresent": [ + "$HOME/.agents/skills/basic/.kitup.json" + ] + } + }, + { + "id": "install-skill-name-mismatch-conflict", + "operation": "install", + "description": "Treats metadata for a different skill name as unmanaged and refuses overwrite.", + "options": { + "appId": "example-cli", + "skillBundleDir": "testdata/skills/basic", + "scope": "user", + "agents": [ + "codex" + ], + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [ + "$HOME/.agents/skills/basic" + ], + "files": { + "$HOME/.agents/skills/basic/.kitup.json": { + "schemaVersion": 1, + "appId": "example-cli", + "skillName": "other", + "source": "bundled", + "hash": "sha256:old" + } + } + }, + "expected": { + "report": { + "installed": [], + "updated": [], + "skipped": [], + "conflicts": [ + { + "hostId": "codex", + "skillName": "basic", + "targetDir": "$HOME/.agents/skills/basic", + "reason": "unmanaged" + } + ], + "errors": [] + }, + "filesPresent": [ + "$HOME/.agents/skills/basic/.kitup.json" + ] + } + }, + { + "id": "update-skill-name-mismatch-with-force", + "operation": "update", + "description": "Force replaces metadata for a different skill name and records the requested owner.", + "options": { + "appId": "example-cli", + "skillBundleDir": "testdata/skills/basic", + "scope": "user", + "agents": [ + "codex" + ], + "force": true, + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [ + "$HOME/.agents/skills/basic" + ], + "files": { + "$HOME/.agents/skills/basic/.kitup.json": { + "schemaVersion": 1, + "appId": "example-cli", + "skillName": "other", + "source": "bundled", + "hash": "sha256:old" + } + } + }, + "expected": { + "report": { + "installed": [], + "updated": [ + { + "hostId": "codex", + "skillName": "basic", + "targetDir": "$HOME/.agents/skills/basic" + } + ], + "skipped": [], + "conflicts": [], + "errors": [] + }, + "filesPresent": [ + "$HOME/.agents/skills/basic/SKILL.md" + ], + "metadata": { + "path": "$HOME/.agents/skills/basic/.kitup.json", + "hash": "from-skill-bundle-dir", + "fields": { + "schemaVersion": 1, + "appId": "example-cli", + "skillName": "basic", + "source": "bundled" + } + } + } + }, { "id": "different-owner-conflict", "operation": "install", @@ -2526,6 +2679,395 @@ "errors": [] } } + }, + { + "id": "uninstall-rejects-path-traversal-skill-name", + "operation": "uninstall", + "description": "Refuses skill names that escape the install root and never deletes a sibling owned install.", + "options": { + "appId": "example-cli", + "skillName": "../victim", + "scope": "user", + "agents": [ + "codex" + ], + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [ + "$HOME/.agents/victim" + ], + "files": { + "$HOME/.agents/victim/.kitup.json": { + "schemaVersion": 1, + "appId": "example-cli", + "skillName": "victim", + "source": "bundled", + "hash": "sha256:abc" + } + } + }, + "expected": { + "report": { + "removed": [], + "skipped": [], + "conflicts": [], + "errors": [ + { + "skillName": "../victim", + "reason": "invalid-skill-name" + } + ] + }, + "filesPresent": [ + "$HOME/.agents/victim/.kitup.json" + ] + } + }, + { + "id": "uninstall-incomplete-metadata-conflict", + "operation": "uninstall", + "description": "Treats incomplete .kitup.json as unmanaged and refuses deletion.", + "options": { + "appId": "example-cli", + "skillName": "basic", + "scope": "user", + "agents": [ + "codex" + ], + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [ + "$HOME/.agents/skills/basic" + ], + "files": { + "$HOME/.agents/skills/basic/.kitup.json": { + "appId": "example-cli" + } + } + }, + "expected": { + "report": { + "removed": [], + "skipped": [], + "conflicts": [ + { + "hostId": "codex", + "skillName": "basic", + "targetDir": "$HOME/.agents/skills/basic", + "reason": "unmanaged" + } + ], + "errors": [] + }, + "filesPresent": [ + "$HOME/.agents/skills/basic/.kitup.json" + ] + } + }, + { + "id": "uninstall-skill-name-mismatch-conflict", + "operation": "uninstall", + "description": "Refuses deletion when metadata skillName does not exactly match the requested skill.", + "options": { + "appId": "example-cli", + "skillName": "basic", + "scope": "user", + "agents": [ + "codex" + ], + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [ + "$HOME/.agents/skills/basic" + ], + "files": { + "$HOME/.agents/skills/basic/.kitup.json": { + "schemaVersion": 1, + "appId": "example-cli", + "skillName": "other", + "source": "bundled", + "hash": "sha256:abc" + } + } + }, + "expected": { + "report": { + "removed": [], + "skipped": [], + "conflicts": [ + { + "hostId": "codex", + "skillName": "basic", + "targetDir": "$HOME/.agents/skills/basic", + "reason": "unmanaged" + } + ], + "errors": [] + }, + "filesPresent": [ + "$HOME/.agents/skills/basic/.kitup.json" + ] + } + }, + { + "id": "hosts-file-rejects-escaped-project-path", + "operation": "install", + "description": "Rejects a custom hosts file whose project path escapes the workspace root.", + "options": { + "appId": "example-cli", + "skillBundleDir": "testdata/skills/basic", + "scope": "project", + "agents": [ + "evil" + ], + "hostsFile": "$WORKSPACE/hosts.json", + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [], + "files": { + "$WORKSPACE/hosts.json": { + "schemaVersion": 1, + "hosts": [ + { + "id": "evil", + "displayName": "Evil", + "projectSkillsDirs": [ + "../outside" + ], + "userSkillsDirs": [], + "detect": [ + ".agents" + ], + "status": "community" + } + ] + } + } + }, + "expected": { + "throws": true, + "filesAbsent": [ + "$WORKSPACE/../outside/basic", + "$HOME/../outside/basic" + ] + } + }, + { + "id": "hosts-file-rejects-windows-project-path", + "operation": "install", + "description": "Rejects a custom hosts file whose Windows-style project path escapes the workspace root.", + "options": { + "appId": "example-cli", + "skillBundleDir": "testdata/skills/basic", + "scope": "project", + "agents": [ + "evil" + ], + "hostsFile": "$WORKSPACE/hosts.json", + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [], + "files": { + "$WORKSPACE/hosts.json": { + "schemaVersion": 1, + "hosts": [ + { + "id": "evil", + "displayName": "Evil", + "projectSkillsDirs": [ + "..\\outside" + ], + "userSkillsDirs": [], + "detect": [ + ".agents" + ], + "status": "community" + } + ] + } + } + }, + "expected": { + "throws": true, + "filesAbsent": [ + "$WORKSPACE/../outside/basic" + ] + } + }, + { + "id": "hosts-file-rejects-windows-home-path", + "operation": "install", + "description": "Rejects a custom hosts file whose home path contains a Windows volume path.", + "options": { + "appId": "example-cli", + "skillBundleDir": "testdata/skills/basic", + "scope": "user", + "agents": [ + "evil" + ], + "hostsFile": "$WORKSPACE/hosts.json", + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [], + "files": { + "$WORKSPACE/hosts.json": { + "schemaVersion": 1, + "hosts": [ + { + "id": "evil", + "displayName": "Evil", + "projectSkillsDirs": [], + "userSkillsDirs": [ + "~/C:/outside" + ], + "detect": [ + "~/.agents" + ], + "status": "community" + } + ] + } + } + }, + "expected": { + "throws": true, + "filesAbsent": [ + "$HOME/C:/outside/basic" + ] + } + }, + { + "id": "install-rejects-empty-app-id", + "operation": "install", + "description": "Rejects an empty owner id before writing install metadata.", + "options": { + "appId": "", + "skillBundleDir": "testdata/skills/basic", + "scope": "user", + "agents": [ + "codex" + ], + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [ + "$HOME/.agents/skills" + ], + "files": {} + }, + "expected": { + "report": { + "installed": [], + "updated": [], + "skipped": [], + "conflicts": [], + "errors": [ + { + "reason": "invalid-app-id" + } + ] + }, + "filesAbsent": [ + "$HOME/.agents/skills/basic" + ] + } + }, + { + "id": "uninstall-rejects-empty-app-id", + "operation": "uninstall", + "description": "Rejects an empty owner id before considering a managed install for deletion.", + "options": { + "appId": "", + "skillName": "basic", + "scope": "user", + "agents": [ + "codex" + ], + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [ + "$HOME/.agents/skills/basic" + ], + "files": { + "$HOME/.agents/skills/basic/.kitup.json": { + "schemaVersion": 1, + "appId": "example-cli", + "skillName": "basic", + "source": "bundled", + "hash": "sha256:abc" + } + } + }, + "expected": { + "report": { + "removed": [], + "skipped": [], + "conflicts": [], + "errors": [ + { + "reason": "invalid-app-id" + } + ] + }, + "filesPresent": [ + "$HOME/.agents/skills/basic/.kitup.json" + ] + } + }, + { + "id": "initial-install-copy-failure-is-atomic", + "operation": "install", + "description": "Leaves no target behind when a new install fails while copying its bundle.", + "options": { + "appId": "example-cli", + "skillFiles": [ + { + "path": "SKILL.md", + "contents": "---\nname: atomic-failure\ndescription: Atomic initial install fixture.\n---\n" + }, + { + "path": "a", + "contents": "file\n" + }, + { + "path": "a/b", + "contents": "nested\n" + } + ], + "scope": "user", + "agents": [ + "codex" + ], + "home": "$HOME", + "cwd": "$WORKSPACE" + }, + "given": { + "dirs": [ + "$HOME/.agents/skills" + ], + "files": {} + }, + "expected": { + "throws": true, + "filesAbsent": [ + "$HOME/.agents/skills/atomic-failure" + ] + } } ] } diff --git a/ts/src/index.ts b/ts/src/index.ts index dc0ff21..0c74fb5 100644 --- a/ts/src/index.ts +++ b/ts/src/index.ts @@ -2,6 +2,7 @@ import { createHash } from "node:crypto"; import { chmod, mkdir, + mkdtemp, lstat, readdir, readFile, @@ -11,7 +12,7 @@ import { writeFile, } from "node:fs/promises"; import { homedir } from "node:os"; -import { dirname, join, relative, resolve, sep } from "node:path"; +import { basename, dirname, join, relative, resolve, sep } from "node:path"; import { fileURLToPath } from "node:url"; import { defaultHostsSpecJson } from "./hosts.generated.js"; @@ -177,8 +178,20 @@ export type SkillError = { reason: SkillInfo["errorCode"] }; export type BundleError = { reason: "bundle-resolve-failed"; }; +export type InvalidSkillNameError = { + skillName: string; + reason: "invalid-skill-name"; +}; +export type InvalidAppIdError = { reason: "invalid-app-id" }; export type TargetError = - UnknownHostError | UnsupportedScopeError | SkillError | BundleError; + | UnknownHostError + | UnsupportedScopeError + | SkillError + | BundleError + | InvalidSkillNameError + | InvalidAppIdError; + +const skillNamePattern = /^[a-z0-9]+(-[a-z0-9]+)*$/; export interface InstallReport { installed: TargetResult[]; @@ -377,11 +390,65 @@ export function installFlagError( } export async function loadHostSpec(hostsFile?: string): Promise { - return JSON.parse( + const spec = JSON.parse( hostsFile ? await readFile(hostsFile, "utf8") : defaultHostsSpecJson, + ) as HostSpec; + validateHostSpec(spec); + return spec; +} + +function validateHostSpec(spec: HostSpec) { + for (const host of spec.hosts ?? []) { + for (const path of host.projectSkillsDirs ?? []) { + if (!isProjectHostPath(path)) { + throw new Error( + `invalid project path ${JSON.stringify(path)} for host ${host.id}`, + ); + } + } + for (const path of host.userSkillsDirs ?? []) { + if (!isHomeHostPath(path)) { + throw new Error( + `invalid user path ${JSON.stringify(path)} for host ${host.id}`, + ); + } + } + for (const path of host.detect ?? []) { + if (!isHomeHostPath(path) && !isProjectHostPath(path)) { + throw new Error( + `invalid detect path ${JSON.stringify(path)} for host ${host.id}`, + ); + } + } + } +} + +function isProjectHostPath(path: string) { + return ( + !!path && + !path.startsWith("/") && + !path.startsWith("~") && + isSafeHostPath(path) + ); +} + +function isHomeHostPath(path: string) { + return path.startsWith("~/") && isSafeHostPath(path.slice(2)); +} + +function isSafeHostPath(path: string) { + return ( + !path.includes("\0") && + !path.includes("\\") && + !path.includes(":") && + !path.split("/").some((segment) => segment === ".." || segment === "") ); } +function isValidSkillName(skillName: string) { + return skillNamePattern.test(skillName); +} + export async function resolveHosts(options: { agents: AgentSelector; hostsFile?: string; @@ -678,6 +745,14 @@ export async function resolveInstallTargets( errors: TargetError[]; detectedHostIds: string[]; }> { + if (!isValidSkillName(options.skillName)) { + return { + targets: [], + errors: [{ skillName: options.skillName, reason: "invalid-skill-name" }], + detectedHostIds: [], + }; + } + const spec = await loadHostSpec(options.hostsFile); const home = options.home ?? homedir(); const cwd = options.cwd ?? process.cwd(); @@ -798,7 +873,7 @@ function validateNormalizedSkill(bundle: NormalizedSkillBundle): SkillInfo { const frontmatter = parseFrontmatter(match[1]); const name = frontmatter.get("name") ?? ""; const description = frontmatter.get("description") ?? ""; - if (!/^[a-z0-9]+(-[a-z0-9]+)*$/.test(name)) { + if (!isValidSkillName(name)) { return { valid: false, errorCode: "invalid-frontmatter" }; } if (description.length < 1 || description.length > 1024) { @@ -1027,6 +1102,8 @@ async function installOrPlan( options: InstallOptions, write: boolean, ): Promise { + if (!options.appId) return emptyInstallReport([{ reason: "invalid-app-id" }]); + const cwd = options.cwd ?? process.cwd(); let bundle: NormalizedSkillBundle; let bundleMetadata: BundleMetadata; @@ -1072,7 +1149,10 @@ async function installOrPlan( ); } report.installed.push(result); - } else if (!metadata.value) { + } else if ( + !metadata.value || + metadata.value.skillName !== skill.skillName + ) { if (options.force) { if (write) { await replaceManagedSkill( @@ -1145,6 +1225,15 @@ export async function updateBundledSkill( export async function uninstallBundledSkill( options: UninstallOptions, ): Promise { + if (!options.appId) { + return { + removed: [], + skipped: [], + conflicts: [], + errors: [{ reason: "invalid-app-id" }], + }; + } + const { targets, errors } = await resolveInstallTargets({ ...options, skillName: options.skillName, @@ -1161,7 +1250,10 @@ export async function uninstallBundledSkill( const metadata = await readMetadata(target.targetDir); if (!metadata.exists) { report.skipped.push({ ...result, reason: "missing" }); - } else if (!metadata.value) { + } else if ( + !metadata.value || + metadata.value.skillName !== options.skillName + ) { report.conflicts.push({ ...result, reason: "unmanaged" }); } else if (metadata.value.appId !== options.appId) { report.conflicts.push({ ...result, reason: "owner-mismatch" }); @@ -1182,9 +1274,15 @@ async function copyManagedSkill( hash: string, metadata: BundleMetadata, ) { - await rm(targetDir, { recursive: true, force: true }); - await copySkillBundle(bundle, targetDir); - await writeMetadata(targetDir, appId, skillName, hash, metadata); + const tmp = await makeStagingDir(targetDir); + try { + await copySkillBundle(bundle, tmp); + await writeMetadata(tmp, appId, skillName, hash, metadata); + await rename(tmp, targetDir); + } catch (error) { + await rm(tmp, { recursive: true, force: true }); + throw error; + } } async function replaceManagedSkill( @@ -1195,13 +1293,11 @@ async function replaceManagedSkill( hash: string, metadata: BundleMetadata, ) { - const suffix = `.kitup-${process.pid}-${Date.now()}`; - const tmp = `${targetDir}${suffix}`; - const backup = `${targetDir}${suffix}-backup`; - await rm(tmp, { recursive: true, force: true }); - await copySkillBundle(bundle, tmp); - await writeMetadata(tmp, appId, skillName, hash, metadata); + const tmp = await makeStagingDir(targetDir); + const backup = `${tmp}-backup`; try { + await copySkillBundle(bundle, tmp); + await writeMetadata(tmp, appId, skillName, hash, metadata); await rename(targetDir, backup); await rename(tmp, targetDir); await rm(backup, { recursive: true, force: true }); @@ -1213,6 +1309,19 @@ async function replaceManagedSkill( } } +async function makeStagingDir(targetDir: string) { + const parent = dirname(targetDir); + await mkdir(parent, { recursive: true }); + const tmp = await mkdtemp(join(parent, `.${basename(targetDir)}.kitup-`)); + try { + await chmod(tmp, 0o755); + return tmp; + } catch (error) { + await rm(tmp, { recursive: true, force: true }); + throw error; + } +} + async function copySkillBundle(bundle: NormalizedSkillBundle, dest: string) { await mkdir(dest, { recursive: true }); for (const file of bundle.files) { @@ -1274,15 +1383,42 @@ async function readMetadata( ): Promise<{ exists: boolean; value?: InstallMetadata }> { if (!(await exists(targetDir))) return { exists: false }; try { - return { - exists: true, - value: JSON.parse(await readFile(join(targetDir, ".kitup.json"), "utf8")), - }; + const raw = JSON.parse( + await readFile(join(targetDir, ".kitup.json"), "utf8"), + ); + const value = parseOwnedMetadata(raw); + return value ? { exists: true, value } : { exists: true }; } catch { return { exists: true }; } } +function parseOwnedMetadata(raw: unknown): InstallMetadata | undefined { + if (!raw || typeof raw !== "object") return undefined; + const value = raw as Record; + if (value.schemaVersion !== 1) return undefined; + if (typeof value.appId !== "string" || value.appId.length === 0) + return undefined; + if (typeof value.skillName !== "string" || !isValidSkillName(value.skillName)) + return undefined; + if (value.source !== "bundled" && value.source !== "github") return undefined; + if (typeof value.hash !== "string" || value.hash.length === 0) + return undefined; + const metadata: InstallMetadata = { + schemaVersion: 1, + appId: value.appId, + skillName: value.skillName, + source: value.source, + hash: value.hash, + }; + if (typeof value.sourceId === "string") metadata.sourceId = value.sourceId; + if (typeof value.version === "string") metadata.version = value.version; + if (value.provenance && typeof value.provenance === "object") { + metadata.provenance = value.provenance as Record; + } + return metadata; +} + function targetResult(target: TargetGroup): TargetResult { const base = { skillName: target.skillName, targetDir: target.targetDir }; return target.hostIds.length === 1 diff --git a/ts/test/golden.test.ts b/ts/test/golden.test.ts index 25cbd2f..2ad482d 100644 --- a/ts/test/golden.test.ts +++ b/ts/test/golden.test.ts @@ -4,6 +4,7 @@ import { mkdtemp, mkdir, readFile, + readdir, rm, stat, chmod, @@ -34,7 +35,7 @@ import { const repo = fileURLToPath(new URL("../../", import.meta.url)); const casesFile = join(repo, "testdata/cases/bundled-skill-install.json"); -const hostsFile = join(repo, "spec/hosts.json"); +const defaultHostsFile = join(repo, "spec/hosts.json"); const cases = JSON.parse(await readFile(casesFile, "utf8")).cases; let passed = 0; @@ -56,10 +57,77 @@ for (const testCase of cases) { } } +await assertConcurrentInitialInstallIsolation(); + +function caseHostsFile(options: any, home: string, workspace: string) { + if (!options.hostsFile) return defaultHostsFile; + const expanded = expandValue(options.hostsFile, home, workspace); + return expanded.startsWith("/") ? expanded : resolveRepoPath(expanded); +} + console.log(`ok: ${passed} TypeScript golden cases`); +async function assertConcurrentInitialInstallIsolation() { + const root = await mkdtemp(join(tmpdir(), "kitup-concurrent-install-")); + const home = join(root, "home"); + const workspace = join(root, "workspace"); + await mkdir(home, { recursive: true }); + await mkdir(workspace, { recursive: true }); + + const bundles = ["A", "B"].map((payload) => + filesBundle([ + { + path: "SKILL.md", + contents: + "---\nname: concurrent\ndescription: Concurrent install fixture.\n---\n", + }, + { path: "payload.txt", contents: payload }, + ]), + ); + const originalNow = Date.now; + Date.now = () => 1_700_000_000_000; + try { + const results = await Promise.allSettled( + bundles.map((skillBundle, index) => + installBundledSkill({ + appId: `app-${index}`, + skillBundle, + scope: "user", + agents: ["codex"], + home, + cwd: workspace, + hostsFile: defaultHostsFile, + }), + ), + ); + const installed = results.flatMap((result, index) => + result.status === "fulfilled" && result.value.installed.length === 1 + ? [index] + : [], + ); + assert.equal(installed.length, 1); + const winner = installed[0]; + const target = join(home, ".agents/skills/concurrent"); + const metadata = JSON.parse( + await readFile(join(target, ".kitup.json"), "utf8"), + ); + assert.equal(metadata.appId, `app-${winner}`); + assert.equal( + await readFile(join(target, "payload.txt"), "utf8"), + ["A", "B"][winner], + ); + assert.deepEqual(await readdir(join(home, ".agents/skills")), [ + "concurrent", + ]); + } finally { + Date.now = originalNow; + await rm(root, { recursive: true, force: true }); + } +} + async function runCase(testCase: any, home: string, workspace: string) { const options = expandOptions(testCase.options, home, workspace); + const hostsFile = caseHostsFile(testCase.options, home, workspace); if (testCase.operation === "resolve-hosts") { const spec = await loadHostSpec(resolveRepoPath(testCase.given.hostsFile)); @@ -154,14 +222,22 @@ async function runCase(testCase: any, home: string, workspace: string) { ); } - const report = + const reportPromise = testCase.operation === "uninstall" - ? await uninstallBundledSkill({ ...options, hostsFile }) + ? uninstallBundledSkill({ ...options, hostsFile }) : testCase.operation === "plan" - ? await planBundledSkill({ ...options, hostsFile }) + ? planBundledSkill({ ...options, hostsFile }) : testCase.operation === "update" - ? await updateBundledSkill({ ...options, hostsFile }) - : await installBundledSkill({ ...options, hostsFile }); + ? updateBundledSkill({ ...options, hostsFile }) + : installBundledSkill({ ...options, hostsFile }); + + if (testCase.expected.throws) { + await assert.rejects(reportPromise); + await assertExpectedFiles(testCase, home, workspace); + return; + } + + const report = await reportPromise; if (testCase.expected.report) assert.deepEqual(