diff --git a/docs/design/web-shell-file-upload.md b/docs/design/web-shell-file-upload.md index 86bd6c9d1f8..dd81fe0dfa8 100644 --- a/docs/design/web-shell-file-upload.md +++ b/docs/design/web-shell-file-upload.md @@ -41,6 +41,8 @@ The method is a single-purpose no-clobber create primitive; it cannot modify or - A new file is created at `0o600` (not umask default). - The implementation reuses the existing path lock, temp-file reservation, no-clobber create publication, generation guard, audit, and cleanup machinery. Generalize the current atomic publisher to accept an already validated `Buffer`; do not copy a second binary-specific atomic-write implementation. The byte path must not pass through `atomicWriteTextResolvedFile`, whose internal `enforceWriteSize(buf.length)` intentionally applies the 5 MiB text default. Each public write path validates its final byte buffer with its own policy before calling the shared publisher. +The fs layer also gains `mkdir(p: ResolvedPath, opts?: { recursive?: boolean })` so the upload route can materialize a configured drop folder that does not exist yet. It enforces the same trust boundary and generation guard as the write paths, holds the path lock, creates directories at `0o755` (modulo umask), and re-checks every created component with `lstat` immediately after `mkdir` — plus each component's parent before the next `mkdir` — so a symlink swapped in mid-creation is rejected (`symlink_escape`) instead of followed. An existing directory is a no-op; an existing non-directory or symlink at the target is rejected. + ### Daemon: new `POST /file/upload` endpoint Extend `routes/workspace-file-write.ts`, which already owns the workspace file mutation routes and its private `getFsFactory` / `parseClientId` / `resolveOriginatorClientId` machinery. Keeping upload registration there avoids cloning the trust, identity, and workspace-resolution plumbing into a second module. @@ -77,7 +79,7 @@ Body: raw binary bytes - Rejects missing/invalid `path` and a requested basename over `MAX_UPLOAD_FILENAME_BYTES` with a standard `parse_error` envelope. - If a valid `Content-Length` is present and exceeds `MAX_UPLOAD_BYTES`, returns the upload-specific 413 immediately. The raw parser remains authoritative for chunked bodies and clients that omit or understate the header. - Runs `parseClientId` and `resolveOriginatorClientId` against the selected runtime's bridge. An invalid client id is rejected before buffering. - - Splits `path` into directory + basename, resolves the directory with `fs.resolve(dir, 'write')`, and verifies it is an existing directory with `fs.stat`. Traversal, parent-link escapes, missing/non-directory parents, and other boundary failures are therefore rejected before buffering. The requested final name itself is resolved per candidate in the handler's loop after buffering; an escaping final-component symlink surfaces as the loop's boundary error. + - Splits `path` into directory + basename, resolves the directory with `fs.resolve(dir, 'write')`, and verifies it is an existing directory with `fs.stat`. A missing parent directory is created first via the new `WorkspaceFileSystem.mkdir(..., { recursive: true })` primitive (uploading into a configured drop folder that does not exist yet creates it, including missing parents). Traversal, parent-link escapes, non-directory parents, and other boundary failures are therefore rejected before buffering. The directory path is also capped at `MAX_UPLOAD_DIR_DEPTH = 64` components, so a single request cannot materialize an unbounded directory tree ahead of the concurrency gate. The requested final name itself is resolved per candidate in the handler's loop after buffering; an escaping final-component symlink surfaces as the loop's boundary error. Note the widened surface: an authenticated client can now create directory trees (up to the depth cap) inside a trusted workspace via REST, including dotfile components such as `.git/` — the existing file-write routes already allow writing inside `.git/`, so this adds directory creation, not a new write class. - Stores the requested basename, resolved parent directory, route name, and the per-request fs instance in a private request context for the handler; the handler does not resolve the parent directory again. 3. `fileUploadConcurrencyGate` — admits at most `MAX_CONCURRENT_UPLOADS = 4` requests across the legacy and qualified routes. `createServeApp` creates one shared gate and injects it into both route registrations. A saturated gate returns 429 with `Retry-After: 1` before body parsing. Before the upload handler starts, response `finish` or `close` releases the slot; after the handler starts, the slot remains held until the handler settles so disconnecting clients cannot bypass the memory bound. 4. `fileUploadBodyParser` — wraps `express.raw({ type: 'application/octet-stream', limit: MAX_UPLOAD_BYTES })`. The numeric fs policy constant is the single source of truth for both parser and write limits. Its callback intercepts body-parser `status === 413` and returns the upload-specific `file_too_large` envelope below; other errors call `next(err)`. This prevents the global JSON parser error handler from incorrectly reporting the existing 10 MB JSON limit. @@ -183,7 +185,8 @@ The Web Shell is multi-workspace, so uploads must use the same target as the com - Only the existing legacy composer path with no `atWorkspaceCwd` uses `workspace.client.uploadWorkspaceFile(...)`; a modern multi-workspace composer with a missing cwd is unsupported rather than silently targeting the primary workspace. - Drag-and-drop and the @ panel entry share the selected client. The @ panel additionally supplies a directory within that workspace. - A legacy target requires `workspace_file_upload`; a cwd-qualified target requires both `workspace_file_upload` and `workspace_qualified_rest_core`. The selected workspace must also be present exactly once and trusted in the capabilities snapshot. Otherwise hide both upload entry points. -- Host control: the web-shell accepts an optional `fileUploadEnabled` prop (threaded through the customization context). It is an additional gate, not a replacement for the capability: `fileUploadEnabled === false` force-hides both entry points even when the daemon advertises `workspace_file_upload`, while `true`/omitted still requires the capability (and the trust / qualified-route checks above). It never bypasses the capability. +- Host control: the web-shell accepts an optional `fileUploadEnabled` prop (threaded through the customization context). It is an additional gate, not a replacement for the capability: `fileUploadEnabled === false` force-hides both entry points AND disables file drag-and-drop entirely — no drag highlight, no upload, and no inline image/text ingestion from dropped files — even when the daemon advertises `workspace_file_upload`, while `true`/omitted still requires the capability (and the trust / qualified-route checks above). It never bypasses the capability. Clipboard paste of images/text is unaffected. +- Upload directory: an optional `fileUploadDirectory` prop (threaded through the customization context) sets the directory that drag-and-dropped files upload into. It is a **relative path without a leading `/`** (`'uploads'`, `'uploads/images'`); a leading-slash absolute path is rejected by the daemon as outside the workspace. Omitted (or `'.'`) uploads into the workspace root. The daemon creates the directory (including intermediate components) on upload when it does not exist, so a configured drop folder needs no manual setup. ### Upload versus `@` consumption @@ -247,7 +250,7 @@ Occupied names are always auto-numbered; safety-boundary failures, candidate exh ### Web Shell: composer drag-and-drop 1. Listen for `dragenter` / `dragover` / `dragleave` / `drop` on the composer surface. A batch containing only supported images remains on the existing image-attachment path; ordinary files and mixed batches use workspace upload, so one drop is never handled by both paths. -2. For workspace-upload batches, extract `event.dataTransfer.files` and call `uploadFiles(files, '.', onUploaded)` (target workspace root). +2. For workspace-upload batches, extract `event.dataTransfer.files` and call `uploadFiles(files, fileUploadDirectory ?? '.', onUploaded)` — the configured upload directory, or the target workspace root by default. The daemon creates a missing directory on upload. 3. Progress UI: a thin strip above the composer input surface, one row per queued/uploading/error file — filename, state or percentage, and remove/cancel action. State text is not color-only, and icon actions have localized accessible names. Completed rows disappear after three seconds; error rows remain until dismissed. 4. On completion, add an inline `kind: 'file'` composer tag whose serialized value is `@`, escaping through the same pipeline existing file items use (`escapeAtReferenceText(sanitizeInsertText(path))`) — screenshot filenames with spaces and non-ASCII characters are common. diff --git a/packages/cli/src/serve/bridge-file-system-adapter.test.ts b/packages/cli/src/serve/bridge-file-system-adapter.test.ts index 4c5153cc54e..47a3d3ec0ee 100644 --- a/packages/cli/src/serve/bridge-file-system-adapter.test.ts +++ b/packages/cli/src/serve/bridge-file-system-adapter.test.ts @@ -984,6 +984,7 @@ describe('createBridgeFileSystemAdapter', () => { edit: vi.fn(), editAtomic: vi.fn(), writeBytesAtomic: vi.fn(), + mkdir: vi.fn(), }; }, }; @@ -1036,6 +1037,7 @@ describe('createBridgeFileSystemAdapter', () => { edit: vi.fn(), editAtomic: vi.fn(), writeBytesAtomic: vi.fn(), + mkdir: vi.fn(), }; }, }; diff --git a/packages/cli/src/serve/fs/audit.ts b/packages/cli/src/serve/fs/audit.ts index b506ff54dc3..2f2d5ebb882 100644 --- a/packages/cli/src/serve/fs/audit.ts +++ b/packages/cli/src/serve/fs/audit.ts @@ -79,6 +79,13 @@ export interface FsAccessAuditPayload { sizeBytes?: number; truncated?: boolean; matchedIgnore?: 'file' | 'directory'; + /** + * Distinguishes operations that share an `intent` but are not + * interchangeable in the audit trail — e.g. `mkdir` records + * `intent: 'write'` with `sizeBytes: 0`, exactly like a + * zero-byte file upload; the operation name tells them apart. + */ + operation?: string; durationMs: number; /** * Literal glob pattern. Populated only for `intent === 'glob'`, @@ -256,6 +263,7 @@ export function createAuditPublisher( if (record.sizeBytes !== undefined) payload.sizeBytes = record.sizeBytes; if (record.truncated) payload.truncated = true; if (record.matchedIgnore) payload.matchedIgnore = record.matchedIgnore; + if (record.operation !== undefined) payload.operation = record.operation; // `pattern` shares the same privacy gate as `relPath` and // `message`. Glob patterns commonly embed workspace-relative // or absolute path fragments (`src/secrets/*.env`, diff --git a/packages/cli/src/serve/fs/workspace-file-system.test.ts b/packages/cli/src/serve/fs/workspace-file-system.test.ts index 4b5f5ffafee..4a39b3375c5 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.test.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.test.ts @@ -5,7 +5,7 @@ */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; -import { promises as fsp, writeFileSync } from 'node:fs'; +import { promises as fsp, rmSync, symlinkSync, writeFileSync } from 'node:fs'; import * as os from 'node:os'; import * as path from 'node:path'; import { createHash, randomBytes } from 'node:crypto'; @@ -2495,6 +2495,139 @@ describe('WorkspaceFileSystem - writeBytesAtomic', () => { }); }); +describe('WorkspaceFileSystem - mkdir', () => { + let h: Harness; + beforeEach(async () => { + h = await makeHarness(); + }); + afterEach(async () => { + await teardown(h); + }); + + it('creates a missing directory', async () => { + const r = await h.fs.resolve('new-dir', 'write'); + await h.fs.mkdir(r); + const st = await fsp.stat(path.join(h.workspace, 'new-dir')); + expect(st.isDirectory()).toBe(true); + }); + + it('creates missing parents recursively', async () => { + const r = await h.fs.resolve('a/b/c', 'write'); + await h.fs.mkdir(r, { recursive: true }); + for (const p of ['a', 'a/b', 'a/b/c']) { + const st = await fsp.stat(path.join(h.workspace, p)); + expect(st.isDirectory()).toBe(true); + } + }); + + it('reuses an existing directory without error', async () => { + const target = path.join(h.workspace, 'existing'); + await fsp.mkdir(target); + const r = await h.fs.resolve('existing', 'write'); + await expect(h.fs.mkdir(r)).resolves.toBeUndefined(); + const st = await fsp.stat(target); + expect(st.isDirectory()).toBe(true); + }); + + it('rejects an existing non-directory at the target', async () => { + await fsp.writeFile(path.join(h.workspace, 'file.txt'), 'x'); + const r = await h.fs.resolve('file.txt', 'write'); + const err = await h.fs.mkdir(r).catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('parse_error'); + }); + + it('rejects a target that becomes a symlink before creation completes', async () => { + // `resolve` already rejects symlink escapes at admission; this pins the + // post-create `lstat` re-check that defends the create itself. + const realMkdir = fsp.mkdir; + const target = path.join(h.workspace, 'race-dir'); + const spy = vi + .spyOn(fsp, 'mkdir') + .mockImplementation( + async ( + input: Parameters[0], + options?: Parameters[1], + ) => { + if (String(input) === target) { + await fsp.rm(target, { recursive: true, force: true }); + await fsp.symlink(h.scratch, target, 'dir'); + } + return realMkdir(input, options); + }, + ); + try { + const r = await h.fs.resolve('race-dir', 'write'); + const err = await h.fs.mkdir(r).catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('symlink_escape'); + await expect( + fsp.stat(path.join(h.scratch, 'race-dir')), + ).rejects.toMatchObject({ code: 'ENOENT' }); + } finally { + spy.mockRestore(); + } + }); + + it('rejects a parent swapped for a symlink mid-recursive-create', async () => { + // The per-component `lstat` re-check cannot see an INTERMEDIATE ancestor + // that becomes a symlink (it only refuses to follow the final component); + // the parent re-check before the next `mkdir` is what closes that window. + let checks = 0; + let firstComponent = ''; + await teardown(h); + h = await makeHarness({ + generationGuard: { + assertOpen() { + checks += 1; + // The create loop's second iteration is about to mkdir `a/b`; the + // just-created `a` has been swapped for a symlink out of the + // workspace in the meantime. + if (checks === 5) { + rmSync(firstComponent, { recursive: true, force: true }); + symlinkSync(h.scratch, firstComponent, 'dir'); + } + }, + }, + }); + firstComponent = path.join(h.workspace, 'a'); + const r = await h.fs.resolve('a/b', 'write'); + const err = await h.fs + .mkdir(r, { recursive: true }) + .catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('symlink_escape'); + // Nothing was created through the symlink. + await expect(fsp.stat(path.join(h.scratch, 'b'))).rejects.toMatchObject({ + code: 'ENOENT', + }); + }); + + it('refuses the non-recursive create when a parent is missing', async () => { + const r = await h.fs.resolve('a/b', 'write'); + const err = await h.fs.mkdir(r).catch((e: unknown) => e); + expect(isFsError(err)).toBe(true); + expect((err as { kind: string }).kind).toBe('path_not_found'); + }); + + it('records audit access on success', async () => { + const r = await h.fs.resolve('audit-dir', 'write'); + await h.fs.mkdir(r); + const access = h.events.find( + (e) => + e.type === FS_ACCESS_EVENT_TYPE && + (e.data as { intent: string }).intent === 'write', + ); + expect(access).toBeDefined(); + // Privacy mode hashes the path; only `QWEN_AUDIT_RAW_PATHS=1` exposes it. + expect((access!.data as { pathHash: string }).pathHash).toMatch( + /^[0-9a-f]{16}$/, + ); + // `mkdir` must be distinguishable from a zero-byte file write. + expect((access!.data as { operation?: string }).operation).toBe('mkdir'); + }); +}); + describe('WorkspaceFileSystem - factory', () => { it('canonicalizes the workspace once at factory build', async () => { const scratch = await fsp.mkdtemp( diff --git a/packages/cli/src/serve/fs/workspace-file-system.ts b/packages/cli/src/serve/fs/workspace-file-system.ts index c90f8a12b86..a2d92079a09 100644 --- a/packages/cli/src/serve/fs/workspace-file-system.ts +++ b/packages/cli/src/serve/fs/workspace-file-system.ts @@ -309,6 +309,17 @@ export interface WorkspaceFileSystem { p: ResolvedPath, data: Buffer, ): Promise<{ sizeBytes: number; hash: ContentHash }>; + /** + * Create a directory at `p` (already resolved within the workspace). + * `recursive: true` also creates missing intermediate components; every + * created component is re-checked with `lstat` right after `mkdir` and + * each parent is re-checked before the next `mkdir`, so a symlink + * swapped in mid-creation is rejected (`symlink_escape`) rather than + * followed. An existing directory is left untouched; an existing + * non-directory or symlink at `p` is rejected. Directories are created + * at `0o755` (modulo the process umask). + */ + mkdir(p: ResolvedPath, opts?: { recursive?: boolean }): Promise; } /** @@ -1541,6 +1552,37 @@ class WorkspaceFileSystemImpl implements WorkspaceFileSystem { } } + async mkdir(p: ResolvedPath, opts?: { recursive?: boolean }): Promise { + const start = performance.now(); + try { + this.deps.generationGuard?.assertOpen(); + assertTrustedForIntent(this.deps.trusted, 'write'); + const out = await this.deps.pathLocks.runExclusive( + p as string, + async () => { + await ensureResolvedDirectory(p as string, { + recursive: opts?.recursive ?? false, + assertGenerationOpen: () => this.deps.generationGuard?.assertOpen(), + }); + this.deps.generationGuard?.assertOpen(); + const verdict = this.ignoreVerdict(p, 'directory'); + this.deps.audit.recordAccess(this.deps.ctx, { + intent: 'write', + absolute: p, + durationMs: performance.now() - start, + sizeBytes: 0, + operation: 'mkdir', + matchedIgnore: verdict.ignored ? verdict.category : undefined, + }); + return undefined; + }, + ); + return out; + } catch (err) { + throw this.recordAndWrap(err, 'write', p as string); + } + } + /** * Coerce an arbitrary thrown value into an `FsError`, emit the * matching `fs.denied` audit event, and return the typed error @@ -2268,6 +2310,97 @@ async function atomicWriteTextResolvedFile( }); } +/** + * Ensure `target` exists as a real directory (never a symlink). With + * `recursive`, walk up to the deepest existing ancestor and create each + * missing component one at a time, verifying with `lstat` immediately after + * each `mkdir` — and checking each component's parent before the next + * `mkdir` — so a symlink swapped in mid-creation is rejected instead of + * followed. `target` must already be resolved within the workspace; the + * caller holds the path lock. + */ +async function ensureResolvedDirectory( + target: string, + opts: { + recursive: boolean; + assertGenerationOpen: () => void; + }, +): Promise { + const assertRealDirectory = async (p: string): Promise => { + const st = await fsp.lstat(p); + if (st.isSymbolicLink()) { + throw new FsError('symlink_escape', `directory path is a symlink: ${p}`, { + hint: 're-resolve the target after detecting symlink swaps', + }); + } + if (!st.isDirectory()) { + throw new FsError( + 'parse_error', + `path exists and is not a directory: ${p}`, + ); + } + }; + // `lstat` does not follow the FINAL component, so the per-component + // re-check above cannot see a symlink swapped into an intermediate + // ancestor mid-creation; reject one before the next `mkdir` would + // create through it. + const assertParentNotSymlink = async (p: string): Promise => { + const parent = path.dirname(p); + const st = await fsp.lstat(parent); + if (st.isSymbolicLink()) { + throw new FsError( + 'symlink_escape', + `directory parent is a symlink: ${parent}`, + { hint: 're-resolve the target after detecting symlink swaps' }, + ); + } + }; + try { + await assertRealDirectory(target); + return; + } catch (err) { + if (isFsError(err)) throw err; + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + } + if (!opts.recursive) { + opts.assertGenerationOpen(); + await assertParentNotSymlink(target); + try { + await fsp.mkdir(target, { mode: 0o755 }); + } catch (err) { + // Lost a create race; the winner may be a directory we can reuse. + if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err; + } + await assertRealDirectory(target); + return; + } + const missing: string[] = []; + let current = target; + while (true) { + try { + await assertRealDirectory(current); + break; + } catch (err) { + if (isFsError(err)) throw err; + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + missing.push(current); + const parent = path.dirname(current); + if (parent === current) throw err; + current = parent; + } + } + for (const entry of missing.reverse()) { + opts.assertGenerationOpen(); + await assertParentNotSymlink(entry); + try { + await fsp.mkdir(entry, { mode: 0o755 }); + } catch (err) { + if ((err as NodeJS.ErrnoException).code !== 'EEXIST') throw err; + } + await assertRealDirectory(entry); + } +} + /** * Shared atomic temp+publish core. `buf` MUST already be size-validated by * the caller against its own policy (`MAX_WRITE_BYTES` for text, diff --git a/packages/cli/src/serve/routes/workspace-file-write.test.ts b/packages/cli/src/serve/routes/workspace-file-write.test.ts index 2d68f0d3dea..df957f6a5da 100644 --- a/packages/cli/src/serve/routes/workspace-file-write.test.ts +++ b/packages/cli/src/serve/routes/workspace-file-write.test.ts @@ -592,10 +592,29 @@ describe('POST /file/upload', () => { expect(await fsp.readFile(outside, 'utf-8')).toBe('external'); }); - it('rejects a missing parent directory before buffering', async () => { + it('creates a missing parent directory and uploads into it', async () => { const res = await upload('no/such/dir/a.txt').send(Buffer.from('x')); + expect(res.status).toBe(201); + expect(res.body).toMatchObject({ + kind: 'file_upload', + path: 'no/such/dir/a.txt', + }); + expect( + await fsp.readFile(path.join(h.workspace, 'no/such/dir/a.txt'), 'utf8'), + ).toBe('x'); + }); + + it('rejects a directory path deeper than the creation cap', async () => { + // 65 components exceeds MAX_UPLOAD_DIR_DEPTH; the request must fail + // before any directory tree is materialized. + const deep = `${Array.from({ length: 65 }, (_, i) => `d${i}`).join('/')}/f.txt`; + const res = await upload(deep).send(Buffer.from('x')); expect(res.status).toBe(400); expect(res.body.errorKind).toBe('parse_error'); + expect(res.body.error).toContain('64 components'); + await expect(fsp.stat(path.join(h.workspace, 'd0'))).rejects.toMatchObject({ + code: 'ENOENT', + }); }); it('rejects a non-directory parent before buffering', async () => { diff --git a/packages/cli/src/serve/routes/workspace-file-write.ts b/packages/cli/src/serve/routes/workspace-file-write.ts index 20b0225c006..bafae65d0ed 100644 --- a/packages/cli/src/serve/routes/workspace-file-write.ts +++ b/packages/cli/src/serve/routes/workspace-file-write.ts @@ -377,6 +377,13 @@ export function registerWorkspaceQualifiedFileWriteRoutes( const MAX_CONCURRENT_UPLOADS = 4; const MAX_UPLOAD_FILENAME_BYTES = 255; const NUMBERED_CANDIDATE_CAP = 1000; +/** + * Cap on the number of path components an upload may create recursively + * (the missing parent directory is materialized by `mkdir -p` before the + * body is buffered, so a single request must not be able to spin up an + * unbounded directory tree ahead of the concurrency gate). + */ +const MAX_UPLOAD_DIR_DEPTH = 64; interface UploadGateLease { handlerStarted: boolean; @@ -607,6 +614,17 @@ function fileUploadAdmission( sendParseError(res, ROUTE, '`path` must name a file'); return; } + // The missing parent directory is created recursively before the + // body is buffered; bound how much tree a single request may + // materialize ahead of the concurrency gate. + if (dir.split('/').filter(Boolean).length > MAX_UPLOAD_DIR_DEPTH) { + sendParseError( + res, + ROUTE, + `directory path exceeds ${MAX_UPLOAD_DIR_DEPTH} components`, + ); + return; + } // Reject statically detectable bad names before taking a gate slot // and buffering the body; fs.resolve would throw on them anyway. if (queryPath.includes('\0')) { @@ -658,10 +676,13 @@ function fileUploadAdmission( dirStat = await fs.stat(resolvedDir); } catch (err) { if (isFsError(err) && err.kind === 'path_not_found') { - sendParseError(res, ROUTE, 'parent directory does not exist'); - return; + // The target directory may not exist yet (e.g. a configured + // drop folder); create it, including missing parents. + await fs.mkdir(resolvedDir, { recursive: true }); + dirStat = await fs.stat(resolvedDir); + } else { + throw err; } - throw err; } if (dirStat.kind !== 'directory') { sendParseError(res, ROUTE, 'parent path is not a directory'); diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index f6f8e9c2c9f..35d3b2aff64 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -642,6 +642,7 @@ vi.mock('./components/ChatEditor', async () => { customization.fileUploadEnabled === undefined ? undefined : String(customization.fileUploadEnabled), + 'data-file-upload-directory': customization.fileUploadDirectory, }, React.createElement( 'button', @@ -20190,4 +20191,18 @@ describe('fileUploadEnabled customization plumbing', () => { const composer = container.querySelector('[data-web-shell-composer]'); expect(composer?.hasAttribute('data-file-upload-enabled')).toBe(false); }); + + it('reaches the composer customization with the upload directory', () => { + const { container } = renderApp({ fileUploadDirectory: 'uploads' }); + const composer = container.querySelector('[data-web-shell-composer]'); + expect(composer?.getAttribute('data-file-upload-directory')).toBe( + 'uploads', + ); + }); + + it('leaves the upload directory unset when the prop is omitted', () => { + const { container } = renderApp({}); + const composer = container.querySelector('[data-web-shell-composer]'); + expect(composer?.hasAttribute('data-file-upload-directory')).toBe(false); + }); }); diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index 926e2b16e90..0a42107d0bf 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -949,6 +949,16 @@ export interface WebShellProps { * `true`/omitted still requires the capability to be satisfied. */ fileUploadEnabled?: boolean; + /** + * Directory that drag-and-dropped files upload into, **relative to the + * workspace root**. Use a relative path WITHOUT a leading `/` — e.g. + * `'uploads'`, `'uploads/images'`, or omit it to upload into the + * workspace root (the default). A leading-slash path like `'/uploads'` + * is rejected by the daemon as outside the workspace. The directory + * (including intermediate components) is created automatically on upload + * when it does not exist. + */ + fileUploadDirectory?: string; /** Additional @ mention categories shown alongside built-in files/extensions. */ atProviders?: readonly WebShellAtProvider[]; /** Icon URLs for custom composer tag kinds used by @ mention chips. */ @@ -1816,6 +1826,7 @@ export function App({ atProviders, composerTagIcons, fileUploadEnabled, + fileUploadDirectory, renderToolHeaderExtra, renderWelcomeHeader, renderWelcomeFooter, @@ -2075,6 +2086,7 @@ export function App({ markdown, loadingPhrases, fileUploadEnabled, + fileUploadDirectory, }), [ composerTagIcons, @@ -2101,6 +2113,7 @@ export function App({ markdown, loadingPhrases, fileUploadEnabled, + fileUploadDirectory, ], ); const CustomFooter = renderFooter; diff --git a/packages/web-shell/client/components/ChatEditor.test.tsx b/packages/web-shell/client/components/ChatEditor.test.tsx index 45686dab1af..c82ff296e13 100644 --- a/packages/web-shell/client/components/ChatEditor.test.tsx +++ b/packages/web-shell/client/components/ChatEditor.test.tsx @@ -1608,6 +1608,97 @@ describe('ChatEditor file upload gating', () => { expect(composerCoreState.onFileUploadRequest).toBeUndefined(); }); + it('fileUploadEnabled={false} disables file drag-and-drop in the composer core', () => { + uploadWorkspaceState.current = makeWorkspace(['workspace_file_upload']); + renderChatEditor({ customization: { fileUploadEnabled: false } }); + expect(latestComposerCoreOptions.current?.fileDragEnabled).toBe(false); + }); + + it('enables file drag-and-drop in the composer core by default', () => { + uploadWorkspaceState.current = makeWorkspace(['workspace_file_upload']); + renderChatEditor({}); + expect(latestComposerCoreOptions.current?.fileDragEnabled).toBe(true); + }); + + it('fileUploadEnabled={false} ingests nothing on file drop', () => { + const workspace = makeWorkspace(['workspace_file_upload']); + uploadWorkspaceState.current = workspace; + composerCoreState.imageDropCapture.mockImplementation((event: Event) => { + event.preventDefault(); + event.stopPropagation(); + }); + const container = renderChatEditor({ + customization: { fileUploadEnabled: false }, + }); + const editor = container.querySelector('[data-web-shell-composer-editor]')!; + const drop = dispatchDrag( + editor, + 'drop', + ['Files'], + [new File(['abc'], 'notes.txt')], + ); + // Cancelled so the browser cannot navigate to the dropped file, but no + // lane — upload or inline image/text — reacts. + expect(drop.defaultPrevented).toBe(true); + expect(composerCoreState.imageDropCapture).not.toHaveBeenCalled(); + expect(workspace.client.uploadWorkspaceFile).not.toHaveBeenCalled(); + expect(composerCoreState.addTags).not.toHaveBeenCalled(); + expect(container.querySelector('[data-web-shell-upload-strip]')).toBeNull(); + }); + + it('uploads dropped files into the configured directory', async () => { + const workspace = makeWorkspace(['workspace_file_upload']); + workspace.client.uploadWorkspaceFile.mockResolvedValue({ + kind: 'file_upload', + path: 'uploads/notes.txt', + sizeBytes: 3, + hash: `sha256:${'b'.repeat(64)}`, + }); + uploadWorkspaceState.current = workspace; + const container = renderChatEditor({ + customization: { fileUploadDirectory: 'uploads' }, + }); + const editor = container.querySelector('[data-web-shell-composer-editor]')!; + dispatchDrag(editor, 'drop', ['Files'], [new File(['abc'], 'notes.txt')]); + await act(async () => {}); + + expect(workspace.client.uploadWorkspaceFile).toHaveBeenCalledTimes(1); + expect(workspace.client.uploadWorkspaceFile.mock.calls[0][0].path).toBe( + 'uploads/notes.txt', + ); + // The directory flows through the whole chain: request path, server + // response, and the inserted @reference. + expect(composerCoreState.addTags).toHaveBeenCalledWith( + [ + expect.objectContaining({ + kind: 'file', + value: 'uploads/notes.txt', + }), + ], + { placement: 'inline', position: 'end' }, + ); + }); + + it('uploads dropped files to the workspace root by default', async () => { + const workspace = makeWorkspace(['workspace_file_upload']); + workspace.client.uploadWorkspaceFile.mockResolvedValue({ + kind: 'file_upload', + path: 'notes.txt', + sizeBytes: 3, + hash: `sha256:${'d'.repeat(64)}`, + }); + uploadWorkspaceState.current = workspace; + const container = renderChatEditor({}); + const editor = container.querySelector('[data-web-shell-composer-editor]')!; + dispatchDrag(editor, 'drop', ['Files'], [new File(['abc'], 'notes.txt')]); + await act(async () => {}); + + expect(workspace.client.uploadWorkspaceFile).toHaveBeenCalledTimes(1); + expect(workspace.client.uploadWorkspaceFile.mock.calls[0][0].path).toBe( + 'notes.txt', + ); + }); + it('fileUploadEnabled={true} still requires the capability (AND, not override)', () => { // No workspace_file_upload capability: upload stays disabled even though // the host prop opts in — the prop does not bypass the capability check. diff --git a/packages/web-shell/client/components/ChatEditor.tsx b/packages/web-shell/client/components/ChatEditor.tsx index 02ec0fc4fbc..fce03f67300 100644 --- a/packages/web-shell/client/components/ChatEditor.tsx +++ b/packages/web-shell/client/components/ChatEditor.tsx @@ -1484,9 +1484,12 @@ export const ChatEditor = memo( builtinAtProviders: contextBuiltinAtProviders, atProviders: contextAtProviders, fileUploadEnabled, + fileUploadDirectory, } = useWebShellCustomization(); - // Props win when set (main composer). Split-view ChatPane omits them and - // falls back to the App-level customization context. + // At-mention provider props win when set (main composer). Split-view + // ChatPane omits them and falls back to the App-level customization + // context. (Unlike the providers, file upload control comes ONLY from + // the customization context — no prop override exists.) const resolvedBuiltinAtProviders = builtinAtProviders ?? contextBuiltinAtProviders; const resolvedAtProviders = atProviders ?? contextAtProviders; @@ -1579,6 +1582,7 @@ export const ChatEditor = memo( onCycleMode, onToggleShortcuts, disabled, + fileDragEnabled: fileUploadEnabled !== false, placeholderText, commands, skills, @@ -1727,6 +1731,13 @@ export const ChatEditor = memo( event.preventDefault(); return; } + if (fileUploadEnabled === false) { + // Host force-disables file drag-in entirely: cancel the drop so + // the browser cannot navigate to the file, but ingest nothing on + // any lane (the image lane is gated off too). + event.preventDefault(); + return; + } if ( !uploadEnabled || files.length === 0 || @@ -1738,12 +1749,14 @@ export const ChatEditor = memo( clearImageDragState(); event.preventDefault(); event.stopPropagation(); - uploadFiles(files, '.', insertUploadReference); + uploadFiles(files, fileUploadDirectory ?? '.', insertUploadReference); }, [ core.imageTransferHandlers, clearImageDragState, disabled, + fileUploadEnabled, + fileUploadDirectory, uploadEnabled, uploadFiles, insertUploadReference, diff --git a/packages/web-shell/client/customization.tsx b/packages/web-shell/client/customization.tsx index 62447b4cc57..bc47534b466 100644 --- a/packages/web-shell/client/customization.tsx +++ b/packages/web-shell/client/customization.tsx @@ -518,6 +518,16 @@ export interface WebShellCustomization { * trust / qualified-route safety checks) to be satisfied. */ fileUploadEnabled?: boolean; + /** + * Directory that drag-and-dropped files upload into, **relative to the + * workspace root**. Use a relative path WITHOUT a leading `/` — e.g. + * `'uploads'`, `'uploads/images'`, or omit it to upload into the + * workspace root (the default). A leading-slash path like `'/uploads'` + * is rejected by the daemon as outside the workspace. The directory + * (including intermediate components) is created automatically on upload + * when it does not exist. + */ + fileUploadDirectory?: string; } const WebShellCustomizationContext = createContext({}); diff --git a/packages/web-shell/client/hooks/useComposerCore.dom.test.tsx b/packages/web-shell/client/hooks/useComposerCore.dom.test.tsx index 25220e804d1..1e5bb3f42a7 100644 --- a/packages/web-shell/client/hooks/useComposerCore.dom.test.tsx +++ b/packages/web-shell/client/hooks/useComposerCore.dom.test.tsx @@ -34,6 +34,7 @@ function Harness({ commands, onImageIngestionNotice, workspaceUploadBusy, + fileDragEnabled, }: { composerInput?: WebShellComposerInput; onSubmit: ReturnType; @@ -50,6 +51,7 @@ function Harness({ commands?: UseComposerCoreOptions['commands']; onImageIngestionNotice?: UseComposerCoreOptions['onImageIngestionNotice']; workspaceUploadBusy?: boolean; + fileDragEnabled?: UseComposerCoreOptions['fileDragEnabled']; }) { const composer = useComposerCore({ onSubmit, @@ -65,6 +67,7 @@ function Harness({ composerInputVersion: composerInput ? 1 : undefined, onImageIngestionNotice, workspaceUploadBusy, + fileDragEnabled, }); latest = composer; @@ -87,6 +90,7 @@ async function mount({ commands, onImageIngestionNotice, workspaceUploadBusy, + fileDragEnabled, }: { composerInput?: WebShellComposerInput; onSubmit?: ReturnType; @@ -103,6 +107,7 @@ async function mount({ commands?: UseComposerCoreOptions['commands']; onImageIngestionNotice?: UseComposerCoreOptions['onImageIngestionNotice']; workspaceUploadBusy?: boolean; + fileDragEnabled?: UseComposerCoreOptions['fileDragEnabled']; } = {}) { container = document.createElement('div'); document.body.append(container); @@ -127,6 +132,7 @@ async function mount({ commands={commands} onImageIngestionNotice={onImageIngestionNotice} workspaceUploadBusy={workspaceUploadBusy} + fileDragEnabled={fileDragEnabled} /> , @@ -987,6 +993,53 @@ describe('useComposerCore paste', () => { expect(latest!.imageDragActive).toBe(false); }); + it('fileDragEnabled={false} leaves file drag-and-drop inert', async () => { + await mount({ fileDragEnabled: false }); + const surface = container!.querySelector( + '[data-web-shell-composer-surface]', + )!; + const editor = container!.querySelector('.cm-content')!; + const dataTransfer = { + files: [], + items: [{ kind: 'file', type: 'image/png', getAsFile: () => null }], + types: ['Files'], + dropEffect: 'none', + }; + const dispatchDrag = (target: Element, type: string) => { + const event = new Event(type, { bubbles: true, cancelable: true }); + Object.defineProperty(event, 'dataTransfer', { value: dataTransfer }); + target.dispatchEvent(event); + return event; + }; + + act(() => { + dispatchDrag(editor, 'dragenter'); + dispatchDrag(editor, 'dragover'); + }); + // No drag highlight, no drop-target feedback. + expect(latest!.imageDragActive).toBe(false); + expect(dataTransfer.dropEffect).toBe('none'); + + const drop = new Event('drop', { bubbles: true, cancelable: true }); + Object.defineProperty(drop, 'dataTransfer', { + value: { + files: [new File(['png'], 'photo.png', { type: 'image/png' })], + items: [], + types: ['Files'], + dropEffect: 'none', + }, + }); + act(() => { + surface.dispatchEvent(drop); + }); + await waitForImageIngestion(); + // Nothing is ingested on the inline lane, and the drop itself is + // cancelled so the browser cannot navigate to the dropped file. + expect(drop.defaultPrevented).toBe(true); + expect(latest!.pastedImages).toEqual([]); + expect(latest!.pastedFiles).toEqual([]); + }); + it('keeps batch order, normalizes BMP, and aggregates rejected drops', async () => { const onImageIngestionNotice = vi.fn(); await mount({ onImageIngestionNotice }); diff --git a/packages/web-shell/client/hooks/useComposerCore.ts b/packages/web-shell/client/hooks/useComposerCore.ts index 53945eff7dc..0038aace9a7 100644 --- a/packages/web-shell/client/hooks/useComposerCore.ts +++ b/packages/web-shell/client/hooks/useComposerCore.ts @@ -1094,6 +1094,14 @@ export interface UseComposerCoreOptions { onCycleMode?: () => void; onToggleShortcuts?: () => void; disabled?: boolean; + /** + * Whether the composer may react to FILE drags at all (drag highlight and + * drop ingestion on the inline image/text lane). `false` leaves paste + * working but makes file drag-and-drop inert, matching a host that + * force-disables file upload via `fileUploadEnabled={false}`. Defaults to + * `true`. + */ + fileDragEnabled?: boolean; placeholderText?: string; commands: CommandInfo[]; skills?: SkillInfo[]; @@ -1388,6 +1396,7 @@ export function useComposerCore( onCycleMode, onToggleShortcuts, disabled = false, + fileDragEnabled = true, placeholderText = 'Type a message...', commands, skills = [], @@ -1505,6 +1514,8 @@ export function useComposerCore( onToggleShortcutsRef.current = onToggleShortcuts; const disabledRef = useRef(disabled); disabledRef.current = disabled; + const fileDragEnabledRef = useRef(fileDragEnabled); + fileDragEnabledRef.current = fileDragEnabled; const workspaceUploadBusyRef = useRef(workspaceUploadBusy); workspaceUploadBusyRef.current = workspaceUploadBusy; const commandsRef = useRef(commands); @@ -1859,18 +1870,30 @@ export function useComposerCore( } }, onDragEnterCapture: (event) => { - if (!hasFileTransferPayload(event.dataTransfer)) return; + if ( + !fileDragEnabledRef.current || + !hasFileTransferPayload(event.dataTransfer) + ) + return; event.preventDefault(); imageDragDepthRef.current += 1; if (!disabledRef.current) setImageDragActive(true); }, onDragOverCapture: (event) => { - if (!hasFileTransferPayload(event.dataTransfer)) return; + if ( + !fileDragEnabledRef.current || + !hasFileTransferPayload(event.dataTransfer) + ) + return; event.preventDefault(); event.dataTransfer.dropEffect = 'copy'; }, onDragLeaveCapture: (event) => { - if (!hasFileTransferPayload(event.dataTransfer)) return; + if ( + !fileDragEnabledRef.current || + !hasFileTransferPayload(event.dataTransfer) + ) + return; imageDragDepthRef.current = Math.max(0, imageDragDepthRef.current - 1); const nextTarget = event.relatedTarget; if ( @@ -1882,6 +1905,13 @@ export function useComposerCore( }, onDropCapture: (event) => { if (!hasFileTransferPayload(event.dataTransfer)) return; + if (!fileDragEnabledRef.current) { + // File drags are inert, but still cancel the drop so the browser + // cannot navigate to the file. Capture-phase preventDefault does + // not stop propagation, so a host handler can still react. + event.preventDefault(); + return; + } event.preventDefault(); event.stopPropagation(); clearImageDragState(); @@ -1908,6 +1938,12 @@ export function useComposerCore( useEffect(() => { if (disabled) clearImageDragState(); }, [clearImageDragState, disabled]); + useEffect(() => { + // A host flipping `fileUploadEnabled` to false mid-drag gates the + // leave handler, so a depth already counted would never drain; clear + // the highlight explicitly instead of waiting for dragend/blur. + if (fileDragEnabled === false) clearImageDragState(); + }, [clearImageDragState, fileDragEnabled]); useEffect( () => () => { resetImageIngestion(false);