diff --git a/src/platform/adapters/fs/cache/file-cache.test.ts b/src/platform/adapters/fs/cache/file-cache.test.ts index 0037a21527..b421f21ef5 100644 --- a/src/platform/adapters/fs/cache/file-cache.test.ts +++ b/src/platform/adapters/fs/cache/file-cache.test.ts @@ -6,6 +6,7 @@ import { initializeFileCacheBackend, isFileCacheDistributedEnabled, } from "./file-cache.ts"; +import { CacheBackends } from "#veryfront/cache/backend.ts"; describe("FileCache", () => { let cache: FileCache; @@ -329,6 +330,45 @@ describe("Distributed cache functions", () => { assertEquals(typeof initializeFileCacheBackend, "function"); }); + it("skips non-serializable synchronous writes to a distributed backend", async () => { + // A query-qualified import gives this regression its own module-scoped + // backend state, so the fake distributed backend cannot leak into other + // file-cache tests in the same Deno process. + const distributedModule = await import( + "./file-cache.ts?distributed-serialization-regression" + ); + const descriptor = Object.getOwnPropertyDescriptor(CacheBackends, "file"); + assertExists(descriptor); + let backendWrites = 0; + Object.defineProperty(CacheBackends, "file", { + ...descriptor, + value: () => + Promise.resolve({ + type: "redis", + size: 0, + get: () => Promise.resolve(null), + set: () => { + backendWrites += 1; + return Promise.resolve(); + }, + del: () => Promise.resolve(false), + clear: () => Promise.resolve(), + } as never), + }); + + try { + assertEquals(await distributedModule.initializeFileCacheBackend(), true); + } finally { + Object.defineProperty(CacheBackends, "file", descriptor); + } + + const distributedCache = new distributedModule.FileCache(); + const circular: Record = {}; + circular.self = circular; + distributedCache.set("cyclic", circular); + assertEquals(backendWrites, 0); + }); + it("should return boolean", async () => { assertEquals(typeof (await initializeFileCacheBackend()), "boolean"); }); diff --git a/src/platform/adapters/fs/cache/file-cache.ts b/src/platform/adapters/fs/cache/file-cache.ts index b36afba165..85c03eac1b 100644 --- a/src/platform/adapters/fs/cache/file-cache.ts +++ b/src/platform/adapters/fs/cache/file-cache.ts @@ -205,7 +205,16 @@ export class FileCache { // Note: key already includes the full prefix from buildFileCacheKeyPrefix (e.g., "file:env:project:...") const backend = this.getBackend(); if (backend) { - const serialized = JSON.stringify(entry); + let serialized: string; + try { + serialized = JSON.stringify(entry); + } catch (error) { + logger.debug("Backend set skipped because the cache entry is not serializable", { + key, + error, + }); + return; + } // Update request-scoped cache so subsequent reads in same request see the new value setInRequestCache(key, serialized); backend.set(key, serialized, this.backendTtlSeconds).catch((error) => { diff --git a/src/platform/adapters/fs/github/github-api-client.test.ts b/src/platform/adapters/fs/github/github-api-client.test.ts index c912490df8..691e719271 100644 --- a/src/platform/adapters/fs/github/github-api-client.test.ts +++ b/src/platform/adapters/fs/github/github-api-client.test.ts @@ -1,6 +1,12 @@ import "#veryfront/schemas/_test-setup.ts"; -import { assertEquals, assertExists } from "#veryfront/testing/assert.ts"; +import { + assertEquals, + assertExists, + assertRejects, + assertThrows, +} from "#veryfront/testing/assert.ts"; import { describe, it } from "#veryfront/testing/bdd.ts"; +import { withMockFetch } from "#veryfront/testing/mock-fetch.ts"; import { GitHubApiClient } from "./github-api-client.ts"; const mockConfig = { @@ -37,6 +43,29 @@ describe("GitHubApiClient", () => { it("should be instantiable with config", () => { assertExists(createClient()); }); + + it("rejects repository identity that could escape its URL segments", () => { + for ( + const [field, value] of [ + ["owner", ".."], + ["owner", "%2e%2e"], + ["owner", "%25252e%25252e"], + ["owner", "team/other"], + ["owner", "team%2Fother"], + ["repo", "."], + ["repo", "..\\other"], + ["repo", "repo%5Cother"], + ["repo", "repo\u0000name"], + ["repo", "r".repeat(257)], + ] as const + ) { + assertThrows( + () => new GitHubApiClient({ ...mockConfig, [field]: value }), + TypeError, + "GitHub", + ); + } + }); }); describe("repoId", () => { @@ -70,4 +99,127 @@ describe("GitHubApiClient", () => { assertEquals(createClient().getRateLimitInfo(), null); }); }); + + describe("getContents", () => { + it("encodes path segments and refs before URL construction", async () => { + const requestedUrls: string[] = []; + await withMockFetch( + (input) => { + requestedUrls.push(String(input)); + return Promise.resolve(Response.json({ + type: "file", + name: "file.ts", + path: "file.ts", + sha: "sha-1", + size: 0, + content: "", + encoding: "base64", + })); + }, + async () => { + const client = createClient(); + for ( + const path of [ + "%2e%2e/%2E%2E/user/repos", + "..\\..\\user/repos", + "docs/read me#draft?.md", + ] + ) { + await client.getContents(path, "feature/secure-paths"); + } + }, + ); + + assertEquals(requestedUrls.length, 3); + for (const requestedUrl of requestedUrls) { + const url = new URL(requestedUrl); + assertEquals( + url.pathname.startsWith("/repos/test-owner/test-repo/contents/"), + true, + ); + assertEquals(url.searchParams.get("ref"), "feature/secure-paths"); + } + assertEquals( + new URL(requestedUrls[0]!).pathname, + "/repos/test-owner/test-repo/contents/%252e%252e/%252E%252E/user/repos", + ); + assertEquals( + new URL(requestedUrls[1]!).pathname, + "/repos/test-owner/test-repo/contents/..%5C..%5Cuser/repos", + ); + assertEquals( + new URL(requestedUrls[2]!).pathname, + "/repos/test-owner/test-repo/contents/docs/read%20me%23draft%3F.md", + ); + }); + + it("rejects literal traversal segments before fetching", async () => { + let fetchCalls = 0; + await withMockFetch( + () => { + fetchCalls++; + return Promise.resolve(Response.json({})); + }, + async () => { + await assertRejects( + () => createClient().getContents("../secrets"), + TypeError, + "traversal", + ); + }, + ); + assertEquals(fetchCalls, 0); + }); + }); + + describe("endpoint construction", () => { + it("rejects dot-only endpoint values before fetching", async () => { + let fetchCalls = 0; + await withMockFetch( + () => { + fetchCalls++; + return Promise.resolve(Response.json({})); + }, + async () => { + await assertRejects(() => createClient().getTree(".."), TypeError); + await assertRejects(() => createClient().getBlob("."), TypeError); + }, + ); + assertEquals(fetchCalls, 0); + }); + + it("encodes repository identity, refs, and blob identifiers as path segments", async () => { + const requestedUrls: string[] = []; + const client = new GitHubApiClient({ + ...mockConfig, + owner: "test owner", + repo: "repo#name", + }); + + await withMockFetch( + (input) => { + const url = String(input); + requestedUrls.push(url); + return Promise.resolve( + url.includes("/git/trees/") + ? Response.json({ sha: "tree", tree: [], truncated: false }) + : Response.json({ sha: "blob", size: 0, content: "", encoding: "base64" }), + ); + }, + async () => { + await client.getTree("feature/secure?recursive=0"); + await client.getBlob("sha/../other"); + }, + ); + + assertEquals( + new URL(requestedUrls[0]!).pathname, + "/repos/test%20owner/repo%23name/git/trees/feature%2Fsecure%3Frecursive%3D0", + ); + assertEquals( + new URL(requestedUrls[1]!).pathname, + "/repos/test%20owner/repo%23name/git/blobs/sha%2F..%2Fother", + ); + }); + }); }); diff --git a/src/platform/adapters/fs/github/github-api-client.ts b/src/platform/adapters/fs/github/github-api-client.ts index ead85cb9ea..27f1a05cd0 100644 --- a/src/platform/adapters/fs/github/github-api-client.ts +++ b/src/platform/adapters/fs/github/github-api-client.ts @@ -14,6 +14,79 @@ const LOG_PREFIX = "[GitHubApiClient]"; const RATE_LIMIT_WARNING_THRESHOLD = 100; const RETRY_JITTER_MAX_MS = 1_000; +const MAX_REPOSITORY_SEGMENT_LENGTH = 256; +const MAX_ENDPOINT_VALUE_LENGTH = 4_096; + +function encodeRepositorySegment(value: string, label: string): string { + if ( + typeof value !== "string" || + value.length === 0 || + value.length > MAX_REPOSITORY_SEGMENT_LENGTH || + value.trim() !== value || + value.normalize("NFC") !== value || + /\p{Cc}/u.test(value) + ) { + throw new TypeError(`GitHub ${label} must be a bounded canonical path segment`); + } + + let decoded = value; + for (let depth = 0; depth <= value.length; depth++) { + if ( + decoded === "." || + decoded === ".." || + decoded.includes("/") || + decoded.includes("\\") || + decoded.trim() !== decoded || + /\p{Cc}/u.test(decoded) + ) { + throw new TypeError(`GitHub ${label} must be a single non-traversal path segment`); + } + + let next: string; + try { + next = decodeURIComponent(decoded); + } catch { + throw new TypeError(`GitHub ${label} contains malformed percent-encoding`); + } + if (next === decoded) return encodeURIComponent(value); + decoded = next; + } + + throw new TypeError(`GitHub ${label} contains excessive percent-encoding`); +} + +function encodeEndpointValue(value: string, label: string): string { + if ( + typeof value !== "string" || + value.length === 0 || + value.length > MAX_ENDPOINT_VALUE_LENGTH || + value === "." || + value === ".." || + /\p{Cc}/u.test(value) + ) { + throw new TypeError(`GitHub ${label} must be bounded non-empty text`); + } + return encodeURIComponent(value); +} + +function encodeContentsPath(path: string): { normalized: string; encoded: string } { + if ( + typeof path !== "string" || + path.length > MAX_ENDPOINT_VALUE_LENGTH || + /\p{Cc}/u.test(path) + ) { + throw new TypeError("GitHub contents path must be bounded text without control characters"); + } + const normalized = path.replace(/^\/+/, ""); + const segments = normalized.split("/"); + if (segments.some((segment) => segment === "." || segment === "..")) { + throw new TypeError("GitHub contents path must not contain traversal segments"); + } + return { + normalized, + encoded: segments.map(encodeURIComponent).join("/"), + }; +} class GitHubBlobIntegrityError extends Error {} @@ -28,9 +101,14 @@ type APIError = Error & { statusCode?: number; endpoint?: string; repo?: string export class GitHubApiClient { private readonly baseUrl = "https://api.github.com"; + private readonly repositoryEndpoint: string; private rateLimitInfo: RateLimitInfo | null = null; - constructor(private readonly config: ResolvedGitHubConfig) {} + constructor(private readonly config: ResolvedGitHubConfig) { + const owner = encodeRepositorySegment(config.owner, "owner"); + const repo = encodeRepositorySegment(config.repo, "repository"); + this.repositoryEndpoint = `/repos/${owner}/${repo}`; + } get repoId(): string { return `${this.config.owner}/${this.config.repo}`; @@ -38,8 +116,9 @@ export class GitHubApiClient { async getTree(ref?: string): Promise { const treeRef = ref ?? this.config.ref; - const endpoint = - `/repos/${this.config.owner}/${this.config.repo}/git/trees/${treeRef}?recursive=1`; + const endpoint = `${this.repositoryEndpoint}/git/trees/${ + encodeEndpointValue(treeRef, "tree ref") + }?recursive=1`; logger.debug(`${LOG_PREFIX} Fetching tree`, { ref: treeRef }); @@ -60,18 +139,19 @@ export class GitHubApiClient { ref?: string, ): Promise { const contentRef = ref ?? this.config.ref; - const normalizedPath = path.replace(/^\/+/, ""); - const endpoint = - `/repos/${this.config.owner}/${this.config.repo}/contents/${normalizedPath}?ref=${contentRef}`; + const { normalized, encoded } = encodeContentsPath(path); + const endpoint = `${this.repositoryEndpoint}/contents/${encoded}?ref=${ + encodeEndpointValue(contentRef, "contents ref") + }`; - logger.debug(`${LOG_PREFIX} Fetching contents`, { path: normalizedPath }); + logger.debug(`${LOG_PREFIX} Fetching contents`, { path: normalized }); const raw = await this.request(endpoint); return getGitHubContentsResponseSchema().parse(raw); } async getBlob(sha: string): Promise { - const endpoint = `/repos/${this.config.owner}/${this.config.repo}/git/blobs/${sha}`; + const endpoint = `${this.repositoryEndpoint}/git/blobs/${encodeEndpointValue(sha, "blob SHA")}`; logger.debug(`${LOG_PREFIX} Fetching blob`, { sha }); @@ -95,7 +175,7 @@ export class GitHubApiClient { if (expectedSize > byteLimit) { throw new RangeError(`GitHub blob exceeds ${byteLimit} bytes`); } - const endpoint = `/repos/${this.config.owner}/${this.config.repo}/git/blobs/${sha}`; + const endpoint = `${this.repositoryEndpoint}/git/blobs/${encodeEndpointValue(sha, "blob SHA")}`; logger.debug(`${LOG_PREFIX} Fetching bounded raw blob`, { sha, expectedSize }); diff --git a/src/platform/adapters/fs/github/path-utils.test.ts b/src/platform/adapters/fs/github/path-utils.test.ts index f69a511130..b5fa6b093b 100644 --- a/src/platform/adapters/fs/github/path-utils.test.ts +++ b/src/platform/adapters/fs/github/path-utils.test.ts @@ -54,6 +54,9 @@ describe("platform/adapters/fs/github/path-utils", () => { "src/../secret.ts", "/project/../../secret.ts", "../../../../user/repos", + "%2e%2e/%2e%2e/user/repos", + "%2E%2E/%2E%2E/user/repos", + ".%2e/.%2e/user/repos", ] ) { assertThrows( @@ -64,6 +67,24 @@ describe("platform/adapters/fs/github/path-utils", () => { } }); + it("rejects backslashes, control characters, and unbounded paths", () => { + for ( + const [path, message] of [ + ["..\\..\\user/repos", "forward slashes"], + ["src/\u0000secret.ts", "control characters"], + ["src/\u0080secret.ts", "control characters"], + ["src/\u009fsecret.ts", "control characters"], + ["a".repeat(4_097), "4096-character limit"], + ] as const + ) { + assertThrows( + () => normalizeGitHubPath(path), + TypeError, + message, + ); + } + }); + it("rejects traversal segments in projectDir", () => { assertThrows( () => normalizeGitHubPath("src/file.ts", "/project/../other"), diff --git a/src/platform/adapters/fs/github/path-utils.ts b/src/platform/adapters/fs/github/path-utils.ts index 175b136207..cb9a8c3f8b 100644 --- a/src/platform/adapters/fs/github/path-utils.ts +++ b/src/platform/adapters/fs/github/path-utils.ts @@ -1,3 +1,17 @@ +const MAX_GITHUB_PATH_CODE_UNITS = 4_096; + +function hasControlCharacter(value: string): boolean { + for (let index = 0; index < value.length; index++) { + const codeUnit = value.charCodeAt(index); + if (codeUnit <= 0x1f || (codeUnit >= 0x7f && codeUnit <= 0x9f)) return true; + } + return false; +} + +function isUrlDoubleDotSegment(segment: string): boolean { + return /^(?:\.|%2e)(?:\.|%2e)$/i.test(segment); +} + export function normalizeGitHubPath(path: string, projectDir: string = ""): string { const normalizedPath = normalizePathSegments(path, "path"); const normalizedProjectDir = normalizePathSegments(projectDir, "projectDir"); @@ -17,6 +31,17 @@ function normalizePathSegments(value: string, label: string): string { if (typeof value !== "string") { throw new TypeError(`GitHub ${label} must be a string`); } + if (value.length > MAX_GITHUB_PATH_CODE_UNITS) { + throw new TypeError( + `GitHub ${label} exceeds the ${MAX_GITHUB_PATH_CODE_UNITS}-character limit`, + ); + } + if (hasControlCharacter(value)) { + throw new TypeError(`GitHub ${label} must not contain control characters`); + } + if (value.includes("\\")) { + throw new TypeError(`GitHub ${label} must use forward slashes`); + } const collapsed = value.replace(/^\/+|\/+$/g, "").replace(/\/+/g, "/"); const segments: string[] = []; @@ -26,7 +51,7 @@ function normalizePathSegments(value: string, label: string): string { if (segment === ".") continue; // ".." would escape the repository scope once the path is embedded in a // GitHub API URL (WHATWG URL resolution collapses dot segments): reject. - if (segment === "..") { + if (isUrlDoubleDotSegment(segment)) { throw new TypeError(`GitHub ${label} must not contain ".." traversal segments`); } segments.push(segment); diff --git a/src/platform/adapters/fs/integration.test.ts b/src/platform/adapters/fs/integration.test.ts index 781dfdaa5a..b679b6afdf 100644 --- a/src/platform/adapters/fs/integration.test.ts +++ b/src/platform/adapters/fs/integration.test.ts @@ -116,6 +116,30 @@ describe("integration.ts", () => { assertEquals(rejection.slug, "config-validation-failed"); }); + it("should preserve invalid project scoping instead of falling back to local files", async () => { + const error = await assertRejects( + () => + enhanceAdapterWithFS( + denoAdapter, + { + fs: { + type: "veryfront-api", + veryfront: { + apiBaseUrl: "https://api.example.com", + apiToken: "token", + projectSlug: "project", + }, + }, + }, + "/project/../etc", + ), + VeryfrontError, + "project directory must not contain", + ); + assertInstanceOf(error, VeryfrontError); + assertEquals(error.slug, "config-validation-failed"); + }); + it("should fall back to original adapter for unsupported type", async () => { const adapter = await enhanceAdapterWithFS(denoAdapter, { fs: { type: "unsupported-type" as any }, diff --git a/src/platform/adapters/fs/veryfront/path-normalizer.test.ts b/src/platform/adapters/fs/veryfront/path-normalizer.test.ts index 5765f82d9c..05b0d1dba9 100644 --- a/src/platform/adapters/fs/veryfront/path-normalizer.test.ts +++ b/src/platform/adapters/fs/veryfront/path-normalizer.test.ts @@ -1,7 +1,13 @@ import "#veryfront/schemas/_test-setup.ts"; -import { assertEquals, assertExists, assertThrows } from "#veryfront/testing/assert.ts"; +import { + assertEquals, + assertExists, + assertInstanceOf, + assertThrows, +} from "#veryfront/testing/assert.ts"; import { describe, it } from "#veryfront/testing/bdd.ts"; import { PathNormalizer } from "./path-normalizer.ts"; +import { VeryfrontError } from "#veryfront/errors/types.ts"; describe("PathNormalizer", () => { describe("class", () => { @@ -20,11 +26,13 @@ describe("PathNormalizer", () => { it("should reject traversal segments in projectDir", () => { for (const projectDir of ["/project/..", "../project", "/project//../root"]) { - assertThrows( + const error = assertThrows( () => new PathNormalizer(projectDir), - TypeError, + VeryfrontError, 'project directory must not contain ".." segments', ); + assertInstanceOf(error, VeryfrontError); + assertEquals(error.slug, "config-validation-failed"); } }); }); diff --git a/src/platform/adapters/fs/veryfront/path-normalizer.ts b/src/platform/adapters/fs/veryfront/path-normalizer.ts index ba334dac79..dbf07fe44f 100644 --- a/src/platform/adapters/fs/veryfront/path-normalizer.ts +++ b/src/platform/adapters/fs/veryfront/path-normalizer.ts @@ -1,4 +1,5 @@ import { logger as baseLogger } from "#veryfront/utils"; +import { CONFIG_VALIDATION_FAILED } from "#veryfront/errors/error-registry/config.ts"; const logger = baseLogger.component("path-normalizer"); const MAX_PATH_CODE_UNITS = 4_096; @@ -27,7 +28,16 @@ export class PathNormalizer { constructor(private readonly projectDir?: string) { if (projectDir !== undefined) { - this.assertSafePath(projectDir, "project directory"); + try { + this.assertSafePath(projectDir, "project directory"); + } catch (cause) { + throw CONFIG_VALIDATION_FAILED.create({ + detail: cause instanceof Error + ? cause.message + : "Filesystem project directory is invalid", + cause, + }); + } this.projectDirPrefix = normalizeForComparison(projectDir); } }