Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 40 additions & 0 deletions src/platform/adapters/fs/cache/file-cache.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
initializeFileCacheBackend,
isFileCacheDistributedEnabled,
} from "./file-cache.ts";
import { CacheBackends } from "#veryfront/cache/backend.ts";

describe("FileCache", () => {
let cache: FileCache;
Expand Down Expand Up @@ -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<string, unknown> = {};
circular.self = circular;
distributedCache.set("cyclic", circular);
assertEquals(backendWrites, 0);
});

it("should return boolean", async () => {
assertEquals(typeof (await initializeFileCacheBackend()), "boolean");
});
Expand Down
11 changes: 10 additions & 1 deletion src/platform/adapters/fs/cache/file-cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
55 changes: 55 additions & 0 deletions src/platform/adapters/fs/github/github-api-client.test.ts
Original file line number Diff line number Diff line change
@@ -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 = {
Expand Down Expand Up @@ -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",
);
});
});
});
6 changes: 4 additions & 2 deletions src/platform/adapters/fs/github/github-api-client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,10 @@ export class GitHubApiClient {
): Promise<GitHubContentItem | GitHubContentItem[]> {
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)
}`;
Comment thread
kojiwakayama marked this conversation as resolved.
Outdated

logger.debug(`${LOG_PREFIX} Fetching contents`, { path: normalizedPath });

Expand Down
19 changes: 19 additions & 0 deletions src/platform/adapters/fs/github/path-utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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"),
Expand Down
27 changes: 26 additions & 1 deletion src/platform/adapters/fs/github/path-utils.ts
Original file line number Diff line number Diff line change
@@ -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");
Expand All @@ -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[] = [];
Expand All @@ -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);
Expand Down
24 changes: 24 additions & 0 deletions src/platform/adapters/fs/integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 },
Expand Down
14 changes: 11 additions & 3 deletions src/platform/adapters/fs/veryfront/path-normalizer.test.ts
Original file line number Diff line number Diff line change
@@ -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", () => {
Expand All @@ -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");
}
});
});
Expand Down
12 changes: 11 additions & 1 deletion src/platform/adapters/fs/veryfront/path-normalizer.ts
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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);
}
}
Expand Down