From ffedcbb2ff3e19e152b6054d43dacac7d2cd3616 Mon Sep 17 00:00:00 2001 From: James Maes Date: Sat, 31 Oct 2026 17:50:20 -0500 Subject: [PATCH] core+cli: never write a server-named download outside --out-dir or over a file DOWNLOAD_FORM names come from the server. safeDownloadName keeps the last path segment only (either separator), strips control characters and refuses empty, . and ..; qqq process run writes with flag wx and takes name-1.ext, name-2.ext when the name is taken. Tested with traversal names. Co-Authored-By: Claude Fable 5.1 --- packages/cli/src/commands/process.ts | 20 +++++++++++++++++--- packages/core/src/process/screen.ts | 19 +++++++++++++++++++ packages/core/src/test/runner.test.ts | 11 ++++++++++- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/packages/cli/src/commands/process.ts b/packages/cli/src/commands/process.ts index a84be78..da863c9 100644 --- a/packages/cli/src/commands/process.ts +++ b/packages/cli/src/commands/process.ts @@ -7,7 +7,7 @@ import { mkdirSync, readFileSync, writeFileSync } from 'node:fs' import { basename, join } from 'node:path' import type { Command } from 'commander' import { UiApiError } from '@qrun-io/qqq-ui-api-client' -import { BACK, FormInputError, FormSession, ProcessCancelledError, ProcessError, blocksToText, checkInputRecords, downloadProcessFile, formatTable, helpText, isMissingValue, processHelpText, runProcess, screenToText, summaryLines, type FieldMeta, type ProcessMeta, type RunOutcome, type ScreenBlock } from '@qrun-io/qqq-core' +import { BACK, alternativeNames, safeDownloadName, FormInputError, FormSession, ProcessCancelledError, ProcessError, blocksToText, checkInputRecords, downloadProcessFile, formatTable, helpText, isMissingValue, processHelpText, runProcess, screenToText, summaryLines, type FieldMeta, type ProcessMeta, type RunOutcome, type ScreenBlock } from '@qrun-io/qqq-core' import { UsageError, type Context } from '../context.js' import { ask, promptForm } from '../prompt.js' import { authenticatedFetch } from '../session.js' @@ -94,8 +94,8 @@ export function registerProcess(program: Command, ctx: () => Context): void { if (block.kind !== 'download') continue const file = await downloadProcessFile(c.server, fetchImpl, block) mkdirSync(options.outDir, { recursive: true }) - const path = join(options.outDir, basename(block.fileName)) - writeFileSync(path, file.bytes) + // the name comes from the server: last segment only, and never overwrite an existing file + const path = writeNew(options.outDir, safeDownloadName(block.fileName), file.bytes) saved.push(path) c.note(`saved ${path} (${file.bytes.length} bytes${file.contentType ? `, ${file.contentType}` : ''})`) } @@ -199,6 +199,20 @@ export function registerProcess(program: Command, ctx: () => Context): void { interface RunOptions { values?: string[]; valuesJson?: string; records?: string; filterJson?: string; file?: string[]; interactive?: boolean; prompt: boolean; timeout: string; skipValidation?: boolean; validate?: boolean; outDir: string; recordLimit: string; quiet?: boolean } +/** Writes bytes to a new file in `dir` (flag wx), taking `name-1.ext`, `name-2.ext`, … when the name is taken. */ +function writeNew(dir: string, name: string, bytes: Uint8Array): string { + for (const candidate of alternativeNames(name)) { + const path = join(dir, candidate) + try { + writeFileSync(path, bytes, { flag: 'wx' }) + return path + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== 'EEXIST') throw error + } + } + throw new UsageError(`no free file name for ${name} in ${dir}`) +} + function outcomeStepLabel(meta: ProcessMeta, stepName: string): string { return meta.frontendSteps?.find((s) => s.name === stepName)?.label ?? stepName } diff --git a/packages/core/src/process/screen.ts b/packages/core/src/process/screen.ts index 1a5b854..08cb0be 100644 --- a/packages/core/src/process/screen.ts +++ b/packages/core/src/process/screen.ts @@ -140,6 +140,25 @@ export function downloadReference(values: FieldValues): { fileName: string; quer return undefined } +/** + * A server-supplied file name made safe to write in a chosen directory: the last path segment + * only (either separator), without control characters; empty, `.` and `..` are refused (throws). + */ +export function safeDownloadName(name: string): string { + const base = name.split(/[\\/]/).pop()!.replace(/[\u0000-\u001f\u007f]/g, '').trim() + if (!base || base === '.' || base === '..') throw new Error(`unsafe file name from the server: ${JSON.stringify(name)}`) + return base +} + +/** Candidate names when one exists already: `a.csv`, `a-1.csv`, `a-2.csv`, … */ +export function* alternativeNames(name: string): Generator { + yield name + const dot = name.lastIndexOf('.') + const stem = dot > 0 ? name.slice(0, dot) : name + const ext = dot > 0 ? name.slice(dot) : '' + for (let i = 1; i < 1000; i++) yield `${stem}-${i}${ext}` +} + /** Downloads a DOWNLOAD_FORM file (GET /download/{file}); returns its bytes and content type. */ export async function downloadProcessFile(baseUrl: string, fetchImpl: typeof fetch, reference: { fileName: string; query: Record }): Promise<{ bytes: Uint8Array; contentType: string }> { const result = await callOperation(baseUrl, 'download', { path: { file: reference.fileName }, query: reference.query }, fetchImpl) diff --git a/packages/core/src/test/runner.test.ts b/packages/core/src/test/runner.test.ts index f014606..f2310b9 100644 --- a/packages/core/src/test/runner.test.ts +++ b/packages/core/src/test/runner.test.ts @@ -6,7 +6,7 @@ import assert from 'node:assert/strict' import { test } from 'node:test' import { BACK, ProcessCancelledError, ProcessError, checkInputRecords, hasInputs, inputFields, runProcess, type ProcessMeta, type ProcessStep } from '../process/runner.js' -import { buildScreen, downloadReference, screenToText, summaryLines, type ScreenBlock } from '../process/screen.js' +import { alternativeNames, buildScreen, downloadReference, safeDownloadName, screenToText, summaryLines, type ScreenBlock } from '../process/screen.js' type Handler = (request: Request, url: URL) => Promise | unknown @@ -238,3 +238,12 @@ test('screens: help content, HELP_TEXT preview, WIDGET named and ad hoc, Google assert.equal(downloadReference({ downloadFileName: 'a.csv' }), undefined) assert.deepEqual(summaryLines([{ status: 'OK', tableName: 'person', recordId: 4, linkPreText: 'Created ', linkText: 'Avery' }]), [{ status: 'OK', count: undefined, message: 'Created Avery', bullets: undefined, link: 'person 4' }]) }) + +test('download names from the server are reduced to a safe base name', () => { + assert.equal(safeDownloadName('../../etc/passwd'), 'passwd') + assert.equal(safeDownloadName('..\\..\\Windows\\win.ini'), 'win.ini') + assert.equal(safeDownloadName('report\u0007.csv'), 'report.csv') + for (const bad of ['', '.', '..', '../', 'a/..', '\u0001']) assert.throws(() => safeDownloadName(bad), /unsafe file name/) + const names = alternativeNames('out.csv') + assert.deepEqual([names.next().value, names.next().value, names.next().value], ['out.csv', 'out-1.csv', 'out-2.csv']) +})