From 55bf77304630f96b03bc8a05e69a71935cb6fa59 Mon Sep 17 00:00:00 2001 From: Cliff Date: Wed, 24 Jun 2026 04:53:38 +0000 Subject: [PATCH] feat: add download subcommand for Slack files (0.4.0) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds a first-class `nori-slack download --id --output ` subcommand that resolves a file and writes its raw bytes to disk, replacing the base64 `files.download` proxy hack. - Transport gains `downloadFile()` with proxy- and direct-mode impls. - Direct mode guards against Slack's HTTP-200 HTML sign-in page, which would otherwise be written to disk as a "successful" download. - Test runs are serialized (--no-file-parallelism) to stop subprocess CLI tests flaking under vitest concurrency. 🤖 Generated with [Nori](https://noriagentic.com) Co-Authored-By: Nori --- README.md | 20 +++++ docs.md | 2 + package.json | 4 +- src/download.ts | 23 +++++ src/index.ts | 62 +++++++++++++- src/transport.ts | 56 +++++++++++- test/download-direct.test.ts | 71 ++++++++++++++++ test/download.test.ts | 160 +++++++++++++++++++++++++++++++++++ test/helpers.ts | 23 +++++ 9 files changed, 417 insertions(+), 4 deletions(-) create mode 100644 src/download.ts create mode 100644 test/download-direct.test.ts create mode 100644 test/download.test.ts diff --git a/README.md b/README.md index 13af0c0..8e814bf 100644 --- a/README.md +++ b/README.md @@ -107,6 +107,26 @@ nori-slack upload --file ./report.pdf --channel C123 --dry-run The bytes are POSTed directly to Slack's upload host (the upload URL is itself the credential), so they never pass through the broker. The completing call rides the normal transport, so in proxy mode the broker still enforces the session's channel scoping — uploading into a channel outside the grant fails with a structured error. +### File downloads + +Downloading a Slack file means fetching the bytes from Slack's private file host with a credential, which cannot ride the dynamic `` path, so downloading gets its own subcommand: + +```bash +# Download a file by ID to a local path +nori-slack download --id F123 --output ./report.pdf + +# Preview the planned download without contacting Slack +nori-slack download --id F123 --output ./report.pdf --dry-run +``` + +| Flag | Purpose | +| --- | --- | +| `--id ` | Slack file ID to download (required). | +| `--output ` | Local path to write the bytes to (required). | +| `--dry-run` | Print the planned download (file ID, output, transport) without contacting Slack. | + +In direct mode the CLI looks up the file's private download URL and fetches it with the bot token. In proxy mode the bytes come from the broker's `download` endpoint (the session has no bot token), so the broker enforces that the file is shared in the session's access grant — downloading a file outside the grant fails with a structured error. + ### Top-level flags | Flag | Purpose | diff --git a/docs.md b/docs.md index 7510890..19dec8c 100644 --- a/docs.md +++ b/docs.md @@ -10,6 +10,7 @@ Path: @/ - Supports `--dry-run` to preview resolved API requests without sending them -- designed as a safety net for coding agents to validate parameter resolution - Supports `describe ` to look up parameter documentation for any Slack API method without requiring a token -- the metadata map covers all methods in `KNOWN_METHODS`, so agents always get full parameter documentation rather than a fallback - Supports an `upload` subcommand that drives Slack's modern three-step external file upload flow -- a distinct capability because the raw byte upload cannot ride the dynamic `` dispatch path (see [src/upload.ts](src/upload.ts)) +- Supports a `download` subcommand that fetches a Slack file's bytes by ID and writes them to a local path -- a distinct capability because the authenticated byte fetch from Slack's private file host cannot ride the dynamic `` dispatch path (see [src/download.ts](src/download.ts)) ### How it fits into the larger codebase - Standalone repository (was originally imported from the `nori-integrations` monorepo and now lives on its own). Distributed via the public npm registry as `nori-slack-cli` @@ -26,6 +27,7 @@ Path: @/ - Two input modes: CLI flags (`--channel C123 --text "hi"`) and piped JSON via `--json-input`; when both are provided, CLI flags override stdin values - Two discovery subcommands that do not require credentials: `list-methods` outputs known method names as JSON (supports `--namespace` filtering and `--descriptions` to include method descriptions), and `describe ` returns structured parameter documentation - The `upload` subcommand, orchestrated by [src/upload.ts](src/upload.ts), runs Slack's external upload as three ordered steps: (1) `files.getUploadURLExternal` mints an upload URL + `file_id` via the transport, (2) the raw file bytes are POSTed DIRECTLY to Slack's upload host with no token (the URL is itself the credential, so these bytes never touch the broker proxy), and (3) `files.completeUploadExternal` shares the file via the transport. Because the completing call rides the normal transport, proxy-mode channel scoping is enforced by the broker at that step automatically. This flow is what Bolt exposes as `files.uploadV2` and is why it cannot be reached through the dynamic dispatch path +- The `download` subcommand, orchestrated by [src/download.ts](src/download.ts), delegates the transfer to `transport.downloadFile({ fileId })` and writes the returned bytes to `--output`. Each transport implements the fetch differently: direct mode calls `files.info` for the file's `url_private_download` and GETs it with the bot token as a Bearer credential; proxy mode GETs `/download?file=` with the context token, because the session has no bot token and the broker holds the file-shared access grant. The split lives in the `Transport` interface so `download.ts` never branches on transport mode - `describe` uses [src/method-metadata.ts](src/method-metadata.ts), a hand-curated static map covering every method in `KNOWN_METHODS` -- this is static because `@slack/web-api` erases parameter type information at compile time, so runtime introspection is not possible - For unknown methods (not in `KNOWN_METHODS`), `getMethodMetadata()` returns a fallback entry with empty params and a generated docs URL, so `describe` never errors; the `known` field in the output distinguishes curated entries from fallbacks - When an unknown method is used, [src/suggest.ts](src/suggest.ts) provides fuzzy matching via Levenshtein distance against `KNOWN_METHODS`, surfacing "Did you mean?" suggestions; suggestions are non-blocking -- unknown methods still proceed to the API diff --git a/package.json b/package.json index e6da56b..d4475d8 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "nori-slack-cli", - "version": "0.3.0", + "version": "0.4.0", "description": "CLI for interacting with the Slack Web API, designed for coding agents", "type": "module", "bin": { @@ -13,7 +13,7 @@ "build": "tsc", "postbuild": "chmod +x dist/index.js", "prepare": "npm run build", - "test": "vitest run", + "test": "vitest run --no-file-parallelism", "test:watch": "vitest" }, "dependencies": { diff --git a/src/download.ts b/src/download.ts new file mode 100644 index 0000000..40dd435 --- /dev/null +++ b/src/download.ts @@ -0,0 +1,23 @@ +import { writeFile } from 'node:fs/promises'; +import type { Transport } from './transport.js'; + +export interface DownloadArgs { + transport: Transport; + fileId: string; + outputPath: string; +} + +export async function downloadFile(args: DownloadArgs): Promise> { + const { transport, fileId, outputPath } = args; + const { bytes, contentType, filename } = await transport.downloadFile({ fileId }); + await writeFile(outputPath, bytes); + return { + ok: true, + command: 'download', + file_id: fileId, + output: outputPath, + filename, + bytes: bytes.length, + content_type: contentType, + }; +} diff --git a/src/index.ts b/src/index.ts index b7fa6db..3f44041 100644 --- a/src/index.ts +++ b/src/index.ts @@ -9,6 +9,7 @@ import { detectTransportMode, resolveTransport } from './transport.js'; import { getMethodMetadata, METHOD_METADATA } from './method-metadata.js'; import { findSimilarMethods } from './suggest.js'; import { uploadFile } from './upload.js'; +import { downloadFile } from './download.js'; import { fileURLToPath } from 'node:url'; import { statSync } from 'node:fs'; import path from 'node:path'; @@ -27,7 +28,7 @@ program.enablePositionalOptions(); program .name('nori-slack') .description('CLI for the Slack Web API. Designed for coding agents.\n\nUsage: nori-slack [--param value ...]\n\nExamples:\n nori-slack chat.postMessage --channel C123 --text "Hello"\n nori-slack conversations.list --limit 10\n nori-slack api.test --foo bar\n echo \'{"channel":"C123","text":"hi"}\' | nori-slack chat.postMessage --json-input') - .version('0.3.0'); + .version('0.4.0'); program .command('list-methods') @@ -169,6 +170,65 @@ program } }); +program + .command('download') + .description('Download a Slack file by ID and write its bytes to a local path. Example: nori-slack download --id F123 --output ./report.pdf') + .option('--id ', 'Slack file ID to download (e.g., F123)') + .option('--output ', 'Local path to write the downloaded bytes to') + .option('--dry-run', 'Preview the planned download without contacting Slack') + .action(async (opts: { id?: string; output?: string; dryRun?: boolean }) => { + const fileId = opts.id; + if (typeof fileId !== 'string' || fileId.length === 0) { + const error = formatError( + new Error('download requires --id . Example: nori-slack download --id F123 --output ./report.pdf'), + SOURCE_DIR + ); + process.stdout.write(JSON.stringify(error) + '\n'); + process.exit(2); + } + + const outputPath = opts.output; + if (typeof outputPath !== 'string' || outputPath.length === 0) { + const error = formatError( + new Error('download requires --output . Example: nori-slack download --id F123 --output ./report.pdf'), + SOURCE_DIR + ); + process.stdout.write(JSON.stringify(error) + '\n'); + process.exit(2); + } + + if (opts.dryRun) { + const dryRunResult = { + ok: true, + dry_run: true, + command: 'download', + file_id: fileId, + output: outputPath, + transport: detectTransportMode(), + token_present: !!process.env.SLACK_BOT_TOKEN, + }; + process.stdout.write(JSON.stringify(dryRunResult) + '\n'); + return; + } + + const transport = resolveTransport(); + if (!transport) { + const error = formatError({ code: 'no_token' }, SOURCE_DIR); + process.stdout.write(JSON.stringify(error) + '\n'); + process.exit(1); + } + + try { + const result = await downloadFile({ transport, fileId, outputPath }); + process.stdout.write(JSON.stringify(result) + '\n'); + } catch (err) { + const error = formatError(err, SOURCE_DIR); + process.stdout.write(JSON.stringify(error) + '\n'); + process.stderr.write(`Error: ${error.message}\nSuggestion: ${error.suggestion}\n`); + process.exit(1); + } + }); + program .argument('', 'Slack Web API method (e.g., chat.postMessage)') .option('--json-input', 'Read parameters as JSON from stdin') diff --git a/src/transport.ts b/src/transport.ts index a7fd192..4f5411e 100644 --- a/src/transport.ts +++ b/src/transport.ts @@ -2,9 +2,16 @@ import { WebClient } from '@slack/web-api'; export type TransportMode = 'proxy' | 'direct' | 'none'; +export interface DownloadResult { + bytes: Buffer; + contentType: string | null; + filename: string | null; +} + export interface Transport { mode: 'proxy' | 'direct'; call(method: string, params: Record): Promise>; + downloadFile(args: { fileId: string }): Promise; } export const PROXY_ERROR_CODE = 'nori_slack_proxy_error'; @@ -54,16 +61,63 @@ export function resolveTransport(env: NodeJS.ProcessEnv = process.env): Transpor } return body; }, + async downloadFile({ fileId }) { + const res = await fetch(`${baseUrl}/download?file=${encodeURIComponent(fileId)}`, { + headers: { authorization: `Bearer ${contextToken}` }, + }); + if (!res.ok) { + const text = await res.text(); + let body: any = null; + try { + body = JSON.parse(text); + } catch { + // Non-JSON body: fall through with the raw text as the error message. + } + throw new ProxyError(res.status, typeof body?.error === 'string' ? body.error : text); + } + return { + bytes: Buffer.from(await res.arrayBuffer()), + contentType: res.headers.get('content-type'), + filename: null, + }; + }, }; } if (mode === 'direct') { - const client = new WebClient(env.SLACK_BOT_TOKEN); + const botToken = env.SLACK_BOT_TOKEN!; + const client = new WebClient(botToken); return { mode, call(method, params) { return client.apiCall(method, params) as Promise>; }, + async downloadFile({ fileId }) { + const info = await this.call('files.info', { file: fileId }); + const file = info.file as { url_private_download?: string; name?: string } | undefined; + const url = file?.url_private_download; + if (!url) { + throw new Error(`files.info did not return a download URL for file ${fileId}`); + } + const res = await fetch(url, { headers: { authorization: `Bearer ${botToken}` } }); + if (!res.ok) { + throw new Error(`Failed to download file ${fileId}: HTTP ${res.status}`); + } + const contentType = res.headers.get('content-type'); + // Slack's file host answers an unauthenticated request with HTTP 200 and + // an HTML sign-in page rather than an error status, so a rejected bot + // token would otherwise be written to disk as a "successful" download. + if (contentType != null && contentType.includes('text/html')) { + throw new Error( + `Slack returned an HTML page instead of file ${fileId}; the bot token was not authenticated for this file`, + ); + } + return { + bytes: Buffer.from(await res.arrayBuffer()), + contentType, + filename: file?.name ?? null, + }; + }, }; } diff --git a/test/download-direct.test.ts b/test/download-direct.test.ts new file mode 100644 index 0000000..efe04c0 --- /dev/null +++ b/test/download-direct.test.ts @@ -0,0 +1,71 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import http from 'node:http'; +import type { AddressInfo } from 'node:net'; +import { resolveTransport } from '../src/transport.js'; + +// Direct mode cannot make real Slack calls in tests, so the files.info metadata +// lookup (the Slack boundary) is stubbed by overriding the transport's own +// `call`, while the byte fetch runs for real against a local host. This proves +// the security-relevant behavior: the private download is fetched with the bot +// token as a Bearer credential and the exact bytes are returned uncorrupted. +describe('download (direct mode)', () => { + let fileHost: http.Server; + let fileUrl: string; + let received: { authorization?: string } = {}; + let respond: (res: http.ServerResponse) => void; + + const fileBytes = Buffer.from([0x25, 0x50, 0x44, 0x46, 0xc3, 0xa9, 0x00, 0xff]); + + beforeEach(async () => { + received = {}; + respond = (res) => { + res.writeHead(200, { 'content-type': 'application/pdf' }); + res.end(fileBytes); + }; + fileHost = http.createServer((req, res) => { + received.authorization = req.headers.authorization; + respond(res); + }); + await new Promise((resolve) => fileHost.listen(0, '127.0.0.1', resolve)); + const port = (fileHost.address() as AddressInfo).port; + fileUrl = `http://127.0.0.1:${port}/private/F123`; + }); + + afterEach(async () => { + await new Promise((resolve) => fileHost.close(() => resolve())); + }); + + const directTransport = () => { + const transport = resolveTransport({ SLACK_BOT_TOKEN: 'xoxb-test-token' } as NodeJS.ProcessEnv); + expect(transport?.mode).toBe('direct'); + transport!.call = async (method: string, params: Record) => { + expect(method).toBe('files.info'); + expect(params).toEqual({ file: 'F123' }); + return { + ok: true, + file: { id: 'F123', name: 'doc.pdf', mimetype: 'application/pdf', url_private_download: fileUrl }, + }; + }; + return transport!; + }; + + it('fetches the private download URL with the bot token and returns the exact bytes', async () => { + const transport = directTransport(); + + const result = await transport.downloadFile({ fileId: 'F123' }); + + expect(received.authorization).toBe('Bearer xoxb-test-token'); + expect(result.bytes.equals(fileBytes)).toBe(true); + expect(result.contentType).toBe('application/pdf'); + }); + + it('rejects the HTML login page Slack serves with HTTP 200 when the bot token is rejected', async () => { + respond = (res) => { + res.writeHead(200, { 'content-type': 'text/html; charset=utf-8' }); + res.end('You are not signed in'); + }; + const transport = directTransport(); + + await expect(transport.downloadFile({ fileId: 'F123' })).rejects.toThrow(/not authenticated|html|sign/i); + }); +}); diff --git a/test/download.test.ts b/test/download.test.ts new file mode 100644 index 0000000..2f14e79 --- /dev/null +++ b/test/download.test.ts @@ -0,0 +1,160 @@ +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, readFileSync, existsSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; +import { runCli, startFakeBroker, type FakeBroker } from './helpers.js'; + +describe('download command (proxy mode)', () => { + let broker: FakeBroker; + let dir: string; + + beforeEach(async () => { + broker = await startFakeBroker(); + dir = mkdtempSync(path.join(tmpdir(), 'nori-slack-download-')); + }); + + afterEach(async () => { + await broker.close(); + rmSync(dir, { recursive: true, force: true }); + }); + + function proxyEnv(extra: Record = {}): Record { + return { + NORI_SLACK_PROXY_URL: broker.url, + NORI_SLACK_CONTEXT_TOKEN: 'ctx-token-123', + ...extra, + }; + } + + // Binary bytes (multi-byte UTF-8, a NUL, and 0xff) whose character length + // differs from the byte length. They prove the bytes the broker streams reach + // the output file unchanged (no transcoding) via the equals() check. + const fileBytes = Buffer.from([0x25, 0x50, 0x44, 0x46, 0xc3, 0xa9, 0x00, 0xff]); + + it('GETs the broker download endpoint with the context token and writes the exact bytes', async () => { + const outPath = path.join(dir, 'out.bin'); + broker.queueDownloadResponse({ body: fileBytes, contentType: 'application/pdf' }); + + const result = await runCli( + ['download', '--id', 'F123', '--output', outPath], + proxyEnv(), + ); + + expect(result.exitCode).toBe(0); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(true); + expect(output.file_id).toBe('F123'); + expect(output.bytes).toBe(fileBytes.length); + + expect(broker.downloads).toHaveLength(1); + expect(broker.downloads[0].url).toContain('/slack-proxy/download'); + expect(broker.downloads[0].url).toContain('file=F123'); + expect(broker.downloads[0].headers.authorization).toBe('Bearer ctx-token-123'); + + expect(readFileSync(outPath).equals(fileBytes)).toBe(true); + }); + + it('maps a broker 403 to a structured error and writes no file', async () => { + const outPath = path.join(dir, 'denied.bin'); + broker.queueDownloadResponse({ + status: 403, + body: Buffer.from( + JSON.stringify({ error: 'Slack file is not shared in this session conversation' }), + ), + contentType: 'application/json', + }); + + const result = await runCli( + ['download', '--id', 'F999', '--output', outPath], + proxyEnv(), + ); + + expect(result.exitCode).toBe(1); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + expect(output.message).toContain('not shared'); + expect(result.stderr).toContain('Error:'); + expect(existsSync(outPath)).toBe(false); + }); + + it('maps a broker 404 to a structured error and writes no file', async () => { + const outPath = path.join(dir, 'missing.bin'); + broker.queueDownloadResponse({ + status: 404, + body: Buffer.from(JSON.stringify({ error: 'Slack file not found' })), + contentType: 'application/json', + }); + + const result = await runCli( + ['download', '--id', 'Fnope', '--output', outPath], + proxyEnv(), + ); + + expect(result.exitCode).toBe(1); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + expect(output.message).toContain('not found'); + expect(existsSync(outPath)).toBe(false); + }); + + it('maps a broker 401 to a context-token suggestion', async () => { + const outPath = path.join(dir, 'unauth.bin'); + broker.queueDownloadResponse({ + status: 401, + body: Buffer.from(JSON.stringify({ error: 'Unauthorized' })), + contentType: 'application/json', + }); + + const result = await runCli( + ['download', '--id', 'F123', '--output', outPath], + proxyEnv(), + ); + + expect(result.exitCode).toBe(1); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + expect(output.suggestion).toContain('NORI_SLACK_CONTEXT_TOKEN'); + expect(existsSync(outPath)).toBe(false); + }); + + it('exits 2 without contacting the broker when --id is missing', async () => { + const result = await runCli( + ['download', '--output', path.join(dir, 'x.bin')], + proxyEnv(), + ); + + expect(result.exitCode).toBe(2); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + expect(output.message.toLowerCase()).toContain('id'); + expect(broker.downloads).toHaveLength(0); + }); + + it('exits 2 without contacting the broker when --output is missing', async () => { + const result = await runCli(['download', '--id', 'F123'], proxyEnv()); + + expect(result.exitCode).toBe(2); + const output = JSON.parse(result.stdout); + expect(output.ok).toBe(false); + expect(output.message.toLowerCase()).toContain('output'); + expect(broker.downloads).toHaveLength(0); + }); + + it('--dry-run reports the plan without contacting the broker', async () => { + const outPath = path.join(dir, 'out.bin'); + const result = await runCli( + ['download', '--id', 'F123', '--output', outPath, '--dry-run'], + proxyEnv(), + ); + + expect(result.exitCode).toBe(0); + const output = JSON.parse(result.stdout); + expect(output.dry_run).toBe(true); + expect(output.command).toBe('download'); + expect(output.file_id).toBe('F123'); + expect(output.output).toBe(outPath); + expect(output.transport).toBe('proxy'); + expect(broker.downloads).toHaveLength(0); + expect(existsSync(outPath)).toBe(false); + }); +}); diff --git a/test/helpers.ts b/test/helpers.ts index 43fdda8..c77f5d5 100644 --- a/test/helpers.ts +++ b/test/helpers.ts @@ -79,13 +79,20 @@ export interface RecordedUpload { body: Buffer; } +export interface RecordedDownload { + url: string; + headers: http.IncomingHttpHeaders; +} + export interface FakeBroker { url: string; origin: string; requests: RecordedRequest[]; uploads: RecordedUpload[]; + downloads: RecordedDownload[]; queueResponse(response: { status?: number; body: unknown }): void; queueUploadResponse(response: { status?: number; body?: string }): void; + queueDownloadResponse(response: { status?: number; body?: Buffer; contentType?: string }): void; close(): Promise; } @@ -97,8 +104,10 @@ export interface FakeBroker { export async function startFakeBroker(): Promise { const requests: RecordedRequest[] = []; const uploads: RecordedUpload[] = []; + const downloads: RecordedDownload[] = []; const responses: Array<{ status?: number; body: unknown }> = []; const uploadResponses: Array<{ status?: number; body?: string }> = []; + const downloadResponses: Array<{ status?: number; body?: Buffer; contentType?: string }> = []; const server = http.createServer((req, res) => { const chunks: Buffer[] = []; @@ -106,6 +115,16 @@ export async function startFakeBroker(): Promise { req.on('end', () => { const raw = Buffer.concat(chunks); const url = req.url ?? ''; + const pathOnly = url.split('?')[0]; + if (req.method === 'GET' && pathOnly.endsWith('/download')) { + downloads.push({ url, headers: req.headers }); + const next = downloadResponses.length > 0 ? downloadResponses.shift()! : { status: 200 }; + res.writeHead(next.status ?? 200, { + 'content-type': next.contentType ?? 'application/octet-stream', + }); + res.end(next.body ?? Buffer.alloc(0)); + return; + } if (!url.endsWith('/method')) { uploads.push({ url, headers: req.headers, body: raw }); const next = uploadResponses.length > 0 ? uploadResponses.shift()! : { status: 200 }; @@ -133,12 +152,16 @@ export async function startFakeBroker(): Promise { origin, requests, uploads, + downloads, queueResponse(response) { responses.push(response); }, queueUploadResponse(response) { uploadResponses.push(response); }, + queueDownloadResponse(response) { + downloadResponses.push(response); + }, close() { return new Promise((resolve) => server.close(() => resolve())); },