From 5536cb98d367cc9f0e200d6e088e7918faabaeb9 Mon Sep 17 00:00:00 2001 From: "Robert Kent Jr." Date: Wed, 12 Aug 2026 11:17:27 -0400 Subject: [PATCH 1/2] fix(cli): accept source prefixes in the wizard package prompt The wizard passed manually entered packages through unparsed, so the `npm:` prefix documented for `skilld add` reached `getCacheDir` as a literal package name and aborted the run. Normalise with `parseSkillInput` as `add` does, and direct non-npm sources to `skilld add`. `resolvePkgDir` treats an unusable name as a cache miss rather than propagating the validation error, and rejects an empty name instead of resolving to `node_modules`. --- README.md | 2 ++ src/cli.ts | 28 ++++++++++++++++++++++++---- src/core/prepare.ts | 11 ++++++++++- test/unit/pkg-dir-probe.test.ts | 12 ++++++++++++ 4 files changed, 48 insertions(+), 5 deletions(-) create mode 100644 test/unit/pkg-dir-probe.test.ts diff --git a/README.md b/README.md index 38b09bd7..83894678 100644 --- a/README.md +++ b/README.md @@ -146,6 +146,8 @@ skilld # Add skills for specific package(s) — npm: prefix for registry packages skilld add npm:vue npm:nuxt npm:pinia +# The same prefixes work in the interactive wizard's package prompt + # Add a pre-authored skill from a GitHub repo skilld add gh:vercel-labs/agent-skills diff --git a/src/cli.ts b/src/cli.ts index 4a687a1b..a2665223 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -21,6 +21,7 @@ import { runWizard } from './commands/wizard.ts' import { timedSpinner } from './core/formatting.ts' import { getProjectState, hasCompletedWizard, isOutdated, readConfig, semverGt } from './core/index.ts' import { readPackageJsonSafe } from './core/package-json.ts' +import { parseSkillInput } from './core/prefix.ts' import { COMMA_OR_WHITESPACE_RE, VERSION_RANGE_PREFIX_RE } from './core/regex.ts' import { iterateSkills } from './core/skills.ts' import { fetchLatestVersion, fetchNpmRegistryMeta } from './sources/index.ts' @@ -59,6 +60,19 @@ function deprecatedForwarder( // ── Subcommands (lazy-loaded) ── +function toPackageNames(tokens: string[]): string[] | null { + const names: string[] = [] + for (const token of tokens) { + const source = parseSkillInput(token) + if (source.type !== 'npm' && source.type !== 'bare') { + p.log.error(`${token} is not an npm package. Install it with \`skilld add ${token}\`.`) + return null + } + names.push(source.package) + } + return names +} + const SUBCOMMAND_NAMES = ['add', 'eject', 'update', 'info', 'list', 'config', 'remove', 'install', 'uninstall', 'search', 'cache', 'validate', 'assemble', 'setup', 'prepare', 'author', 'publish', 'upload', 'login', 'logout', 'whoami', 'pull'] // ── Main command ── @@ -277,7 +291,7 @@ const main = defineCommand({ if (source === 'manual') { const input = await p.text({ message: 'Enter package names (space or comma-separated)', - placeholder: 'vue nuxt pinia', + placeholder: 'vue npm:nuxt pinia', }) if (p.isCancel(input)) { if (!hasPkgJson) { @@ -290,7 +304,10 @@ const main = defineCommand({ p.log.warn('No packages entered') continue } - selected = input.split(COMMA_OR_WHITESPACE_RE).map(s => s.trim()).filter(Boolean) + const names = toPackageNames(input.split(COMMA_OR_WHITESPACE_RE).map(s => s.trim()).filter(Boolean)) + if (!names) + continue + selected = names if (selected.length === 0) { p.log.warn('No valid packages entered') continue @@ -534,11 +551,14 @@ const main = defineCommand({ if (source === 'manual') { const input = guard(await p.text({ message: 'Enter package names (space or comma-separated)', - placeholder: 'vue nuxt pinia', + placeholder: 'vue npm:nuxt pinia', })) if (!input) return - selected = input.split(COMMA_OR_WHITESPACE_RE).map(s => s.trim()).filter(Boolean) + const names = toPackageNames(input.split(COMMA_OR_WHITESPACE_RE).map(s => s.trim()).filter(Boolean)) + if (!names) + return + selected = names if (selected.length === 0) return } diff --git a/src/core/prepare.ts b/src/core/prepare.ts index 27d77fd7..1996e64d 100644 --- a/src/core/prepare.ts +++ b/src/core/prepare.ts @@ -24,12 +24,21 @@ function toStorageName(name: string): string { /** Resolve package directory: node_modules first, then global cache */ export function resolvePkgDir(name: string, cwd: string, version?: string): string | null { + if (!name) + return null + const nodeModulesPath = join(cwd, 'node_modules', name) if (existsSync(nodeModulesPath)) return nodeModulesPath if (version) { - const cachedPkgDir = join(getCacheDir(name, version), 'pkg') + let cachedPkgDir: string + try { + cachedPkgDir = join(getCacheDir(name, version), 'pkg') + } + catch { + return null + } if (existsSync(join(cachedPkgDir, 'package.json'))) return cachedPkgDir } diff --git a/test/unit/pkg-dir-probe.test.ts b/test/unit/pkg-dir-probe.test.ts new file mode 100644 index 00000000..a8a09e03 --- /dev/null +++ b/test/unit/pkg-dir-probe.test.ts @@ -0,0 +1,12 @@ +import { describe, expect, it } from 'vitest' +import { getShippedSkills, resolvePkgDir } from '../../src/core/prepare.ts' + +describe('package dir probing', () => { + it.each(['npm:vue', 'gh:owner/repo', '../escape', ''])('returns null for %j', (name) => { + expect(resolvePkgDir(name, process.cwd(), '1.0.0')).toBeNull() + }) + + it.each(['npm:vue', '../escape'])('reports no shipped skills for %j', (name) => { + expect(getShippedSkills(name, process.cwd(), '1.0.0')).toEqual([]) + }) +}) From 94f203dd5fd07b1fa05901fa5be1b6c6c04a8a68 Mon Sep 17 00:00:00 2001 From: Harlan Wilton Date: Thu, 13 Aug 2026 13:45:42 +1000 Subject: [PATCH 2/2] fix(cli): harden wizard package normalization Preserve requested npm tags when the wizard strips source prefixes. Validate package names before probing node_modules so traversal inputs cannot escape the package directory. --- src/cache/internal/version.ts | 12 ++++++++++-- src/cli.ts | 31 +++++++++++------------------- src/core/prefix.ts | 20 +++++++++++++++++++ src/core/prepare.ts | 12 ++++-------- test/unit/pkg-dir-probe.test.ts | 32 +++++++++++++++++++++++++++++-- test/unit/prefix.test.ts | 15 ++++++++++++++- test/unit/prepare-restore.test.ts | 10 +++++++--- 7 files changed, 96 insertions(+), 36 deletions(-) diff --git a/src/cache/internal/version.ts b/src/cache/internal/version.ts index 53e7b1c0..c3083731 100644 --- a/src/cache/internal/version.ts +++ b/src/cache/internal/version.ts @@ -11,6 +11,14 @@ const VALID_PKG_NAME = /^(?:@[a-z0-9][-a-z0-9._]*\/)?[a-z0-9][-a-z0-9._]*$/ /** Validate version string (semver-ish, no path separators) */ const VALID_VERSION = /^[a-z0-9][-\w.+]*$/i +export function isValidCachePackageName(name: string): boolean { + return VALID_PKG_NAME.test(name) +} + +export function isValidCacheVersion(version: string): boolean { + return VALID_VERSION.test(version) +} + /** * Get exact version key for cache keying */ @@ -30,9 +38,9 @@ export function getCacheKey(name: string, version: string): string { * Validates name/version to prevent path traversal. */ export function getCacheDir(name: string, version: string): string { - if (!VALID_PKG_NAME.test(name)) + if (!isValidCachePackageName(name)) throw new Error(`Invalid package name: ${name}`) - if (!VALID_VERSION.test(version)) + if (!isValidCacheVersion(version)) throw new Error(`Invalid version: ${version}`) const dir = resolve(REFERENCES_DIR, getCacheKey(name, version)) diff --git a/src/cli.ts b/src/cli.ts index a2665223..d4834201 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -21,7 +21,7 @@ import { runWizard } from './commands/wizard.ts' import { timedSpinner } from './core/formatting.ts' import { getProjectState, hasCompletedWizard, isOutdated, readConfig, semverGt } from './core/index.ts' import { readPackageJsonSafe } from './core/package-json.ts' -import { parseSkillInput } from './core/prefix.ts' +import { parseNpmPackageInputs } from './core/prefix.ts' import { COMMA_OR_WHITESPACE_RE, VERSION_RANGE_PREFIX_RE } from './core/regex.ts' import { iterateSkills } from './core/skills.ts' import { fetchLatestVersion, fetchNpmRegistryMeta } from './sources/index.ts' @@ -60,19 +60,6 @@ function deprecatedForwarder( // ── Subcommands (lazy-loaded) ── -function toPackageNames(tokens: string[]): string[] | null { - const names: string[] = [] - for (const token of tokens) { - const source = parseSkillInput(token) - if (source.type !== 'npm' && source.type !== 'bare') { - p.log.error(`${token} is not an npm package. Install it with \`skilld add ${token}\`.`) - return null - } - names.push(source.package) - } - return names -} - const SUBCOMMAND_NAMES = ['add', 'eject', 'update', 'info', 'list', 'config', 'remove', 'install', 'uninstall', 'search', 'cache', 'validate', 'assemble', 'setup', 'prepare', 'author', 'publish', 'upload', 'login', 'logout', 'whoami', 'pull'] // ── Main command ── @@ -304,10 +291,12 @@ const main = defineCommand({ p.log.warn('No packages entered') continue } - const names = toPackageNames(input.split(COMMA_OR_WHITESPACE_RE).map(s => s.trim()).filter(Boolean)) - if (!names) + const parsed = parseNpmPackageInputs(input.split(COMMA_OR_WHITESPACE_RE).map(s => s.trim()).filter(Boolean)) + if (parsed._tag === 'Err') { + p.log.error(`${parsed.input} is not an npm package. Install it with \`skilld add ${parsed.input}\`.`) continue - selected = names + } + selected = parsed.packageSpecs if (selected.length === 0) { p.log.warn('No valid packages entered') continue @@ -555,10 +544,12 @@ const main = defineCommand({ })) if (!input) return - const names = toPackageNames(input.split(COMMA_OR_WHITESPACE_RE).map(s => s.trim()).filter(Boolean)) - if (!names) + const parsed = parseNpmPackageInputs(input.split(COMMA_OR_WHITESPACE_RE).map(s => s.trim()).filter(Boolean)) + if (parsed._tag === 'Err') { + p.log.error(`${parsed.input} is not an npm package. Install it with \`skilld add ${parsed.input}\`.`) return - selected = names + } + selected = parsed.packageSpecs if (selected.length === 0) return } diff --git a/src/core/prefix.ts b/src/core/prefix.ts index 51ed5b53..42d40e3f 100644 --- a/src/core/prefix.ts +++ b/src/core/prefix.ts @@ -16,6 +16,7 @@ import type { GitSkillSource } from '../sources/git-skills.ts' import { parseGitSkillInput } from '../sources/git-skills.ts' const STATIC_REGEX_1 = /^[\w.-]+\/[\w.-]+/ +const EXPLICIT_NON_NPM_PREFIX_RE = /^(?:crate|gh|github):/ export type SkillSource = | { type: 'npm', package: string, tag?: string } @@ -25,6 +26,25 @@ export type SkillSource | { type: 'collection', handle: string, name: string } | { type: 'bare', package: string, tag?: string } +export type NpmPackageInputResult + = | { _tag: 'Ok', packageSpecs: string[] } + | { _tag: 'Err', input: string } + +export function parseNpmPackageInputs(inputs: string[]): NpmPackageInputResult { + const packageSpecs: string[] = [] + + for (const input of inputs) { + const source = parseSkillInput(input) + const isMalformedExplicitSource = source.type === 'bare' && EXPLICIT_NON_NPM_PREFIX_RE.test(input) + if ((source.type !== 'npm' && source.type !== 'bare') || isMalformedExplicitSource || !source.package) + return { _tag: 'Err', input } + + packageSpecs.push(source.tag ? `${source.package}@${source.tag}` : source.package) + } + + return { _tag: 'Ok', packageSpecs } +} + /** * Parse a single CLI input token into a typed SkillSource. * diff --git a/src/core/prepare.ts b/src/core/prepare.ts index 1996e64d..7f9205ef 100644 --- a/src/core/prepare.ts +++ b/src/core/prepare.ts @@ -9,7 +9,7 @@ import type { SkillInfo } from './lockfile.ts' import { existsSync, lstatSync, mkdirSync, readdirSync, rmSync, symlinkSync, unlinkSync } from 'node:fs' import { basename, join } from 'pathe' -import { getCacheDir } from '../cache/internal/version.ts' +import { getCacheDir, isValidCachePackageName, isValidCacheVersion } from '../cache/internal/version.ts' import { readPackageJsonSafe } from './package-json.ts' import { README_FILENAME_RE } from './regex.ts' @@ -24,7 +24,7 @@ function toStorageName(name: string): string { /** Resolve package directory: node_modules first, then global cache */ export function resolvePkgDir(name: string, cwd: string, version?: string): string | null { - if (!name) + if (!isValidCachePackageName(name)) return null const nodeModulesPath = join(cwd, 'node_modules', name) @@ -32,13 +32,9 @@ export function resolvePkgDir(name: string, cwd: string, version?: string): stri return nodeModulesPath if (version) { - let cachedPkgDir: string - try { - cachedPkgDir = join(getCacheDir(name, version), 'pkg') - } - catch { + if (!isValidCacheVersion(version)) return null - } + const cachedPkgDir = join(getCacheDir(name, version), 'pkg') if (existsSync(join(cachedPkgDir, 'package.json'))) return cachedPkgDir } diff --git a/test/unit/pkg-dir-probe.test.ts b/test/unit/pkg-dir-probe.test.ts index a8a09e03..bcd69a0a 100644 --- a/test/unit/pkg-dir-probe.test.ts +++ b/test/unit/pkg-dir-probe.test.ts @@ -1,11 +1,39 @@ -import { describe, expect, it } from 'vitest' +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'pathe' +import { afterEach, describe, expect, it } from 'vitest' import { getShippedSkills, resolvePkgDir } from '../../src/core/prepare.ts' describe('package dir probing', () => { - it.each(['npm:vue', 'gh:owner/repo', '../escape', ''])('returns null for %j', (name) => { + const fixtureDirs: string[] = [] + + afterEach(() => { + for (const dir of fixtureDirs) + rmSync(dir, { recursive: true, force: true }) + fixtureDirs.length = 0 + }) + + it.each(['npm:vue', 'gh:owner/repo', ''])('returns null for %j', (name) => { expect(resolvePkgDir(name, process.cwd(), '1.0.0')).toBeNull() }) + it('rejects traversal when the escaped directory exists', () => { + const cwd = mkdtempSync(join(tmpdir(), 'skilld-pkg-probe-')) + fixtureDirs.push(cwd) + mkdirSync(join(cwd, 'escape')) + + expect(resolvePkgDir('../escape', cwd, '1.0.0')).toBeNull() + }) + + it('returns an installed package before validating the cache version', () => { + const cwd = mkdtempSync(join(tmpdir(), 'skilld-pkg-probe-')) + fixtureDirs.push(cwd) + const packageDir = join(cwd, 'node_modules', 'vue') + mkdirSync(packageDir, { recursive: true }) + + expect(resolvePkgDir('vue', cwd, '../invalid')).toBe(packageDir) + }) + it.each(['npm:vue', '../escape'])('reports no shipped skills for %j', (name) => { expect(getShippedSkills(name, process.cwd(), '1.0.0')).toEqual([]) }) diff --git a/test/unit/prefix.test.ts b/test/unit/prefix.test.ts index eacca7eb..79380b47 100644 --- a/test/unit/prefix.test.ts +++ b/test/unit/prefix.test.ts @@ -1,7 +1,20 @@ import { describe, expect, it } from 'vitest' -import { parseSkillInput, resolveSkillName } from '../../src/core/prefix' +import { parseNpmPackageInputs, parseSkillInput, resolveSkillName } from '../../src/core/prefix' describe('prefix parser', () => { + describe('wizard npm inputs', () => { + it('normalizes prefixes without dropping npm tags', () => { + expect(parseNpmPackageInputs(['npm:vue@beta', '@nuxt/ui@3.0.0', 'pinia'])).toEqual({ + _tag: 'Ok', + packageSpecs: ['vue@beta', '@nuxt/ui@3.0.0', 'pinia'], + }) + }) + + it.each(['gh:owner/repo', 'gh:not-a-repo', 'crate:serde', '@curator'])('rejects non-npm input %s', (input) => { + expect(parseNpmPackageInputs([input])).toEqual({ _tag: 'Err', input }) + }) + }) + describe('npm: prefix', () => { it('parses simple package name', () => { expect(parseSkillInput('npm:vue')).toEqual({ diff --git a/test/unit/prepare-restore.test.ts b/test/unit/prepare-restore.test.ts index be9804fa..ddebf095 100644 --- a/test/unit/prepare-restore.test.ts +++ b/test/unit/prepare-restore.test.ts @@ -12,9 +12,13 @@ vi.mock('node:fs', async () => { } }) -vi.mock('../../src/cache/internal/version', () => ({ - getCacheDir: (name: string, version: string) => `/home/.skilld/references/${name}@${version}`, -})) +vi.mock('../../src/cache/internal/version', async (importOriginal) => { + const actual = await importOriginal() + return { + ...actual, + getCacheDir: (name: string, version: string) => `/home/.skilld/references/${name}@${version}`, + } +}) describe('restorePkgSymlink', () => { beforeEach(() => vi.resetAllMocks())