From 548c0bcbefb2d633f4ff5ec403fd6e2a5e27d587 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 11:55:52 +0200 Subject: [PATCH 1/4] fix(platform): close filesystem hardening review gaps --- .../adapters/fs/cache/file-cache.test.ts | 40 ++++++++++++++ src/platform/adapters/fs/cache/file-cache.ts | 11 +++- .../fs/github/github-api-client.test.ts | 55 +++++++++++++++++++ .../adapters/fs/github/github-api-client.ts | 6 +- .../adapters/fs/github/path-utils.test.ts | 19 +++++++ src/platform/adapters/fs/github/path-utils.ts | 27 ++++++++- src/platform/adapters/fs/integration.test.ts | 24 ++++++++ .../fs/veryfront/path-normalizer.test.ts | 14 ++++- .../adapters/fs/veryfront/path-normalizer.ts | 12 +++- 9 files changed, 200 insertions(+), 8 deletions(-) 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..e2f530dca8 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,7 @@ import "#veryfront/schemas/_test-setup.ts"; import { assertEquals, assertExists } 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 = { @@ -70,4 +71,58 @@ 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", + ); + }); + }); }); diff --git a/src/platform/adapters/fs/github/github-api-client.ts b/src/platform/adapters/fs/github/github-api-client.ts index ead85cb9ea..14e38e025d 100644 --- a/src/platform/adapters/fs/github/github-api-client.ts +++ b/src/platform/adapters/fs/github/github-api-client.ts @@ -61,8 +61,10 @@ export class GitHubApiClient { ): 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 encodedPath = normalizedPath.split("/").map(encodeURIComponent).join("/"); + const endpoint = `/repos/${this.config.owner}/${this.config.repo}/contents/${encodedPath}?ref=${ + encodeURIComponent(contentRef) + }`; logger.debug(`${LOG_PREFIX} Fetching contents`, { path: normalizedPath }); diff --git a/src/platform/adapters/fs/github/path-utils.test.ts b/src/platform/adapters/fs/github/path-utils.test.ts index f69a511130..167a0c90f4 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,22 @@ 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"], + ["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..e30e90fee6 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 hasAsciiControlCharacter(value: string): boolean { + for (let index = 0; index < value.length; index++) { + const codeUnit = value.charCodeAt(index); + if (codeUnit <= 0x1f || codeUnit === 0x7f) 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 (hasAsciiControlCharacter(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); } } From 3f561044a20b773581c9a01cc146e033eed6e776 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 12:16:30 +0200 Subject: [PATCH 2/4] Keep authenticated GitHub requests inside the configured repository Repository identity and dynamic API path values were accepted as raw URL segments. Validate repository identity at the client boundary and encode every path segment so malformed configuration cannot redirect an authenticated request through URL normalization. Constraint: Content paths remain hierarchical, but each individual segment must be encoded and dot traversal must fail closed. Rejected: Encode the complete endpoint string | would also encode required API separators Confidence: high Scope-risk: narrow Tested: GitHub adapter suite (118 steps), targeted fmt, lint, check, diff check --- .../fs/github/github-api-client.test.ts | 32 ++++++++++++++++- .../adapters/fs/github/github-api-client.ts | 34 +++++++++++++++---- 2 files changed, 58 insertions(+), 8 deletions(-) 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 e2f530dca8..b64a7ef2eb 100644 --- a/src/platform/adapters/fs/github/github-api-client.test.ts +++ b/src/platform/adapters/fs/github/github-api-client.test.ts @@ -1,5 +1,10 @@ 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"; @@ -44,6 +49,23 @@ describe("GitHubApiClient", () => { it("should return owner/repo format", () => { assertEquals(createClient().repoId, "test-owner/test-repo"); }); + + it("rejects repository identities that can escape their URL segments", () => { + for ( + const [field, value] of [ + ["owner", "trusted/../../users"], + ["owner", "trusted\\..\\users"], + ["repo", ".."], + ["repo", "."], + ] as const + ) { + assertThrows( + () => new GitHubApiClient({ ...mockConfig, [field]: value }), + TypeError, + `GitHub ${field} must be a single URL path segment`, + ); + } + }); }); describe("methods", () => { @@ -124,5 +146,13 @@ describe("GitHubApiClient", () => { "/repos/test-owner/test-repo/contents/docs/read%20me%23draft%3F.md", ); }); + + it("rejects raw dot segments before URL construction", async () => { + await assertRejects( + () => createClient().getContents("../../user/repos"), + TypeError, + "GitHub URL paths must not contain dot segments", + ); + }); }); }); diff --git a/src/platform/adapters/fs/github/github-api-client.ts b/src/platform/adapters/fs/github/github-api-client.ts index 14e38e025d..a2f569e4b5 100644 --- a/src/platform/adapters/fs/github/github-api-client.ts +++ b/src/platform/adapters/fs/github/github-api-client.ts @@ -15,6 +15,20 @@ const LOG_PREFIX = "[GitHubApiClient]"; const RATE_LIMIT_WARNING_THRESHOLD = 100; const RETRY_JITTER_MAX_MS = 1_000; +function encodeUrlPathSegment(value: string): string { + if (value === "." || value === "..") { + throw new TypeError("GitHub URL paths must not contain dot segments"); + } + return encodeURIComponent(value); +} + +function encodeRepositorySegment(value: string, field: "owner" | "repo"): string { + if (value === "." || value === ".." || /[\\/]/u.test(value)) { + throw new TypeError(`GitHub ${field} must be a single URL path segment`); + } + return encodeUrlPathSegment(value); +} + class GitHubBlobIntegrityError extends Error {} interface RateLimitInfo { @@ -28,9 +42,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, "repo"); + this.repositoryEndpoint = `/repos/${owner}/${repo}`; + } get repoId(): string { return `${this.config.owner}/${this.config.repo}`; @@ -38,8 +57,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/${ + encodeUrlPathSegment(treeRef) + }?recursive=1`; logger.debug(`${LOG_PREFIX} Fetching tree`, { ref: treeRef }); @@ -61,8 +81,8 @@ export class GitHubApiClient { ): Promise { const contentRef = ref ?? this.config.ref; const normalizedPath = path.replace(/^\/+/, ""); - const encodedPath = normalizedPath.split("/").map(encodeURIComponent).join("/"); - const endpoint = `/repos/${this.config.owner}/${this.config.repo}/contents/${encodedPath}?ref=${ + const encodedPath = normalizedPath.split("/").map(encodeUrlPathSegment).join("/"); + const endpoint = `${this.repositoryEndpoint}/contents/${encodedPath}?ref=${ encodeURIComponent(contentRef) }`; @@ -73,7 +93,7 @@ export class GitHubApiClient { } async getBlob(sha: string): Promise { - const endpoint = `/repos/${this.config.owner}/${this.config.repo}/git/blobs/${sha}`; + const endpoint = `${this.repositoryEndpoint}/git/blobs/${encodeUrlPathSegment(sha)}`; logger.debug(`${LOG_PREFIX} Fetching blob`, { sha }); @@ -97,7 +117,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/${encodeUrlPathSegment(sha)}`; logger.debug(`${LOG_PREFIX} Fetching bounded raw blob`, { sha, expectedSize }); From b297c45373649807ead62a87bfb419065c5ef590 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 12:21:48 +0200 Subject: [PATCH 3/4] fix(platform): bind GitHub API repository segments --- .../fs/github/github-api-client.test.ts | 99 ++++++++++++++++--- .../adapters/fs/github/github-api-client.ts | 90 ++++++++++++++--- 2 files changed, 157 insertions(+), 32 deletions(-) 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 b64a7ef2eb..691e719271 100644 --- a/src/platform/adapters/fs/github/github-api-client.test.ts +++ b/src/platform/adapters/fs/github/github-api-client.test.ts @@ -43,31 +43,37 @@ describe("GitHubApiClient", () => { it("should be instantiable with config", () => { assertExists(createClient()); }); - }); - - describe("repoId", () => { - it("should return owner/repo format", () => { - assertEquals(createClient().repoId, "test-owner/test-repo"); - }); - it("rejects repository identities that can escape their URL segments", () => { + it("rejects repository identity that could escape its URL segments", () => { for ( const [field, value] of [ - ["owner", "trusted/../../users"], - ["owner", "trusted\\..\\users"], - ["repo", ".."], + ["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 ${field} must be a single URL path segment`, + "GitHub", ); } }); }); + describe("repoId", () => { + it("should return owner/repo format", () => { + assertEquals(createClient().repoId, "test-owner/test-repo"); + }); + }); + describe("methods", () => { it("should have getTree method", () => { assertMethod(createClient(), "getTree"); @@ -147,11 +153,72 @@ describe("GitHubApiClient", () => { ); }); - it("rejects raw dot segments before URL construction", async () => { - await assertRejects( - () => createClient().getContents("../../user/repos"), - TypeError, - "GitHub URL paths must not contain dot segments", + 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 a2f569e4b5..27f1a05cd0 100644 --- a/src/platform/adapters/fs/github/github-api-client.ts +++ b/src/platform/adapters/fs/github/github-api-client.ts @@ -14,19 +14,78 @@ 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; + } -function encodeUrlPathSegment(value: string): string { - if (value === "." || value === "..") { - throw new TypeError("GitHub URL paths must not contain dot segments"); + 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 encodeRepositorySegment(value: string, field: "owner" | "repo"): string { - if (value === "." || value === ".." || /[\\/]/u.test(value)) { - throw new TypeError(`GitHub ${field} must be a single URL path segment`); +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 encodeUrlPathSegment(value); + return { + normalized, + encoded: segments.map(encodeURIComponent).join("/"), + }; } class GitHubBlobIntegrityError extends Error {} @@ -47,7 +106,7 @@ export class GitHubApiClient { constructor(private readonly config: ResolvedGitHubConfig) { const owner = encodeRepositorySegment(config.owner, "owner"); - const repo = encodeRepositorySegment(config.repo, "repo"); + const repo = encodeRepositorySegment(config.repo, "repository"); this.repositoryEndpoint = `/repos/${owner}/${repo}`; } @@ -58,7 +117,7 @@ export class GitHubApiClient { async getTree(ref?: string): Promise { const treeRef = ref ?? this.config.ref; const endpoint = `${this.repositoryEndpoint}/git/trees/${ - encodeUrlPathSegment(treeRef) + encodeEndpointValue(treeRef, "tree ref") }?recursive=1`; logger.debug(`${LOG_PREFIX} Fetching tree`, { ref: treeRef }); @@ -80,20 +139,19 @@ export class GitHubApiClient { ref?: string, ): Promise { const contentRef = ref ?? this.config.ref; - const normalizedPath = path.replace(/^\/+/, ""); - const encodedPath = normalizedPath.split("/").map(encodeUrlPathSegment).join("/"); - const endpoint = `${this.repositoryEndpoint}/contents/${encodedPath}?ref=${ - encodeURIComponent(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 = `${this.repositoryEndpoint}/git/blobs/${encodeUrlPathSegment(sha)}`; + const endpoint = `${this.repositoryEndpoint}/git/blobs/${encodeEndpointValue(sha, "blob SHA")}`; logger.debug(`${LOG_PREFIX} Fetching blob`, { sha }); @@ -117,7 +175,7 @@ export class GitHubApiClient { if (expectedSize > byteLimit) { throw new RangeError(`GitHub blob exceeds ${byteLimit} bytes`); } - const endpoint = `${this.repositoryEndpoint}/git/blobs/${encodeUrlPathSegment(sha)}`; + const endpoint = `${this.repositoryEndpoint}/git/blobs/${encodeEndpointValue(sha, "blob SHA")}`; logger.debug(`${LOG_PREFIX} Fetching bounded raw blob`, { sha, expectedSize }); From a37064a6fa8d42938487ffd030f02fee8395df38 Mon Sep 17 00:00:00 2001 From: Koji Wakayama Date: Mon, 3 Aug 2026 13:25:15 +0200 Subject: [PATCH 4/4] Reject non-printable C1 bytes at GitHub path boundaries The URL-facing path normalizer already rejected C0 and DEL controls, but the adjacent C1 range remained admissible. Treat the complete C0, DEL, and C1 control ranges consistently before URL construction. Constraint: Preserve existing path normalization and public error wording. Rejected: Match only ASCII C0 controls | leaves U+0080 through U+009F available as non-printable path input. Confidence: high Scope-risk: narrow Tested: Focused GitHub path and API client suites, 36 steps Tested: Touched-file format, lint, typecheck, and git diff checks --- src/platform/adapters/fs/github/path-utils.test.ts | 2 ++ src/platform/adapters/fs/github/path-utils.ts | 6 +++--- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/src/platform/adapters/fs/github/path-utils.test.ts b/src/platform/adapters/fs/github/path-utils.test.ts index 167a0c90f4..b5fa6b093b 100644 --- a/src/platform/adapters/fs/github/path-utils.test.ts +++ b/src/platform/adapters/fs/github/path-utils.test.ts @@ -72,6 +72,8 @@ describe("platform/adapters/fs/github/path-utils", () => { 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 ) { diff --git a/src/platform/adapters/fs/github/path-utils.ts b/src/platform/adapters/fs/github/path-utils.ts index e30e90fee6..cb9a8c3f8b 100644 --- a/src/platform/adapters/fs/github/path-utils.ts +++ b/src/platform/adapters/fs/github/path-utils.ts @@ -1,9 +1,9 @@ const MAX_GITHUB_PATH_CODE_UNITS = 4_096; -function hasAsciiControlCharacter(value: string): boolean { +function hasControlCharacter(value: string): boolean { for (let index = 0; index < value.length; index++) { const codeUnit = value.charCodeAt(index); - if (codeUnit <= 0x1f || codeUnit === 0x7f) return true; + if (codeUnit <= 0x1f || (codeUnit >= 0x7f && codeUnit <= 0x9f)) return true; } return false; } @@ -36,7 +36,7 @@ function normalizePathSegments(value: string, label: string): string { `GitHub ${label} exceeds the ${MAX_GITHUB_PATH_CODE_UNITS}-character limit`, ); } - if (hasAsciiControlCharacter(value)) { + if (hasControlCharacter(value)) { throw new TypeError(`GitHub ${label} must not contain control characters`); } if (value.includes("\\")) {