From 8919437397f024632585916e387eaeef1632776c Mon Sep 17 00:00:00 2001 From: CarmenDou Date: Wed, 12 Aug 2026 16:21:18 -0700 Subject: [PATCH 1/6] feat(storage): browse, download, and delete a bucket's objects MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an `insta storage` group over the three new platform object routes: insta storage list [--prefix

] [--cursor ] [--limit ] [--service ] [--branch ] [--json] insta storage get [-o ] [--service ] [--branch ] [--json] insta storage delete [-y] [--service ] [--branch ] [--json] `get` resolves a short-lived presigned URL and pulls the bytes straight from the provider, so nothing large streams through the control plane; it writes to the key's last segment unless `-o` names a file. `delete` is irreversible and runs immediately (a data operation, not staged infrastructure), so on a terminal it confirms first — `-y` skips the prompt and a non-TTY proceeds. `resolveComputeServiceId` is generalized into `resolveSoleService` so storage gets the same named-or-sole resolution with identical messages. --- src/commands/services.ts | 23 ++++--- src/commands/storage.ts | 121 ++++++++++++++++++++++++++++++++++++ src/index.ts | 23 ++++++- test/storage.test.ts | 131 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 288 insertions(+), 10 deletions(-) create mode 100644 src/commands/storage.ts create mode 100644 test/storage.test.ts diff --git a/src/commands/services.ts b/src/commands/services.ts index e5bdf03..c255f14 100644 --- a/src/commands/services.ts +++ b/src/commands/services.ts @@ -47,17 +47,22 @@ export function resolveServiceId(services: Array<{ id: string; type: string; nam return svc.id } -// Resolve a compute service id: by name, or the sole compute service when name is omitted. -export function resolveComputeServiceId(services: Array<{ id: string; type: string; name: string }>, name?: string): string { - const compute = services.filter((s) => s.type === 'compute') +// Resolve one service of a type: by name, or the sole one of that type when name is omitted. +export function resolveSoleService(services: T[], type: string, name?: string): T { + const of = services.filter((s) => s.type === type) if (name) { - const svc = compute.find((s) => s.name === name) - if (!svc) throw new Error(`compute service not found: ${name}`) - return svc.id + const svc = of.find((s) => s.name === name) + if (!svc) throw new Error(`${type} service not found: ${name}`) + return svc } - if (compute.length === 0) throw new Error('no compute service in this project (add one with `insta services add compute `)') - if (compute.length > 1) throw new Error(`multiple compute services — specify one: ${compute.map((s) => s.name).join(', ')}`) - return compute[0]!.id + if (of.length === 0) throw new Error(`no ${type} service in this project (add one with \`insta services add ${type} \`)`) + if (of.length > 1) throw new Error(`multiple ${type} services — specify one: ${of.map((s) => s.name).join(', ')}`) + return of[0]! +} + +// Resolve a compute service id: by name, or the sole compute service when name is omitted. +export function resolveComputeServiceId(services: Array<{ id: string; type: string; name: string }>, name?: string): string { + return resolveSoleService(services, 'compute', name).id } // ---- commands ---- diff --git a/src/commands/storage.ts b/src/commands/storage.ts new file mode 100644 index 0000000..48eb0df --- /dev/null +++ b/src/commands/storage.ts @@ -0,0 +1,121 @@ +// `insta storage` — browse, download, and delete the objects in a storage service's bucket. +import { writeFile } from 'node:fs/promises' +import { ApiClient, requireProject } from '../api.js' +import { info, printJson, handleApproval } from '../util.js' +import { q, resolveSoleService } from './services.js' +import { fmtBytes } from './db.js' // the repo's tested bytes formatter — don't grow a third copy + +type Common = { branch?: string; service?: string; json?: boolean } + +function qs(params: Record): string { + const u = new URLSearchParams() + for (const [k, v] of Object.entries(params)) if (v !== undefined && v !== '') u.set(k, String(v)) + const s = u.toString() + return s ? `?${s}` : '' +} + +// Page size the listing route accepts. Junk must fail here, not travel as `limit=NaN`. +export function parseObjectLimit(raw: string): number { + const n = Number(raw) + if (!Number.isInteger(n) || n < 1 || n > 1000) throw new Error(`--limit must be an integer 1..1000, got: ${raw}`) + return n +} + +type ObjectParams = { branch?: string; prefix?: string; cursor?: string; limit?: number; key?: string } + +// pure: platform path for the objects collection — GET lists it, DELETE removes one `key`. +export function objectsPath(projectId: string, serviceId: string, params: ObjectParams): string { + const { limit, ...rest } = params + return `/projects/${projectId}/services/${serviceId}/objects${qs({ ...rest, limit: limit === undefined ? undefined : String(limit) })}` +} + +// pure: the presign route — a static subpath, so keys containing `/` stay in the query. +export function objectDownloadPath(projectId: string, serviceId: string, params: { branch?: string; key: string }): string { + return `/projects/${projectId}/services/${serviceId}/objects/download${qs(params)}` +} + +// pure: one `storage list` row, size-first so the columns line up over variable-length keys. +export function objectListLine(o: { key: string; size?: number; lastModified?: string }): string { + const size = typeof o.size === 'number' ? fmtBytes(o.size) : '—' + return `${size.padStart(10)} ${(o.lastModified ?? '—').padEnd(24)} ${o.key}` +} + +// Resolve the branch's storage service (named, or the sole one) — its bucket is what we browse. +async function storageTarget(api: ApiClient, projectId: string, branch: string | undefined, name?: string): Promise<{ id: string; name: string }> { + const { services } = await api.request('GET', `/projects/${projectId}/services${q(branch)}`) + return resolveSoleService(services as Array<{ id: string; type: string; name: string }>, 'storage', name) +} + +type ListOpts = Common & { prefix?: string; cursor?: string; limit?: string } + +// S3 filters by prefix only — there is no substring search, so `--prefix` is the query surface. +export async function storageList(opts: ListOpts): Promise { + const limit = opts.limit === undefined ? undefined : parseObjectLimit(opts.limit) + const api = await ApiClient.load() + const p = await requireProject() + const branch = opts.branch ?? p.branch + const svc = await storageTarget(api, p.projectId, branch, opts.service) + const res = await api.rawRequest('GET', objectsPath(p.projectId, svc.id, { branch, prefix: opts.prefix, cursor: opts.cursor, limit })) + if (handleApproval(res)) return + if (opts.json) return printJson(res.body) + const objects: Array<{ key: string; size?: number; lastModified?: string }> = res.body.objects ?? [] + if (!objects.length) { + return info(opts.prefix ? `(no objects under prefix ${opts.prefix} in storage/${svc.name})` : `(storage/${svc.name} is empty)`) + } + for (const o of objects) info(objectListLine(o)) + if (res.body.nextCursor) info(` (more — next page: insta storage list --cursor ${res.body.nextCursor})`) +} + +export type GetDeps = { fetchBytes?: (url: string) => Promise; writeImpl?: (path: string, data: Uint8Array) => Promise } + +// pure: where the bytes land. Only the key's LAST segment is used, so no key can escape cwd. +export function outputPath(key: string, output?: string): string { + if (output) return output + const base = key.split('/').pop() ?? '' + if (!base) throw new Error(`cannot infer a filename from key "${key}" — pass -o `) + return base +} + +// Pull the bytes from the provider (never through the platform, which only signs the URL). +export async function fetchPresigned(url: string, fetchImpl: typeof fetch = fetch): Promise { + const res = await fetchImpl(url) + if (!res.ok) throw new Error(`download failed: HTTP ${res.status} (a presigned URL lives ~60s — re-run to mint a fresh one)`) + return new Uint8Array(await res.arrayBuffer()) +} + +// Core, dependency-injected for tests (mirrors runWithSecrets): fetch → write, return byte count. +export async function saveObject(url: string, out: string, deps: GetDeps = {}): Promise { + const bytes = await (deps.fetchBytes ?? fetchPresigned)(url) + await (deps.writeImpl ?? writeFile)(out, bytes) + return bytes.byteLength +} + +type GetOpts = Common & { output?: string } + +export async function storageGet(key: string, opts: GetOpts, deps: GetDeps = {}): Promise { + if (!key) throw new Error('key is required') + const out = outputPath(key, opts.output) + const api = await ApiClient.load() + const p = await requireProject() + const branch = opts.branch ?? p.branch + const svc = await storageTarget(api, p.projectId, branch, opts.service) + const res = await api.rawRequest('GET', objectDownloadPath(p.projectId, svc.id, { branch, key })) + if (handleApproval(res)) return + // --json hands over the presigned URL instead of downloading, as `insta secrets --json` does. + if (opts.json) return printJson(res.body) + const bytes = await saveObject(res.body.url, out, deps) + info(`wrote ${fmtBytes(bytes)} to ${out} (${key} from storage/${svc.name}, branch ${branch})`) +} + +// No prompt, matching every other destructive command here — the governance gate is the guard. +export async function storageDelete(key: string, opts: Common): Promise { + if (!key) throw new Error('key is required') + const api = await ApiClient.load() + const p = await requireProject() + const branch = opts.branch ?? p.branch + const svc = await storageTarget(api, p.projectId, branch, opts.service) + const res = await api.rawRequest('DELETE', objectsPath(p.projectId, svc.id, { branch, key })) + if (handleApproval(res)) return + if (opts.json) return printJson(res.body) + info(`deleted ${key} from storage/${svc.name} (branch ${branch})`) +} diff --git a/src/index.ts b/src/index.ts index 35c8e49..1af52bd 100644 --- a/src/index.ts +++ b/src/index.ts @@ -18,6 +18,7 @@ import * as secretsCmd from './commands/secrets.js' import { deploy } from './commands/deploy.js' import * as computeCmd from './commands/compute.js' import * as dbCmd from './commands/db.js' +import * as storageCmd from './commands/storage.js' import { manifest } from './commands/manifest.js' import * as govern from './commands/govern.js' import * as observe from './commands/observe.js' @@ -204,6 +205,26 @@ db.command('volume').description("Show or grow a postgres service's provisioned .option('--json').option('--branch ', 'branch (default: current)').option('--group ', 'postgres service name (default: the sole/default one)') .action(guard((o) => dbCmd.dbVolume(o))) +// ---- storage (bucket objects) ---- +const storage = program.command('storage').description("Browse, download, and delete a storage service's bucket objects") +storage.command('list').description("List the bucket's objects. S3 filters by prefix only — there is no substring search") + .option('--prefix

', 'only keys starting with this prefix (applied server-side)') + .option('--cursor ', 'continue from the nextCursor a previous page printed') + .option('--limit ', 'page size, 1..1000 (default 100)') + .option('--service ', 'storage service (default: the sole one on the branch)') + .option('--branch ', 'branch (default: current)').option('--json') + .action(guard((o) => storageCmd.storageList(o))) +storage.command('get ').description('Download one object to disk through a short-lived presigned URL (bytes come straight from the provider)') + .option('-o, --output ', "output file (default: the key's last segment)") + .option('--service ', 'storage service (default: the sole one on the branch)') + .option('--branch ', 'branch (default: current)') + .option('--json', 'print the presigned URL + expiry instead of downloading') + .action(guard((key, o) => storageCmd.storageGet(key, o))) +storage.command('delete ').description('DELETES one object from the bucket immediately — no undo, and an already-gone key still reports success (gated: storage.delete)') + .option('--service ', 'storage service (default: the sole one on the branch)') + .option('--branch ', 'branch (default: current)').option('--json') + .action(guard((key, o) => storageCmd.storageDelete(key, o))) + // ---- manifest ---- program.command('manifest').description('Print an agent-legible view of the project environments').option('--json').action(guard((o) => manifest(o))) @@ -249,7 +270,7 @@ ob.command('sync').description('Upload findings into the project timeline').acti // ---- policy ---- const pol = program.command('policy').description('Governance policy') pol.command('get').option('--json').action(guard((o) => govern.policyGet(o))) -pol.command('set ').description('action: secrets.read|secrets.write|deploy|project.delete|branch.delete|service.add|service.remove|service.scale|service.upgrade|service.setAccess; decision: allow|deny|approve').action(guard((a, d) => govern.policySet(a, d))) +pol.command('set ').description('action: secrets.read|secrets.write|deploy|project.delete|branch.delete|service.add|service.remove|service.scale|service.upgrade|service.setAccess|storage.read|storage.delete; decision: allow|deny|approve').action(guard((a, d) => govern.policySet(a, d))) // ---- self-update ---- program.command('upgrade').description('Update the insta CLI to the latest release (binary or npm install)') diff --git a/test/storage.test.ts b/test/storage.test.ts new file mode 100644 index 0000000..bc48c6c --- /dev/null +++ b/test/storage.test.ts @@ -0,0 +1,131 @@ +// `insta storage` seams — all pure or DI'd, so nothing here reaches a backend. +import { describe, it, expect } from 'vitest' +import { mkdtempSync, readFileSync } from 'node:fs' +import { tmpdir } from 'node:os' +import { join } from 'node:path' +import { + parseObjectLimit, objectsPath, objectDownloadPath, objectListLine, + outputPath, fetchPresigned, saveObject, +} from '../src/commands/storage.js' +import { resolveSoleService } from '../src/commands/services.js' + +describe('parseObjectLimit', () => { + it('accepts the route range', () => { + expect(parseObjectLimit('1')).toBe(1) + expect(parseObjectLimit('100')).toBe(100) + expect(parseObjectLimit('1000')).toBe(1000) + }) + it('rejects out-of-range and junk locally, before any request', () => { + for (const raw of ['0', '-1', '1001', '2.5', '', 'lots']) { + expect(() => parseObjectLimit(raw), raw).toThrow(/1\.\.1000/) + } + }) +}) + +describe('objectsPath', () => { + it('builds the listing query, omitting absent params', () => { + expect(objectsPath('pr_1', 'svc_1', { branch: 'main', prefix: 'docs/', limit: 50 })) + .toBe('/projects/pr_1/services/svc_1/objects?branch=main&prefix=docs%2F&limit=50') + expect(objectsPath('pr_1', 'svc_1', {})).toBe('/projects/pr_1/services/svc_1/objects') + }) + it('carries the cursor for the next page', () => { + expect(objectsPath('pr_1', 'svc_1', { cursor: 'tok/en+1' })).toContain('cursor=tok%2Fen%2B1') + }) + it('is also the DELETE target, with the key in the query — never a path segment', () => { + const path = objectsPath('pr_1', 'svc_1', { branch: 'main', key: 'a/b/c.txt' }) + expect(path).toBe('/projects/pr_1/services/svc_1/objects?branch=main&key=a%2Fb%2Fc.txt') + }) + // Keys are arbitrary bytes; an unencoded `&` or `#` would break the query. + it('encodes awkward keys and prefixes (& # space non-ASCII)', () => { + expect(objectsPath('pr_1', 'svc_1', { key: 'a&b #1 café.png' })) + .toBe('/projects/pr_1/services/svc_1/objects?key=a%26b+%231+caf%C3%A9.png') + }) +}) + +describe('objectDownloadPath', () => { + it('is a static subpath of the collection, so the listing route cannot shadow it', () => { + expect(objectDownloadPath('pr_1', 'svc_1', { branch: 'main', key: 'a/b.txt' })) + .toBe('/projects/pr_1/services/svc_1/objects/download?branch=main&key=a%2Fb.txt') + }) + it('omits branch when the project default is wanted', () => { + expect(objectDownloadPath('pr_1', 'svc_1', { key: 'x.txt' })) + .toBe('/projects/pr_1/services/svc_1/objects/download?key=x.txt') + }) +}) + +describe('objectListLine', () => { + it('renders size, modified, key with the fixed-width columns aligned', () => { + expect(objectListLine({ key: 'docs/a.pdf', size: 8_000_000, lastModified: '2026-08-12T10:00:00.000Z' })) + .toBe(' 7.6 MiB 2026-08-12T10:00:00.000Z docs/a.pdf') + }) + it('never fakes a zero for a field the platform omitted', () => { + expect(objectListLine({ key: 'x' })).toBe(' — — x') + }) +}) + +describe('outputPath', () => { + it('defaults to the last segment of the key', () => { + expect(outputPath('docs/reports/q3.pdf')).toBe('q3.pdf') + expect(outputPath('flat.txt')).toBe('flat.txt') + }) + it('-o wins verbatim', () => { + expect(outputPath('docs/q3.pdf', 'out/other.pdf')).toBe('out/other.pdf') + }) + // Using only the last segment is what makes a hostile key harmless — nothing escapes cwd. + it('cannot be steered out of cwd by a traversal key', () => { + expect(outputPath('../../etc/passwd')).toBe('passwd') + expect(outputPath('/etc/passwd')).toBe('passwd') + }) + it('asks for -o when the key has no filename', () => { + expect(() => outputPath('docs/')).toThrow(/pass -o/) + expect(() => outputPath('')).toThrow(/pass -o/) + }) +}) + +describe('fetchPresigned', () => { + it('returns the provider bytes on 200', async () => { + const fake = (async () => new Response(new Uint8Array([1, 2, 3]))) as unknown as typeof fetch + expect(Array.from(await fetchPresigned('https://provider/x', fake))).toEqual([1, 2, 3]) + }) + // A 60s TTL means an expired link is the likely failure, so say what to do about it. + it('names the expiry as the likely cause when the provider refuses', async () => { + const fake = (async () => new Response('', { status: 403 })) as unknown as typeof fetch + await expect(fetchPresigned('https://provider/x', fake)).rejects.toThrow(/presigned URL lives ~60s/) + }) +}) + +describe('saveObject', () => { + it('writes the fetched bytes to the given path and reports the size', async () => { + const dir = mkdtempSync(join(tmpdir(), 'insta-storage-')) + const out = join(dir, 'q3.pdf') + const n = await saveObject('https://provider/q3.pdf', out, { + fetchBytes: async () => new Uint8Array([0x25, 0x50, 0x44, 0x46]), + }) + expect(n).toBe(4) + expect(readFileSync(out).toString('latin1')).toBe('%PDF') + }) + it('writes nothing when the fetch fails', async () => { + let wrote = false + await expect(saveObject('https://provider/x', 'x', { + fetchBytes: async () => { throw new Error('boom') }, + writeImpl: async () => { wrote = true }, + })).rejects.toThrow('boom') + expect(wrote).toBe(false) + }) +}) + +describe('resolveSoleService (storage)', () => { + const one = [{ id: 'a', type: 'postgres', name: 'db' }, { id: 'b', type: 'storage', name: 'files' }] + const two = [...one, { id: 'c', type: 'storage', name: 'assets' }] + it('returns the sole storage service when --service is omitted', () => { + expect(resolveSoleService(one, 'storage').id).toBe('b') + }) + it('resolves by name and lists the choices when ambiguous', () => { + expect(resolveSoleService(two, 'storage', 'assets').id).toBe('c') + expect(() => resolveSoleService(two, 'storage')).toThrow(/multiple storage services — specify one: files, assets/) + }) + it('errors when the branch has no storage service, pointing at `services add`', () => { + expect(() => resolveSoleService([one[0]!], 'storage')).toThrow(/insta services add storage /) + expect(() => resolveSoleService(two, 'storage', 'nope')).toThrow(/storage service not found: nope/) + }) +}) From 97b310c26850c12271509854dfd7af8978ad3515 Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Fri, 14 Aug 2026 11:15:08 -0700 Subject: [PATCH 2/6] =?UTF-8?q?fix(storage):=20address=20review=20?= =?UTF-8?q?=E2=80=94=20Windows=20paths,=20streaming,=20and=20page=20filter?= =?UTF-8?q?s?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings from review, all valid. `outputPath` split on `/` only, so a key holding backslashes kept them and, on Windows, `..\..\Windows\...\hosts` would have escaped the working directory. It now splits on both separators — the traversal guard has to cover the platform the CLI actually runs on. Downloads buffered the whole object through `arrayBuffer()` before writing, which kills the process rather than producing a file once an object outgrows memory. `streamPresignedTo` pipes the provider's body straight to disk and counts bytes on the way through, and removes the partial file if the stream fails — a truncated file must not pass for a finished download. The DI seam moves from fetch-bytes/write to stream-to-path accordingly. The "next page" hint printed a bare `--cursor`, so following it after `--prefix docs/` paged through a different set of objects. `nextPageCommand` repeats every filter that shaped the page. Also: `res.body` is dereferenced defensively now that an empty 2xx body parses to null; `--json` no longer requires a key it can infer a filename from, since it writes nothing; and the test's `mkdtempSync` directory is removed rather than leaked once per run. `policy set` help lists `storage.write`, which the merged platform now advertises alongside read and delete. --- src/commands/storage.ts | 59 ++++++++++++++++++++------- src/index.ts | 2 +- test/storage.test.ts | 88 ++++++++++++++++++++++++++++++----------- 3 files changed, 111 insertions(+), 38 deletions(-) diff --git a/src/commands/storage.ts b/src/commands/storage.ts index 48eb0df..fa6bca0 100644 --- a/src/commands/storage.ts +++ b/src/commands/storage.ts @@ -1,5 +1,8 @@ // `insta storage` — browse, download, and delete the objects in a storage service's bucket. -import { writeFile } from 'node:fs/promises' +import { rm } from 'node:fs/promises' +import { createWriteStream } from 'node:fs' +import { Readable } from 'node:stream' +import { pipeline } from 'node:stream/promises' import { ApiClient, requireProject } from '../api.js' import { info, printJson, handleApproval } from '../util.js' import { q, resolveSoleService } from './services.js' @@ -58,43 +61,66 @@ export async function storageList(opts: ListOpts): Promise { const res = await api.rawRequest('GET', objectsPath(p.projectId, svc.id, { branch, prefix: opts.prefix, cursor: opts.cursor, limit })) if (handleApproval(res)) return if (opts.json) return printJson(res.body) - const objects: Array<{ key: string; size?: number; lastModified?: string }> = res.body.objects ?? [] + const objects: Array<{ key: string; size?: number; lastModified?: string }> = res.body?.objects ?? [] if (!objects.length) { return info(opts.prefix ? `(no objects under prefix ${opts.prefix} in storage/${svc.name})` : `(storage/${svc.name} is empty)`) } for (const o of objects) info(objectListLine(o)) - if (res.body.nextCursor) info(` (more — next page: insta storage list --cursor ${res.body.nextCursor})`) + const next = res.body?.nextCursor + if (next) info(` (more — next page: ${nextPageCommand({ ...opts, limit }, next)})`) } -export type GetDeps = { fetchBytes?: (url: string) => Promise; writeImpl?: (path: string, data: Uint8Array) => Promise } +// The continuation command must repeat the filters, or following it lists a different set. +export function nextPageCommand(opts: { branch?: string; service?: string; prefix?: string; limit?: number }, cursor: string): string { + const flags = [ + opts.service ? `--service ${opts.service}` : '', + opts.branch ? `--branch ${opts.branch}` : '', + opts.prefix ? `--prefix ${opts.prefix}` : '', + opts.limit === undefined ? '' : `--limit ${opts.limit}`, + `--cursor ${cursor}`, + ].filter(Boolean) + return `insta storage list ${flags.join(' ')}` +} + +export type GetDeps = { streamTo?: (url: string, out: string) => Promise } -// pure: where the bytes land. Only the key's LAST segment is used, so no key can escape cwd. +// pure: where the bytes land. Only the last segment is used, so no key can escape cwd. export function outputPath(key: string, output?: string): string { if (output) return output - const base = key.split('/').pop() ?? '' + // Split on `\` too: a key may contain one, and on Windows that is also a separator. + const base = key.split(/[\\/]/).pop() ?? '' if (!base) throw new Error(`cannot infer a filename from key "${key}" — pass -o `) return base } -// Pull the bytes from the provider (never through the platform, which only signs the URL). -export async function fetchPresigned(url: string, fetchImpl: typeof fetch = fetch): Promise { +// Stream from the provider (never through the platform, which only signs) straight to disk, so a +// multi-gigabyte object never has to fit in memory. Returns the byte count written. +export async function streamPresignedTo(url: string, out: string, fetchImpl: typeof fetch = fetch): Promise { const res = await fetchImpl(url) - if (!res.ok) throw new Error(`download failed: HTTP ${res.status} (a presigned URL lives ~60s — re-run to mint a fresh one)`) - return new Uint8Array(await res.arrayBuffer()) + if (!res.ok || !res.body) throw new Error(`download failed: HTTP ${res.status} (a presigned URL lives ~60s — re-run to mint a fresh one)`) + let written = 0 + const counting = new TransformStream({ + transform(chunk, controller) { written += chunk.byteLength; controller.enqueue(chunk) }, + }) + // A failed write must not leave a truncated file passing for a complete download. + try { + await pipeline(Readable.fromWeb(res.body.pipeThrough(counting) as ReadableStream), createWriteStream(out)) + } catch (e) { + await rm(out, { force: true }) + throw e + } + return written } -// Core, dependency-injected for tests (mirrors runWithSecrets): fetch → write, return byte count. +// Core, dependency-injected for tests (mirrors runWithSecrets): stream → disk, return byte count. export async function saveObject(url: string, out: string, deps: GetDeps = {}): Promise { - const bytes = await (deps.fetchBytes ?? fetchPresigned)(url) - await (deps.writeImpl ?? writeFile)(out, bytes) - return bytes.byteLength + return (deps.streamTo ?? streamPresignedTo)(url, out) } type GetOpts = Common & { output?: string } export async function storageGet(key: string, opts: GetOpts, deps: GetDeps = {}): Promise { if (!key) throw new Error('key is required') - const out = outputPath(key, opts.output) const api = await ApiClient.load() const p = await requireProject() const branch = opts.branch ?? p.branch @@ -102,7 +128,10 @@ export async function storageGet(key: string, opts: GetOpts, deps: GetDeps = {}) const res = await api.rawRequest('GET', objectDownloadPath(p.projectId, svc.id, { branch, key })) if (handleApproval(res)) return // --json hands over the presigned URL instead of downloading, as `insta secrets --json` does. + // Before outputPath, so a key with no filename still works when nothing is written to disk. if (opts.json) return printJson(res.body) + const out = outputPath(key, opts.output) + if (!res.body?.url) throw new Error('the platform returned no download URL') const bytes = await saveObject(res.body.url, out, deps) info(`wrote ${fmtBytes(bytes)} to ${out} (${key} from storage/${svc.name}, branch ${branch})`) } diff --git a/src/index.ts b/src/index.ts index 1af52bd..d971de1 100644 --- a/src/index.ts +++ b/src/index.ts @@ -270,7 +270,7 @@ ob.command('sync').description('Upload findings into the project timeline').acti // ---- policy ---- const pol = program.command('policy').description('Governance policy') pol.command('get').option('--json').action(guard((o) => govern.policyGet(o))) -pol.command('set ').description('action: secrets.read|secrets.write|deploy|project.delete|branch.delete|service.add|service.remove|service.scale|service.upgrade|service.setAccess|storage.read|storage.delete; decision: allow|deny|approve').action(guard((a, d) => govern.policySet(a, d))) +pol.command('set ').description('action: secrets.read|secrets.write|deploy|project.delete|branch.delete|service.add|service.remove|service.scale|service.upgrade|service.setAccess|storage.read|storage.write|storage.delete; decision: allow|deny|approve').action(guard((a, d) => govern.policySet(a, d))) // ---- self-update ---- program.command('upgrade').description('Update the insta CLI to the latest release (binary or npm install)') diff --git a/test/storage.test.ts b/test/storage.test.ts index bc48c6c..df64b77 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -1,11 +1,11 @@ // `insta storage` seams — all pure or DI'd, so nothing here reaches a backend. -import { describe, it, expect } from 'vitest' -import { mkdtempSync, readFileSync } from 'node:fs' +import { describe, it, expect, beforeEach, afterEach } from 'vitest' +import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { parseObjectLimit, objectsPath, objectDownloadPath, objectListLine, - outputPath, fetchPresigned, saveObject, + outputPath, streamPresignedTo, saveObject, nextPageCommand, } from '../src/commands/storage.js' import { resolveSoleService } from '../src/commands/services.js' @@ -76,41 +76,85 @@ describe('outputPath', () => { expect(outputPath('../../etc/passwd')).toBe('passwd') expect(outputPath('/etc/passwd')).toBe('passwd') }) + // A key may hold a backslash, which is also a separator on Windows. + it('treats a backslash as a separator too', () => { + expect(outputPath('..\\..\\Windows\\system32\\drivers\\etc\\hosts')).toBe('hosts') + expect(outputPath('docs\\q3.pdf')).toBe('q3.pdf') + }) it('asks for -o when the key has no filename', () => { expect(() => outputPath('docs/')).toThrow(/pass -o/) expect(() => outputPath('')).toThrow(/pass -o/) }) }) -describe('fetchPresigned', () => { - it('returns the provider bytes on 200', async () => { - const fake = (async () => new Response(new Uint8Array([1, 2, 3]))) as unknown as typeof fetch - expect(Array.from(await fetchPresigned('https://provider/x', fake))).toEqual([1, 2, 3]) +describe('nextPageCommand', () => { + // Following a command that dropped --prefix would page through a different set of objects. + it('repeats every filter that shaped the page', () => { + expect(nextPageCommand({ branch: 'feat-x', service: 'files', prefix: 'docs/', limit: 25 }, 'tok-2')) + .toBe('insta storage list --service files --branch feat-x --prefix docs/ --limit 25 --cursor tok-2') + }) + it('omits the flags that were never given', () => { + expect(nextPageCommand({}, 'tok-2')).toBe('insta storage list --cursor tok-2') + }) +}) + +describe('streamPresignedTo', () => { + let dir: string + beforeEach(() => { dir = mkdtempSync(join(tmpdir(), 'insta-storage-')) }) + // mkdtemp leaks a directory per run without this, including every CI pass. + afterEach(() => rmSync(dir, { recursive: true, force: true })) + + it('streams the provider body to disk and reports the byte count', async () => { + const out = join(dir, 'q3.pdf') + const fake = (async () => new Response(new Uint8Array([0x25, 0x50, 0x44, 0x46]))) as unknown as typeof fetch + expect(await streamPresignedTo('https://provider/q3.pdf', out, fake)).toBe(4) + expect(readFileSync(out).toString('latin1')).toBe('%PDF') + }) + + // The point of streaming: a body larger than memory must still land, chunk by chunk. + it('never holds the whole object at once', async () => { + const out = join(dir, 'big.bin') + const chunk = new Uint8Array(64 * 1024) + let queued = 0 + const body = new ReadableStream({ + pull(controller) { + if (queued++ >= 200) return controller.close() + controller.enqueue(chunk) + }, + }) + const fake = (async () => new Response(body)) as unknown as typeof fetch + expect(await streamPresignedTo('https://provider/big.bin', out, fake)).toBe(200 * chunk.byteLength) }) + // A 60s TTL means an expired link is the likely failure, so say what to do about it. it('names the expiry as the likely cause when the provider refuses', async () => { const fake = (async () => new Response('', { status: 403 })) as unknown as typeof fetch - await expect(fetchPresigned('https://provider/x', fake)).rejects.toThrow(/presigned URL lives ~60s/) + await expect(streamPresignedTo('https://provider/x', join(dir, 'x'), fake)).rejects.toThrow(/presigned URL lives ~60s/) + }) + + // A half-written file must not pass for a finished download. + it('removes the partial file when the stream fails mid-way', async () => { + const out = join(dir, 'partial.bin') + const body = new ReadableStream({ + start(controller) { + controller.enqueue(new Uint8Array([1, 2, 3])) + controller.error(new Error('connection reset')) + }, + }) + const fake = (async () => new Response(body)) as unknown as typeof fetch + await expect(streamPresignedTo('https://provider/partial.bin', out, fake)).rejects.toThrow(/connection reset/) + expect(existsSync(out)).toBe(false) }) }) describe('saveObject', () => { - it('writes the fetched bytes to the given path and reports the size', async () => { - const dir = mkdtempSync(join(tmpdir(), 'insta-storage-')) - const out = join(dir, 'q3.pdf') - const n = await saveObject('https://provider/q3.pdf', out, { - fetchBytes: async () => new Uint8Array([0x25, 0x50, 0x44, 0x46]), + it('delegates to the injected streamer and returns its count', async () => { + const seen: string[] = [] + const n = await saveObject('https://provider/q3.pdf', 'out.pdf', { + streamTo: async (url, out) => { seen.push(url, out); return 4 }, }) expect(n).toBe(4) - expect(readFileSync(out).toString('latin1')).toBe('%PDF') - }) - it('writes nothing when the fetch fails', async () => { - let wrote = false - await expect(saveObject('https://provider/x', 'x', { - fetchBytes: async () => { throw new Error('boom') }, - writeImpl: async () => { wrote = true }, - })).rejects.toThrow('boom') - expect(wrote).toBe(false) + expect(seen).toEqual(['https://provider/q3.pdf', 'out.pdf']) }) }) From d162eb98052f52cec40693780d34a7c0e10978df Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Fri, 14 Aug 2026 11:45:26 -0700 Subject: [PATCH 3/6] fix(storage): download to a temp file, and quote the page hint Both from review, and the first one is a regression the streaming change introduced. `createWriteStream(out)` truncates on open, so `-o important.pdf` on a download that then failed left the cleanup deleting the user's existing file. The buffered version it replaced never touched the target until it had all the bytes, so the streaming fix traded a truncated file for data loss. Bytes now land in a `.insta-part-` file beside the target and are renamed over it only once the pipeline completes; the part file is what gets removed on failure. Same directory, so the rename is atomic rather than a cross-device copy. Two tests pin it: an existing target still holds its original contents after a mid-stream failure with no part file left behind, and a successful download replaces it. `nextPageCommand` interpolated values raw, so a prefix holding a space or `&` printed something that is not a runnable command. Values a shell would reinterpret are single-quoted now, embedded quotes included. --- src/commands/storage.ts | 25 +++++++++++++++-------- test/storage.test.ts | 45 +++++++++++++++++++++++++++++++++-------- 2 files changed, 54 insertions(+), 16 deletions(-) diff --git a/src/commands/storage.ts b/src/commands/storage.ts index fa6bca0..15e00f9 100644 --- a/src/commands/storage.ts +++ b/src/commands/storage.ts @@ -1,6 +1,7 @@ // `insta storage` — browse, download, and delete the objects in a storage service's bucket. -import { rm } from 'node:fs/promises' +import { rename, rm } from 'node:fs/promises' import { createWriteStream } from 'node:fs' +import { randomBytes } from 'node:crypto' import { Readable } from 'node:stream' import { pipeline } from 'node:stream/promises' import { ApiClient, requireProject } from '../api.js' @@ -70,14 +71,19 @@ export async function storageList(opts: ListOpts): Promise { if (next) info(` (more — next page: ${nextPageCommand({ ...opts, limit }, next)})`) } +// Single-quote anything a shell would reinterpret; a prefix or cursor may hold spaces, & or $. +function shellQuote(value: string): string { + return /^[\w./:@=+-]+$/.test(value) ? value : `'${value.replace(/'/g, `'\\''`)}'` +} + // The continuation command must repeat the filters, or following it lists a different set. export function nextPageCommand(opts: { branch?: string; service?: string; prefix?: string; limit?: number }, cursor: string): string { const flags = [ - opts.service ? `--service ${opts.service}` : '', - opts.branch ? `--branch ${opts.branch}` : '', - opts.prefix ? `--prefix ${opts.prefix}` : '', + opts.service ? `--service ${shellQuote(opts.service)}` : '', + opts.branch ? `--branch ${shellQuote(opts.branch)}` : '', + opts.prefix ? `--prefix ${shellQuote(opts.prefix)}` : '', opts.limit === undefined ? '' : `--limit ${opts.limit}`, - `--cursor ${cursor}`, + `--cursor ${shellQuote(cursor)}`, ].filter(Boolean) return `insta storage list ${flags.join(' ')}` } @@ -102,11 +108,14 @@ export async function streamPresignedTo(url: string, out: string, fetchImpl: typ const counting = new TransformStream({ transform(chunk, controller) { written += chunk.byteLength; controller.enqueue(chunk) }, }) - // A failed write must not leave a truncated file passing for a complete download. + // Write beside the target, then rename: opening `out` directly would truncate an existing file + // that a failed download then deletes. Same directory keeps the rename atomic. + const part = `${out}.insta-part-${randomBytes(4).toString('hex')}` try { - await pipeline(Readable.fromWeb(res.body.pipeThrough(counting) as ReadableStream), createWriteStream(out)) + await pipeline(Readable.fromWeb(res.body.pipeThrough(counting) as ReadableStream), createWriteStream(part)) + await rename(part, out) } catch (e) { - await rm(out, { force: true }) + await rm(part, { force: true }) throw e } return written diff --git a/test/storage.test.ts b/test/storage.test.ts index df64b77..7356fdc 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -1,6 +1,6 @@ // `insta storage` seams — all pure or DI'd, so nothing here reaches a backend. import { describe, it, expect, beforeEach, afterEach } from 'vitest' -import { existsSync, mkdtempSync, readFileSync, rmSync } from 'node:fs' +import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { @@ -96,8 +96,24 @@ describe('nextPageCommand', () => { it('omits the flags that were never given', () => { expect(nextPageCommand({}, 'tok-2')).toBe('insta storage list --cursor tok-2') }) + // An unquoted prefix with a space or & would not survive a paste into a shell. + it('quotes values a shell would reinterpret', () => { + expect(nextPageCommand({ prefix: 'my docs/a&b' }, 'tok-2')) + .toBe("insta storage list --prefix 'my docs/a&b' --cursor tok-2") + expect(nextPageCommand({ prefix: "it's" }, 'tok-2')) + .toBe("insta storage list --prefix 'it'\\''s' --cursor tok-2") + }) }) +// A body that hands over some bytes and then dies, as a dropped connection would. +const failingBody = () => + new ReadableStream({ + start(controller) { + controller.enqueue(new Uint8Array([1, 2, 3])) + controller.error(new Error('connection reset')) + }, + }) + describe('streamPresignedTo', () => { let dir: string beforeEach(() => { dir = mkdtempSync(join(tmpdir(), 'insta-storage-')) }) @@ -135,16 +151,29 @@ describe('streamPresignedTo', () => { // A half-written file must not pass for a finished download. it('removes the partial file when the stream fails mid-way', async () => { const out = join(dir, 'partial.bin') - const body = new ReadableStream({ - start(controller) { - controller.enqueue(new Uint8Array([1, 2, 3])) - controller.error(new Error('connection reset')) - }, - }) - const fake = (async () => new Response(body)) as unknown as typeof fetch + const fake = (async () => new Response(failingBody())) as unknown as typeof fetch await expect(streamPresignedTo('https://provider/partial.bin', out, fake)).rejects.toThrow(/connection reset/) expect(existsSync(out)).toBe(false) }) + + // Opening `out` directly would truncate it, so a failed download used to destroy the old file. + it('leaves an existing -o target untouched when the download fails', async () => { + const out = join(dir, 'important.pdf') + writeFileSync(out, 'ORIGINAL') + const fake = (async () => new Response(failingBody())) as unknown as typeof fetch + await expect(streamPresignedTo('https://provider/x', out, fake)).rejects.toThrow(/connection reset/) + expect(readFileSync(out).toString()).toBe('ORIGINAL') + expect(readdirSync(dir)).toEqual(['important.pdf']) + }) + + it('replaces an existing target once the download completes', async () => { + const out = join(dir, 'report.txt') + writeFileSync(out, 'OLD') + const fake = (async () => new Response(new TextEncoder().encode('NEW'))) as unknown as typeof fetch + expect(await streamPresignedTo('https://provider/report.txt', out, fake)).toBe(3) + expect(readFileSync(out).toString()).toBe('NEW') + expect(readdirSync(dir)).toEqual(['report.txt']) + }) }) describe('saveObject', () => { From 60803e9b5b565d01dc723daeb23c86091cb6cf18 Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Fri, 14 Aug 2026 11:52:15 -0700 Subject: [PATCH 4/6] fix(storage): keep the replaced file's mode, and sweep the part on Ctrl-C MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two more from review, both consequences of writing to a part file and renaming. The part file is created at the umask default, so renaming it over a `0600` target silently widened that file to `0644`. It now inherits the target's mode before the rename, when a target exists. A test pins it: replacing a `0600` file leaves it `0600`. Ctrl-C kills the process without unwinding, so the `catch` never ran and a `.insta-part-*` stayed behind in the user's directory — worse than a temp dir because that is where they are working. SIGINT and SIGTERM now remove it synchronously and exit 130; the handlers are detached in a `finally` so repeated downloads do not stack listeners. --- src/commands/storage.ts | 14 ++++++++++++-- test/storage.test.ts | 11 ++++++++++- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/commands/storage.ts b/src/commands/storage.ts index 15e00f9..8d221d0 100644 --- a/src/commands/storage.ts +++ b/src/commands/storage.ts @@ -1,6 +1,6 @@ // `insta storage` — browse, download, and delete the objects in a storage service's bucket. -import { rename, rm } from 'node:fs/promises' -import { createWriteStream } from 'node:fs' +import { chmod, rename, rm, stat } from 'node:fs/promises' +import { createWriteStream, rmSync } from 'node:fs' import { randomBytes } from 'node:crypto' import { Readable } from 'node:stream' import { pipeline } from 'node:stream/promises' @@ -111,12 +111,22 @@ export async function streamPresignedTo(url: string, out: string, fetchImpl: typ // Write beside the target, then rename: opening `out` directly would truncate an existing file // that a failed download then deletes. Same directory keeps the rename atomic. const part = `${out}.insta-part-${randomBytes(4).toString('hex')}` + // Ctrl-C kills the process without unwinding, so the part file needs a synchronous sweep. + const onSignal = () => { rmSync(part, { force: true }); process.exit(130) } + process.once('SIGINT', onSignal) + process.once('SIGTERM', onSignal) try { await pipeline(Readable.fromWeb(res.body.pipeThrough(counting) as ReadableStream), createWriteStream(part)) + // Replacing a 0600 file must not widen it to the umask default the part was created with. + const mode = await stat(out).then((s) => s.mode, () => undefined) + if (mode !== undefined) await chmod(part, mode) await rename(part, out) } catch (e) { await rm(part, { force: true }) throw e + } finally { + process.off('SIGINT', onSignal) + process.off('SIGTERM', onSignal) } return written } diff --git a/test/storage.test.ts b/test/storage.test.ts index 7356fdc..9703abd 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -1,6 +1,6 @@ // `insta storage` seams — all pure or DI'd, so nothing here reaches a backend. import { describe, it, expect, beforeEach, afterEach } from 'vitest' -import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs' +import { existsSync, mkdtempSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import { join } from 'node:path' import { @@ -174,6 +174,15 @@ describe('streamPresignedTo', () => { expect(readFileSync(out).toString()).toBe('NEW') expect(readdirSync(dir)).toEqual(['report.txt']) }) + + // The part file is born at the umask default, so replacing a private file would widen it. + it('keeps the replaced file as private as it was', async () => { + const out = join(dir, 'secret.pem') + writeFileSync(out, 'OLD', { mode: 0o600 }) + const fake = (async () => new Response(new TextEncoder().encode('NEW'))) as unknown as typeof fetch + await streamPresignedTo('https://provider/secret.pem', out, fake) + expect(statSync(out).mode & 0o777).toBe(0o600) + }) }) describe('saveObject', () => { From 8c0dc7244ffccc3e5c2c377e04fd6576f09a19e9 Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Fri, 14 Aug 2026 11:59:31 -0700 Subject: [PATCH 5/6] fix(storage): exit 128+signo, and skip the mode assertion off POSIX MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both from review. The sweep exited 130 for SIGTERM as well as SIGINT, so a supervisor that reads 143 as terminated and 130 as interrupted would misreport a download killed by systemd or CI as someone pressing Ctrl-C. The handler is now per-signal and exits 128 plus the signal number. The mode-preservation test asserts POSIX permission bits. Windows chmod only toggles the read-only attribute and stat reports 0o666 for any writable file, so that assertion cannot hold there — and CI is ubuntu, so it would only ever fail on a contributor's machine while this CLI ships a Windows binary. Skipped off POSIX rather than weakened, since the behaviour it pins is real where modes exist. --- src/commands/storage.ts | 15 +++++++++------ test/storage.test.ts | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/src/commands/storage.ts b/src/commands/storage.ts index 8d221d0..0f8dc7d 100644 --- a/src/commands/storage.ts +++ b/src/commands/storage.ts @@ -111,10 +111,13 @@ export async function streamPresignedTo(url: string, out: string, fetchImpl: typ // Write beside the target, then rename: opening `out` directly would truncate an existing file // that a failed download then deletes. Same directory keeps the rename atomic. const part = `${out}.insta-part-${randomBytes(4).toString('hex')}` - // Ctrl-C kills the process without unwinding, so the part file needs a synchronous sweep. - const onSignal = () => { rmSync(part, { force: true }); process.exit(130) } - process.once('SIGINT', onSignal) - process.once('SIGTERM', onSignal) + // A signal kills the process without unwinding, so the part file needs a synchronous sweep. + // Exit 128+signo, so a supervisor still reads interrupted (130) apart from terminated (143). + const sweep = (signo: number) => () => { rmSync(part, { force: true }); process.exit(128 + signo) } + const onInt = sweep(2) + const onTerm = sweep(15) + process.once('SIGINT', onInt) + process.once('SIGTERM', onTerm) try { await pipeline(Readable.fromWeb(res.body.pipeThrough(counting) as ReadableStream), createWriteStream(part)) // Replacing a 0600 file must not widen it to the umask default the part was created with. @@ -125,8 +128,8 @@ export async function streamPresignedTo(url: string, out: string, fetchImpl: typ await rm(part, { force: true }) throw e } finally { - process.off('SIGINT', onSignal) - process.off('SIGTERM', onSignal) + process.off('SIGINT', onInt) + process.off('SIGTERM', onTerm) } return written } diff --git a/test/storage.test.ts b/test/storage.test.ts index 9703abd..64d0d50 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -176,7 +176,8 @@ describe('streamPresignedTo', () => { }) // The part file is born at the umask default, so replacing a private file would widen it. - it('keeps the replaced file as private as it was', async () => { + // POSIX-only: Windows chmod just toggles read-only and stat reports 0o666 for any writable file. + it.skipIf(process.platform === 'win32')('keeps the replaced file as private as it was', async () => { const out = join(dir, 'secret.pem') writeFileSync(out, 'OLD', { mode: 0o600 }) const fake = (async () => new Response(new TextEncoder().encode('NEW'))) as unknown as typeof fetch From 97ce6d1eeb71715214d640499e77aa8bb02bee61 Mon Sep 17 00:00:00 2001 From: CarmenDou <15951653662@163.com> Date: Fri, 14 Aug 2026 12:03:55 -0700 Subject: [PATCH 6/6] fix(storage): stop emitting shell syntax that only works in one shell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The page hint quoted POSIX-style, so a prefix holding an apostrophe printed Bash's '\'' escape — which PowerShell cannot parse. Per-shell quoting was the obvious fix and the wrong one: detecting the shell is the unreliable part, and a wrong guess prints something broken in a different way. So the hint no longer needs quoting. Every value it would interpolate is checked against a conservative shell-safe pattern; if any of them fails, the hint becomes a sentence naming the cursor instead of a command. Base64 cursors carry only `+ / =`, which are safe unquoted, so the copy-pasteable form survives for the common case — a prefix with a space or quote is what trades it for prose. --- src/commands/storage.ts | 22 +++++++++++++--------- test/storage.test.ts | 17 ++++++++++++----- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/src/commands/storage.ts b/src/commands/storage.ts index 0f8dc7d..da8ae17 100644 --- a/src/commands/storage.ts +++ b/src/commands/storage.ts @@ -71,19 +71,23 @@ export async function storageList(opts: ListOpts): Promise { if (next) info(` (more — next page: ${nextPageCommand({ ...opts, limit }, next)})`) } -// Single-quote anything a shell would reinterpret; a prefix or cursor may hold spaces, & or $. -function shellQuote(value: string): string { - return /^[\w./:@=+-]+$/.test(value) ? value : `'${value.replace(/'/g, `'\\''`)}'` -} +// Safe unquoted in every shell we care about — no spaces, quotes, or expansion characters. +const SHELL_SAFE = /^[\w./:@=+-]+$/ -// The continuation command must repeat the filters, or following it lists a different set. +// The continuation must repeat the filters, or following it lists a different set. Quoting rules +// differ between sh and PowerShell, so rather than guess the shell, a value that would need quotes +// gets a sentence instead of syntax that is broken somewhere. export function nextPageCommand(opts: { branch?: string; service?: string; prefix?: string; limit?: number }, cursor: string): string { + const values = [opts.service, opts.branch, opts.prefix, cursor].filter((v): v is string => !!v) + if (!values.every((v) => SHELL_SAFE.test(v))) { + return `re-run this command with --cursor set to ${cursor}` + } const flags = [ - opts.service ? `--service ${shellQuote(opts.service)}` : '', - opts.branch ? `--branch ${shellQuote(opts.branch)}` : '', - opts.prefix ? `--prefix ${shellQuote(opts.prefix)}` : '', + opts.service ? `--service ${opts.service}` : '', + opts.branch ? `--branch ${opts.branch}` : '', + opts.prefix ? `--prefix ${opts.prefix}` : '', opts.limit === undefined ? '' : `--limit ${opts.limit}`, - `--cursor ${shellQuote(cursor)}`, + `--cursor ${cursor}`, ].filter(Boolean) return `insta storage list ${flags.join(' ')}` } diff --git a/test/storage.test.ts b/test/storage.test.ts index 64d0d50..0c80f21 100644 --- a/test/storage.test.ts +++ b/test/storage.test.ts @@ -96,12 +96,19 @@ describe('nextPageCommand', () => { it('omits the flags that were never given', () => { expect(nextPageCommand({}, 'tok-2')).toBe('insta storage list --cursor tok-2') }) - // An unquoted prefix with a space or & would not survive a paste into a shell. - it('quotes values a shell would reinterpret', () => { - expect(nextPageCommand({ prefix: 'my docs/a&b' }, 'tok-2')) - .toBe("insta storage list --prefix 'my docs/a&b' --cursor tok-2") + // Quoting rules differ per shell, so a value needing quotes gets prose, not broken syntax. + it('falls back to an instruction when a value would need quoting', () => { + expect(nextPageCommand({ prefix: 'my docs/' }, 'tok-2')) + .toBe('re-run this command with --cursor set to tok-2') expect(nextPageCommand({ prefix: "it's" }, 'tok-2')) - .toBe("insta storage list --prefix 'it'\\''s' --cursor tok-2") + .toBe('re-run this command with --cursor set to tok-2') + expect(nextPageCommand({ prefix: 'a&b' }, 'tok-2')) + .toBe('re-run this command with --cursor set to tok-2') + }) + // Base64 cursors carry + / = which are safe unquoted, so the common case stays copy-pasteable. + it('still emits a command for a base64 cursor', () => { + expect(nextPageCommand({ prefix: 'docs/' }, 'dG9rZW4rMi8=')). + toBe('insta storage list --prefix docs/ --cursor dG9rZW4rMi8=') }) })