diff --git a/docs/design/acp-skill-management-module.md b/docs/design/acp-skill-management-module.md new file mode 100644 index 00000000000..c2fc7321798 --- /dev/null +++ b/docs/design/acp-skill-management-module.md @@ -0,0 +1,34 @@ +# ACP Skill management module + +## Context + +The ACP agent currently owns remote Skill source validation, GitHub download and archive extraction, local installation, deletion, and enablement inside the same file as session and workspace control. The Skill logic is cohesive but its implementation is spread between top-level helpers, private agent methods, and three extension-method branches. + +## Goals and non-goals + +This refactor gives Skill source acquisition and managed Skill mutation dedicated modules while preserving the existing extension-method interface, validation, filesystem safety, cache refresh ordering, responses, and errors. + +It does not change Skill discovery or status projection, add new scopes or source hosts, alter session Skill refresh, or change any Web Shell, bridge, or SDK contract. + +## Module seams + +The source module owns HTTPS and GitHub host validation, redirect validation, download limits, GitHub directory traversal, archive fallback, and tar extraction. Its primary interface resolves one source URL into the manifest content and files to install. + +The management module owns request validation, global and project Skill resolution, frontmatter enablement, atomic installation, guarded deletion, and cache refresh. The ACP agent delegates install, delete, and set-enabled requests without interpreting their payloads. + +The filesystem and network implementations remain direct dependencies. Tests use temporary directories and stubbed fetch responses; no new adapter layer is introduced. + +## Preserved invariants + +- Installation and deletion remain global-only; enablement retains global and project scopes. +- Skill slugs reject traversal and path separators. +- Remote sources remain HTTPS-only and limited to the existing GitHub host set. +- Redirect, compressed-size, decompressed-size, directory-depth, file-count, and cumulative-size guards remain unchanged. +- Installation validates the parsed Skill name, stages every file in a sibling directory, and refreshes the cache only after the swap succeeds. +- Deletion only removes a dedicated directory containing the validated `SKILL.md` and never removes a filesystem root or the global Qwen directory. +- Enablement edits only the top-level `disable-model-invocation` field and preserves comments, nested frontmatter, and body content. +- Extension-method response shapes, error types, error text, and requested working-directory behavior remain unchanged. + +## Verification + +Focused tests cover source and archive safety, global and project mutations, frontmatter preservation, route delegation, and the existing ACP integration behavior. The CLI package tests run alongside the repository build and typecheck. diff --git a/packages/cli/src/acp-integration/acpAgent.test.ts b/packages/cli/src/acp-integration/acpAgent.test.ts index 973fdb365ff..d3c7ae4d526 100644 --- a/packages/cli/src/acp-integration/acpAgent.test.ts +++ b/packages/cli/src/acp-integration/acpAgent.test.ts @@ -879,14 +879,11 @@ import { toSseServer, toHttpServer, normalizeCoreSettingValue, - extractFilesFromTarGz, - fetchAllowedGitHub, createWorkspaceMcpBudget, deliverClientMcpMessage, selectVisibleHistoryRecords, createManagedExternalToolGuard, } from './acpAgent.js'; -import { gzipSync } from 'node:zlib'; import type { Config, GoalSnapshotV2 } from '@qwen-code/qwen-code-core'; import type { LoadedSettings } from '../config/settings.js'; import type { CliArgs } from '../config/config.js'; @@ -18644,149 +18641,6 @@ describe('normalizeCoreSettingValue', () => { }); }); -describe('extractFilesFromTarGz', () => { - // Minimal tar (ustar) entry builder — only the fields the parser reads. - function tarEntry(name: string, content: string): Buffer { - const header = Buffer.alloc(512); - header.write(name, 0, 'utf8'); // name @ 0 (100 bytes) - const size = Buffer.byteLength(content); - header.write(`${size.toString(8).padStart(11, '0')}\0`, 124, 'utf8'); // size @ 124 (octal) - header.write('0', 156, 'utf8'); // typeflag '0' = regular file - const data = Buffer.alloc(Math.ceil(size / 512) * 512); - data.write(content, 0, 'utf8'); - return Buffer.concat([header, data]); - } - - function makeTarGz(name: string, content: string): Uint8Array { - const tar = Buffer.concat([tarEntry(name, content), Buffer.alloc(1024)]); // + end blocks - return new Uint8Array(gzipSync(tar)); - } - - it('extracts files under the requested directory (stripping the archive root)', async () => { - const archive = makeTarGz('repo-main/skills/SKILL.md', 'hello skill'); - const files = await extractFilesFromTarGz(archive, 'skills'); - expect(files).toHaveLength(1); - expect(files[0]!.relativePath).toBe('SKILL.md'); - expect(Buffer.from(files[0]!.content).toString('utf8')).toBe('hello skill'); - }); - - it('rejects an archive whose compressed size exceeds the limit', async () => { - await expect( - extractFilesFromTarGz(new Uint8Array(64), 'skills', { - maxCompressedBytes: 16, - }), - ).rejects.toThrowError(/exceeds the maximum allowed size/); - }); - - it('rejects an archive that fails to decompress', async () => { - await expect( - extractFilesFromTarGz(new Uint8Array([1, 2, 3, 4, 5]), 'skills'), - ).rejects.toThrowError(/Failed to decompress skill archive/); - }); - - it('rejects an archive whose decompressed size exceeds the limit', async () => { - const archive = makeTarGz('repo-main/skills/SKILL.md', 'x'.repeat(2048)); - await expect( - extractFilesFromTarGz(archive, 'skills', { - maxDecompressedBytes: 16, - }), - ).rejects.toThrowError(/Decompressed skill archive exceeds/); - }); -}); - -describe('fetchAllowedGitHub', () => { - afterEach(() => { - vi.unstubAllGlobals(); - }); - - function fakeResponse(status: number, location?: string) { - return { - status, - ok: status >= 200 && status < 300, - headers: { - get: (key: string) => - key.toLowerCase() === 'location' && location ? location : null, - }, - }; - } - - it('returns the response directly when there is no redirect', async () => { - const res = fakeResponse(200); - vi.stubGlobal('fetch', vi.fn().mockResolvedValue(res)); - await expect( - fetchAllowedGitHub('https://raw.githubusercontent.com/a/b/main/SKILL.md'), - ).resolves.toBe(res); - }); - - it('follows a redirect to an allowed GitHub CDN host', async () => { - const final = fakeResponse(200); - const fetchMock = vi - .fn() - .mockResolvedValueOnce( - fakeResponse(302, 'https://objects.githubusercontent.com/x'), - ) - .mockResolvedValueOnce(final); - vi.stubGlobal('fetch', fetchMock); - await expect( - fetchAllowedGitHub('https://codeload.github.com/a/b/tar.gz/main'), - ).resolves.toBe(final); - expect(fetchMock).toHaveBeenCalledTimes(2); - }); - - it('rejects a redirect to a disallowed host', async () => { - vi.stubGlobal( - 'fetch', - vi.fn().mockResolvedValue(fakeResponse(302, 'https://evil.com/x')), - ); - await expect( - fetchAllowedGitHub('https://raw.githubusercontent.com/a/b/main/SKILL.md'), - ).rejects.toThrow(/disallowed host/); - }); - - it('rejects a non-https redirect target', async () => { - vi.stubGlobal( - 'fetch', - vi - .fn() - .mockResolvedValue( - fakeResponse(302, 'http://raw.githubusercontent.com/x'), - ), - ); - await expect( - fetchAllowedGitHub('https://raw.githubusercontent.com/a/b/main/SKILL.md'), - ).rejects.toThrow(/disallowed host/); - }); - - it('rejects when the redirect limit is exceeded', async () => { - vi.stubGlobal( - 'fetch', - vi - .fn() - .mockResolvedValue( - fakeResponse(302, 'https://raw.githubusercontent.com/loop'), - ), - ); - await expect( - fetchAllowedGitHub('https://raw.githubusercontent.com/a', {}, 2), - ).rejects.toThrow(/maximum number of redirects/); - }); - - it('resolves a relative Location against the current URL', async () => { - const final = fakeResponse(200); - const fetchMock = vi - .fn() - .mockResolvedValueOnce(fakeResponse(302, '/a/b/SKILL.md')) - .mockResolvedValueOnce(final); - vi.stubGlobal('fetch', fetchMock); - await expect( - fetchAllowedGitHub('https://raw.githubusercontent.com/start'), - ).resolves.toBe(final); - expect(fetchMock.mock.calls[1]![0]).toBe( - 'https://raw.githubusercontent.com/a/b/SKILL.md', - ); - }); -}); - // --------------------------------------------------------------------------- // Multi-session language propagation // --------------------------------------------------------------------------- diff --git a/packages/cli/src/acp-integration/acpAgent.ts b/packages/cli/src/acp-integration/acpAgent.ts index 52ace9ca899..44ae7c54543 100644 --- a/packages/cli/src/acp-integration/acpAgent.ts +++ b/packages/cli/src/acp-integration/acpAgent.ts @@ -179,12 +179,10 @@ import { } from '@qwen-code/channel-base'; import { Readable, Writable } from 'node:stream'; import { normalizeDisabledToolList } from '../config/normalizeDisabledTools.js'; -import { pipeline } from 'node:stream/promises'; import type { Stats } from 'node:fs'; import * as fs from 'node:fs/promises'; import * as os from 'node:os'; import * as path from 'node:path'; -import { createGunzip } from 'node:zlib'; import type { LoadedSettings } from '../config/settings.js'; import { loadSettings, @@ -243,6 +241,11 @@ import { getModelConfiguration, type ModelReasoningConfiguration, } from './model-configuration.js'; +import { + deleteManagedSkill, + installManagedSkill, + setManagedSkillEnabled, +} from './skill-management.js'; import { buildSessionTasksStatus } from './session/tasksSnapshot.js'; import { collectHistoryReplayUpdates, @@ -1071,52 +1074,6 @@ type QwenMemoryPaths = { autoMemoryDir: string; }; -type QwenSkillInstallRequest = { - id: string; - slug: string; - name: string; - description?: string; - sourceUrl: string; - scope: 'global'; -}; - -type QwenSkillDeleteRequest = { - slug: string; - scope: 'global'; -}; - -type QwenSkillSetEnabledRequest = { - slug: string; - enabled: boolean; - scope: 'global' | 'project'; -}; - -type QwenManagedSkillFile = { - skillDir: string; - skillFile: string; - content: string; -}; - -const PROJECT_SKILL_DIRS = ['.qwen', '.agents'] as const; -const SKILLS_DIR = 'skills'; - -type DownloadedSkillFile = { - relativePath: string; - content: Uint8Array; -}; - -type DownloadedSkill = { - skillContent: string; - files: DownloadedSkillFile[]; -}; - -type GitHubBlobSkillUrl = { - owner: string; - repo: string; - ref: string; - filePath: string; -}; - type QwenSettingsScope = 'user' | 'workspace'; type QwenSettingValue = string | number | boolean | string[] | undefined; type QwenMcpTransport = 'stdio' | 'http' | 'sse'; @@ -1320,738 +1277,6 @@ function readRequiredString(value: unknown, fieldName: string): string { return stringValue; } -// Skill slugs are used to build filesystem paths under `/skills`. -// The character allowlist below already excludes `/` and `\`, but `.` and `..` -// would still slip through and let `path.join` traverse out of the skills dir -// (e.g. slug `..` resolves to the global config dir). Reject them explicitly. -function validateSkillSlug(slug: string): void { - if ( - !slug || - slug === '.' || - slug === '..' || - slug.includes('/') || - slug.includes(path.sep) || - !/^[a-zA-Z0-9._-]+$/.test(slug) - ) { - throw RequestError.invalidParams(undefined, 'Invalid skill.slug'); - } -} - -function readSkillInstallRequest( - params: Record, -): QwenSkillInstallRequest { - const skillParams = toRecord(params['skill']); - const input = Object.keys(skillParams).length > 0 ? skillParams : params; - const slug = readRequiredString(input['slug'], 'skill.slug'); - validateSkillSlug(slug); - - const scope = readOptionalString(input['scope'], 'skill.scope') ?? 'global'; - if (scope !== 'global') { - throw RequestError.invalidParams( - undefined, - 'Only global skill installation is supported', - ); - } - - const description = readOptionalString( - input['description'], - 'skill.description', - ); - return { - id: readOptionalString(input['id'], 'skill.id') ?? slug, - slug, - name: readOptionalString(input['name'], 'skill.name') ?? slug, - ...(description ? { description } : {}), - sourceUrl: readRequiredString(input['sourceUrl'], 'skill.sourceUrl'), - scope, - }; -} - -function readSkillSlugRequest( - params: Record, -): QwenSkillDeleteRequest { - const skillParams = toRecord(params['skill']); - const input = Object.keys(skillParams).length > 0 ? skillParams : params; - const slug = readRequiredString(input['slug'], 'skill.slug'); - validateSkillSlug(slug); - - const scope = readOptionalString(input['scope'], 'skill.scope') ?? 'global'; - if (scope !== 'global') { - throw RequestError.invalidParams( - undefined, - 'Only global skill management is supported', - ); - } - - return { slug, scope }; -} - -function readSkillSetEnabledRequest( - params: Record, -): QwenSkillSetEnabledRequest { - const skillParams = toRecord(params['skill']); - const input = Object.keys(skillParams).length > 0 ? skillParams : params; - const slug = readRequiredString(input['slug'], 'skill.slug'); - validateSkillSlug(slug); - - const scope = readOptionalString(input['scope'], 'skill.scope') ?? 'global'; - if (scope !== 'global' && scope !== 'project') { - throw RequestError.invalidParams( - undefined, - 'Only global or project skill management is supported', - ); - } - - if (typeof input['enabled'] !== 'boolean') { - throw RequestError.invalidParams( - undefined, - 'Invalid skill.enabled: expected boolean', - ); - } - return { - slug, - scope, - enabled: input['enabled'], - }; -} - -function splitSkillMarkdown(content: string): { - frontmatter: string; - body: string; -} { - const normalized = content.replace(/^\uFEFF/, '').replace(/\r\n?/g, '\n'); - const match = normalized.match(/^---\n([\s\S]*?)\n---(?:\n|$)([\s\S]*)$/); - if (!match) { - throw RequestError.invalidParams( - undefined, - 'Invalid skill file: missing YAML frontmatter', - ); - } - return { - frontmatter: match[1], - body: match[2], - }; -} - -function setSkillFrontmatterEnabled(content: string, enabled: boolean): string { - const { frontmatter, body } = splitSkillMarkdown(content); - - // Surgically add/remove only the top-level `disable-model-invocation:` line - // instead of round-tripping the whole frontmatter through a YAML - // parse/stringify. The minimal core YAML serializer drops comments and - // flattens nested structures (e.g. `hooks:`), so reserializing here would - // corrupt hooks-bearing skills and strip user comments. Working on the raw - // text leaves every other byte untouched. - const lines = frontmatter.split('\n'); - const disabledLineIndex = lines.findIndex((line) => - /^disable-model-invocation\s*:/.test(line), - ); - - if (enabled) { - if (disabledLineIndex !== -1) { - lines.splice(disabledLineIndex, 1); - } - } else if (disabledLineIndex !== -1) { - lines[disabledLineIndex] = 'disable-model-invocation: true'; - } else { - let insertIndex = lines.length; - while (insertIndex > 0 && lines[insertIndex - 1].trim() === '') { - insertIndex -= 1; - } - lines.splice(insertIndex, 0, 'disable-model-invocation: true'); - } - - const nextFrontmatter = lines.join('\n'); - return `---\n${nextFrontmatter}\n---\n${body}`; -} - -// Skill downloads must come from the GitHub host set. Restricting the host -// here prevents the client-supplied `sourceUrl` from driving server-side -// fetches at internal/loopback/link-local endpoints (SSRF), e.g. -// `http://169.254.169.254/` cloud-metadata or `http://localhost:/`. -const ALLOWED_SKILL_SOURCE_HOSTS = new Set([ - 'github.com', - 'raw.githubusercontent.com', - 'codeload.github.com', - 'api.github.com', -]); - -function assertAllowedSkillSourceUrl(sourceUrl: string): void { - let parsed: URL; - try { - parsed = new URL(sourceUrl); - } catch { - throw RequestError.invalidParams( - undefined, - 'Skill sourceUrl must be a valid URL', - ); - } - // Require HTTPS: a plaintext http: fetch of skill content (which can include - // executable hooks) is MITM-able by a network-position attacker, so the host - // allowlist alone is not sufficient. All supported GitHub hosts serve HTTPS. - if (parsed.protocol !== 'https:') { - throw RequestError.invalidParams( - undefined, - 'Skill sourceUrl must be an HTTPS URL', - ); - } - if (!ALLOWED_SKILL_SOURCE_HOSTS.has(parsed.hostname)) { - throw RequestError.invalidParams( - undefined, - 'Skill sourceUrl host is not allowed (only github.com sources are supported)', - ); - } -} - -function parseGitHubBlobSkillUrl(sourceUrl: string): GitHubBlobSkillUrl | null { - const parsed = new URL(sourceUrl); - // HTTPS-only, consistent with assertAllowedSkillSourceUrl (skill content can - // include executable hooks, so plaintext http: is MITM-able). - if (parsed.protocol !== 'https:') { - throw RequestError.invalidParams( - undefined, - 'Skill sourceUrl must be an HTTPS URL', - ); - } - - if (parsed.hostname !== 'github.com') return null; - const parts = parsed.pathname.split('/').filter(Boolean); - if (parts.length < 5 || parts[2] !== 'blob') return null; - - const owner = parts[0]; - const repo = parts[1]; - const ref = parts[3]; - const filePathParts = parts.slice(4); - if (!owner || !repo || !ref || filePathParts.length === 0) return null; - - return { - owner, - repo, - ref, - filePath: filePathParts.join('/'), - }; -} - -function toRawGitHubUrl(githubUrl: GitHubBlobSkillUrl): string { - return `https://raw.githubusercontent.com/${githubUrl.owner}/${githubUrl.repo}/${githubUrl.ref}/${githubUrl.filePath}`; -} - -function encodeGitHubPath(filePath: string): string { - if (!filePath || filePath === '.') return ''; - return filePath.split('/').map(encodeURIComponent).join('/'); -} - -function readTarString( - archive: Uint8Array, - offset: number, - length: number, -): string { - const bytes = archive.subarray(offset, offset + length); - const nul = bytes.indexOf(0); - const end = nul >= 0 ? nul : bytes.length; - return Buffer.from(bytes.subarray(0, end)).toString('utf8').trim(); -} - -function readTarSize(archive: Uint8Array, offset: number): number { - const raw = readTarString(archive, offset + 124, 12); - return raw ? Number.parseInt(raw, 8) : 0; -} - -function isZeroTarBlock(archive: Uint8Array, offset: number): boolean { - for (let i = 0; i < 512; i += 1) { - if (archive[offset + i] !== 0) return false; - } - return true; -} - -function readTarPath(archive: Uint8Array, offset: number): string { - const name = readTarString(archive, offset, 100); - const prefix = readTarString(archive, offset + 345, 155); - return prefix ? `${prefix}/${name}` : name; -} - -function stripArchiveRoot(filePath: string): string { - const parts = filePath.split('/').filter(Boolean); - return parts.length > 1 ? parts.slice(1).join('/') : ''; -} - -// Bound the work done on untrusted skill archives so a malicious or oversized -// download cannot exhaust memory. Decompression is streamed (createGunzip) and -// aborted the moment the cumulative inflated size crosses the cap, so a -// decompression bomb can never fully inflate into memory. -const MAX_SKILL_DOWNLOAD_BYTES = 100 * 1024 * 1024; // 100 MB compressed -const MAX_SKILL_DECOMPRESSED_BYTES = 500 * 1024 * 1024; // 500 MB decompressed -// Bounds for the GitHub Contents-API directory walk (the archive path is -// already bounded by the byte caps above). -const MAX_SKILL_API_DIR_DEPTH = 16; -const MAX_SKILL_API_FILE_COUNT = 2000; - -// Sentinel so the streaming decompression's size-limit abort can be told apart -// from a genuine gunzip/format error in the catch below. -class DecompressedSizeExceededError extends Error {} - -export async function extractFilesFromTarGz( - archiveBytes: Uint8Array, - directoryPath: string, - // Limits are injectable so the size-guard branches can be exercised in tests - // without allocating the 100MB/500MB production thresholds. - limits: { - maxCompressedBytes?: number; - maxDecompressedBytes?: number; - } = {}, -): Promise { - const maxCompressedBytes = - limits.maxCompressedBytes ?? MAX_SKILL_DOWNLOAD_BYTES; - const maxDecompressedBytes = - limits.maxDecompressedBytes ?? MAX_SKILL_DECOMPRESSED_BYTES; - - if (archiveBytes.length > maxCompressedBytes) { - throw RequestError.invalidParams( - undefined, - 'Skill archive exceeds the maximum allowed size', - ); - } - - let archive: Buffer; - try { - // Stream the inflate so we can abort as soon as the cumulative output - // exceeds the cap, instead of materializing the entire decompressed buffer - // first (a ~1000:1 gzip ratio could otherwise inflate a small archive to - // many GB before any post-hoc length check fires). - const chunks: Buffer[] = []; - let total = 0; - await pipeline( - // Wrap in an array so the whole archive is emitted as a single chunk; - // `Readable.from(uint8array)` would otherwise iterate it byte-by-byte. - Readable.from([Buffer.from(archiveBytes)]), - createGunzip(), - new Writable({ - write(chunk: Buffer, _enc, cb) { - total += chunk.length; - if (total > maxDecompressedBytes) { - cb(new DecompressedSizeExceededError()); - return; - } - chunks.push(chunk); - cb(); - }, - }), - ); - archive = Buffer.concat(chunks); - } catch (error) { - if (error instanceof DecompressedSizeExceededError) { - throw RequestError.invalidParams( - undefined, - 'Decompressed skill archive exceeds the maximum allowed size', - ); - } - throw RequestError.invalidParams( - undefined, - `Failed to decompress skill archive: ${ - error instanceof Error ? error.message : String(error) - }`, - ); - } - - const normalizedDirectory = directoryPath.replace(/^\/+|\/+$/g, ''); - // Treat '.' (SKILL.md at the repository root) as the empty prefix; otherwise - // the prefix becomes './' and never matches the root-stripped archive paths - // (e.g. 'SKILL.md'), yielding zero extracted files. - const directoryPrefix = - normalizedDirectory && normalizedDirectory !== '.' - ? `${normalizedDirectory}/` - : ''; - const files: DownloadedSkillFile[] = []; - - for (let offset = 0; offset + 512 <= archive.length; ) { - if (isZeroTarBlock(archive, offset)) break; - - const fullPath = readTarPath(archive, offset); - const typeFlag = String.fromCharCode(archive[offset + 156] || 0); - const size = readTarSize(archive, offset); - const dataOffset = offset + 512; - const nextOffset = dataOffset + Math.ceil(size / 512) * 512; - - if (typeFlag === '0' || typeFlag === '\0') { - const repoPath = stripArchiveRoot(fullPath); - if (repoPath.startsWith(directoryPrefix)) { - const relativePath = repoPath.slice(directoryPrefix.length); - if (relativePath) { - files.push({ - relativePath, - content: archive.subarray(dataOffset, dataOffset + size), - }); - } - } - } - - offset = nextOffset; - } - - return files; -} - -// GitHub host suffixes a download may legitimately redirect to (raw/codeload -// commonly 302 to their object CDN for geo/CDN routing). Redirects to anything -// outside these are rejected, preserving the SSRF guard while not breaking -// real downloads. -const ALLOWED_REDIRECT_HOST_SUFFIXES = [ - '.githubusercontent.com', - '.github.com', - // Note: '.github.io' is intentionally excluded — *.github.io are - // user-controlled GitHub Pages sites, so allowing redirects there would - // reopen the SSRF/exfiltration surface this allowlist exists to close. -]; - -function isAllowedSkillFetchHost(hostname: string): boolean { - if (ALLOWED_SKILL_SOURCE_HOSTS.has(hostname)) return true; - return ALLOWED_REDIRECT_HOST_SUFFIXES.some((suffix) => - hostname.endsWith(suffix), - ); -} - -/** - * Fetch that follows redirects manually, validating every hop stays on an - * allowed GitHub host over HTTPS. This keeps the SSRF protection of - * `redirect: 'manual'` (a malicious repo cannot bounce the fetch to an internal - * endpoint) while still following GitHub's legitimate CDN redirects, which - * plain `redirect: 'manual'` would surface as a download failure. - */ -export async function fetchAllowedGitHub( - url: string, - init: RequestInit = {}, - maxRedirects = 5, -): Promise { - let current = url; - for (let hop = 0; hop <= maxRedirects; hop += 1) { - const response = await fetch(current, { ...init, redirect: 'manual' }); - if (response.status < 300 || response.status >= 400) { - return response; - } - const location = response.headers?.get('location'); - if (!location) return response; - let next: URL; - try { - next = new URL(location, current); - } catch { - throw RequestError.invalidParams( - undefined, - 'Skill download redirected to an invalid URL', - ); - } - if (next.protocol !== 'https:' || !isAllowedSkillFetchHost(next.hostname)) { - throw RequestError.invalidParams( - undefined, - 'Skill download redirected to a disallowed host', - ); - } - current = next.toString(); - } - throw RequestError.invalidParams( - undefined, - 'Skill download exceeded the maximum number of redirects', - ); -} - -// Read a response body while enforcing a hard byte cap against the *actual* -// streamed bytes. The Content-Length pre-checks at the call sites are advisory -// only — a server that omits the header (chunked transfer, CDN redirect) could -// otherwise stream an arbitrarily large body straight into memory via -// `arrayBuffer()`. -async function readBodyWithLimit( - response: Response, - maxBytes: number, -): Promise { - const body = response.body; - if (!body) { - const buf = new Uint8Array(await response.arrayBuffer()); - if (buf.byteLength > maxBytes) { - throw RequestError.invalidParams( - undefined, - 'Skill download exceeds the maximum allowed size', - ); - } - return buf; - } - - const reader = body.getReader(); - const chunks: Uint8Array[] = []; - let total = 0; - for (;;) { - const { done, value } = await reader.read(); - if (done) break; - if (!value) continue; - total += value.byteLength; - if (total > maxBytes) { - await reader.cancel(); - throw RequestError.invalidParams( - undefined, - 'Skill download exceeds the maximum allowed size', - ); - } - chunks.push(value); - } - - const result = new Uint8Array(total); - let offset = 0; - for (const chunk of chunks) { - result.set(chunk, offset); - offset += chunk.byteLength; - } - return result; -} - -async function fetchBytes(url: string): Promise { - const response = await fetchAllowedGitHub(url); - if (!response.ok) { - throw RequestError.invalidParams( - undefined, - `Failed to download skill (${response.status})`, - ); - } - - const contentLength = response.headers?.get('content-length'); - if (contentLength) { - const declaredSize = Number.parseInt(contentLength, 10); - if ( - Number.isFinite(declaredSize) && - declaredSize > MAX_SKILL_DOWNLOAD_BYTES - ) { - throw RequestError.invalidParams( - undefined, - 'Skill download exceeds the maximum allowed size', - ); - } - } - - return readBodyWithLimit(response, MAX_SKILL_DOWNLOAD_BYTES); -} - -async function downloadSingleSkillFile( - sourceUrl: string, -): Promise { - const githubUrl = parseGitHubBlobSkillUrl(sourceUrl); - const fetchUrl = githubUrl ? toRawGitHubUrl(githubUrl) : sourceUrl; - const content = await fetchBytes(fetchUrl); - return { - skillContent: Buffer.from(content).toString('utf8'), - files: [{ relativePath: 'SKILL.md', content }], - }; -} - -async function downloadGitHubSkillDirectoryFromArchive( - githubUrl: GitHubBlobSkillUrl, - directoryPath: string, -): Promise { - const archiveUrl = `https://codeload.github.com/${githubUrl.owner}/${githubUrl.repo}/tar.gz/${encodeURIComponent( - githubUrl.ref, - )}`; - const response = await fetchAllowedGitHub(archiveUrl, { - headers: { - 'User-Agent': 'qwen-code', - }, - }); - if (!response.ok) { - throw RequestError.invalidParams( - undefined, - `Failed to download GitHub skill archive (${response.status})`, - ); - } - - // Reject oversized archives by declared Content-Length before buffering the - // whole body into memory, mirroring the guard in fetchBytes. - const contentLength = response.headers?.get('content-length'); - if (contentLength) { - const declaredSize = Number.parseInt(contentLength, 10); - if ( - Number.isFinite(declaredSize) && - declaredSize > MAX_SKILL_DOWNLOAD_BYTES - ) { - throw RequestError.invalidParams( - undefined, - 'Skill archive exceeds the maximum allowed size', - ); - } - } - - return extractFilesFromTarGz( - await readBodyWithLimit(response, MAX_SKILL_DOWNLOAD_BYTES), - directoryPath, - ); -} - -async function fetchGitHubDirectoryItems( - githubUrl: GitHubBlobSkillUrl, - directoryPath: string, -): Promise { - const encodedPath = encodeGitHubPath(directoryPath); - const apiUrl = `https://api.github.com/repos/${githubUrl.owner}/${githubUrl.repo}/contents/${encodedPath}?ref=${encodeURIComponent(githubUrl.ref)}`; - const response = await fetchAllowedGitHub(apiUrl, { - headers: { - Accept: 'application/vnd.github+json', - 'User-Agent': 'qwen-code', - }, - }); - if (!response.ok) { - throw RequestError.invalidParams( - undefined, - `Failed to list GitHub skill files (${response.status})`, - ); - } - - const data = await response.json(); - if (!Array.isArray(data)) { - throw RequestError.invalidParams( - undefined, - 'GitHub skill URL must point to a directory-backed SKILL.md file', - ); - } - return data; -} - -async function downloadGitHubSkillDirectoryFromApi( - githubUrl: GitHubBlobSkillUrl, - directoryPath: string, - relativeRoot = '', - // Bound the recursive API walk so a crafted repo (deeply nested dirs, huge - // file counts, or large cumulative size) can't exhaust memory/time. The - // archive fallback already enforces size caps; this gives the API path - // equivalent guards. - depth = 0, - budget: { files: number; bytes: number } = { files: 0, bytes: 0 }, -): Promise { - if (depth > MAX_SKILL_API_DIR_DEPTH) { - throw RequestError.invalidParams( - undefined, - 'Skill directory nesting exceeds the maximum allowed depth', - ); - } - const items = await fetchGitHubDirectoryItems(githubUrl, directoryPath); - const files: DownloadedSkillFile[] = []; - - for (const item of items) { - const record = toRecord(item); - const name = readRequiredString(record['name'], 'github.name'); - const itemPath = readRequiredString(record['path'], 'github.path'); - const type = readRequiredString(record['type'], 'github.type'); - const relativePath = relativeRoot - ? path.posix.join(relativeRoot, name) - : name; - - if (type === 'dir') { - files.push( - ...(await downloadGitHubSkillDirectoryFromApi( - githubUrl, - itemPath, - relativePath, - depth + 1, - budget, - )), - ); - continue; - } - - if (type !== 'file') continue; - budget.files += 1; - if (budget.files > MAX_SKILL_API_FILE_COUNT) { - throw RequestError.invalidParams( - undefined, - 'Skill directory contains too many files', - ); - } - const downloadUrl = readRequiredString( - record['download_url'], - 'github.download_url', - ); - // SSRF defense: the API-provided download_url is attacker-influenced, so - // run it through the same host allowlist + HTTPS check as the initial URL. - assertAllowedSkillSourceUrl(downloadUrl); - const content = await fetchBytes(downloadUrl); - budget.bytes += content.length; - if (budget.bytes > MAX_SKILL_DECOMPRESSED_BYTES) { - throw RequestError.invalidParams( - undefined, - 'Skill directory exceeds the maximum allowed size', - ); - } - files.push({ - relativePath, - content, - }); - } - - return files; -} - -async function downloadGitHubSkillDirectory( - githubUrl: GitHubBlobSkillUrl, - directoryPath: string, -): Promise { - const apiFiles = await downloadGitHubSkillDirectoryFromApi( - githubUrl, - directoryPath, - ).catch((error) => { - debugLogger.warn( - 'GitHub API directory listing failed, falling back to archive download:', - error, - ); - return null; - }); - if (apiFiles) return apiFiles; - - return downloadGitHubSkillDirectoryFromArchive(githubUrl, directoryPath); -} - -async function downloadSkill(sourceUrl: string): Promise { - assertAllowedSkillSourceUrl(sourceUrl); - const githubUrl = parseGitHubBlobSkillUrl(sourceUrl); - if (!githubUrl || path.posix.basename(githubUrl.filePath) !== 'SKILL.md') { - return downloadSingleSkillFile(sourceUrl); - } - - const skillDirectory = path.posix.dirname(githubUrl.filePath); - const files = await downloadGitHubSkillDirectory(githubUrl, skillDirectory); - const skillFile = files.find((file) => file.relativePath === 'SKILL.md'); - if (!skillFile) { - throw RequestError.invalidParams( - undefined, - 'GitHub skill directory does not contain SKILL.md', - ); - } - - return { - skillContent: Buffer.from(skillFile.content).toString('utf8'), - files, - }; -} - -function resolveSkillInstallPath( - skillDir: string, - relativePath: string, -): string { - const root = path.resolve(skillDir); - const target = path.resolve(skillDir, relativePath); - if (target !== root && !target.startsWith(root + path.sep)) { - throw RequestError.invalidParams( - undefined, - `Invalid skill file path: ${relativePath}`, - ); - } - return target; -} - -// Builds the per-skill directory and asserts (defense-in-depth, on top of -// validateSkillSlug) that it stays strictly under the managed skills root, so a -// crafted slug can never make install/delete operate on `` itself. -function resolveManagedSkillDir(skillsBaseDir: string, slug: string): string { - const root = path.resolve(skillsBaseDir); - const skillDir = path.resolve(skillsBaseDir, slug); - if (!skillDir.startsWith(root + path.sep)) { - throw RequestError.invalidParams(undefined, 'Invalid skill.slug'); - } - return skillDir; -} - function readStringArray(value: unknown, fieldName: string): string[] { if (value === undefined || value === null) return []; if (!Array.isArray(value)) { @@ -7882,257 +7107,6 @@ class QwenAgent implements Agent { } } - private async installSkillFromUrl( - request: QwenSkillInstallRequest, - ): Promise> { - const skillManager = this.config.getSkillManager(); - if (!skillManager) { - throw RequestError.invalidParams( - undefined, - 'SkillManager is not available', - ); - } - - const download = await downloadSkill(request.sourceUrl); - const skillsBaseDir = path.join(Storage.getGlobalQwenDir(), 'skills'); - const skillDir = resolveManagedSkillDir(skillsBaseDir, request.slug); - const skillFile = path.join(skillDir, 'SKILL.md'); - const parsed = skillManager.parseSkillContent( - download.skillContent, - skillFile, - 'user', - ); - if (parsed.name !== request.slug) { - throw RequestError.invalidParams( - undefined, - `Skill name "${parsed.name}" does not match requested slug "${request.slug}"`, - ); - } - - // Install atomically: stage all files in a sibling temp directory, then - // swap it in with a single rename. A mid-write failure (disk full, - // permission error) therefore leaves the previously installed skill - // intact instead of deleting it up front and ending up with a partial - // install. Removing the old dir before writing also dropped orphaned - // files from older versions; the rename preserves that property. - const stagingDir = `${skillDir}.installing-${process.pid}-${Date.now()}`; - try { - await fs.rm(stagingDir, { recursive: true, force: true }); - for (const file of download.files) { - const targetPath = resolveSkillInstallPath( - stagingDir, - file.relativePath, - ); - await fs.mkdir(path.dirname(targetPath), { recursive: true }); - await fs.writeFile(targetPath, file.content); - } - // stagingDir is a sibling of skillDir (same filesystem), so the rename - // is atomic; the only gap is between the rm and rename, during which - // the fully-staged copy still exists for recovery. - await fs.rm(skillDir, { recursive: true, force: true }); - await fs.rename(stagingDir, skillDir); - } catch (error) { - await fs.rm(stagingDir, { recursive: true, force: true }).catch(() => {}); - throw error; - } - await skillManager.refreshCache(); - - return { - id: request.id, - slug: parsed.name, - installed: true, - installedPath: skillFile, - sourceUrl: request.sourceUrl, - }; - } - - private async deleteGlobalSkill( - request: QwenSkillDeleteRequest, - ): Promise> { - const skillManager = this.config.getSkillManager(); - if (!skillManager) { - throw RequestError.invalidParams( - undefined, - 'SkillManager is not available', - ); - } - - const { skillDir, skillFile, content } = await this.readManagedSkillFile( - request.slug, - 'global', - skillManager, - ); - const parsed = skillManager.parseSkillContent(content, skillFile, 'user'); - if (parsed.name !== request.slug) { - throw RequestError.invalidParams( - undefined, - `Skill name "${parsed.name}" does not match requested slug "${request.slug}"`, - ); - } - - // Guard the recursive delete: readManagedSkillFile's generic fallback can - // resolve skillDir from listSkills() to an arbitrary path. Only ever remove - // the directory that directly contains the SKILL.md we just validated, and - // never a filesystem root or the global Qwen dir itself, so a malformed - // skill entry can't trigger a destructive rm of a shared/parent directory. - const resolvedSkillDir = path.resolve(skillDir); - const resolvedSkillFile = path.resolve(skillFile); - const globalDir = path.resolve(Storage.getGlobalQwenDir()); - const isDedicatedSkillDir = - resolvedSkillFile === path.join(resolvedSkillDir, 'SKILL.md'); - if ( - !isDedicatedSkillDir || - resolvedSkillDir === path.parse(resolvedSkillDir).root || - resolvedSkillDir === globalDir - ) { - throw RequestError.invalidParams( - undefined, - `Refusing to delete unexpected skill directory: ${skillDir}`, - ); - } - - await fs.rm(skillDir, { recursive: true, force: true }); - await skillManager.refreshCache(); - return { - slug: request.slug, - deleted: true, - }; - } - - private async readManagedSkillFile( - slug: string, - scope: QwenSkillSetEnabledRequest['scope'], - skillManager: NonNullable>, - cwd?: string, - ): Promise { - if (scope === 'global') { - const qwenSkillDir = resolveManagedSkillDir( - path.join(Storage.getGlobalQwenDir(), 'skills'), - slug, - ); - const qwenSkillFile = path.join(qwenSkillDir, 'SKILL.md'); - const qwenContent = await fs - .readFile(qwenSkillFile, 'utf8') - .catch(() => undefined); - if (qwenContent !== undefined) { - return { - skillDir: qwenSkillDir, - skillFile: qwenSkillFile, - content: qwenContent, - }; - } - } - - if (scope === 'project' && cwd?.trim()) { - const projectSkill = await this.findProjectSkillFileFromCwd( - slug, - cwd, - skillManager, - ); - if (projectSkill) return projectSkill; - } - - const level = scope === 'project' ? 'project' : 'user'; - const skill = (await skillManager.listSkills({ level })).find( - (candidate) => candidate.name === slug, - ); - const skillFile = skill?.filePath; - if (!skillFile) { - throw RequestError.invalidParams( - undefined, - `${scope === 'project' ? 'Project' : 'Global'} skill not found: ${slug}`, - ); - } - - const content = await fs.readFile(skillFile, 'utf8').catch(() => { - throw RequestError.invalidParams( - undefined, - `${scope === 'project' ? 'Project' : 'Global'} skill not found: ${slug}`, - ); - }); - return { - skillDir: path.dirname(skillFile), - skillFile, - content, - }; - } - - private async findProjectSkillFileFromCwd( - slug: string, - cwd: string, - skillManager: NonNullable>, - ): Promise { - const projectRoot = path.resolve(cwd); - for (const configDir of PROJECT_SKILL_DIRS) { - const baseDir = path.join(projectRoot, configDir, SKILLS_DIR); - const skills = await skillManager.loadSkillsFromDir(baseDir, 'project'); - const skill = skills.find((candidate) => candidate.name === slug); - const skillFile = skill?.filePath; - if (!skillFile) continue; - - const content = await fs.readFile(skillFile, 'utf8').catch(() => { - throw RequestError.invalidParams( - undefined, - `Project skill not found: ${slug}`, - ); - }); - return { - skillDir: path.dirname(skillFile), - skillFile, - content, - }; - } - return undefined; - } - - private async setGlobalSkillEnabled( - request: QwenSkillSetEnabledRequest, - cwd?: string, - ): Promise> { - const skillManager = this.config.getSkillManager(); - if (!skillManager) { - throw RequestError.invalidParams( - undefined, - 'SkillManager is not available', - ); - } - - const { skillFile, content } = await this.readManagedSkillFile( - request.slug, - request.scope, - skillManager, - cwd, - ); - const level = request.scope === 'project' ? 'project' : 'user'; - const parsed = skillManager.parseSkillContent(content, skillFile, level); - if (parsed.name !== request.slug) { - throw RequestError.invalidParams( - undefined, - `Skill name "${parsed.name}" does not match requested slug "${request.slug}"`, - ); - } - - const nextContent = setSkillFrontmatterEnabled(content, request.enabled); - skillManager.parseSkillContent(nextContent, skillFile, level); - // Defense-in-depth (consistent with deleteGlobalSkill): readManagedSkillFile's - // generic fallback can resolve skillFile from listSkills() to an arbitrary - // path. We only ever write back to the SKILL.md manifest we just read and - // whose parsed name matched the slug, so refuse to write anything else. - if (path.basename(skillFile) !== 'SKILL.md') { - throw RequestError.invalidParams( - undefined, - `Refusing to write to unexpected skill file: ${skillFile}`, - ); - } - await fs.writeFile(skillFile, nextContent, 'utf8'); - await skillManager.refreshCache(); - return { - slug: request.slug, - enabled: request.enabled, - installedPath: skillFile, - }; - } - async extMethod( method: string, params: Record, @@ -8291,16 +7265,13 @@ class QwenAgent implements Agent { }; } case 'qwen/skills/install': { - return this.installSkillFromUrl(readSkillInstallRequest(params)); + return installManagedSkill(this.config, params); } case 'qwen/skills/delete': { - return this.deleteGlobalSkill(readSkillSlugRequest(params)); + return deleteManagedSkill(this.config, params); } case 'qwen/skills/setEnabled': { - return this.setGlobalSkillEnabled( - readSkillSetEnabledRequest(params), - requestedCwd, - ); + return setManagedSkillEnabled(this.config, params, requestedCwd); } case 'qwen/settings/getMemory': { const settings = loadSettings(cwd); diff --git a/packages/cli/src/acp-integration/skill-management.test.ts b/packages/cli/src/acp-integration/skill-management.test.ts new file mode 100644 index 00000000000..f134160e586 --- /dev/null +++ b/packages/cli/src/acp-integration/skill-management.test.ts @@ -0,0 +1,344 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { + Storage, + type Config, + type SkillLevel, +} from '@qwen-code/qwen-code-core'; +import * as fs from 'node:fs/promises'; +import * as os from 'node:os'; +import * as path from 'node:path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +const downloadSkillMock = vi.hoisted(() => vi.fn()); + +vi.mock('./skill-source-download.js', () => ({ + downloadSkill: downloadSkillMock, +})); + +import { + deleteManagedSkill, + installManagedSkill, + setManagedSkillEnabled, +} from './skill-management.js'; + +type SkillManager = NonNullable>; + +function configWith(skillManager: object): Config { + return { + getSkillManager: () => skillManager as SkillManager, + } as unknown as Config; +} + +function managerFor(name: string) { + const parseSkillContent = vi.fn( + (_content: string, filePath: string, level: SkillLevel) => ({ + name, + description: `${name} skill`, + level, + filePath, + skillRoot: path.dirname(filePath), + body: 'Body', + }), + ); + const refreshCache = vi.fn().mockResolvedValue(undefined); + return { parseSkillContent, refreshCache }; +} + +async function writeSkill(root: string, relativeDir: string, name: string) { + const skillDir = path.join(root, relativeDir, name); + const skillFile = path.join(skillDir, 'SKILL.md'); + await fs.mkdir(skillDir, { recursive: true }); + await fs.writeFile( + skillFile, + `---\nname: ${name}\ndescription: ${name} skill\n---\nBody\n`, + 'utf8', + ); + return { skillDir, skillFile }; +} + +afterEach(() => { + downloadSkillMock.mockReset(); + vi.restoreAllMocks(); +}); + +describe('managed Skill mutations', () => { + it('installs every downloaded file and refreshes the cache', async () => { + const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-')); + vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome); + const manager = managerFor('pptx'); + downloadSkillMock.mockResolvedValue({ + skillContent: + '---\nname: pptx\ndescription: Create slide decks\n---\nBody\n', + files: [ + { + relativePath: 'SKILL.md', + content: Buffer.from('---\nname: pptx\n---\nBody\n'), + }, + { + relativePath: 'references/editing.md', + content: Buffer.from('# Editing guide\n'), + }, + ], + }); + + try { + const result = await installManagedSkill(configWith(manager), { + skill: { + id: 'pptx-id', + slug: 'pptx', + name: 'PPTX', + sourceUrl: + 'https://github.com/anthropics/skills/blob/main/skills/pptx/SKILL.md', + }, + }); + const installedPath = path.join(tempHome, 'skills', 'pptx', 'SKILL.md'); + + expect(result).toMatchObject({ + id: 'pptx-id', + slug: 'pptx', + installed: true, + installedPath, + }); + await expect(fs.readFile(installedPath, 'utf8')).resolves.toContain( + 'name: pptx', + ); + await expect( + fs.readFile( + path.join(tempHome, 'skills', 'pptx', 'references', 'editing.md'), + 'utf8', + ), + ).resolves.toBe('# Editing guide\n'); + expect(manager.parseSkillContent).toHaveBeenCalledWith( + expect.stringContaining('name: pptx'), + installedPath, + 'user', + ); + expect(manager.refreshCache).toHaveBeenCalledTimes(1); + } finally { + await fs.rm(tempHome, { recursive: true, force: true }); + } + }); + + it('enables, disables, and deletes a global Skill', async () => { + const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-')); + vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome); + const { skillDir, skillFile } = await writeSkill( + tempHome, + 'skills', + 'pptx', + ); + const manager = managerFor('pptx'); + const config = configWith(manager); + + try { + await expect( + setManagedSkillEnabled(config, { + skill: { slug: 'pptx', enabled: false }, + }), + ).resolves.toMatchObject({ + slug: 'pptx', + enabled: false, + installedPath: skillFile, + }); + await expect(fs.readFile(skillFile, 'utf8')).resolves.toContain( + 'disable-model-invocation: true', + ); + + await setManagedSkillEnabled(config, { + skill: { slug: 'pptx', enabled: true }, + }); + await expect(fs.readFile(skillFile, 'utf8')).resolves.not.toContain( + 'disable-model-invocation', + ); + + await expect( + deleteManagedSkill(config, { skill: { slug: 'pptx' } }), + ).resolves.toEqual({ slug: 'pptx', deleted: true }); + await expect(fs.stat(skillDir)).rejects.toThrow(); + expect(manager.refreshCache).toHaveBeenCalledTimes(3); + } finally { + await fs.rm(tempHome, { recursive: true, force: true }); + } + }); + + it('preserves comments and nested hooks when toggling frontmatter', async () => { + const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-')); + vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome); + const skillDir = path.join(tempHome, 'skills', 'pptx'); + const skillFile = path.join(skillDir, 'SKILL.md'); + await fs.mkdir(skillDir, { recursive: true }); + await fs.writeFile( + skillFile, + '---\n# keep this comment\nname: pptx\nhooks:\n PreToolUse:\n - matcher: Bash\n command: echo hi\n---\nBody\n', + 'utf8', + ); + const config = configWith(managerFor('pptx')); + + try { + await setManagedSkillEnabled(config, { + skill: { slug: 'pptx', enabled: false }, + }); + let content = await fs.readFile(skillFile, 'utf8'); + expect(content).toContain('# keep this comment'); + expect(content).toContain('hooks:'); + expect(content).toContain('matcher: Bash'); + expect(content).toContain('disable-model-invocation: true'); + + await setManagedSkillEnabled(config, { + skill: { slug: 'pptx', enabled: true }, + }); + content = await fs.readFile(skillFile, 'utf8'); + expect(content).toContain('# keep this comment'); + expect(content).toContain('hooks:'); + expect(content).not.toContain('disable-model-invocation'); + } finally { + await fs.rm(tempHome, { recursive: true, force: true }); + } + }); + + it('resolves user and project Skills through the existing manager fallbacks', async () => { + const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-')); + const tempProject = await fs.mkdtemp( + path.join(os.tmpdir(), 'qwen-project-skill-'), + ); + vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome); + const userSkill = await writeSkill(tempHome, '.agents/skills', 'course'); + const projectSkill = await writeSkill( + tempProject, + '.qwen/skills', + 'project-course', + ); + const manager = managerFor('unused'); + manager.parseSkillContent.mockImplementation( + (content: string, filePath: string, level: SkillLevel) => { + const name = content.match(/^name:\s*(.+)$/m)?.[1] ?? 'unknown'; + return { + name, + description: `${name} skill`, + level, + filePath, + skillRoot: path.dirname(filePath), + body: 'Body', + }; + }, + ); + const listSkills = vi.fn(({ level }: { level: 'user' | 'project' }) => + Promise.resolve( + level === 'user' + ? [{ name: 'course', filePath: userSkill.skillFile }] + : [{ name: 'project-course', filePath: projectSkill.skillFile }], + ), + ); + const config = configWith({ ...manager, listSkills }); + + try { + await setManagedSkillEnabled(config, { + skill: { slug: 'course', enabled: false }, + }); + await setManagedSkillEnabled(config, { + skill: { + slug: 'project-course', + enabled: false, + scope: 'project', + }, + }); + + await expect(fs.readFile(userSkill.skillFile, 'utf8')).resolves.toContain( + 'disable-model-invocation: true', + ); + await expect( + fs.readFile(projectSkill.skillFile, 'utf8'), + ).resolves.toContain('disable-model-invocation: true'); + expect(listSkills).toHaveBeenCalledWith({ level: 'user' }); + expect(listSkills).toHaveBeenCalledWith({ level: 'project' }); + } finally { + await fs.rm(tempHome, { recursive: true, force: true }); + await fs.rm(tempProject, { recursive: true, force: true }); + } + }); + + it('resolves project Skills from the requested working directory', async () => { + const tempProject = await fs.mkdtemp( + path.join(os.tmpdir(), 'qwen-project-cwd-skill-'), + ); + const skillDir = path.join(tempProject, '.qwen', 'skills', 'issue-fixer'); + const skillFile = path.join(skillDir, 'SKILL.md'); + await fs.mkdir(skillDir, { recursive: true }); + await fs.writeFile( + skillFile, + '---\nname: bugfix\ndescription: Bugfix skill\n---\nBody\n', + 'utf8', + ); + const manager = managerFor('bugfix'); + const loadSkillsFromDir = vi.fn().mockResolvedValue([ + { + name: 'bugfix', + filePath: skillFile, + }, + ]); + const listSkills = vi.fn().mockResolvedValue([]); + const config = configWith({ ...manager, loadSkillsFromDir, listSkills }); + + try { + await expect( + setManagedSkillEnabled( + config, + { + skill: { slug: 'bugfix', enabled: false, scope: 'project' }, + }, + tempProject, + ), + ).resolves.toMatchObject({ + slug: 'bugfix', + enabled: false, + installedPath: skillFile, + }); + expect(loadSkillsFromDir).toHaveBeenCalledWith( + path.join(tempProject, '.qwen', 'skills'), + 'project', + ); + expect(listSkills).not.toHaveBeenCalled(); + } finally { + await fs.rm(tempProject, { recursive: true, force: true }); + } + }); + + it('rejects traversal slugs before downloading or touching disk', async () => { + const tempHome = await fs.mkdtemp(path.join(os.tmpdir(), 'qwen-skill-')); + vi.spyOn(Storage, 'getGlobalQwenDir').mockReturnValue(tempHome); + const sentinel = path.join(tempHome, 'settings.json'); + await fs.writeFile(sentinel, '{"keep":true}', 'utf8'); + const config = configWith(managerFor('unused')); + + try { + for (const slug of ['..', '.']) { + await expect( + installManagedSkill(config, { + skill: { + slug, + sourceUrl: + 'https://github.com/anthropics/skills/blob/main/SKILL.md', + }, + }), + ).rejects.toThrow('Invalid skill.slug'); + await expect( + deleteManagedSkill(config, { skill: { slug } }), + ).rejects.toThrow('Invalid skill.slug'); + await expect( + setManagedSkillEnabled(config, { + skill: { slug, enabled: false }, + }), + ).rejects.toThrow('Invalid skill.slug'); + } + expect(downloadSkillMock).not.toHaveBeenCalled(); + await expect(fs.readFile(sentinel, 'utf8')).resolves.toContain('keep'); + } finally { + await fs.rm(tempHome, { recursive: true, force: true }); + } + }); +}); diff --git a/packages/cli/src/acp-integration/skill-management.ts b/packages/cli/src/acp-integration/skill-management.ts new file mode 100644 index 00000000000..55038c30854 --- /dev/null +++ b/packages/cli/src/acp-integration/skill-management.ts @@ -0,0 +1,517 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { Storage, type Config } from '@qwen-code/qwen-code-core'; +import { RequestError } from '@agentclientprotocol/sdk'; +import * as fs from 'node:fs/promises'; +import * as path from 'node:path'; +import { downloadSkill } from './skill-source-download.js'; + +function toRecord(value: unknown): Record { + return !!value && typeof value === 'object' && !Array.isArray(value) + ? (value as Record) + : {}; +} + +function readOptionalString( + value: unknown, + fieldName: string, +): string | undefined { + if (value === undefined || value === null) return undefined; + if (typeof value !== 'string') { + throw RequestError.invalidParams( + undefined, + `Invalid ${fieldName}: expected string`, + ); + } + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : undefined; +} + +function readRequiredString(value: unknown, fieldName: string): string { + const stringValue = readOptionalString(value, fieldName); + if (!stringValue) { + throw RequestError.invalidParams( + undefined, + `Invalid or missing ${fieldName}`, + ); + } + return stringValue; +} + +type QwenSkillInstallRequest = { + id: string; + slug: string; + name: string; + description?: string; + sourceUrl: string; + scope: 'global'; +}; + +type QwenSkillDeleteRequest = { + slug: string; + scope: 'global'; +}; + +type QwenSkillSetEnabledRequest = { + slug: string; + enabled: boolean; + scope: 'global' | 'project'; +}; + +type QwenManagedSkillFile = { + skillDir: string; + skillFile: string; + content: string; +}; + +const PROJECT_SKILL_DIRS = ['.qwen', '.agents'] as const; +const SKILLS_DIR = 'skills'; + +// Skill slugs are used to build filesystem paths under `/skills`. +// The character allowlist below already excludes `/` and `\`, but `.` and `..` +// would still slip through and let `path.join` traverse out of the skills dir +// (e.g. slug `..` resolves to the global config dir). Reject them explicitly. +function validateSkillSlug(slug: string): void { + if ( + !slug || + slug === '.' || + slug === '..' || + slug.includes('/') || + slug.includes(path.sep) || + !/^[a-zA-Z0-9._-]+$/.test(slug) + ) { + throw RequestError.invalidParams(undefined, 'Invalid skill.slug'); + } +} + +function readSkillInstallRequest( + params: Record, +): QwenSkillInstallRequest { + const skillParams = toRecord(params['skill']); + const input = Object.keys(skillParams).length > 0 ? skillParams : params; + const slug = readRequiredString(input['slug'], 'skill.slug'); + validateSkillSlug(slug); + + const scope = readOptionalString(input['scope'], 'skill.scope') ?? 'global'; + if (scope !== 'global') { + throw RequestError.invalidParams( + undefined, + 'Only global skill installation is supported', + ); + } + + const description = readOptionalString( + input['description'], + 'skill.description', + ); + return { + id: readOptionalString(input['id'], 'skill.id') ?? slug, + slug, + name: readOptionalString(input['name'], 'skill.name') ?? slug, + ...(description ? { description } : {}), + sourceUrl: readRequiredString(input['sourceUrl'], 'skill.sourceUrl'), + scope, + }; +} + +function readSkillSlugRequest( + params: Record, +): QwenSkillDeleteRequest { + const skillParams = toRecord(params['skill']); + const input = Object.keys(skillParams).length > 0 ? skillParams : params; + const slug = readRequiredString(input['slug'], 'skill.slug'); + validateSkillSlug(slug); + + const scope = readOptionalString(input['scope'], 'skill.scope') ?? 'global'; + if (scope !== 'global') { + throw RequestError.invalidParams( + undefined, + 'Only global skill management is supported', + ); + } + + return { slug, scope }; +} + +function readSkillSetEnabledRequest( + params: Record, +): QwenSkillSetEnabledRequest { + const skillParams = toRecord(params['skill']); + const input = Object.keys(skillParams).length > 0 ? skillParams : params; + const slug = readRequiredString(input['slug'], 'skill.slug'); + validateSkillSlug(slug); + + const scope = readOptionalString(input['scope'], 'skill.scope') ?? 'global'; + if (scope !== 'global' && scope !== 'project') { + throw RequestError.invalidParams( + undefined, + 'Only global or project skill management is supported', + ); + } + + if (typeof input['enabled'] !== 'boolean') { + throw RequestError.invalidParams( + undefined, + 'Invalid skill.enabled: expected boolean', + ); + } + return { + slug, + scope, + enabled: input['enabled'], + }; +} + +function splitSkillMarkdown(content: string): { + frontmatter: string; + body: string; +} { + const normalized = content.replace(/^\uFEFF/, '').replace(/\r\n?/g, '\n'); + const match = normalized.match(/^---\n([\s\S]*?)\n---(?:\n|$)([\s\S]*)$/); + if (!match) { + throw RequestError.invalidParams( + undefined, + 'Invalid skill file: missing YAML frontmatter', + ); + } + return { + frontmatter: match[1], + body: match[2], + }; +} + +function setSkillFrontmatterEnabled(content: string, enabled: boolean): string { + const { frontmatter, body } = splitSkillMarkdown(content); + + // Surgically add/remove only the top-level `disable-model-invocation:` line + // instead of round-tripping the whole frontmatter through a YAML + // parse/stringify. The minimal core YAML serializer drops comments and + // flattens nested structures (e.g. `hooks:`), so reserializing here would + // corrupt hooks-bearing skills and strip user comments. Working on the raw + // text leaves every other byte untouched. + const lines = frontmatter.split('\n'); + const disabledLineIndex = lines.findIndex((line) => + /^disable-model-invocation\s*:/.test(line), + ); + + if (enabled) { + if (disabledLineIndex !== -1) { + lines.splice(disabledLineIndex, 1); + } + } else if (disabledLineIndex !== -1) { + lines[disabledLineIndex] = 'disable-model-invocation: true'; + } else { + let insertIndex = lines.length; + while (insertIndex > 0 && lines[insertIndex - 1].trim() === '') { + insertIndex -= 1; + } + lines.splice(insertIndex, 0, 'disable-model-invocation: true'); + } + + const nextFrontmatter = lines.join('\n'); + return `---\n${nextFrontmatter}\n---\n${body}`; +} + +function resolveSkillInstallPath( + skillDir: string, + relativePath: string, +): string { + const root = path.resolve(skillDir); + const target = path.resolve(skillDir, relativePath); + if (target !== root && !target.startsWith(root + path.sep)) { + throw RequestError.invalidParams( + undefined, + `Invalid skill file path: ${relativePath}`, + ); + } + return target; +} + +// Builds the per-skill directory and asserts (defense-in-depth, on top of +// validateSkillSlug) that it stays strictly under the managed skills root, so a +// crafted slug can never make install/delete operate on `` itself. +function resolveManagedSkillDir(skillsBaseDir: string, slug: string): string { + const root = path.resolve(skillsBaseDir); + const skillDir = path.resolve(skillsBaseDir, slug); + if (!skillDir.startsWith(root + path.sep)) { + throw RequestError.invalidParams(undefined, 'Invalid skill.slug'); + } + return skillDir; +} + +export async function installManagedSkill( + config: Config, + params: Record, +): Promise> { + return installSkillFromUrl(config, readSkillInstallRequest(params)); +} + +export async function deleteManagedSkill( + config: Config, + params: Record, +): Promise> { + return deleteGlobalSkill(config, readSkillSlugRequest(params)); +} + +export async function setManagedSkillEnabled( + config: Config, + params: Record, + cwd?: string, +): Promise> { + return setGlobalSkillEnabled(config, readSkillSetEnabledRequest(params), cwd); +} + +async function installSkillFromUrl( + config: Config, + request: QwenSkillInstallRequest, +): Promise> { + const skillManager = config.getSkillManager(); + if (!skillManager) { + throw RequestError.invalidParams( + undefined, + 'SkillManager is not available', + ); + } + + const download = await downloadSkill(request.sourceUrl); + const skillsBaseDir = path.join(Storage.getGlobalQwenDir(), 'skills'); + const skillDir = resolveManagedSkillDir(skillsBaseDir, request.slug); + const skillFile = path.join(skillDir, 'SKILL.md'); + const parsed = skillManager.parseSkillContent( + download.skillContent, + skillFile, + 'user', + ); + if (parsed.name !== request.slug) { + throw RequestError.invalidParams( + undefined, + `Skill name "${parsed.name}" does not match requested slug "${request.slug}"`, + ); + } + + // Install atomically: stage all files in a sibling temp directory, then + // swap it in with a single rename. A mid-write failure (disk full, + // permission error) therefore leaves the previously installed skill + // intact instead of deleting it up front and ending up with a partial + // install. Removing the old dir before writing also dropped orphaned + // files from older versions; the rename preserves that property. + const stagingDir = `${skillDir}.installing-${process.pid}-${Date.now()}`; + try { + await fs.rm(stagingDir, { recursive: true, force: true }); + for (const file of download.files) { + const targetPath = resolveSkillInstallPath(stagingDir, file.relativePath); + await fs.mkdir(path.dirname(targetPath), { recursive: true }); + await fs.writeFile(targetPath, file.content); + } + // stagingDir is a sibling of skillDir (same filesystem), so the rename + // is atomic; the only gap is between the rm and rename, during which + // the fully-staged copy still exists for recovery. + await fs.rm(skillDir, { recursive: true, force: true }); + await fs.rename(stagingDir, skillDir); + } catch (error) { + await fs.rm(stagingDir, { recursive: true, force: true }).catch(() => {}); + throw error; + } + await skillManager.refreshCache(); + + return { + id: request.id, + slug: parsed.name, + installed: true, + installedPath: skillFile, + sourceUrl: request.sourceUrl, + }; +} + +async function deleteGlobalSkill( + config: Config, + request: QwenSkillDeleteRequest, +): Promise> { + const skillManager = config.getSkillManager(); + if (!skillManager) { + throw RequestError.invalidParams( + undefined, + 'SkillManager is not available', + ); + } + + const { skillDir, skillFile, content } = await readManagedSkillFile( + request.slug, + 'global', + skillManager, + ); + const parsed = skillManager.parseSkillContent(content, skillFile, 'user'); + if (parsed.name !== request.slug) { + throw RequestError.invalidParams( + undefined, + `Skill name "${parsed.name}" does not match requested slug "${request.slug}"`, + ); + } + + // Guard the recursive delete: readManagedSkillFile's generic fallback can + // resolve skillDir from listSkills() to an arbitrary path. Only ever remove + // the directory that directly contains the SKILL.md we just validated, and + // never a filesystem root or the global Qwen dir itself, so a malformed + // skill entry can't trigger a destructive rm of a shared/parent directory. + const resolvedSkillDir = path.resolve(skillDir); + const resolvedSkillFile = path.resolve(skillFile); + const globalDir = path.resolve(Storage.getGlobalQwenDir()); + const isDedicatedSkillDir = + resolvedSkillFile === path.join(resolvedSkillDir, 'SKILL.md'); + if ( + !isDedicatedSkillDir || + resolvedSkillDir === path.parse(resolvedSkillDir).root || + resolvedSkillDir === globalDir + ) { + throw RequestError.invalidParams( + undefined, + `Refusing to delete unexpected skill directory: ${skillDir}`, + ); + } + + await fs.rm(skillDir, { recursive: true, force: true }); + await skillManager.refreshCache(); + return { + slug: request.slug, + deleted: true, + }; +} + +async function readManagedSkillFile( + slug: string, + scope: QwenSkillSetEnabledRequest['scope'], + skillManager: NonNullable>, + cwd?: string, +): Promise { + if (scope === 'global') { + const qwenSkillDir = resolveManagedSkillDir( + path.join(Storage.getGlobalQwenDir(), 'skills'), + slug, + ); + const qwenSkillFile = path.join(qwenSkillDir, 'SKILL.md'); + const qwenContent = await fs + .readFile(qwenSkillFile, 'utf8') + .catch(() => undefined); + if (qwenContent !== undefined) { + return { + skillDir: qwenSkillDir, + skillFile: qwenSkillFile, + content: qwenContent, + }; + } + } + + if (scope === 'project' && cwd?.trim()) { + const projectSkill = await findProjectSkillFileFromCwd( + slug, + cwd, + skillManager, + ); + if (projectSkill) return projectSkill; + } + + const level = scope === 'project' ? 'project' : 'user'; + const skill = (await skillManager.listSkills({ level })).find( + (candidate) => candidate.name === slug, + ); + const skillFile = skill?.filePath; + if (!skillFile) { + throw RequestError.invalidParams( + undefined, + `${scope === 'project' ? 'Project' : 'Global'} skill not found: ${slug}`, + ); + } + + const content = await fs.readFile(skillFile, 'utf8').catch(() => { + throw RequestError.invalidParams( + undefined, + `${scope === 'project' ? 'Project' : 'Global'} skill not found: ${slug}`, + ); + }); + return { + skillDir: path.dirname(skillFile), + skillFile, + content, + }; +} + +async function findProjectSkillFileFromCwd( + slug: string, + cwd: string, + skillManager: NonNullable>, +): Promise { + const projectRoot = path.resolve(cwd); + for (const configDir of PROJECT_SKILL_DIRS) { + const baseDir = path.join(projectRoot, configDir, SKILLS_DIR); + const skills = await skillManager.loadSkillsFromDir(baseDir, 'project'); + const skill = skills.find((candidate) => candidate.name === slug); + const skillFile = skill?.filePath; + if (!skillFile) continue; + + const content = await fs.readFile(skillFile, 'utf8').catch(() => { + throw RequestError.invalidParams( + undefined, + `Project skill not found: ${slug}`, + ); + }); + return { + skillDir: path.dirname(skillFile), + skillFile, + content, + }; + } + return undefined; +} + +async function setGlobalSkillEnabled( + config: Config, + request: QwenSkillSetEnabledRequest, + cwd?: string, +): Promise> { + const skillManager = config.getSkillManager(); + if (!skillManager) { + throw RequestError.invalidParams( + undefined, + 'SkillManager is not available', + ); + } + + const { skillFile, content } = await readManagedSkillFile( + request.slug, + request.scope, + skillManager, + cwd, + ); + const level = request.scope === 'project' ? 'project' : 'user'; + const parsed = skillManager.parseSkillContent(content, skillFile, level); + if (parsed.name !== request.slug) { + throw RequestError.invalidParams( + undefined, + `Skill name "${parsed.name}" does not match requested slug "${request.slug}"`, + ); + } + + const nextContent = setSkillFrontmatterEnabled(content, request.enabled); + skillManager.parseSkillContent(nextContent, skillFile, level); + // Defense-in-depth (consistent with deleteGlobalSkill): readManagedSkillFile's + // generic fallback can resolve skillFile from listSkills() to an arbitrary + // path. We only ever write back to the SKILL.md manifest we just read and + // whose parsed name matched the slug, so refuse to write anything else. + if (path.basename(skillFile) !== 'SKILL.md') { + throw RequestError.invalidParams( + undefined, + `Refusing to write to unexpected skill file: ${skillFile}`, + ); + } + await fs.writeFile(skillFile, nextContent, 'utf8'); + await skillManager.refreshCache(); + return { + slug: request.slug, + enabled: request.enabled, + installedPath: skillFile, + }; +} diff --git a/packages/cli/src/acp-integration/skill-source-download.test.ts b/packages/cli/src/acp-integration/skill-source-download.test.ts new file mode 100644 index 00000000000..f8f819c6bc1 --- /dev/null +++ b/packages/cli/src/acp-integration/skill-source-download.test.ts @@ -0,0 +1,229 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { gzipSync } from 'node:zlib'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { + downloadSkill, + extractFilesFromTarGz, + fetchAllowedGitHub, +} from './skill-source-download.js'; + +function tarEntry(name: string, content: string): Buffer { + const header = Buffer.alloc(512); + header.write(name, 0, 'utf8'); + const size = Buffer.byteLength(content); + header.write(`${size.toString(8).padStart(11, '0')}\0`, 124, 'utf8'); + header.write('0', 156, 'utf8'); + const data = Buffer.alloc(Math.ceil(size / 512) * 512); + data.write(content, 0, 'utf8'); + return Buffer.concat([header, data]); +} + +function makeTarGz(name: string, content: string): Uint8Array { + const tar = Buffer.concat([tarEntry(name, content), Buffer.alloc(1024)]); + return new Uint8Array(gzipSync(tar)); +} + +function toArrayBuffer(buffer: Uint8Array): ArrayBuffer { + return buffer.buffer.slice( + buffer.byteOffset, + buffer.byteOffset + buffer.byteLength, + ) as ArrayBuffer; +} + +afterEach(() => { + vi.unstubAllGlobals(); +}); + +describe('extractFilesFromTarGz', () => { + it('extracts files under the requested directory', async () => { + const archive = makeTarGz('repo-main/skills/SKILL.md', 'hello skill'); + const files = await extractFilesFromTarGz(archive, 'skills'); + + expect(files).toHaveLength(1); + expect(files[0]!.relativePath).toBe('SKILL.md'); + expect(Buffer.from(files[0]!.content).toString('utf8')).toBe('hello skill'); + }); + + it('rejects an archive whose compressed size exceeds the limit', async () => { + await expect( + extractFilesFromTarGz(new Uint8Array(64), 'skills', { + maxCompressedBytes: 16, + }), + ).rejects.toThrowError(/exceeds the maximum allowed size/); + }); + + it('rejects an archive that fails to decompress', async () => { + await expect( + extractFilesFromTarGz(new Uint8Array([1, 2, 3, 4, 5]), 'skills'), + ).rejects.toThrowError(/Failed to decompress skill archive/); + }); + + it('rejects an archive whose decompressed size exceeds the limit', async () => { + const archive = makeTarGz('repo-main/skills/SKILL.md', 'x'.repeat(2048)); + await expect( + extractFilesFromTarGz(archive, 'skills', { + maxDecompressedBytes: 16, + }), + ).rejects.toThrowError(/Decompressed skill archive exceeds/); + }); +}); + +describe('fetchAllowedGitHub', () => { + function fakeResponse(status: number, location?: string) { + return { + status, + ok: status >= 200 && status < 300, + headers: { + get: (key: string) => + key.toLowerCase() === 'location' && location ? location : null, + }, + }; + } + + it('returns the response directly when there is no redirect', async () => { + const response = fakeResponse(200); + vi.stubGlobal('fetch', vi.fn().mockResolvedValue(response)); + + await expect( + fetchAllowedGitHub('https://raw.githubusercontent.com/a/b/main/SKILL.md'), + ).resolves.toBe(response); + }); + + it('follows a redirect to an allowed GitHub CDN host', async () => { + const final = fakeResponse(200); + const fetchMock = vi + .fn() + .mockResolvedValueOnce( + fakeResponse(302, 'https://objects.githubusercontent.com/x'), + ) + .mockResolvedValueOnce(final); + vi.stubGlobal('fetch', fetchMock); + + await expect( + fetchAllowedGitHub('https://codeload.github.com/a/b/tar.gz/main'), + ).resolves.toBe(final); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it.each(['https://evil.com/x', 'http://raw.githubusercontent.com/x'])( + 'rejects a redirect to %s', + async (location) => { + vi.stubGlobal( + 'fetch', + vi.fn().mockResolvedValue(fakeResponse(302, location)), + ); + + await expect( + fetchAllowedGitHub('https://raw.githubusercontent.com/a/b/SKILL.md'), + ).rejects.toThrow(/disallowed host/); + }, + ); + + it('rejects when the redirect limit is exceeded', async () => { + vi.stubGlobal( + 'fetch', + vi + .fn() + .mockResolvedValue( + fakeResponse(302, 'https://raw.githubusercontent.com/loop'), + ), + ); + + await expect( + fetchAllowedGitHub('https://raw.githubusercontent.com/a', {}, 2), + ).rejects.toThrow(/maximum number of redirects/); + }); + + it('resolves a relative Location against the current URL', async () => { + const final = fakeResponse(200); + const fetchMock = vi + .fn() + .mockResolvedValueOnce(fakeResponse(302, '/a/b/SKILL.md')) + .mockResolvedValueOnce(final); + vi.stubGlobal('fetch', fetchMock); + + await expect( + fetchAllowedGitHub('https://raw.githubusercontent.com/start'), + ).resolves.toBe(final); + expect(fetchMock.mock.calls[1]![0]).toBe( + 'https://raw.githubusercontent.com/a/b/SKILL.md', + ); + }); +}); + +describe('downloadSkill', () => { + it.each([ + 'http://github.com/owner/repo/blob/main/skills/x/SKILL.md', + 'https://evil.com/owner/repo/blob/main/skills/x/SKILL.md', + 'https://github.com.attacker.com/owner/repo/blob/main/SKILL.md', + ])('rejects the unsupported source %s', async (sourceUrl) => { + await expect(downloadSkill(sourceUrl)).rejects.toThrow(); + }); + + it('downloads every file from a GitHub skill directory', async () => { + const skillContent = + '---\nname: pptx\ndescription: Create slide decks\n---\nCreate slide decks\n'; + const editingContent = '# Editing guide\n'; + const directoryUrl = + 'https://api.github.com/repos/anthropics/skills/contents/skills/pptx?ref=main'; + const skillUrl = + 'https://raw.githubusercontent.com/anthropics/skills/main/skills/pptx/SKILL.md'; + const editingUrl = + 'https://raw.githubusercontent.com/anthropics/skills/main/skills/pptx/editing.md'; + const fetchMock = vi.fn(async (url: string) => { + if (url === directoryUrl) { + return { + ok: true, + status: 200, + json: vi.fn().mockResolvedValue([ + { + name: 'SKILL.md', + path: 'skills/pptx/SKILL.md', + type: 'file', + download_url: skillUrl, + }, + { + name: 'editing.md', + path: 'skills/pptx/editing.md', + type: 'file', + download_url: editingUrl, + }, + ]), + }; + } + const content = url === skillUrl ? skillContent : editingContent; + return { + ok: true, + status: 200, + arrayBuffer: vi + .fn() + .mockResolvedValue(toArrayBuffer(Buffer.from(content))), + }; + }); + vi.stubGlobal('fetch', fetchMock); + + const skill = await downloadSkill( + 'https://github.com/anthropics/skills/blob/main/skills/pptx/SKILL.md', + ); + + expect(skill.skillContent).toBe(skillContent); + expect(skill.files.map((file) => file.relativePath)).toEqual([ + 'SKILL.md', + 'editing.md', + ]); + expect(fetchMock).toHaveBeenCalledWith( + directoryUrl, + expect.objectContaining({ + headers: expect.objectContaining({ + Accept: 'application/vnd.github+json', + 'User-Agent': 'qwen-code', + }), + }), + ); + }); +}); diff --git a/packages/cli/src/acp-integration/skill-source-download.ts b/packages/cli/src/acp-integration/skill-source-download.ts new file mode 100644 index 00000000000..a21fff07e68 --- /dev/null +++ b/packages/cli/src/acp-integration/skill-source-download.ts @@ -0,0 +1,625 @@ +/** + * @license + * Copyright 2025 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { createDebugLogger } from '@qwen-code/qwen-code-core'; +import { RequestError } from '@agentclientprotocol/sdk'; +import { Readable, Writable } from 'node:stream'; +import { pipeline } from 'node:stream/promises'; +import * as path from 'node:path'; +import { createGunzip } from 'node:zlib'; + +const debugLogger = createDebugLogger('ACP_AGENT'); + +function toRecord(value: unknown): Record { + return !!value && typeof value === 'object' && !Array.isArray(value) + ? (value as Record) + : {}; +} + +function readOptionalString( + value: unknown, + fieldName: string, +): string | undefined { + if (value === undefined || value === null) return undefined; + if (typeof value !== 'string') { + throw RequestError.invalidParams( + undefined, + `Invalid ${fieldName}: expected string`, + ); + } + const trimmed = value.trim(); + return trimmed.length > 0 ? trimmed : undefined; +} + +function readRequiredString(value: unknown, fieldName: string): string { + const stringValue = readOptionalString(value, fieldName); + if (!stringValue) { + throw RequestError.invalidParams( + undefined, + `Invalid or missing ${fieldName}`, + ); + } + return stringValue; +} + +type DownloadedSkillFile = { + relativePath: string; + content: Uint8Array; +}; + +type DownloadedSkill = { + skillContent: string; + files: DownloadedSkillFile[]; +}; + +type GitHubBlobSkillUrl = { + owner: string; + repo: string; + ref: string; + filePath: string; +}; + +// Skill downloads must come from the GitHub host set. Restricting the host +// here prevents the client-supplied `sourceUrl` from driving server-side +// fetches at internal/loopback/link-local endpoints (SSRF), e.g. +// `http://169.254.169.254/` cloud-metadata or `http://localhost:/`. +const ALLOWED_SKILL_SOURCE_HOSTS = new Set([ + 'github.com', + 'raw.githubusercontent.com', + 'codeload.github.com', + 'api.github.com', +]); + +function assertAllowedSkillSourceUrl(sourceUrl: string): void { + let parsed: URL; + try { + parsed = new URL(sourceUrl); + } catch { + throw RequestError.invalidParams( + undefined, + 'Skill sourceUrl must be a valid URL', + ); + } + // Require HTTPS: a plaintext http: fetch of skill content (which can include + // executable hooks) is MITM-able by a network-position attacker, so the host + // allowlist alone is not sufficient. All supported GitHub hosts serve HTTPS. + if (parsed.protocol !== 'https:') { + throw RequestError.invalidParams( + undefined, + 'Skill sourceUrl must be an HTTPS URL', + ); + } + if (!ALLOWED_SKILL_SOURCE_HOSTS.has(parsed.hostname)) { + throw RequestError.invalidParams( + undefined, + 'Skill sourceUrl host is not allowed (only github.com sources are supported)', + ); + } +} + +function parseGitHubBlobSkillUrl(sourceUrl: string): GitHubBlobSkillUrl | null { + const parsed = new URL(sourceUrl); + // HTTPS-only, consistent with assertAllowedSkillSourceUrl (skill content can + // include executable hooks, so plaintext http: is MITM-able). + if (parsed.protocol !== 'https:') { + throw RequestError.invalidParams( + undefined, + 'Skill sourceUrl must be an HTTPS URL', + ); + } + + if (parsed.hostname !== 'github.com') return null; + const parts = parsed.pathname.split('/').filter(Boolean); + if (parts.length < 5 || parts[2] !== 'blob') return null; + + const owner = parts[0]; + const repo = parts[1]; + const ref = parts[3]; + const filePathParts = parts.slice(4); + if (!owner || !repo || !ref || filePathParts.length === 0) return null; + + return { + owner, + repo, + ref, + filePath: filePathParts.join('/'), + }; +} + +function toRawGitHubUrl(githubUrl: GitHubBlobSkillUrl): string { + return `https://raw.githubusercontent.com/${githubUrl.owner}/${githubUrl.repo}/${githubUrl.ref}/${githubUrl.filePath}`; +} + +function encodeGitHubPath(filePath: string): string { + if (!filePath || filePath === '.') return ''; + return filePath.split('/').map(encodeURIComponent).join('/'); +} + +function readTarString( + archive: Uint8Array, + offset: number, + length: number, +): string { + const bytes = archive.subarray(offset, offset + length); + const nul = bytes.indexOf(0); + const end = nul >= 0 ? nul : bytes.length; + return Buffer.from(bytes.subarray(0, end)).toString('utf8').trim(); +} + +function readTarSize(archive: Uint8Array, offset: number): number { + const raw = readTarString(archive, offset + 124, 12); + return raw ? Number.parseInt(raw, 8) : 0; +} + +function isZeroTarBlock(archive: Uint8Array, offset: number): boolean { + for (let i = 0; i < 512; i += 1) { + if (archive[offset + i] !== 0) return false; + } + return true; +} + +function readTarPath(archive: Uint8Array, offset: number): string { + const name = readTarString(archive, offset, 100); + const prefix = readTarString(archive, offset + 345, 155); + return prefix ? `${prefix}/${name}` : name; +} + +function stripArchiveRoot(filePath: string): string { + const parts = filePath.split('/').filter(Boolean); + return parts.length > 1 ? parts.slice(1).join('/') : ''; +} + +// Bound the work done on untrusted skill archives so a malicious or oversized +// download cannot exhaust memory. Decompression is streamed (createGunzip) and +// aborted the moment the cumulative inflated size crosses the cap, so a +// decompression bomb can never fully inflate into memory. +const MAX_SKILL_DOWNLOAD_BYTES = 100 * 1024 * 1024; // 100 MB compressed +const MAX_SKILL_DECOMPRESSED_BYTES = 500 * 1024 * 1024; // 500 MB decompressed +// Bounds for the GitHub Contents-API directory walk (the archive path is +// already bounded by the byte caps above). +const MAX_SKILL_API_DIR_DEPTH = 16; +const MAX_SKILL_API_FILE_COUNT = 2000; + +// Sentinel so the streaming decompression's size-limit abort can be told apart +// from a genuine gunzip/format error in the catch below. +class DecompressedSizeExceededError extends Error {} + +export async function extractFilesFromTarGz( + archiveBytes: Uint8Array, + directoryPath: string, + // Limits are injectable so the size-guard branches can be exercised in tests + // without allocating the 100MB/500MB production thresholds. + limits: { + maxCompressedBytes?: number; + maxDecompressedBytes?: number; + } = {}, +): Promise { + const maxCompressedBytes = + limits.maxCompressedBytes ?? MAX_SKILL_DOWNLOAD_BYTES; + const maxDecompressedBytes = + limits.maxDecompressedBytes ?? MAX_SKILL_DECOMPRESSED_BYTES; + + if (archiveBytes.length > maxCompressedBytes) { + throw RequestError.invalidParams( + undefined, + 'Skill archive exceeds the maximum allowed size', + ); + } + + let archive: Buffer; + try { + // Stream the inflate so we can abort as soon as the cumulative output + // exceeds the cap, instead of materializing the entire decompressed buffer + // first (a ~1000:1 gzip ratio could otherwise inflate a small archive to + // many GB before any post-hoc length check fires). + const chunks: Buffer[] = []; + let total = 0; + await pipeline( + // Wrap in an array so the whole archive is emitted as a single chunk; + // `Readable.from(uint8array)` would otherwise iterate it byte-by-byte. + Readable.from([Buffer.from(archiveBytes)]), + createGunzip(), + new Writable({ + write(chunk: Buffer, _enc, cb) { + total += chunk.length; + if (total > maxDecompressedBytes) { + cb(new DecompressedSizeExceededError()); + return; + } + chunks.push(chunk); + cb(); + }, + }), + ); + archive = Buffer.concat(chunks); + } catch (error) { + if (error instanceof DecompressedSizeExceededError) { + throw RequestError.invalidParams( + undefined, + 'Decompressed skill archive exceeds the maximum allowed size', + ); + } + throw RequestError.invalidParams( + undefined, + `Failed to decompress skill archive: ${ + error instanceof Error ? error.message : String(error) + }`, + ); + } + + const normalizedDirectory = directoryPath.replace(/^\/+|\/+$/g, ''); + // Treat '.' (SKILL.md at the repository root) as the empty prefix; otherwise + // the prefix becomes './' and never matches the root-stripped archive paths + // (e.g. 'SKILL.md'), yielding zero extracted files. + const directoryPrefix = + normalizedDirectory && normalizedDirectory !== '.' + ? `${normalizedDirectory}/` + : ''; + const files: DownloadedSkillFile[] = []; + + for (let offset = 0; offset + 512 <= archive.length; ) { + if (isZeroTarBlock(archive, offset)) break; + + const fullPath = readTarPath(archive, offset); + const typeFlag = String.fromCharCode(archive[offset + 156] || 0); + const size = readTarSize(archive, offset); + const dataOffset = offset + 512; + const nextOffset = dataOffset + Math.ceil(size / 512) * 512; + + if (typeFlag === '0' || typeFlag === '\0') { + const repoPath = stripArchiveRoot(fullPath); + if (repoPath.startsWith(directoryPrefix)) { + const relativePath = repoPath.slice(directoryPrefix.length); + if (relativePath) { + files.push({ + relativePath, + content: archive.subarray(dataOffset, dataOffset + size), + }); + } + } + } + + offset = nextOffset; + } + + return files; +} + +// GitHub host suffixes a download may legitimately redirect to (raw/codeload +// commonly 302 to their object CDN for geo/CDN routing). Redirects to anything +// outside these are rejected, preserving the SSRF guard while not breaking +// real downloads. +const ALLOWED_REDIRECT_HOST_SUFFIXES = [ + '.githubusercontent.com', + '.github.com', + // Note: '.github.io' is intentionally excluded — *.github.io are + // user-controlled GitHub Pages sites, so allowing redirects there would + // reopen the SSRF/exfiltration surface this allowlist exists to close. +]; + +function isAllowedSkillFetchHost(hostname: string): boolean { + if (ALLOWED_SKILL_SOURCE_HOSTS.has(hostname)) return true; + return ALLOWED_REDIRECT_HOST_SUFFIXES.some((suffix) => + hostname.endsWith(suffix), + ); +} + +/** + * Fetch that follows redirects manually, validating every hop stays on an + * allowed GitHub host over HTTPS. This keeps the SSRF protection of + * `redirect: 'manual'` (a malicious repo cannot bounce the fetch to an internal + * endpoint) while still following GitHub's legitimate CDN redirects, which + * plain `redirect: 'manual'` would surface as a download failure. + */ +export async function fetchAllowedGitHub( + url: string, + init: RequestInit = {}, + maxRedirects = 5, +): Promise { + let current = url; + for (let hop = 0; hop <= maxRedirects; hop += 1) { + const response = await fetch(current, { ...init, redirect: 'manual' }); + if (response.status < 300 || response.status >= 400) { + return response; + } + const location = response.headers?.get('location'); + if (!location) return response; + let next: URL; + try { + next = new URL(location, current); + } catch { + throw RequestError.invalidParams( + undefined, + 'Skill download redirected to an invalid URL', + ); + } + if (next.protocol !== 'https:' || !isAllowedSkillFetchHost(next.hostname)) { + throw RequestError.invalidParams( + undefined, + 'Skill download redirected to a disallowed host', + ); + } + current = next.toString(); + } + throw RequestError.invalidParams( + undefined, + 'Skill download exceeded the maximum number of redirects', + ); +} + +// Read a response body while enforcing a hard byte cap against the *actual* +// streamed bytes. The Content-Length pre-checks at the call sites are advisory +// only — a server that omits the header (chunked transfer, CDN redirect) could +// otherwise stream an arbitrarily large body straight into memory via +// `arrayBuffer()`. +async function readBodyWithLimit( + response: Response, + maxBytes: number, +): Promise { + const body = response.body; + if (!body) { + const buf = new Uint8Array(await response.arrayBuffer()); + if (buf.byteLength > maxBytes) { + throw RequestError.invalidParams( + undefined, + 'Skill download exceeds the maximum allowed size', + ); + } + return buf; + } + + const reader = body.getReader(); + const chunks: Uint8Array[] = []; + let total = 0; + for (;;) { + const { done, value } = await reader.read(); + if (done) break; + if (!value) continue; + total += value.byteLength; + if (total > maxBytes) { + await reader.cancel(); + throw RequestError.invalidParams( + undefined, + 'Skill download exceeds the maximum allowed size', + ); + } + chunks.push(value); + } + + const result = new Uint8Array(total); + let offset = 0; + for (const chunk of chunks) { + result.set(chunk, offset); + offset += chunk.byteLength; + } + return result; +} + +async function fetchBytes(url: string): Promise { + const response = await fetchAllowedGitHub(url); + if (!response.ok) { + throw RequestError.invalidParams( + undefined, + `Failed to download skill (${response.status})`, + ); + } + + const contentLength = response.headers?.get('content-length'); + if (contentLength) { + const declaredSize = Number.parseInt(contentLength, 10); + if ( + Number.isFinite(declaredSize) && + declaredSize > MAX_SKILL_DOWNLOAD_BYTES + ) { + throw RequestError.invalidParams( + undefined, + 'Skill download exceeds the maximum allowed size', + ); + } + } + + return readBodyWithLimit(response, MAX_SKILL_DOWNLOAD_BYTES); +} + +async function downloadSingleSkillFile( + sourceUrl: string, +): Promise { + const githubUrl = parseGitHubBlobSkillUrl(sourceUrl); + const fetchUrl = githubUrl ? toRawGitHubUrl(githubUrl) : sourceUrl; + const content = await fetchBytes(fetchUrl); + return { + skillContent: Buffer.from(content).toString('utf8'), + files: [{ relativePath: 'SKILL.md', content }], + }; +} + +async function downloadGitHubSkillDirectoryFromArchive( + githubUrl: GitHubBlobSkillUrl, + directoryPath: string, +): Promise { + const archiveUrl = `https://codeload.github.com/${githubUrl.owner}/${githubUrl.repo}/tar.gz/${encodeURIComponent( + githubUrl.ref, + )}`; + const response = await fetchAllowedGitHub(archiveUrl, { + headers: { + 'User-Agent': 'qwen-code', + }, + }); + if (!response.ok) { + throw RequestError.invalidParams( + undefined, + `Failed to download GitHub skill archive (${response.status})`, + ); + } + + // Reject oversized archives by declared Content-Length before buffering the + // whole body into memory, mirroring the guard in fetchBytes. + const contentLength = response.headers?.get('content-length'); + if (contentLength) { + const declaredSize = Number.parseInt(contentLength, 10); + if ( + Number.isFinite(declaredSize) && + declaredSize > MAX_SKILL_DOWNLOAD_BYTES + ) { + throw RequestError.invalidParams( + undefined, + 'Skill archive exceeds the maximum allowed size', + ); + } + } + + return extractFilesFromTarGz( + await readBodyWithLimit(response, MAX_SKILL_DOWNLOAD_BYTES), + directoryPath, + ); +} + +async function fetchGitHubDirectoryItems( + githubUrl: GitHubBlobSkillUrl, + directoryPath: string, +): Promise { + const encodedPath = encodeGitHubPath(directoryPath); + const apiUrl = `https://api.github.com/repos/${githubUrl.owner}/${githubUrl.repo}/contents/${encodedPath}?ref=${encodeURIComponent(githubUrl.ref)}`; + const response = await fetchAllowedGitHub(apiUrl, { + headers: { + Accept: 'application/vnd.github+json', + 'User-Agent': 'qwen-code', + }, + }); + if (!response.ok) { + throw RequestError.invalidParams( + undefined, + `Failed to list GitHub skill files (${response.status})`, + ); + } + + const data = await response.json(); + if (!Array.isArray(data)) { + throw RequestError.invalidParams( + undefined, + 'GitHub skill URL must point to a directory-backed SKILL.md file', + ); + } + return data; +} + +async function downloadGitHubSkillDirectoryFromApi( + githubUrl: GitHubBlobSkillUrl, + directoryPath: string, + relativeRoot = '', + // Bound the recursive API walk so a crafted repo (deeply nested dirs, huge + // file counts, or large cumulative size) can't exhaust memory/time. The + // archive fallback already enforces size caps; this gives the API path + // equivalent guards. + depth = 0, + budget: { files: number; bytes: number } = { files: 0, bytes: 0 }, +): Promise { + if (depth > MAX_SKILL_API_DIR_DEPTH) { + throw RequestError.invalidParams( + undefined, + 'Skill directory nesting exceeds the maximum allowed depth', + ); + } + const items = await fetchGitHubDirectoryItems(githubUrl, directoryPath); + const files: DownloadedSkillFile[] = []; + + for (const item of items) { + const record = toRecord(item); + const name = readRequiredString(record['name'], 'github.name'); + const itemPath = readRequiredString(record['path'], 'github.path'); + const type = readRequiredString(record['type'], 'github.type'); + const relativePath = relativeRoot + ? path.posix.join(relativeRoot, name) + : name; + + if (type === 'dir') { + files.push( + ...(await downloadGitHubSkillDirectoryFromApi( + githubUrl, + itemPath, + relativePath, + depth + 1, + budget, + )), + ); + continue; + } + + if (type !== 'file') continue; + budget.files += 1; + if (budget.files > MAX_SKILL_API_FILE_COUNT) { + throw RequestError.invalidParams( + undefined, + 'Skill directory contains too many files', + ); + } + const downloadUrl = readRequiredString( + record['download_url'], + 'github.download_url', + ); + // SSRF defense: the API-provided download_url is attacker-influenced, so + // run it through the same host allowlist + HTTPS check as the initial URL. + assertAllowedSkillSourceUrl(downloadUrl); + const content = await fetchBytes(downloadUrl); + budget.bytes += content.length; + if (budget.bytes > MAX_SKILL_DECOMPRESSED_BYTES) { + throw RequestError.invalidParams( + undefined, + 'Skill directory exceeds the maximum allowed size', + ); + } + files.push({ + relativePath, + content, + }); + } + + return files; +} + +async function downloadGitHubSkillDirectory( + githubUrl: GitHubBlobSkillUrl, + directoryPath: string, +): Promise { + const apiFiles = await downloadGitHubSkillDirectoryFromApi( + githubUrl, + directoryPath, + ).catch((error) => { + debugLogger.warn( + 'GitHub API directory listing failed, falling back to archive download:', + error, + ); + return null; + }); + if (apiFiles) return apiFiles; + + return downloadGitHubSkillDirectoryFromArchive(githubUrl, directoryPath); +} + +export async function downloadSkill( + sourceUrl: string, +): Promise { + assertAllowedSkillSourceUrl(sourceUrl); + const githubUrl = parseGitHubBlobSkillUrl(sourceUrl); + if (!githubUrl || path.posix.basename(githubUrl.filePath) !== 'SKILL.md') { + return downloadSingleSkillFile(sourceUrl); + } + + const skillDirectory = path.posix.dirname(githubUrl.filePath); + const files = await downloadGitHubSkillDirectory(githubUrl, skillDirectory); + const skillFile = files.find((file) => file.relativePath === 'SKILL.md'); + if (!skillFile) { + throw RequestError.invalidParams( + undefined, + 'GitHub skill directory does not contain SKILL.md', + ); + } + + return { + skillContent: Buffer.from(skillFile.content).toString('utf8'), + files, + }; +}