Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions docs/design/web-shell-file-upload.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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 `@<finalPath>`, escaping through the same pipeline existing file items use (`escapeAtReferenceText(sanitizeInsertText(path))`) — screenshot filenames with spaces and non-ASCII characters are common.

Expand Down
2 changes: 2 additions & 0 deletions packages/cli/src/serve/bridge-file-system-adapter.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -984,6 +984,7 @@ describe('createBridgeFileSystemAdapter', () => {
edit: vi.fn(),
editAtomic: vi.fn(),
writeBytesAtomic: vi.fn(),
mkdir: vi.fn(),
};
},
};
Expand Down Expand Up @@ -1036,6 +1037,7 @@ describe('createBridgeFileSystemAdapter', () => {
edit: vi.fn(),
editAtomic: vi.fn(),
writeBytesAtomic: vi.fn(),
mkdir: vi.fn(),
};
},
};
Expand Down
8 changes: 8 additions & 0 deletions packages/cli/src/serve/fs/audit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'`,
Expand Down Expand Up @@ -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`,
Expand Down
135 changes: 134 additions & 1 deletion packages/cli/src/serve/fs/workspace-file-system.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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'));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] R1-4: Documented 0o755 create mode asserted by no test — the interface JSDoc added by this diff and the design doc both state directories are created at 0o755 (modulo umask), and ensureResolvedDirectory passes { mode: 0o755 } in both create branches, but none of the eight new mkdir tests asserts the resulting mode (the same file pins 0o600 for created files at lines 1272 and 2389-2394). — Failure scenario: mutating the mode to 0o700 or 0o777 ships silently; under umask 002, 0o777 yields group/world-writable daemon-created directories, contradicting the documented contract.

Suggested fix:

// pin umask in the harness so the assert is environment-independent
process.umask(0o022);
// in the create tests (single and recursive):
expect(st.mode & 0o777).toBe(0o755);
中文说明

[Suggestion] R1-4:文档所述的 0o755 创建模式没有任何测试断言 —— 本 diff 新增的接口 JSDoc 和设计文档都声明目录以 0o755(受 umask 影响)创建,ensureResolvedDirectory 的两个创建分支也都传入 { mode: 0o755 },但新增的 8 个 mkdir 测试没有一个断言实际模式(同一文件在 1272 行和 2389-2394 行为创建的文件固定了 0o600)。— 失败场景:把模式变异为 0o7000o777 可以悄无声息地合入;在 umask 002 下 0o777 会产生组/全局可写的 daemon 创建目录,与文档契约矛盾。

建议修复:在测试框架中固定 umask(如 process.umask(0o022)),并在单级与递归创建测试中断言 expect(st.mode & 0o777).toBe(0o755)

— qwen3.8-max via Qwen Code /review (v0.21.13)

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<typeof fsp.mkdir>[0],
options?: Parameters<typeof fsp.mkdir>[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(
Expand Down
Loading
Loading