Harden the installer engine against untrusted packs + lifecycle data loss

apps/marketplace/src/lib/types.ts

@@ -96,6 +96,7 @@ export interface InstallResult {
   unchanged: number;
   backedUp: number;
   gitHook: boolean;
+  orphansRemoved?: number;
 }
 
 export interface InstallResponse {
@@ -106,6 +107,7 @@ export interface InstallResponse {
 
 export interface UninstallResult {
   removed: number;
+  keptModified?: number;
   gitHook: boolean;
   dryRun: boolean;
   notFound?: boolean;

packages/core/src/apply.mjs

@@ -1,9 +1,9 @@
 // Execute an install plan: copy files, back up user-modified files, install the git
 // pre-commit hook, and record everything in the lockfile.
-import { writeFileSync, chmodSync, copyFileSync, existsSync } from "node:fs";
+import { writeFileSync, chmodSync, copyFileSync, existsSync, rmSync } from "node:fs";
 import { dirname } from "node:path";
 import { copy, ensureDir, sha256 } from "./util.mjs";
-import { lockPath, readLock, writeLock } from "./lock.mjs";
+import { lockPath, readLock, writeLock, findInstall } from "./lock.mjs";
 
 const GIT_HOOK = `#!/usr/bin/env bash
 # oh-my-cursor defense-in-depth: catch anti-pattern commits regardless of HOW the commit is made
@@ -52,11 +52,32 @@ export function applyInstall(plan, { dryRun = false } = {}) {
     summary.gitHook = true;
   }
 
+  summary.orphansRemoved = removeUpgradeOrphans(plan, lockFiles, dryRun);
+
   if (!dryRun) writeLockEntry(plan, lockFiles, extraPaths);
 
   return summary;
 }
 
+// On upgrade (install over an existing install), delete files the PRIOR version wrote that the
+// new version no longer ships — otherwise they linger forever and vanish from the lockfile, so
+// even a later uninstall can't remove them. Only touch files we still own (current sha matches
+// what we recorded); a user-modified orphan is left in place. Must run before writeLockEntry,
+// which replaces the prior entry.
+function removeUpgradeOrphans(plan, newFiles, dryRun) {
+  const prior = findInstall(readLock(lockPath(plan.primaryCursorDir)), plan.packId, plan.scope);
+  if (!prior) return 0;
+  const keep = new Set(newFiles.map((f) => f.path));
+  let removed = 0;
+  for (const f of prior.files || []) {
+    if (keep.has(f.path) || !existsSync(f.path)) continue;
+    if (f.sha256 != null && safeSha(f.path) !== f.sha256) continue; // user changed it → leave it
+    if (!dryRun) rmSync(f.path);
+    removed++;
+  }
+  return removed;
+}
+
 function safeSha(path) {
   return existsSync(path) ? sha256(path) : null;
 }

packages/core/src/lock.mjs

@@ -1,6 +1,6 @@
 // The lockfile records every file an install wrote, so uninstall/upgrade are exact.
 // Lives at <primaryCursorDir>/.oh-my-cursor/lock.json.
-import { readFileSync, writeFileSync, existsSync, rmSync, readdirSync } from "node:fs";
+import { readFileSync, writeFileSync, existsSync, rmSync, readdirSync, renameSync } from "node:fs";
 import { join, dirname } from "node:path";
 import { ensureDir } from "./util.mjs";
 
@@ -19,7 +19,12 @@ export function readLock(path) {
 
 export function writeLock(path, lock) {
   ensureDir(dirname(path));
-  writeFileSync(path, JSON.stringify(lock, null, 2) + "\n");
+  // Write to a temp file then rename — an atomic swap on the same filesystem, so a crash or a
+  // concurrent writer can never leave a half-written / truncated lock.json (readLock silently
+  // treats a corrupt file as empty, which would orphan every prior install).
+  const tmp = `${path}.tmp`;
+  writeFileSync(tmp, JSON.stringify(lock, null, 2) + "\n");
+  renameSync(tmp, path);
 }
 
 export function removeLockIfEmpty(path) {

packages/core/src/pack.mjs

@@ -2,12 +2,38 @@
 import { readFileSync, existsSync } from "node:fs";
 import { join, resolve } from "node:path";
 
+// Lowercase-kebab slug. Also the shape the schema declares for pack id and skill names —
+// crucially it forbids "/", "." and "..", which is what keeps these fields from being used
+// as path-traversal payloads once the same string is joined into a filesystem destination.
+const SLUG = /^[a-z0-9][a-z0-9-]*$/;
+
+// Minimal, security-focused validation of an untrusted manifest. The full JSON-Schema check
+// lives in scripts/validate-packs.mjs (CI only); this runs on EVERY install so path-bearing
+// fields can never reach the planner unvalidated. Intentionally narrow: it guards the fields
+// the engine turns into filesystem paths, not the whole schema.
+export function validateManifest(pack, packDir) {
+  if (!pack || typeof pack !== "object" || Array.isArray(pack)) {
+    throw new Error(`invalid pack.json at ${packDir}: not an object`);
+  }
+  if (typeof pack.id !== "string" || !SLUG.test(pack.id)) {
+    throw new Error(`invalid pack id ${JSON.stringify(pack.id)} at ${packDir}: must match ${SLUG}`);
+  }
+  for (const skill of pack.skills || []) {
+    if (!skill || typeof skill.name !== "string" || !SLUG.test(skill.name)) {
+      throw new Error(
+        `invalid skill name ${JSON.stringify(skill?.name)} in pack "${pack.id}": must match ${SLUG}`,
+      );
+    }
+  }
+}
+
 export function loadPack(packDir) {
   const manifestPath = join(packDir, "pack.json");
   if (!existsSync(manifestPath)) {
     throw new Error(`No pack.json at ${packDir}`);
   }
   const pack = JSON.parse(readFileSync(manifestPath, "utf8"));
+  validateManifest(pack, packDir);
   const sourceRoot = resolve(packDir, pack.sourceRoot || ".");
   return { pack, packDir, sourceRoot };
 }

packages/core/src/plan.mjs

@@ -9,7 +9,7 @@
 //   - git pre-commit hook -> project scope + cursor only, and only if no foreign hook exists
 import { existsSync, readFileSync } from "node:fs";
 import { join } from "node:path";
-import { listFiles, matchGlobs, sha256 } from "./util.mjs";
+import { listFiles, matchGlobs, sha256, assertWithin } from "./util.mjs";
 import { toolDir, primaryCursorDir } from "./paths.mjs";
 import { findInstall } from "./lock.mjs";
 
@@ -19,7 +19,12 @@ function classify(src, dest, priorSha) {
   if (!existsSync(dest)) return "new";
   const destSha = sha256(dest);
   if (destSha === sha256(src)) return "unchanged";
-  if (priorSha && destSha !== priorSha) return "userModified";
+  // dest exists and differs from what we'd install:
+  //  - no prior lock entry  -> a pre-existing file we don't own (the user's) -> back it up
+  //  - prior entry, content changed since we wrote it -> user edited our file -> back it up
+  //  - prior entry, content still matches what we wrote -> our own file, a plain upgrade
+  if (!priorSha) return "userModified";
+  if (destSha !== priorSha) return "userModified";
   return "update";
 }
 
@@ -45,6 +50,7 @@ export function planInstall({ pack, sourceRoot }, opts = {}, lock = { installs:
       for (const rel of matched) {
         const src = join(sourceRoot, rel);
         const dest = join(dir, rel);
+        assertWithin(dir, dest, `${group}/${rel}`);
         items.push({ tool, group, rel, src, dest, status: classify(src, dest, priorByPath.get(dest)) });
       }
     }
@@ -56,6 +62,7 @@ export function planInstall({ pack, sourceRoot }, opts = {}, lock = { installs:
           const rel = `skills/${skill.name}/${f}`;
           const src = join(sourceRoot, rel);
           const dest = join(dir, rel);
+          assertWithin(dir, dest, `skills/${skill.name}`);
           items.push({ tool, group: "skills", rel, src, dest, status: classify(src, dest, priorByPath.get(dest)) });
         }
       }
@@ -79,8 +86,10 @@ export function planInstall({ pack, sourceRoot }, opts = {}, lock = { installs:
 }
 
 function planGitHook(pack, scope, tools, repo) {
-  const declares = (pack.uninstall?.extraPaths || []).some((p) => p.includes("pre-commit"));
-  if (!declares || scope !== "project" || !tools.includes("cursor")) return null;
+  // Gate on the DECLARED trust surface (permissions.gitHooks) — the same field the CLI and GUI
+  // render — not on a substring in uninstall.extraPaths. A pack that shows gitHooks:false must
+  // never get a project-scope pre-commit hook that runs its shell on every commit.
+  if (!pack.permissions?.gitHooks || scope !== "project" || !tools.includes("cursor")) return null;
   const path = join(repo, ".git", "hooks", "pre-commit");
   if (!existsSync(join(repo, ".git"))) return { path, willInstall: false, reason: "not a git repo" };
   if (existsSync(path) && !readFileSync(path, "utf8").includes("oh-my-cursor")) {

packages/core/src/uninstall.mjs

@@ -2,7 +2,7 @@
 // hook), prune now-empty directories, and drop the install entry.
 import { existsSync, rmSync, readFileSync } from "node:fs";
 import { homedir } from "node:os";
-import { pruneEmptyDirsRecursive } from "./util.mjs";
+import { pruneEmptyDirsRecursive, sha256 } from "./util.mjs";
 import { lockPath, readLock, writeLock, removeLockIfEmpty, findInstall } from "./lock.mjs";
 import { primaryCursorDir, toolDir } from "./paths.mjs";
 
@@ -15,13 +15,18 @@ export function uninstallPack({ packId, scope = "user", home = homedir(), repo =
 
   if (!install) return { notFound: true, removed: 0 };
 
-  const summary = { removed: 0, gitHook: false, dryRun };
+  const summary = { removed: 0, keptModified: 0, gitHook: false, dryRun };
 
   for (const f of install.files || []) {
-    if (existsSync(f.path)) {
-      if (!dryRun) rmSync(f.path);
-      summary.removed++;
+    if (!existsSync(f.path)) continue;
+    // Don't destroy a file the user has edited since we installed it. Only remove files whose
+    // content still matches what we recorded (we own them); leave modified ones and report them.
+    if (f.sha256 != null && sha256(f.path) !== f.sha256) {
+      summary.keptModified++;
+      continue;
     }
+    if (!dryRun) rmSync(f.path);
+    summary.removed++;
   }
 
   for (const extra of install.extraPaths || []) {

packages/core/src/util.mjs

@@ -9,7 +9,7 @@ import {
   copyFileSync,
   rmdirSync,
 } from "node:fs";
-import { dirname, join } from "node:path";
+import { dirname, join, resolve, relative, isAbsolute } from "node:path";
 
 // Translate the glob subset our manifests use (* ** ? literals) to a RegExp.
 export function globToRegExp(glob) {
@@ -64,6 +64,19 @@ export function copy(src, dest) {
   copyFileSync(src, dest);
 }
 
+// Path-traversal guard: return the resolved `target` iff it stays inside `base`, else throw.
+// Used to ensure a (potentially attacker-controlled) pack file can never resolve to a
+// destination outside its intended install directory.
+export function assertWithin(base, target, label = "path") {
+  const b = resolve(base);
+  const t = resolve(target);
+  const rel = relative(b, t);
+  if (rel !== "" && (rel === ".." || rel.startsWith(".." + "/") || rel.startsWith(".." + "\\") || isAbsolute(rel))) {
+    throw new Error(`refusing to write outside the install directory: "${label}" escapes to ${t}`);
+  }
+  return t;
+}
+
 // Post-order remove every empty directory under `dir` (and `dir` itself if it ends up
 // empty). Returns true if `dir` was removed.
 export function pruneEmptyDirsRecursive(dir) {