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 <noreply@anthropic.com>
This commit is contained in:
1 parent
0ef9bfb364
commit
ffedcbb2ff
3 files changed
+46
-4
No files matched your search
@@ -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
|
||||
}
|
||||
|
||||
@@ -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<string> {
|
||||
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<string, string> }): Promise<{ bytes: Uint8Array; contentType: string }> {
|
||||
const result = await callOperation(baseUrl, 'download', { path: { file: reference.fileName }, query: reference.query }, fetchImpl)
|
||||
|
||||
@@ -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> | 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'])
|
||||
})
|
||||
Reference in new issue
Block a user