diff --git a/src/hooks/install.test.ts b/src/hooks/install.test.ts index 1f4e7d6197a..11b9456e79f 100644 --- a/src/hooks/install.test.ts +++ b/src/hooks/install.test.ts @@ -45,102 +45,96 @@ beforeEach(() => { vi.clearAllMocks(); }); +function writeArchiveFixture(params: { fileName: string; contents: Buffer }) { + const stateDir = makeTempDir(); + const workDir = makeTempDir(); + const archivePath = path.join(workDir, params.fileName); + fs.writeFileSync(archivePath, params.contents); + return { + stateDir, + archivePath, + hooksDir: path.join(stateDir, "hooks"), + }; +} + describe("installHooksFromArchive", () => { - it("installs hook packs from zip archives", async () => { - const stateDir = makeTempDir(); - const workDir = makeTempDir(); - const archivePath = path.join(workDir, "hooks.zip"); - fs.writeFileSync(archivePath, zipHooksBuffer); - - const hooksDir = path.join(stateDir, "hooks"); - const result = await installHooksFromArchive({ archivePath, hooksDir }); + it.each([ + { + name: "zip", + fileName: "hooks.zip", + contents: zipHooksBuffer, + expectedPackId: "zip-hooks", + expectedHook: "zip-hook", + }, + { + name: "tar", + fileName: "hooks.tar", + contents: tarHooksBuffer, + expectedPackId: "tar-hooks", + expectedHook: "tar-hook", + }, + ])("installs hook packs from $name archives", async (tc) => { + const fixture = writeArchiveFixture({ fileName: tc.fileName, contents: tc.contents }); + const result = await installHooksFromArchive({ + archivePath: fixture.archivePath, + hooksDir: fixture.hooksDir, + }); expect(result.ok).toBe(true); if (!result.ok) { return; } - expect(result.hookPackId).toBe("zip-hooks"); - expect(result.hooks).toContain("zip-hook"); - expect(result.targetDir).toBe(path.join(stateDir, "hooks", "zip-hooks")); - expect(fs.existsSync(path.join(result.targetDir, "hooks", "zip-hook", "HOOK.md"))).toBe(true); + expect(result.hookPackId).toBe(tc.expectedPackId); + expect(result.hooks).toContain(tc.expectedHook); + expect(result.targetDir).toBe(path.join(fixture.stateDir, "hooks", tc.expectedPackId)); + expect(fs.existsSync(path.join(result.targetDir, "hooks", tc.expectedHook, "HOOK.md"))).toBe( + true, + ); }); - it("rejects zip archives with traversal entries", async () => { - const stateDir = makeTempDir(); - const workDir = makeTempDir(); - const archivePath = path.join(workDir, "traversal.zip"); - fs.writeFileSync(archivePath, zipTraversalBuffer); - - const hooksDir = path.join(stateDir, "hooks"); - const result = await installHooksFromArchive({ archivePath, hooksDir }); + it.each([ + { + name: "zip", + fileName: "traversal.zip", + contents: zipTraversalBuffer, + expectedDetail: "archive entry", + }, + { + name: "tar", + fileName: "traversal.tar", + contents: tarTraversalBuffer, + expectedDetail: "escapes destination", + }, + ])("rejects $name archives with traversal entries", async (tc) => { + const fixture = writeArchiveFixture({ fileName: tc.fileName, contents: tc.contents }); + const result = await installHooksFromArchive({ + archivePath: fixture.archivePath, + hooksDir: fixture.hooksDir, + }); expect(result.ok).toBe(false); if (result.ok) { return; } expect(result.error).toContain("failed to extract archive"); - expect(result.error).toContain("archive entry"); + expect(result.error).toContain(tc.expectedDetail); }); - it("installs hook packs from tar archives", async () => { - const stateDir = makeTempDir(); - const workDir = makeTempDir(); - const archivePath = path.join(workDir, "hooks.tar"); - fs.writeFileSync(archivePath, tarHooksBuffer); - - const hooksDir = path.join(stateDir, "hooks"); - const result = await installHooksFromArchive({ archivePath, hooksDir }); - - expect(result.ok).toBe(true); - if (!result.ok) { - return; - } - expect(result.hookPackId).toBe("tar-hooks"); - expect(result.hooks).toContain("tar-hook"); - expect(result.targetDir).toBe(path.join(stateDir, "hooks", "tar-hooks")); - }); - - it("rejects tar archives with traversal entries", async () => { - const stateDir = makeTempDir(); - const workDir = makeTempDir(); - const archivePath = path.join(workDir, "traversal.tar"); - fs.writeFileSync(archivePath, tarTraversalBuffer); - - const hooksDir = path.join(stateDir, "hooks"); - const result = await installHooksFromArchive({ archivePath, hooksDir }); - - expect(result.ok).toBe(false); - if (result.ok) { - return; - } - expect(result.error).toContain("failed to extract archive"); - expect(result.error).toContain("escapes destination"); - }); - - it("rejects hook packs with traversal-like ids", async () => { - const stateDir = makeTempDir(); - const workDir = makeTempDir(); - const archivePath = path.join(workDir, "hooks.tar"); - fs.writeFileSync(archivePath, tarEvilIdBuffer); - - const hooksDir = path.join(stateDir, "hooks"); - const result = await installHooksFromArchive({ archivePath, hooksDir }); - - expect(result.ok).toBe(false); - if (result.ok) { - return; - } - expect(result.error).toContain("reserved path segment"); - }); - - it("rejects hook packs with reserved ids", async () => { - const stateDir = makeTempDir(); - const workDir = makeTempDir(); - const archivePath = path.join(workDir, "hooks.tar"); - fs.writeFileSync(archivePath, tarReservedIdBuffer); - - const hooksDir = path.join(stateDir, "hooks"); - const result = await installHooksFromArchive({ archivePath, hooksDir }); + it.each([ + { + name: "traversal-like ids", + contents: tarEvilIdBuffer, + }, + { + name: "reserved ids", + contents: tarReservedIdBuffer, + }, + ])("rejects hook packs with $name", async (tc) => { + const fixture = writeArchiveFixture({ fileName: "hooks.tar", contents: tc.contents }); + const result = await installHooksFromArchive({ + archivePath: fixture.archivePath, + hooksDir: fixture.hooksDir, + }); expect(result.ok).toBe(false); if (result.ok) { @@ -201,9 +195,7 @@ describe("installHooksFromPath", () => { expectedCwd: res.targetDir, }); }); -}); -describe("installHooksFromPath", () => { it("installs a single hook directory", async () => { const stateDir = makeTempDir(); const workDir = makeTempDir(); diff --git a/src/hooks/install.ts b/src/hooks/install.ts index 90738629097..99ffb263436 100644 --- a/src/hooks/install.ts +++ b/src/hooks/install.ts @@ -105,6 +105,33 @@ function resolveTimedHookInstallModeOptions(params: { }; } +async function withTempDir(prefix: string, fn: (tmpDir: string) => Promise): Promise { + const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), prefix)); + try { + return await fn(tmpDir); + } finally { + await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => undefined); + } +} + +async function resolveInstallTargetDir( + id: string, + hooksDir?: string, +): Promise<{ ok: true; targetDir: string } | { ok: false; error: string }> { + const baseHooksDir = hooksDir ? resolveUserPath(hooksDir) : path.join(CONFIG_DIR, "hooks"); + await fs.mkdir(baseHooksDir, { recursive: true }); + + const targetDirResult = resolveSafeInstallDir({ + baseDir: baseHooksDir, + id, + invalidNameMessage: "invalid hook name: path traversal detected", + }); + if (!targetDirResult.ok) { + return { ok: false, error: targetDirResult.error }; + } + return { ok: true, targetDir: targetDirResult.path }; +} + async function resolveHookNameFromDir(hookDir: string): Promise { const hookMdPath = path.join(hookDir, "HOOK.md"); if (!(await fileExists(hookMdPath))) { @@ -174,20 +201,11 @@ async function installHookPackageFromDir(params: { }; } - const hooksDir = params.hooksDir - ? resolveUserPath(params.hooksDir) - : path.join(CONFIG_DIR, "hooks"); - await fs.mkdir(hooksDir, { recursive: true }); - - const targetDirResult = resolveSafeInstallDir({ - baseDir: hooksDir, - id: hookPackId, - invalidNameMessage: "invalid hook name: path traversal detected", - }); + const targetDirResult = await resolveInstallTargetDir(hookPackId, params.hooksDir); if (!targetDirResult.ok) { return { ok: false, error: targetDirResult.error }; } - const targetDir = targetDirResult.path; + const targetDir = targetDirResult.targetDir; if (mode === "install" && (await fileExists(targetDir))) { return { ok: false, error: `hook pack already exists: ${targetDir} (delete it first)` }; } @@ -259,20 +277,11 @@ async function installHookFromDir(params: { }; } - const hooksDir = params.hooksDir - ? resolveUserPath(params.hooksDir) - : path.join(CONFIG_DIR, "hooks"); - await fs.mkdir(hooksDir, { recursive: true }); - - const targetDirResult = resolveSafeInstallDir({ - baseDir: hooksDir, - id: hookName, - invalidNameMessage: "invalid hook name: path traversal detected", - }); + const targetDirResult = await resolveInstallTargetDir(hookName, params.hooksDir); if (!targetDirResult.ok) { return { ok: false, error: targetDirResult.error }; } - const targetDir = targetDirResult.path; + const targetDir = targetDirResult.targetDir; if (mode === "install" && (await fileExists(targetDir))) { return { ok: false, error: `hook already exists: ${targetDir} (delete it first)` }; } @@ -326,8 +335,7 @@ export async function installHooksFromArchive(params: { return { ok: false, error: `unsupported archive: ${archivePath}` }; } - const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-hook-")); - try { + return await withTempDir("openclaw-hook-", async (tmpDir) => { const extractDir = path.join(tmpDir, "extract"); await fs.mkdir(extractDir, { recursive: true }); @@ -366,9 +374,7 @@ export async function installHooksFromArchive(params: { dryRun: params.dryRun, expectedHookPackId: params.expectedHookPackId, }); - } finally { - await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => undefined); - } + }); } export async function installHooksFromNpmSpec(params: { @@ -388,8 +394,7 @@ export async function installHooksFromNpmSpec(params: { return { ok: false, error: specError }; } - const tmpDir = await fs.mkdtemp(path.join(os.tmpdir(), "openclaw-hook-pack-")); - try { + return await withTempDir("openclaw-hook-pack-", async (tmpDir) => { logger.info?.(`Downloading ${spec}…`); const res = await runCommandWithTimeout(["npm", "pack", spec, "--ignore-scripts"], { timeoutMs: Math.max(timeoutMs, 300_000), @@ -422,9 +427,7 @@ export async function installHooksFromNpmSpec(params: { dryRun, expectedHookPackId, }); - } finally { - await fs.rm(tmpDir, { recursive: true, force: true }).catch(() => undefined); - } + }); } export async function installHooksFromPath(params: {