Skip to content

fix: harden command execution boundaries #47

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Railly merged 1 commit into from
Aug 22, 2026
Merged

fix: harden command execution boundaries #47

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 17 additions & 17 deletions main.js
View file
Open in desktop

Large diffs are not rendered by default.

33 changes: 33 additions & 0 deletions src/command-safety.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { describe, expect, test } from "bun:test";
import {
areSafeCommandArguments,
isSafeMarketplaceSource,
isSafeSkillName,
resolveContainedSkillPath,
} from "./command-safety";

describe("command safety", () => {
test("accepts supported skill names and repository sources", () => {
expect(isSafeSkillName("my-skill_2.0")).toBe(true);
expect(isSafeSkillName("Diseño de prompts")).toBe(true);
expect(isSafeMarketplaceSource("owner/repo-name.js")).toBe(true);
});

test("rejects shell metacharacters and traversal", () => {
for (const value of ["skill && command", "$(command)", "../outside", "name/child"])
expect(isSafeSkillName(value)).toBe(false);

for (const value of ["owner/repo;command", "../repo", "owner/repo/extra"])
expect(isSafeMarketplaceSource(value)).toBe(false);

expect(areSafeCommandArguments(["trace", "--skill", "safe-name"])).toBe(true);
expect(areSafeCommandArguments(["trace", "--skill", "unsafe & command"])).toBe(false);
});

test("keeps cleanup paths inside their configured root", () => {
expect(resolveContainedSkillPath("/home/user/.agents/skills", "safe-name"))
.toBe("/home/user/.agents/skills/safe-name");
expect(resolveContainedSkillPath("/home/user/.agents/skills", "../../outside"))
.toBeNull();
});
});
28 changes: 28 additions & 0 deletions src/command-safety.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
import { isAbsolute, relative, resolve } from "path";

const SAFE_SKILL_NAME = /^[\p{L}\p{N}][\p{L}\p{N} ._@+-]{0,199}$/u;
const SAFE_SOURCE_PART = /^[A-Za-z0-9][A-Za-z0-9._-]{0,99}$/;
const UNSAFE_COMMAND_CHARACTERS = /[\0\r\n&|<>^%!`"'()]/;

export function isSafeSkillName(name: string): boolean {
return name !== "." && name !== ".." && SAFE_SKILL_NAME.test(name);
}

export function isSafeMarketplaceSource(source: string): boolean {
const parts = source.split("/");
return parts.length === 2 && parts.every((part) =>
part !== "." && part !== ".." && SAFE_SOURCE_PART.test(part)
);
}

export function areSafeCommandArguments(args: string[]): boolean {
return args.every((arg) => arg.length <= 512 && !UNSAFE_COMMAND_CHARACTERS.test(arg));
}

export function resolveContainedSkillPath(root: string, skillName: string): string | null {
if (!isSafeSkillName(skillName)) return null;
const target = resolve(root, skillName);
const rel = relative(resolve(root), target);
if (!rel || rel.startsWith("..") || isAbsolute(rel)) return null;
return target;
}
80 changes: 54 additions & 26 deletions src/marketplace.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import { execSync, exec } from "child_process";
import { execFile, execFileSync } from "child_process";
import { existsSync, readFileSync, readdirSync, rmSync, writeFileSync } from "fs";
import { join, delimiter } from "path";
import { homedir, platform } from "os";
import { requestUrl } from "obsidian";
import {
areSafeCommandArguments,
isSafeMarketplaceSource,
isSafeSkillName,
resolveContainedSkillPath,
} from "./command-safety";

const HOME = homedir();
const IS_WIN = platform() === "win32";
Expand Down Expand Up @@ -40,10 +46,12 @@ export async function searchSkills(query: string): Promise<MarketplaceSkill[]> {
const data = res.json as SearchApiResponse;
if (!data.skills) return [];
const installed = getInstalledNames();
return data.skills.map((s) => ({
...s,
installed: installed.has(s.name),
}));
return data.skills
.filter((s) => isSafeMarketplaceSource(s.source) && isSafeSkillName(s.name))
.map((s) => ({
...s,
installed: installed.has(s.name),
}));
} catch { /* empty */
return [];
}
Expand All @@ -60,6 +68,7 @@ interface GitHubTreeResponse {
}

async function getRepoTree(source: string): Promise<{ branch: string; files: string[] }> {
if (!isSafeMarketplaceSource(source)) throw new Error("Invalid marketplace source");
const cached = treeCache.get(source);
if (cached) return cached;

Expand Down Expand Up @@ -99,6 +108,7 @@ function buildCandidateNames(skillName: string, skillId: string, source: string)
}

export async function fetchSkillContent(source: string, skillName: string, skillId: string): Promise<string | null> {
if (!isSafeMarketplaceSource(source) || !isSafeSkillName(skillName)) return null;
try {
const { branch, files } = await getRepoTree(source);
const candidates = buildCandidateNames(skillName, skillId, source);
Expand Down Expand Up @@ -216,6 +226,24 @@ export const VALID_AGENTS: { id: string; label: string }[] = [
{ id: "replit", label: "Replit" },
];

const VALID_AGENT_IDS = new Set(VALID_AGENTS.map(({ id }) => id));

function buildInstallArgs(
source: string,
agents: string[],
options: { globalInstall?: boolean; skillName?: string },
): string[] | null {
if (!isSafeMarketplaceSource(source)) return null;
if (agents.some((agent) => !VALID_AGENT_IDS.has(agent))) return null;
if (options.skillName && !isSafeSkillName(options.skillName)) return null;

const args = ["skills", "add", source, "-a", ...(agents.length > 0 ? agents : ["*"])];
if (options.globalInstall) args.push("-g");
if (options.skillName) args.push("-s", options.skillName);
args.push("-y");
return areSafeCommandArguments(args) ? args : null;
}

export const TOOL_TO_AGENT: Record<string, string> = {
"claude-code": "claude-code",
"cursor": "cursor",
Expand All @@ -242,13 +270,11 @@ export function installSkill(
agents: string[],
options: { runner?: "auto" | "npx" | "bunx"; globalInstall?: boolean; skillName?: string } = {}
): { success: boolean; output: string } {
const agentFlag = agents.length > 0 ? `-a ${agents.join(" ")}` : "-a '*'";
const globalFlag = options.globalInstall ? "-g" : "";
const skillFlag = options.skillName ? `-s ${options.skillName}` : "";
const args = buildInstallArgs(source, agents, options);
if (!args) return { success: false, output: "Invalid marketplace install request" };
const resolvedRunner = getRunner(options.runner || "auto");
const cmd = `${resolvedRunner} skills add ${source} ${agentFlag} ${globalFlag} ${skillFlag} -y`.replace(/\s+/g, " ").trim();
try {
const out = execSync(cmd, {
const out = execFileSync(resolvedRunner, args, {
encoding: "utf-8",
timeout: 120000,
env: { ...process.env, PATH: buildPath(), NO_COLOR: "1" },
Expand Down Expand Up @@ -298,7 +324,8 @@ const AGENT_SKILL_DIRS = [

function cleanupCopies(skillName: string): void {
for (const dir of AGENT_SKILL_DIRS) {
const skillPath = join(dir, skillName);
const skillPath = resolveContainedSkillPath(dir, skillName);
if (!skillPath) return;
if (existsSync(skillPath)) {
try {
rmSync(skillPath, { recursive: true, force: true });
Expand All @@ -309,6 +336,7 @@ function cleanupCopies(skillName: string): void {
}

function cleanLockFile(skillName: string): void {
if (!isSafeSkillName(skillName)) return;
const lockPath = join(HOME, ".agents", ".skill-lock.json");
if (!existsSync(lockPath)) return;
try {
Expand All @@ -321,12 +349,13 @@ function cleanLockFile(skillName: string): void {
}

export function removeSkill(skillName: string, runner: "auto" | "npx" | "bunx" = "auto"): { success: boolean; output: string } {
if (!isSafeSkillName(skillName)) return { success: false, output: "Invalid skill name" };
const resolvedRunner = getRunner(runner);
const cmd = `${resolvedRunner} skills remove ${skillName} -y`;
const args = ["skills", "remove", skillName, "-y"];
let cliSuccess = false;
let output = "";
try {
output = execSync(cmd, {
output = execFileSync(resolvedRunner, args, {
encoding: "utf-8",
timeout: 30000,
env: { ...process.env, PATH: buildPath(), NO_COLOR: "1" },
Expand All @@ -353,9 +382,8 @@ export function removeSkill(skillName: string, runner: "auto" | "npx" | "bunx" =

export function updateAllSkills(runner: "auto" | "npx" | "bunx" = "auto"): { success: boolean; output: string; count: number } {
const resolvedRunner = getRunner(runner);
const cmd = `${resolvedRunner} skills update`;
try {
const out = execSync(cmd, {
const out = execFileSync(resolvedRunner, ["skills", "update"], {
encoding: "utf-8",
timeout: 120000,
env: { ...process.env, PATH: buildPath(), NO_COLOR: "1" },
Expand Down Expand Up @@ -385,9 +413,12 @@ export function refreshInstalledStatus(skills: MarketplaceSkill[]): MarketplaceS
return skills;
}

function execAsync(cmd: string, timeout = 120000): Promise<{ success: boolean; output: string }> {
function execAsync(file: string, args: string[], timeout = 120000): Promise<{ success: boolean; output: string }> {
if (!areSafeCommandArguments(args)) {
return Promise.resolve({ success: false, output: "Invalid command arguments" });
}
return new Promise((resolve) => {
exec(cmd, {
execFile(file, args, {
encoding: "utf-8",
timeout,
env: { ...process.env, PATH: buildPath(), NO_COLOR: "1" },
Expand All @@ -408,26 +439,23 @@ export async function installSkillAsync(
agents: string[],
options: { runner?: "auto" | "npx" | "bunx"; globalInstall?: boolean; skillName?: string } = {}
): Promise<{ success: boolean; output: string }> {
const agentFlag = agents.length > 0 ? `-a ${agents.join(" ")}` : "-a '*'";
const globalFlag = options.globalInstall ? "-g" : "";
const skillFlag = options.skillName ? `-s ${options.skillName}` : "";
const args = buildInstallArgs(source, agents, options);
if (!args) return { success: false, output: "Invalid marketplace install request" };
const resolvedRunner = getRunner(options.runner || "auto");
const cmd = `${resolvedRunner} skills add ${source} ${agentFlag} ${globalFlag} ${skillFlag} -y`.replace(/\s+/g, " ").trim();
return execAsync(cmd);
return execAsync(resolvedRunner, args);
}

export async function removeSkillAsync(skillName: string, runner: "auto" | "npx" | "bunx" = "auto"): Promise<{ success: boolean; output: string }> {
if (!isSafeSkillName(skillName)) return { success: false, output: "Invalid skill name" };
const resolvedRunner = getRunner(runner);
const cmd = `${resolvedRunner} skills remove ${skillName} -y`;
const result = await execAsync(cmd, 30000);
const result = await execAsync(resolvedRunner, ["skills", "remove", skillName, "-y"], 30000);
cleanupCopies(skillName);
return { success: true, output: result.output || `Cleaned ${skillName}` };
}

export async function updateAllSkillsAsync(runner: "auto" | "npx" | "bunx" = "auto"): Promise<{ success: boolean; output: string; count: number }> {
const resolvedRunner = getRunner(runner);
const cmd = `${resolvedRunner} skills update`;
const result = await execAsync(cmd);
const result = await execAsync(resolvedRunner, ["skills", "update"]);
const match = result.output.match(/Updated (\d+) skill/);
return { ...result, count: match ? parseInt(match[1]) : 0 };
}
Expand Down
42 changes: 22 additions & 20 deletions src/skillkit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import { execSync, exec } from "child_process";
import { execFile, execFileSync } from "child_process";
import { existsSync, readdirSync } from "fs";
import { join, delimiter } from "path";
import { homedir, platform } from "os";
import { areSafeCommandArguments, isSafeSkillName } from "./command-safety";

const BUILTIN_TOOL_NAMES_PLUGIN = new Set([
"Read", "Write", "Edit", "MultiEdit", "Bash", "Glob", "Grep",
Expand Down Expand Up @@ -84,7 +85,7 @@ function buildPath(): string {

function isCrafterSkillkit(binPath: string): boolean {
try {
const out = execSync(`"${binPath}" help`, {
const out = execFileSync(binPath, ["help"], {
encoding: "utf-8",
timeout: 5000,
env: { ...process.env, NO_COLOR: "1", PATH: buildPath() },
Expand Down Expand Up @@ -154,7 +155,7 @@ function findSkillkitBin(): string | null {
];
for (const args of dynamicCmds) {
try {
const dir = execSync(args.join(" "), {
const dir = execFileSync(args[0], args.slice(1), {
encoding: "utf-8",
timeout: 5000,
stdio: ["pipe", "pipe", "pipe"],
Expand Down Expand Up @@ -192,11 +193,11 @@ export function isSkillkitAvailable(): boolean {
return getSkillkitBin() !== null || existsSync(DB_PATH);
}

export function runSkillkitJson(cmd: string): Record<string, unknown> | unknown[] | null {
export function runSkillkitJson(args: string[]): Record<string, unknown> | unknown[] | null {
const bin = getSkillkitBin();
if (!bin) return null;
if (!bin || !areSafeCommandArguments(args)) return null;
try {
const out = execSync(`${bin} ${cmd} --json`, {
const out = execFileSync(bin, [...args, "--json"], {
encoding: "utf-8",
timeout: 15000,
env: { ...process.env, NO_COLOR: "1", PATH: buildPath() },
Expand All @@ -215,11 +216,11 @@ function parseJsonOutput(out: string): Record<string, unknown> | unknown[] | nul
return JSON.parse(out.slice(start)) as Record<string, unknown> | unknown[];
}

export function runSkillkitJsonAsync(cmd: string): Promise<Record<string, unknown> | unknown[] | null> {
export function runSkillkitJsonAsync(args: string[]): Promise<Record<string, unknown> | unknown[] | null> {
const bin = getSkillkitBin();
if (!bin) return Promise.resolve(null);
if (!bin || !areSafeCommandArguments(args)) return Promise.resolve(null);
return new Promise((resolve) => {
exec(`${bin} ${cmd} --json`, {
execFile(bin, [...args, "--json"], {
encoding: "utf-8",
timeout: 15000,
env: { ...process.env, NO_COLOR: "1", PATH: buildPath() },
Expand All @@ -236,7 +237,7 @@ export function getSkillkitStats(): Map<string, SkillkitStats> {
const stats = new Map<string, SkillkitStats>();
if (!isSkillkitAvailable()) return stats;

const data = runSkillkitJson("stats") as {
const data = runSkillkitJson(["stats"]) as {
top_skills: { name: string; total: number; daily: { date: string; count: number }[] }[];
} | null;

Expand Down Expand Up @@ -274,7 +275,7 @@ export function getSkillkitStatsWithDaily(): Map<string, SkillkitStatsWithDaily>
const stats = new Map<string, SkillkitStatsWithDaily>();
if (!isSkillkitAvailable()) return stats;

const data = runSkillkitJson("stats") as {
const data = runSkillkitJson(["stats"]) as {
top_skills: { name: string; total: number; daily: { date: string; count: number }[] }[];
} | null;

Expand Down Expand Up @@ -307,7 +308,7 @@ export function getSkillConflicts(): Map<string, { skillName: string; similarity
const conflicts = new Map<string, { skillName: string; similarity: number }[]>();
if (!isSkillkitAvailable()) return conflicts;

const data = runSkillkitJson("conflicts --dry-run") as {
const data = runSkillkitJson(["conflicts", "--dry-run"]) as {
pairs?: { skill_a: string; skill_b: string; similarity: number }[];
} | null;

Expand All @@ -323,9 +324,9 @@ export function getSkillConflicts(): Map<string, { skillName: string; similarity
}

export function getSkillTraces(skillName: string): { traceId: string; timestamp: string; tokens: number; cost: number; duration: number; model: string }[] {
if (!isSkillkitAvailable()) return [];
if (!isSkillkitAvailable() || !isSafeSkillName(skillName)) return [];

const data = runSkillkitJson(`trace --list --skill ${skillName} --limit 5`) as {
const data = runSkillkitJson(["trace", "--list", "--skill", skillName, "--limit", "5"]) as {
trace_id: string; timestamp: string; tokens_total: number; cost_estimate: number; duration_ms: number; model: string;
}[] | null;

Expand All @@ -344,7 +345,7 @@ export function getSkillTraces(skillName: string): { traceId: string; timestamp:
export function getSkillWarnings(): { oversized: { name: string; lines: number }[]; longDesc: { name: string; chars: number }[] } {
if (!isSkillkitAvailable()) return { oversized: [], longDesc: [] };

const data = runSkillkitJson("health") as {
const data = runSkillkitJson(["health"]) as {
warnings?: { oversized: { name: string; lines: number }[]; long_descriptions: { name: string; chars: number }[] };
} | null;

Expand All @@ -359,7 +360,7 @@ export async function getSkillkitStatsWithDailyAsync(): Promise<Map<string, Skil
const stats = new Map<string, SkillkitStatsWithDaily>();
if (!isSkillkitAvailable()) return stats;

const data = await runSkillkitJsonAsync("stats") as {
const data = await runSkillkitJsonAsync(["stats"]) as {
top_skills: { name: string; total: number; daily: { date: string; count: number }[] }[];
} | null;

Expand Down Expand Up @@ -392,7 +393,7 @@ export async function getSkillConflictsAsync(): Promise<Map<string, { skillName:
const conflicts = new Map<string, { skillName: string; similarity: number }[]>();
if (!isSkillkitAvailable()) return conflicts;

const data = await runSkillkitJsonAsync("conflicts --dry-run") as {
const data = await runSkillkitJsonAsync(["conflicts", "--dry-run"]) as {
pairs?: { skill_a: string; skill_b: string; similarity: number }[];
} | null;

Expand All @@ -410,7 +411,7 @@ export async function getSkillConflictsAsync(): Promise<Map<string, { skillName:
export async function getSkillWarningsAsync(): Promise<{ oversized: { name: string; lines: number }[]; longDesc: { name: string; chars: number }[] }> {
if (!isSkillkitAvailable()) return { oversized: [], longDesc: [] };

const data = await runSkillkitJsonAsync("health") as {
const data = await runSkillkitJsonAsync(["health"]) as {
warnings?: { oversized: { name: string; lines: number }[]; long_descriptions: { name: string; chars: number }[] };
} | null;

Expand All @@ -421,11 +422,12 @@ export async function getSkillWarningsAsync(): Promise<{ oversized: { name: stri
};
}

export function runSkillkitAction(cmd: string): { success: boolean; output: string } {
export function runSkillkitAction(args: string[]): { success: boolean; output: string } {
const bin = getSkillkitBin();
if (!bin) return { success: false, output: "skillkit not found" };
if (!areSafeCommandArguments(args)) return { success: false, output: "Invalid skillkit arguments" };
try {
const out = execSync(`${bin} ${cmd}`, {
const out = execFileSync(bin, args, {
encoding: "utf-8",
timeout: 30000,
env: { ...process.env, NO_COLOR: "1", PATH: buildPath() },
Expand Down
Loading
Loading