From 6bffc0481cca1ff10f43c047823b4375a61e9ab9 Mon Sep 17 00:00:00 2001 From: Randalix Date: Mon, 12 Oct 2026 12:04:34 +0200 Subject: [PATCH] fix(preview): hold a folder file's resolved target to the attachment gate; accept long names Folder routes only looked at the requested name, so `shot.png -> ../secret.bin` and `notes.txt -> ../history` were served (registration answers 400 for both), and a FIFO or device behind a supported name parked a worker on an open that never returned. After resolving, the target's extension must be supported and it must be a regular file (locally fs.stat, remotely the probe kind). A 101..255-character file name answered 404 "Route not found": Fastify caps a path parameter at 100 characters. The server now sets routerOptions.maxParamLength (MAX_ROUTE_PARAM_LENGTH, shared with the route test harness) and a wiring guard pins it. Co-Authored-By: Claude Sonnet 5.5 --- src/web/route-helpers.ts | 9 ++ src/web/routes/file-routes.ts | 18 +++ src/web/server.ts | 10 +- test/routes/_route-test-utils.ts | 3 +- test/routes/folder-grid-routes.test.ts | 146 ++++++++++++++++++++++++- 5 files changed, 182 insertions(+), 4 deletions(-) diff --git a/src/web/route-helpers.ts b/src/web/route-helpers.ts index 08e1f683..75a92fcb 100644 --- a/src/web/route-helpers.ts +++ b/src/web/route-helpers.ts @@ -17,6 +17,15 @@ import { parseRalphLoopConfig, extractCompletionPhrase } from '../ralph-config.j import { SseEvent } from './sse-events.js'; import type { SessionPort } from './ports/session-port.js'; import type { EventPort } from './ports/event-port.js'; + +/** + * Longest path PARAMETER the router accepts (Fastify's default is 100). A file + * name goes up to 255 characters and arrives percent-encoded, so the folder + * grid's `…/files/:name/raw` answered 404 "Route not found" for a name the + * listing happily showed. Raw length, not decoded: 255 four-byte characters are + * 3060 characters of `%XX`. + */ +export const MAX_ROUTE_PARAM_LENGTH = 4096; import type { AuthSessionRecord } from './ports/auth-port.js'; import type { StaleExpirationMap } from '../utils/index.js'; import { dataPath } from '../config/instance.js'; diff --git a/src/web/routes/file-routes.ts b/src/web/routes/file-routes.ts index 576775c2..440ed572 100644 --- a/src/web/routes/file-routes.ts +++ b/src/web/routes/file-routes.ts @@ -2469,6 +2469,24 @@ export function registerFileRoutes(app: FastifyInstance, ctx: SessionPort & Even }; const servable = await resolveServableAttachmentPath(reply, record, scope); if (!servable) return null; + // The requested NAME only opens the door; registration judges the file a + // symlink resolves to (supported extension, regular file) and so does this. + // Without it `shot.png -> ../secret.bin` is served, and a FIFO or a device + // behind a supported name parks a worker on an open that never returns. + if (!isSupportedAttachmentExtension(extname(servable.path))) { + reply.code(400).send(createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Unsupported attachment type')); + return null; + } + const isRegularFile = scope.remote + ? servable.probe?.kind === 'file' + : await fs.stat(servable.path).then( + (stat) => stat.isFile(), + () => false + ); + if (!isRegularFile) { + reply.code(400).send(createErrorResponse(ApiErrorCode.INVALID_INPUT, 'Attachment path is not a file')); + return null; + } if (!folderAllowedForUser(servable.path, getAuthUser(req), scope)) { reply.code(403).send(createErrorResponse(ApiErrorCode.INVALID_INPUT, 'File is outside your space')); return null; diff --git a/src/web/server.ts b/src/web/server.ts index 65cb9659..91f9296b 100644 --- a/src/web/server.ts +++ b/src/web/server.ts @@ -159,6 +159,7 @@ import { MAX_CONCURRENT_SESSIONS, MAX_SSE_CLIENTS } from '../config/map-limits.j import { MAX_PASTE_IMAGE_BYTES } from '../config/buffer-limits.js'; import { resolveTerminalHistoryConfig } from '../config/terminal-history.js'; import { SseEvent } from './sse-events.js'; +import { MAX_ROUTE_PARAM_LENGTH } from './route-helpers.js'; import { getLatestPlanUsage, setLatestCodexPlanUsage } from './plan-usage-latest.js'; import { telemetrySignature } from '../usage-telemetry.js'; import { readCodexPlanUsage, resolveCodexBinaryPath } from '../utils/codex-cli-resolver.js'; @@ -433,9 +434,14 @@ export class WebServer extends EventEmitter { const rewriteUrl = (req: { url?: string }): string => rewriteApiV1Url(stripBasePath(this.basePath, req.url || '')); if (https) { const { key, cert } = getOrCreateSelfSignedCert(); - this.app = Fastify({ logger: false, https: { key, cert }, rewriteUrl }); + this.app = Fastify({ + logger: false, + https: { key, cert }, + rewriteUrl, + routerOptions: { maxParamLength: MAX_ROUTE_PARAM_LENGTH }, + }); } else { - this.app = Fastify({ logger: false, rewriteUrl }); + this.app = Fastify({ logger: false, rewriteUrl, routerOptions: { maxParamLength: MAX_ROUTE_PARAM_LENGTH } }); } this.mux = createMultiplexer(); this.sse = new SseStreamManager( diff --git a/test/routes/_route-test-utils.ts b/test/routes/_route-test-utils.ts index 2a2facf7..42a4c291 100644 --- a/test/routes/_route-test-utils.ts +++ b/test/routes/_route-test-utils.ts @@ -8,6 +8,7 @@ import Fastify, { type FastifyInstance } from 'fastify'; import fastifyCookie from '@fastify/cookie'; import { createMockRouteContext, type MockRouteContext } from '../mocks/index.js'; import { installRouteErrorHandler } from '../../src/web/route-error-handler.js'; +import { MAX_ROUTE_PARAM_LENGTH } from '../../src/web/route-helpers.js'; export interface RouteTestHarness { app: FastifyInstance; @@ -30,7 +31,7 @@ export async function createRouteTestHarness( registerFn: (app: FastifyInstance, ctx: any) => void, ctxOptions?: { sessionId?: string; authUser?: { username: string; role: 'admin' | 'user' } } ): Promise { - const app = Fastify({ logger: false }); + const app = Fastify({ logger: false, routerOptions: { maxParamLength: MAX_ROUTE_PARAM_LENGTH } }); // Register cookie plugin — some routes access req.cookies await app.register(fastifyCookie); diff --git a/test/routes/folder-grid-routes.test.ts b/test/routes/folder-grid-routes.test.ts index a678656f..7fad29bd 100644 --- a/test/routes/folder-grid-routes.test.ts +++ b/test/routes/folder-grid-routes.test.ts @@ -10,7 +10,17 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { Readable } from 'node:stream'; import { execFileSync } from 'node:child_process'; -import { mkdirSync, mkdtempSync, rmSync, symlinkSync, utimesSync, writeFileSync } from 'node:fs'; +import { + closeSync, + mkdirSync, + mkdtempSync, + openSync, + readFileSync, + rmSync, + symlinkSync, + utimesSync, + writeFileSync, +} from 'node:fs'; import fs from 'node:fs/promises'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; @@ -225,6 +235,91 @@ describe('folder grid routes — local folder', () => { expect(res.statusCode).toBe(403); }); + // The attachment gate (`registerExternalAttachment`) judges the file a symlink + // RESOLVES to: its extension must be a supported one and it must be a regular + // file. The folder routes only looked at the requested name, so a name that + // passed (`shot.png`) could serve anything it pointed at. + describe('the resolved target is held to the attachment gate', () => { + const files = (folderId: string, name: string, leaf: 'raw' | 'preview' | 'thumbnail' = 'raw') => + harness.app.inject({ + method: 'GET', + url: `/api/sessions/${sessionId}/folders/${folderId}/files/${encodeURIComponent(name)}/${leaf}`, + }); + + it('refuses a supported-looking name whose target has an unsupported extension', async () => { + writeFileSync(join(root, 'secret.bin'), 'secret'); + symlinkSync(join(root, 'secret.bin'), join(dir, 'shot.png')); + const { folderId } = JSON.parse((await grant(dir)).body).data; + for (const leaf of ['raw', 'preview', 'thumbnail'] as const) { + const res = await files(folderId, 'shot.png', leaf); + expect(res.statusCode, leaf).toBe(400); + expect(res.body, leaf).not.toContain('secret'); + } + }); + + it('refuses a name whose target has no extension at all', async () => { + writeFileSync(join(root, 'history'), 'private history'); + symlinkSync(join(root, 'history'), join(dir, 'notes.txt')); + const { folderId } = JSON.parse((await grant(dir)).body).data; + const res = await files(folderId, 'notes.txt'); + expect(res.statusCode).toBe(400); + expect(res.body).not.toContain('private history'); + }); + + it('refuses a device node behind a supported name', async () => { + // /dev/null, not /dev/zero: the same class (character device, no + // extension), but it ends the stream instead of filling the test's memory + // should the guard ever be missing. + symlinkSync('/dev/null', join(dir, 'zero.txt')); + const { folderId } = JSON.parse((await grant(dir)).body).data; + const res = await files(folderId, 'zero.txt'); + expect(res.statusCode).toBe(400); + }); + + it('refuses a FIFO without opening it', async () => { + const fifo = join(dir, 'pipe.txt'); + execFileSync('mkfifo', [fifo]); + // Hold both ends open from here: if the route ever did open it, the open + // returns at once and the read blocks, which the race below reports as a + // hang instead of leaving a libuv worker parked on a writer that never comes. + const fd = openSync(fifo, 'r+'); + try { + const { folderId } = JSON.parse((await grant(dir)).body).data; + const outcome = await Promise.race([ + files(folderId, 'pipe.txt').then((res) => res.statusCode), + new Promise((resolve) => setTimeout(() => resolve('hung'), 2000)), + ]); + expect(outcome).toBe(400); + } finally { + closeSync(fd); + } + }); + + it('still serves a symlink whose target is a supported regular file', async () => { + writeFileSync(join(root, 'real.png'), 'real-png'); + symlinkSync(join(root, 'real.png'), join(dir, 'alias.png')); + const { folderId } = JSON.parse((await grant(dir)).body).data; + const res = await files(folderId, 'alias.png'); + expect(res.statusCode).toBe(200); + expect(res.body).toBe('real-png'); + }); + }); + + it('serves a file whose name is longer than the 100-character default of a route parameter', async () => { + // The listing shows it (names go up to 255), so the raw/preview/thumbnail + // routes have to be able to take it back. + const longName = `${'n'.repeat(146)}.png`; + expect(longName.length).toBe(150); + touch(join(dir, longName), 'long-png', 1_700_000_300); + const { folderId } = JSON.parse((await grant(dir)).body).data; + const res = await harness.app.inject({ + method: 'GET', + url: `/api/sessions/${sessionId}/folders/${folderId}/files/${longName}/raw`, + }); + expect(res.statusCode).toBe(200); + expect(res.body).toBe('long-png'); + }); + it('404s an unknown folder id and another session’s grant', async () => { const res = await harness.app.inject({ method: 'GET', @@ -314,6 +409,41 @@ describe('folder grid routes — remote (SSH) case', () => { expect(thumb.statusCode).toBe(400); }); + describe('the resolved remote target is held to the attachment gate', () => { + async function rawOf(name: string) { + const res = await harness.app.inject({ + method: 'POST', + url: `/api/sessions/${sessionId}/folders`, + payload: { path: '~/styleframes' }, + }); + const { folderId } = JSON.parse(res.body).data; + return harness.app.inject({ + method: 'GET', + url: `/api/sessions/${sessionId}/folders/${folderId}/files/${name}/raw`, + }); + } + + it('refuses a name whose remote symlink resolves to an unsupported extension', async () => { + mockedProbePaths.mockResolvedValue([ + { realPath: '/mnt/raid/secret.bin', kind: 'file', size: 6, mtimeMs: 1000 }, + { realPath: REMOTE_DIR, kind: 'directory', size: 0, mtimeMs: 0 }, + ]); + const res = await rawOf('a.png'); + expect(res.statusCode).toBe(400); + expect(mockedCreateReadStream).not.toHaveBeenCalled(); + }); + + it('refuses a name that resolves to something that is not a regular file', async () => { + mockedProbePaths.mockResolvedValue([ + { realPath: '/mnt/raid/styleframes/a.png', kind: 'other', size: 0, mtimeMs: 0 }, + { realPath: REMOTE_DIR, kind: 'directory', size: 0, mtimeMs: 0 }, + ]); + const res = await rawOf('a.png'); + expect(res.statusCode).toBe(400); + expect(mockedCreateReadStream).not.toHaveBeenCalled(); + }); + }); + it('answers 400 "Not a folder" for a remote file, so the client falls back to the preview', async () => { mockedListFolder.mockResolvedValue('not-a-folder'); const res = await harness.app.inject({ @@ -390,3 +520,17 @@ describe('validFolderFileName', () => { } }); }); + +describe('server wiring', () => { + // The route harness builds its own Fastify instance, so a test through it + // cannot notice the real server losing the option. Same style as the other + // wiring guards: read the source, not a comment. + it('gives the real server the same route-parameter limit the folder routes are tested with', () => { + const source = readFileSync(join(__dirname, '../../src/web/server.ts'), 'utf-8'); + const instances = source.match(/Fastify\(\{[^}]*\}[^)]*\)/g) ?? []; + expect(instances.length).toBeGreaterThanOrEqual(2); + for (const instance of instances) { + expect(instance).toContain('maxParamLength: MAX_ROUTE_PARAM_LENGTH'); + } + }); +});