diff --git a/packages/clawdhub/src/cli/commands/skills.test.ts b/packages/clawdhub/src/cli/commands/skills.test.ts index b03d37f0e..30086a682 100644 --- a/packages/clawdhub/src/cli/commands/skills.test.ts +++ b/packages/clawdhub/src/cli/commands/skills.test.ts @@ -1,6 +1,6 @@ /* @vitest-environment node */ -import { afterEach, describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi, beforeEach } from 'vitest' import { ApiRoutes } from '../../schema/index.js' import type { GlobalOpts } from '../types' @@ -35,7 +35,7 @@ vi.mock('../ui.js', () => ({ throw new Error(message) }, formatError: (error: unknown) => (error instanceof Error ? error.message : String(error)), - isInteractive: () => false, + isInteractive: vi.fn(() => false), promptConfirm: vi.fn(async () => false), })) @@ -50,12 +50,20 @@ vi.mock('../../skills.js', () => ({ })) vi.mock('node:fs/promises', () => ({ + cp: vi.fn(), mkdir: vi.fn(), + rename: vi.fn(), rm: vi.fn(), stat: vi.fn(), })) -const { clampLimit, cmdExplore, cmdInstall, cmdUpdate, formatExploreLine } = await import('./skills') +vi.mock('node:os', () => ({ + tmpdir: () => '/tmp', +})) + +const { clampLimit, cmdExplore, cmdInstall, cmdUpdate, formatExploreLine } = await import( + './skills' +) const { extractZipToDir, hashSkillFiles, @@ -65,9 +73,16 @@ const { writeLockfile, writeSkillOrigin, } = await import('../../skills.js') -const { rm, stat } = await import('node:fs/promises') +const { cp, rename, rm, stat } = await import('node:fs/promises') +const { isInteractive, promptConfirm } = await import('../ui.js') +const { execFileSync } = await import('node:child_process') + +vi.mock('node:child_process', () => ({ + execFileSync: vi.fn(), +})) const mockLog = vi.spyOn(console, 'log').mockImplementation(() => {}) +const mockWarn = vi.spyOn(console, 'warn').mockImplementation(() => {}) function makeOpts(): GlobalOpts { return { @@ -83,6 +98,145 @@ afterEach(() => { vi.clearAllMocks() }) +describe('cmdInstall', () => { + beforeEach(() => { + // Default mocks for a successful installation path + mockApiRequest.mockResolvedValue({ + latestVersion: { version: '1.0.0' }, + moderation: { isMalwareBlocked: false, isSuspicious: false }, + }) + mockDownloadZip.mockResolvedValue(new Uint8Array([1, 2, 3])) + vi.mocked(readLockfile).mockResolvedValue({ version: 1, skills: {} }) + vi.mocked(writeLockfile).mockResolvedValue() + vi.mocked(writeSkillOrigin).mockResolvedValue() + vi.mocked(extractZipToDir).mockResolvedValue() + vi.mocked(stat).mockRejectedValue(new Error('missing')) // Simulate file not existing + vi.mocked(rm).mockResolvedValue() + vi.mocked(rename).mockResolvedValue() + vi.mocked(cp).mockResolvedValue() + vi.mocked(execFileSync).mockReturnValue('{}') // Clean scan + }) + + it('installs a skill successfully when scan finds no violations', async () => { + await cmdInstall(makeOpts(), 'test-skill') + expect(extractZipToDir).toHaveBeenCalledWith( + expect.any(Uint8Array), + expect.stringMatching(/\/tmp\/clawhub-install-test-skill-.*/), + ) + expect(execFileSync).toHaveBeenCalledWith( + 'uvx', + [ + 'mcp-scan@latest', + '--skills', + expect.stringMatching(/\/tmp\/clawhub-install-test-skill-.*/), + '--json', + ], + { encoding: 'utf-8' }, + ) + expect(rename).toHaveBeenCalledWith( + expect.stringMatching(/\/tmp\/clawhub-install-test-skill-.*/), + '/work/skills/test-skill', + ) + expect(mockSpinner.succeed).toHaveBeenCalledWith( + 'OK. Installed test-skill -> /work/skills/test-skill', + ) + }) + + it('installs a skill if user accepts after a violation warning', async () => { + const violation = { issues: [{ code: 'W011', message: 'Third-party content' }] } + vi.mocked(execFileSync).mockReturnValue(JSON.stringify({ '/path/to/skill': violation })) + vi.mocked(isInteractive).mockReturnValue(true) + vi.mocked(promptConfirm).mockResolvedValue(true) + + await cmdInstall(makeOpts(), 'test-skill') + + expect(mockLog).toHaveBeenCalledWith(expect.stringContaining('⚠️ Warning')) + expect(promptConfirm).toHaveBeenCalledWith('Install anyway?') + expect(rename).toHaveBeenCalled() + expect(mockSpinner.succeed).toHaveBeenCalledWith( + 'OK. Installed test-skill -> /work/skills/test-skill', + ) + }) + + it('aborts installation if user rejects after a violation warning', async () => { + const violation = { issues: [{ code: 'W011', message: 'Third-party content' }] } + vi.mocked(execFileSync).mockReturnValue(JSON.stringify({ '/path/to/skill': violation })) + vi.mocked(isInteractive).mockReturnValue(true) + vi.mocked(promptConfirm).mockResolvedValue(false) + + await expect(cmdInstall(makeOpts(), 'test-skill')).rejects.toThrow('Installation cancelled') + + expect(promptConfirm).toHaveBeenCalledWith('Install anyway?') + expect(rename).not.toHaveBeenCalled() + expect(mockSpinner.succeed).not.toHaveBeenCalled() + }) + + it('skips scan and continues installation if scanner fails', async () => { + vi.mocked(execFileSync).mockImplementation(() => { + throw new Error('Rate limit exceeded') + }) + + await cmdInstall(makeOpts(), 'test-skill') + + expect(mockWarn).toHaveBeenCalledWith( + expect.stringContaining('⚠️ Skipping Snyk Agent Scan: Rate limit exceeded'), + ) + expect(rename).toHaveBeenCalled() + expect(mockSpinner.succeed).toHaveBeenCalledWith( + 'OK. Installed test-skill -> /work/skills/test-skill', + ) + }) + + it('falls back to copy when rename fails with EXDEV', async () => { + const exdevError = new Error('Cross-device link') + ;(exdevError as any).code = 'EXDEV' + vi.mocked(rename).mockRejectedValueOnce(exdevError) + + await cmdInstall(makeOpts(), 'test-skill') + + expect(rename).toHaveBeenCalled() + expect(cp).toHaveBeenCalledWith( + expect.stringMatching(/\/tmp\/clawhub-install-test-skill-.*/), + '/work/skills/test-skill', + { recursive: true }, + ) + expect(mockSpinner.succeed).toHaveBeenCalledWith( + 'OK. Installed test-skill -> /work/skills/test-skill', + ) + }) + + it('fails installation if rename fails with non-EXDEV error', async () => { + const otherError = new Error('Permission denied') + vi.mocked(rename).mockRejectedValueOnce(otherError) + + await expect(cmdInstall(makeOpts(), 'test-skill')).rejects.toThrow('Permission denied') + + expect(rename).toHaveBeenCalled() + expect(cp).not.toHaveBeenCalled() + expect(mockSpinner.succeed).not.toHaveBeenCalled() + }) + + it('passes optional auth token to API + download requests', async () => { + mockGetOptionalAuthToken.mockResolvedValue('tkn') + // Re-setup mocks as they might be overwritten by beforeEach if they clash, + // but here we are specific about return values. + mockApiRequest.mockResolvedValue({ + skill: { slug: 'demo', displayName: 'Demo', summary: null, tags: {}, stats: {}, createdAt: 0, updatedAt: 0 }, + latestVersion: { version: '1.0.0' }, + owner: null, + moderation: null, + }) + mockDownloadZip.mockResolvedValue(new Uint8Array([1, 2, 3])) + + await cmdInstall(makeOpts(), 'demo') + + const [, requestArgs] = mockApiRequest.mock.calls[0] ?? [] + expect(requestArgs?.token).toBe('tkn') + const [, zipArgs] = mockDownloadZip.mock.calls[0] ?? [] + expect(zipArgs?.token).toBe('tkn') + }) +}) + describe('explore helpers', () => { it('clamps explore limits and handles non-finite values', () => { expect(clampLimit(-5)).toBe(1) @@ -194,29 +348,3 @@ describe('cmdUpdate', () => { expect(args?.url).toBeUndefined() }) }) - -describe('cmdInstall', () => { - it('passes optional auth token to API + download requests', async () => { - mockGetOptionalAuthToken.mockResolvedValue('tkn') - mockApiRequest.mockResolvedValue({ - skill: { slug: 'demo', displayName: 'Demo', summary: null, tags: {}, stats: {}, createdAt: 0, updatedAt: 0 }, - latestVersion: { version: '1.0.0' }, - owner: null, - moderation: null, - }) - mockDownloadZip.mockResolvedValue(new Uint8Array([1, 2, 3])) - vi.mocked(readLockfile).mockResolvedValue({ version: 1, skills: {} }) - vi.mocked(writeLockfile).mockResolvedValue() - vi.mocked(writeSkillOrigin).mockResolvedValue() - vi.mocked(extractZipToDir).mockResolvedValue() - vi.mocked(stat).mockRejectedValue(new Error('missing')) - vi.mocked(rm).mockResolvedValue() - - await cmdInstall(makeOpts(), 'demo') - - const [, requestArgs] = mockApiRequest.mock.calls[0] ?? [] - expect(requestArgs?.token).toBe('tkn') - const [, zipArgs] = mockDownloadZip.mock.calls[0] ?? [] - expect(zipArgs?.token).toBe('tkn') - }) -}) diff --git a/packages/clawdhub/src/cli/commands/skills.ts b/packages/clawdhub/src/cli/commands/skills.ts index 69788ade8..b413c0ccd 100644 --- a/packages/clawdhub/src/cli/commands/skills.ts +++ b/packages/clawdhub/src/cli/commands/skills.ts @@ -1,4 +1,6 @@ -import { mkdir, rm, stat } from 'node:fs/promises' +import { execFileSync } from 'node:child_process' +import { cp, mkdir, rename, rm, stat } from 'node:fs/promises' +import { tmpdir } from 'node:os' import { join } from 'node:path' import semver from 'semver' import { apiRequest, downloadZip } from '../../http.js' @@ -53,6 +55,55 @@ export async function cmdSearch(opts: GlobalOpts, query: string, limit?: number) } } +// mcp-scan types +interface ScanIssue { + code: string + message: string +} + +interface ServerResult { + error?: { message: string } +} + +interface PathResult { + error?: { message: string } + servers?: ServerResult[] + issues: ScanIssue[] +} + +type ScanResult = Record + +async function checkSkillSecurity(targetPath: string): Promise { + const scanOutput = execFileSync('uvx', ['mcp-scan@latest', '--skills', targetPath, '--json'], { + encoding: 'utf-8', + }) + const scanResult = JSON.parse(scanOutput) as ScanResult + + // Internal codes to ignore + const IGNORE_CODES = ['W003', 'W004', 'W005', 'W006', 'X002'] + const violations: string[] = [] + + for (const [path, result] of Object.entries(scanResult)) { + // 1. Check for top-level execution failures + if (result.error) { + throw new Error(`Scan failed for ${path}: ${result.error.message}`) + } + + // 2. Check for server-level startup failures + const startupError = result.servers?.find((s) => s.error)?.error + if (startupError) { + throw new Error(`Server failed to start: ${startupError.message}`) + } + + // 3. Filter for real policy violations (Errors or Warnings) + const filteredViolations = result.issues.filter((issue) => !IGNORE_CODES.includes(issue.code)) + if (filteredViolations.length > 0) { + violations.push(...filteredViolations.map((v) => `[${v.code}] ${v.message}`)) + } + } + return violations +} + export async function cmdInstall( opts: GlobalOpts, slug: string, @@ -110,7 +161,54 @@ export async function cmdInstall( spinner.text = `Downloading ${trimmed}@${resolvedVersion}` const zip = await downloadZip(registry, { slug: trimmed, version: resolvedVersion, token }) - await extractZipToDir(zip, target) + + const tempTarget = join( + tmpdir(), + `clawhub-install-${trimmed}-${Math.random().toString(36).slice(2, 7)}`, + ) + try { + await extractZipToDir(zip, tempTarget) + + spinner.text = `Scanning ${trimmed}@${resolvedVersion}` + let scanViolations: string[] = [] + try { + scanViolations = await checkSkillSecurity(tempTarget) + } catch (error) { + spinner.stop() + console.warn(`⚠️ Skipping Snyk Agent Scan: ${formatError(error)}`) + spinner.start(`Resolving ${trimmed}`) + } + + if (scanViolations.length > 0 && !force) { + spinner.stop() + console.log( + `\n⚠️ Warning: "${trimmed}" has security policy violations by Snyk Agent Scan:\n` + + scanViolations.map((msg) => ` - ${msg}`).join('\n') + + '\n Review the skill code before use.\n', + ) + if (isInteractive()) { + const confirm = await promptConfirm('Install anyway?') + if (!confirm) fail('Installation cancelled') + spinner.start(`Resolving ${trimmed}`) + } else { + fail( + 'Use --force to install skills with security policy violations in non-interactive mode', + ) + } + } + + try { + await rename(tempTarget, target) + } catch (err: any) { + if (err.code === 'EXDEV') { + await cp(tempTarget, target, { recursive: true }) + } else { + throw err + } + } + } finally { + await rm(tempTarget, { recursive: true, force: true }) + } await writeSkillOrigin(target, { version: 1,