diff --git a/toolkit-docs-generator/src/cli/index.ts b/toolkit-docs-generator/src/cli/index.ts index bb05939f6..d57b30863 100644 --- a/toolkit-docs-generator/src/cli/index.ts +++ b/toolkit-docs-generator/src/cli/index.ts @@ -13,7 +13,7 @@ import chalk from "chalk"; import { Command } from "commander"; -import { readdir, readFile, rm } from "fs/promises"; +import { readdir, readFile } from "fs/promises"; import ora from "ora"; import { join, resolve } from "path"; import { @@ -53,6 +53,10 @@ import { } from "../sources/toolkit-data-source.js"; import { readExclusionList } from "../utils/exclusion-list.js"; import { readIgnoreList } from "../utils/ignore-list.js"; +import { + clearSafeOutputDir, + resolveDefaultOutputDir, +} from "../utils/output-dir.js"; import { createProgressTracker, formatToolkitComplete, @@ -118,17 +122,6 @@ const parseProviders = (input: string): ProviderVersion[] => { */ const getDefaultMockDataDir = (): string => join(process.cwd(), "mock-data"); -/** - * Get the default output directory for docs JSON. - */ -const getDefaultOutputDir = (): string => { - const cwd = process.cwd(); - if (cwd.endsWith("toolkit-docs-generator")) { - return resolve(cwd, "..", "data", "toolkits"); - } - return resolve(cwd, "data", "toolkits"); -}; - const getDefaultVerificationDir = (): string => { const cwd = process.cwd(); if (cwd.endsWith("toolkit-docs-generator")) { @@ -265,12 +258,7 @@ const clearOutputDir = async ( outputDir: string, verbose: boolean ): Promise => { - const resolvedDir = resolve(outputDir); - const repoRoot = resolve(process.cwd()); - if (resolvedDir === "/" || resolvedDir === repoRoot) { - throw new Error(`Refusing to overwrite output directory: ${resolvedDir}`); - } - await rm(resolvedDir, { recursive: true, force: true }); + const resolvedDir = await clearSafeOutputDir(outputDir); if (verbose) { console.log(chalk.dim(`Cleared output directory: ${resolvedDir}`)); } @@ -878,7 +866,7 @@ program "Path to failed tools report to rerun only impacted toolkits" ) .option("--all", "Generate documentation for all toolkits", false) - .option("-o, --output ", "Output directory", getDefaultOutputDir()) + .option("-o, --output ", "Output directory", resolveDefaultOutputDir()) .option("--log-dir ", "Directory for run logs", getDefaultLogDir()) .option("--mock-data-dir ", "Path to mock data directory") .option("--metadata-file ", "Path to metadata JSON file") @@ -1924,7 +1912,7 @@ program program .command("generate-all") .description("Generate documentation for all toolkits in mock data") - .option("-o, --output ", "Output directory", getDefaultOutputDir()) + .option("-o, --output ", "Output directory", resolveDefaultOutputDir()) .option("--mock-data-dir ", "Path to mock data directory") .option("--metadata-file ", "Path to metadata JSON file") .option( @@ -2627,7 +2615,7 @@ program program .command("verify-output") .description("Verify output directory structure and schema") - .option("-o, --output ", "Output directory", getDefaultOutputDir()) + .option("-o, --output ", "Output directory", resolveDefaultOutputDir()) .option("--verbose", "Show detailed progress", false) .action(async (options: { output: string; verbose: boolean }) => { const spinner = ora("Verifying output...").start(); @@ -2667,7 +2655,7 @@ program .option( "-o, --output ", "Previous output directory", - getDefaultOutputDir() + resolveDefaultOutputDir() ) .option("--log-dir ", "Directory for run logs", getDefaultLogDir()) .option("--mock-data-dir ", "Path to mock data directory") diff --git a/toolkit-docs-generator/src/generator/json-generator.ts b/toolkit-docs-generator/src/generator/json-generator.ts index 2991d3b6a..1b7ebdec4 100644 --- a/toolkit-docs-generator/src/generator/json-generator.ts +++ b/toolkit-docs-generator/src/generator/json-generator.ts @@ -3,7 +3,8 @@ * * Outputs merged toolkit data as JSON files. */ -import { mkdir, readFile, stat, writeFile } from "fs/promises"; +import { randomUUID } from "node:crypto"; +import { mkdir, readFile, rename, rm, stat, writeFile } from "fs/promises"; import { dirname, join } from "path"; import { parsePreviousToolkitForDiff } from "../diff/previous-output.js"; import type { @@ -17,6 +18,72 @@ import { type ToolkitReadResult, } from "./output-verifier.js"; +const SAFE_TOOLKIT_ID = /^[a-z0-9][a-z0-9_-]*$/i; +const RESERVED_TOOLKIT_ID = "index"; + +const getToolkitFileName = (toolkitId: string): string => { + const normalizedId = toolkitId.toLowerCase(); + if (normalizedId === RESERVED_TOOLKIT_ID) { + throw new Error(`"${toolkitId}" is a reserved toolkit ID`); + } + if (!SAFE_TOOLKIT_ID.test(toolkitId)) { + throw new Error(`Unsafe toolkit ID: "${toolkitId}"`); + } + return `${normalizedId}.json`; +}; + +const validateToolkitFileNames = ( + toolkits: readonly MergedToolkit[] +): string[] => { + const errors: string[] = []; + const toolkitIdsByFile = new Map(); + + for (const toolkit of toolkits) { + try { + const fileName = getToolkitFileName(toolkit.id); + const toolkitIds = toolkitIdsByFile.get(fileName) ?? []; + toolkitIdsByFile.set(fileName, [...toolkitIds, toolkit.id]); + } catch (error) { + errors.push(error instanceof Error ? error.message : String(error)); + } + } + + for (const [fileName, toolkitIds] of toolkitIdsByFile) { + if (toolkitIds.length > 1) { + errors.push( + `Toolkit IDs collide on ${fileName}: ${toolkitIds.join(", ")}` + ); + } + } + + return errors; +}; + +const compareToolkitIds = (left: string, right: string): number => { + const normalizedLeft = left.toLowerCase(); + const normalizedRight = right.toLowerCase(); + if (normalizedLeft !== normalizedRight) { + return normalizedLeft < normalizedRight ? -1 : 1; + } + if (left === right) { + return 0; + } + return left < right ? -1 : 1; +}; + +const writeFileAtomically = async ( + filePath: string, + content: string +): Promise => { + const tempPath = `${filePath}.${process.pid}.${randomUUID()}.tmp`; + try { + await writeFile(tempPath, content, "utf-8"); + await rename(tempPath, filePath); + } finally { + await rm(tempPath, { force: true }); + } +}; + // ============================================================================ // Generator Configuration // ============================================================================ @@ -63,6 +130,7 @@ export class JsonGenerator { private readonly generateIndex: boolean; private readonly indexSource: "current" | "output"; private readonly validateOutput: boolean; + private readonly toolkitIdsByFile = new Map(); constructor(config: JsonGeneratorConfig) { this.outputDir = config.outputDir; @@ -86,28 +154,44 @@ export class JsonGenerator { } } - const fileName = `${toolkit.id.toLowerCase()}.json`; + const fileName = getToolkitFileName(toolkit.id); const filePath = join(this.outputDir, fileName); + const previousToolkitId = this.toolkitIdsByFile.get(fileName); + if (previousToolkitId && previousToolkitId !== toolkit.id) { + throw new Error( + `Toolkit IDs collide on ${fileName}: ${previousToolkitId}, ${toolkit.id}` + ); + } + this.toolkitIdsByFile.set(fileName, toolkit.id); // Ensure directory exists - await mkdir(dirname(filePath), { recursive: true }); + try { + await mkdir(dirname(filePath), { recursive: true }); - // Write file - const content = this.prettyPrint - ? JSON.stringify(toolkit, null, 2) - : JSON.stringify(toolkit); + // Write file + const content = this.prettyPrint + ? JSON.stringify(toolkit, null, 2) + : JSON.stringify(toolkit); - await writeFile(filePath, content, "utf-8"); - return filePath; + await writeFileAtomically(filePath, content); + return filePath; + } catch (error) { + if (previousToolkitId) { + this.toolkitIdsByFile.set(fileName, previousToolkitId); + } else { + this.toolkitIdsByFile.delete(fileName); + } + throw error; + } } /** * Check if a toolkit file already exists in the output directory */ async hasToolkitFile(toolkitId: string): Promise { - const fileName = `${toolkitId.toLowerCase()}.json`; - const filePath = join(this.outputDir, fileName); try { + const fileName = getToolkitFileName(toolkitId); + const filePath = join(this.outputDir, fileName); const stats = await stat(filePath); return stats.isFile(); } catch { @@ -137,9 +221,9 @@ export class JsonGenerator { * Load an existing toolkit file */ async loadToolkitFile(toolkitId: string): Promise { - const fileName = `${toolkitId.toLowerCase()}.json`; - const filePath = join(this.outputDir, fileName); try { + const fileName = getToolkitFileName(toolkitId); + const filePath = join(this.outputDir, fileName); const content = await readFile(filePath, "utf-8"); const parsed = JSON.parse(content) as unknown; const result = MergedToolkitSchema.safeParse(parsed); @@ -160,7 +244,10 @@ export class JsonGenerator { toolkits: readonly MergedToolkit[] ): Promise { const filesWritten: string[] = []; - const errors: string[] = []; + const errors = validateToolkitFileNames(toolkits); + if (errors.length > 0) { + return { filesWritten, errors }; + } // Generate per-toolkit files for (const toolkit of toolkits) { @@ -199,15 +286,17 @@ export class JsonGenerator { private async generateIndexFile( toolkits: readonly MergedToolkit[] ): Promise { - const entries: ToolkitIndexEntry[] = toolkits.map((t) => ({ - id: t.id, - label: t.label, - version: t.version, - category: t.metadata.category, - type: t.metadata.type, - toolCount: t.tools.length, - authType: t.auth?.type ?? "none", - })); + const entries: ToolkitIndexEntry[] = toolkits + .map((t) => ({ + id: t.id, + label: t.label, + version: t.version, + category: t.metadata.category, + type: t.metadata.type, + toolCount: t.tools.length, + authType: t.auth?.type ?? "none", + })) + .sort((left, right) => compareToolkitIds(left.id, right.id)); const index: ToolkitIndex = { generatedAt: new Date().toISOString(), @@ -223,7 +312,7 @@ export class JsonGenerator { ? JSON.stringify(index, null, 2) : JSON.stringify(index); - await writeFile(filePath, content, "utf-8"); + await writeFileAtomically(filePath, content); return filePath; } diff --git a/toolkit-docs-generator/src/utils/output-dir.ts b/toolkit-docs-generator/src/utils/output-dir.ts index 22b925a36..37105eefe 100644 --- a/toolkit-docs-generator/src/utils/output-dir.ts +++ b/toolkit-docs-generator/src/utils/output-dir.ts @@ -1,11 +1,27 @@ -import { realpath } from "fs/promises"; -import { isAbsolute, resolve, sep } from "path"; +import { realpath, rm } from "fs/promises"; +import { basename, isAbsolute, resolve, sep } from "path"; type ResolveOptions = { repoRoot?: string; homeDir?: string; }; +export const resolveRepositoryRoot = (cwd: string = process.cwd()): string => + basename(cwd) === "toolkit-docs-generator" + ? resolve(cwd, "..") + : resolve(cwd); + +export const resolveDefaultOutputDir = ( + cwd: string = process.cwd() +): string => { + const repoRoot = resolveRepositoryRoot(cwd); + const generatorRoot = + basename(cwd) === "toolkit-docs-generator" + ? cwd + : resolve(repoRoot, "toolkit-docs-generator"); + return resolve(generatorRoot, "data", "toolkits"); +}; + const isSubpath = (parent: string, child: string): boolean => { const normalizedParent = parent.endsWith(sep) ? parent : `${parent}${sep}`; return child === parent || child.startsWith(normalizedParent); @@ -15,10 +31,16 @@ export const resolveSafeOutputDir = async ( outputDir: string, options: ResolveOptions = {} ): Promise => { - const resolvedRepoRoot = resolve(options.repoRoot ?? process.cwd()); + const resolvedRepoRoot = resolve( + options.repoRoot ?? resolveRepositoryRoot(process.cwd()) + ); const repoRoot = await realpath(resolvedRepoRoot).catch( () => resolvedRepoRoot ); + const resolvedGeneratorRoot = resolve(repoRoot, "toolkit-docs-generator"); + const generatorRoot = await realpath(resolvedGeneratorRoot).catch( + () => resolvedGeneratorRoot + ); const resolvedHomeDir = options.homeDir ? resolve(options.homeDir) : process.env.HOME @@ -30,12 +52,10 @@ export const resolveSafeOutputDir = async ( const resolvedDir = resolve(outputDir); const realDir = await realpath(resolvedDir).catch(() => resolvedDir); - const forbidden = new Set(["/", repoRoot]); - if (homeDir) { - forbidden.add(homeDir); - } - - if (forbidden.has(realDir)) { + const containsRepoRoot = isSubpath(realDir, repoRoot); + const containsGeneratorRoot = isSubpath(realDir, generatorRoot); + const containsHomeDir = homeDir ? isSubpath(realDir, homeDir) : false; + if (containsRepoRoot || containsGeneratorRoot || containsHomeDir) { throw new Error(`Refusing to delete unsafe output directory: ${realDir}`); } @@ -48,3 +68,12 @@ export const resolveSafeOutputDir = async ( return realDir; }; + +export const clearSafeOutputDir = async ( + outputDir: string, + options: ResolveOptions = {} +): Promise => { + const safeDir = await resolveSafeOutputDir(outputDir, options); + await rm(resolve(outputDir), { recursive: true, force: true }); + return safeDir; +}; diff --git a/toolkit-docs-generator/tests/generator/output-verifier.test.ts b/toolkit-docs-generator/tests/generator/output-verifier.test.ts index ada7865fb..daabcb57c 100644 --- a/toolkit-docs-generator/tests/generator/output-verifier.test.ts +++ b/toolkit-docs-generator/tests/generator/output-verifier.test.ts @@ -1,6 +1,6 @@ import { mkdtemp, readFile, rename, rm, writeFile } from "fs/promises"; import { tmpdir } from "os"; -import { join } from "path"; +import { basename, join } from "path"; import { describe, expect, it } from "vitest"; import { @@ -410,3 +410,96 @@ describe("JsonGenerator.rebuildIndexFromOutput", () => { }); }); }); + +describe("JsonGenerator toolkit file paths", () => { + it("rejects toolkit IDs that escape the output directory", async () => { + await withTempDir(async (dir) => { + const toolkit = await loadFixture("github-toolkit.json"); + const escapeName = `${basename(dir)}-escape`; + const escapePath = join(dir, "..", `${escapeName}.json`); + const generator = createJsonGenerator({ outputDir: dir }); + + try { + await expect( + generator.generateToolkitFile({ + ...toolkit, + id: `../${escapeName}`, + }) + ).rejects.toThrow("Unsafe toolkit ID"); + await expect(readFile(escapePath, "utf-8")).rejects.toThrow(); + } finally { + await rm(escapePath, { force: true }); + } + }); + }); + + it('reserves "index" for the generated index file', async () => { + await withTempDir(async (dir) => { + const toolkit = await loadFixture("github-toolkit.json"); + const generator = createJsonGenerator({ outputDir: dir }); + + await expect( + generator.generateToolkitFile({ ...toolkit, id: "INDEX" }) + ).rejects.toThrow("reserved toolkit ID"); + await expect( + readFile(join(dir, "index.json"), "utf-8") + ).rejects.toThrow(); + }); + }); + + it("rejects case-insensitive toolkit ID collisions before writing", async () => { + await withTempDir(async (dir) => { + const toolkit = await loadFixture("github-toolkit.json"); + const generator = createJsonGenerator({ outputDir: dir }); + + const result = await generator.generateAll([ + toolkit, + { ...toolkit, id: toolkit.id.toUpperCase() }, + ]); + + expect(result.filesWritten).toEqual([]); + expect(result.errors).toEqual([ + `Toolkit IDs collide on github.json: ${toolkit.id}, ${toolkit.id.toUpperCase()}`, + ]); + await expect( + readFile(join(dir, "github.json"), "utf-8") + ).rejects.toThrow(); + await expect( + readFile(join(dir, "index.json"), "utf-8") + ).rejects.toThrow(); + }); + }); + + it("rejects case-insensitive toolkit ID collisions during incremental writes", async () => { + await withTempDir(async (dir) => { + const toolkit = await loadFixture("github-toolkit.json"); + const generator = createJsonGenerator({ outputDir: dir }); + + await generator.generateToolkitFile(toolkit); + + await expect( + generator.generateToolkitFile({ + ...toolkit, + id: toolkit.id.toUpperCase(), + }) + ).rejects.toThrow("Toolkit IDs collide on github.json"); + }); + }); + + it("sorts index entries by normalized toolkit ID", async () => { + await withTempDir(async (dir) => { + const [githubToolkit, slackToolkit] = await Promise.all([ + loadFixture("github-toolkit.json"), + loadFixture("slack-toolkit.json"), + ]); + const generator = createJsonGenerator({ outputDir: dir }); + + await generator.generateAll([slackToolkit, githubToolkit]); + + const index = JSON.parse( + await readFile(join(dir, "index.json"), "utf-8") + ) as { toolkits: Array<{ id: string }> }; + expect(index.toolkits.map(({ id }) => id)).toEqual(["Github", "Slack"]); + }); + }); +}); diff --git a/toolkit-docs-generator/tests/utils/output-dir.test.ts b/toolkit-docs-generator/tests/utils/output-dir.test.ts index 6f58f95c2..278cda2f9 100644 --- a/toolkit-docs-generator/tests/utils/output-dir.test.ts +++ b/toolkit-docs-generator/tests/utils/output-dir.test.ts @@ -1,8 +1,12 @@ import { mkdir, mkdtemp, realpath, rm } from "fs/promises"; import { tmpdir } from "os"; -import { join } from "path"; +import { basename, join } from "path"; import { afterEach, beforeEach, describe, expect, it } from "vitest"; -import { resolveSafeOutputDir } from "../../src/utils/output-dir.js"; +import { + clearSafeOutputDir, + resolveDefaultOutputDir, + resolveSafeOutputDir, +} from "../../src/utils/output-dir.js"; describe("resolveSafeOutputDir", () => { const originalCwd = process.cwd(); @@ -65,4 +69,74 @@ describe("resolveSafeOutputDir", () => { }) ).rejects.toThrow("unsafe output directory"); }); + + it("rejects the repository root when run from the generator directory", async () => { + const generatorDir = join(repoRoot ?? "", "toolkit-docs-generator"); + await mkdir(generatorDir); + process.chdir(generatorDir); + + await expect(clearSafeOutputDir(repoRoot ?? "")).rejects.toThrow( + "unsafe output directory" + ); + expect(await realpath(repoRoot ?? "")).toContain( + basename(repoRoot ?? "repo") + ); + }); + + it("rejects the generator directory when run from that directory", async () => { + const generatorDir = join(repoRoot ?? "", "toolkit-docs-generator"); + await mkdir(generatorDir); + const expected = await realpath(generatorDir); + process.chdir(generatorDir); + + await expect(clearSafeOutputDir(".")).rejects.toThrow( + "unsafe output directory" + ); + expect(await realpath(generatorDir)).toBe(expected); + }); + + it("clears a safe output directory", async () => { + const outputDir = join(repoRoot ?? "", "output"); + await mkdir(outputDir, { recursive: true }); + const expected = await realpath(outputDir); + + const cleared = await clearSafeOutputDir(outputDir, { + repoRoot: repoRoot ?? undefined, + homeDir: homeDir ?? undefined, + }); + + expect(cleared).toBe(expected); + await expect(realpath(outputDir)).rejects.toThrow(); + }); + + it("does not clear a relative directory outside the repository", async () => { + const outsideName = `${basename(repoRoot ?? "repo")}-outside`; + const outsideDir = join(repoRoot ?? "", "..", outsideName); + await mkdir(outsideDir); + try { + await expect( + clearSafeOutputDir(`../${outsideName}`, { + repoRoot: repoRoot ?? undefined, + homeDir: homeDir ?? undefined, + }) + ).rejects.toThrow("outside repo root"); + expect(await realpath(outsideDir)).toContain(outsideName); + } finally { + await rm(outsideDir, { recursive: true, force: true }); + } + }); +}); + +describe("resolveDefaultOutputDir", () => { + it("uses the generator data directory from the repository root", () => { + expect(resolveDefaultOutputDir("/repo")).toBe( + "/repo/toolkit-docs-generator/data/toolkits" + ); + }); + + it("uses the local data directory from the generator directory", () => { + expect(resolveDefaultOutputDir("/repo/toolkit-docs-generator")).toBe( + "/repo/toolkit-docs-generator/data/toolkits" + ); + }); });