From 70c9147c7fa86ada23c55fa17b3a63f6a832f5f4 Mon Sep 17 00:00:00 2001 From: Julius Marminge Date: Sun, 1 Mar 2026 10:31:43 -0800 Subject: [PATCH] Harden attachment ID resolution and missing-file handling - Sanitize thread-derived attachment ID segments to safe, bounded tokens - Resolve attachment IDs only when a matching file exists on disk - Return 404 (not 400) for missing attachment IDs in the attachment route - Add attachmentStore tests for ID sanitization and extension-based path lookup --- apps/server/src/attachmentStore.test.ts | 56 +++++++++++++++++++ apps/server/src/attachmentStore.ts | 22 ++++---- apps/server/src/git/Layers/GitCore.ts | 2 +- .../src/provider/Layers/CodexAdapter.ts | 10 ++-- apps/server/src/wsServer.ts | 27 +++++---- 5 files changed, 90 insertions(+), 27 deletions(-) create mode 100644 apps/server/src/attachmentStore.test.ts diff --git a/apps/server/src/attachmentStore.test.ts b/apps/server/src/attachmentStore.test.ts new file mode 100644 index 0000000000000000000000000000000000000000..b3da749dda1154188cf872682d55c98550accac4 --- /dev/null +++ b/apps/server/src/attachmentStore.test.ts @@ -0,0 +1,56 @@ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { describe, expect, it } from "vitest"; + +import { createAttachmentId, resolveAttachmentPathById } from "./attachmentStore.ts"; + +describe("attachmentStore", () => { + it("sanitizes thread ids when creating attachment ids", () => { + const attachmentId = createAttachmentId("thread.folder/unsafe space"); + expect(attachmentId).toBeTruthy(); + if (!attachmentId) { + return; + } + + const [threadSegment] = attachmentId.split("-", 1); + expect(threadSegment).toBeTruthy(); + expect(threadSegment).toMatch(/^[a-z0-9_-]+$/i); + expect(threadSegment).not.toContain("."); + expect(threadSegment).not.toContain("%"); + expect(threadSegment).not.toContain("/"); + }); + + it("resolves attachment path by id using the extension that exists on disk", () => { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "t3code-attachment-store-")); + try { + const attachmentId = "thread-1-attachment"; + const attachmentsDir = path.join(stateDir, "attachments"); + fs.mkdirSync(attachmentsDir, { recursive: true }); + const pngPath = path.join(attachmentsDir, `${attachmentId}.png`); + fs.writeFileSync(pngPath, Buffer.from("hello")); + + const resolved = resolveAttachmentPathById({ + stateDir, + attachmentId, + }); + expect(resolved).toBe(pngPath); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + } + }); + + it("returns null when no attachment file exists for the id", () => { + const stateDir = fs.mkdtempSync(path.join(os.tmpdir(), "t3code-attachment-store-")); + try { + const resolved = resolveAttachmentPathById({ + stateDir, + attachmentId: "thread-1-missing", + }); + expect(resolved).toBeNull(); + } finally { + fs.rmSync(stateDir, { recursive: true, force: true }); + } + }); +}); diff --git a/apps/server/src/attachmentStore.ts b/apps/server/src/attachmentStore.ts index 39f80a3cab2dee081a8dc926117e40a889472804..4f9bb4be9311c705e7579dd0fe6b3c8a28a2b959 100644 --- a/apps/server/src/attachmentStore.ts +++ b/apps/server/src/attachmentStore.ts @@ -1,4 +1,5 @@ import { randomUUID } from "node:crypto"; +import { existsSync } from "node:fs"; import type { ChatAttachment } from "@t3tools/contracts"; @@ -10,17 +11,17 @@ import { import { inferImageExtension, SAFE_IMAGE_FILE_EXTENSIONS } from "./imageMime.ts"; const ATTACHMENT_FILENAME_EXTENSIONS = [...SAFE_IMAGE_FILE_EXTENSIONS, ".bin"]; +const ATTACHMENT_ID_THREAD_SEGMENT_MAX_CHARS = 80; function toSafeThreadAttachmentSegment(threadId: string): string | null { - const segment = encodeURIComponent(threadId); - if ( - segment.length === 0 || - segment === "." || - segment === ".." || - segment.includes("/") || - segment.includes("\\") || - segment.includes("\0") - ) { + const segment = threadId + .trim() + .replace(/[^a-z0-9_-]+/gi, "-") + .replace(/-+/g, "-") + .replace(/^[-_]+|[-_]+$/g, "") + .slice(0, ATTACHMENT_ID_THREAD_SEGMENT_MAX_CHARS) + .replace(/[-_]+$/g, ""); + if (segment.length === 0) { return null; } return segment; @@ -73,7 +74,7 @@ export function resolveAttachmentPathById(input: { stateDir: input.stateDir, relativePath: `${normalizedId}${extension}`, }); - if (maybePath) { + if (maybePath && existsSync(maybePath)) { return maybePath; } } @@ -92,4 +93,3 @@ export function parseAttachmentIdFromRelativePath(relativePath: string): string const id = normalized.slice(0, extensionIndex); return id.length > 0 && !id.includes(".") ? id : null; } - diff --git a/apps/server/src/git/Layers/GitCore.ts b/apps/server/src/git/Layers/GitCore.ts index 25848ad5f3dfd44dd6eb67b3f3b299e4d665b1c9..5dffecbae82bdc2bc55f4ac1d644b762a5ab1325 100644 --- a/apps/server/src/git/Layers/GitCore.ts +++ b/apps/server/src/git/Layers/GitCore.ts @@ -1167,7 +1167,7 @@ const makeGitCore = Effect.gen(function* () { }); // Refresh upstream refs in the background so checkout remains responsive. - yield* Effect.forkScoped( + yield* Effect.forkDetach( refreshCheckedOutBranchUpstream(input.cwd).pipe(Effect.catch(() => Effect.void)), ); }); diff --git a/apps/server/src/provider/Layers/CodexAdapter.ts b/apps/server/src/provider/Layers/CodexAdapter.ts index 768af2bb2123917f00a106fa4bcece479298fd92..cd678c84ff7906894fb4d3430aa55d0247ab0b83 100644 --- a/apps/server/src/provider/Layers/CodexAdapter.ts +++ b/apps/server/src/provider/Layers/CodexAdapter.ts @@ -489,10 +489,12 @@ const makeCodexAdapter = (options?: CodexAdapterLiveOptions) => attachment, }); if (!attachmentPath) { - return yield* toRequestError( - input.sessionId, - "turn/start", - new Error(`Invalid attachment id '${attachment.id}'.`), + return yield* Effect.fail( + toRequestError( + input.sessionId, + "turn/start", + new Error(`Invalid attachment id '${attachment.id}'.`), + ), ); } const bytes = yield* fileSystem.readFile(attachmentPath).pipe( diff --git a/apps/server/src/wsServer.ts b/apps/server/src/wsServer.ts index b15677a68c81c9d3bf14cab4661762a0462a04ff..025f8179f5d76c6f11f87c6bdae09a1c32e7e22b 100644 --- a/apps/server/src/wsServer.ts +++ b/apps/server/src/wsServer.ts @@ -348,18 +348,23 @@ export const createServer = Effect.fn(function* (): Effect.fn.Return< return; } - const filePath = - !normalizedRelativePath.includes("/") && !normalizedRelativePath.includes(".") - ? resolveAttachmentPathById({ - stateDir: serverConfig.stateDir, - attachmentId: normalizedRelativePath, - }) - : resolveAttachmentRelativePath({ - stateDir: serverConfig.stateDir, - relativePath: normalizedRelativePath, - }); + const isIdLookup = + !normalizedRelativePath.includes("/") && !normalizedRelativePath.includes("."); + const filePath = isIdLookup + ? resolveAttachmentPathById({ + stateDir: serverConfig.stateDir, + attachmentId: normalizedRelativePath, + }) + : resolveAttachmentRelativePath({ + stateDir: serverConfig.stateDir, + relativePath: normalizedRelativePath, + }); if (!filePath) { - respond(400, { "Content-Type": "text/plain" }, "Invalid attachment path"); + respond( + isIdLookup ? 404 : 400, + { "Content-Type": "text/plain" }, + isIdLookup ? "Not Found" : "Invalid attachment path", + ); return; } -- 2.54.0