Skip to content

Commit a852ebb

Browse files
ThomasK33claude
andcommitted
fix(agent): harden file uploads (code-review findings)
- uploadChatFile now throws when a 2xx response carries no file id, instead of silently returning id "" (which emitted a `file_id: ""` reference to nothing). - coderFileId requires a non-empty id, so a blank reused handle re-uploads rather than referencing nothing. - Normalize the media type (strip `;` params, lowercase) before the allowlist check and Content-Type, so `text/plain; charset=utf-8` (common from a browser File) is accepted instead of rejected. - Content-Disposition now uses RFC 6266 (sanitized ASCII `filename` + RFC 5987 `filename*`): non-ASCII names (CJK, emoji, accents) no longer throw a ByteString error in fetch, and CR/LF can't break or inject the header. - Guard the constructor against a `workspaceFiles` adapter bound to a different workspace than the chat. - Run `resolveModelConfigId` and the file uploads concurrently (`Promise.all`). - `base64ToBytes` returns the Buffer directly (no extra copy); `#send` reuses `#json` for its error body. Adds hermetic CoderChatClient.uploadChatFile unit tests (mocked fetch). Change-Id: Ifb493026f9ed5f4ab6fdf0c00a3170fe47240dbf Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Thomas Kosiewski <tk@coder.com>
1 parent d0edfe7 commit a852ebb

6 files changed

Lines changed: 177 additions & 23 deletions

File tree

packages/agent/src/agent/coder-agent.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,10 @@ export interface ChatAttachment extends UploadedChatFile {
2828
* A native AI SDK `file` part that references this upload by id. Drop it into
2929
* a user message's `content` to reuse the file across turns without
3030
* re-uploading — the agent recognizes the id and skips the upload.
31+
*
32+
* The reference travels in `providerOptions.coder.fileId`; if you persist the
33+
* part through a store that strips `providerOptions`, the reference is lost
34+
* (re-`attach()` instead of persisting the handle).
3135
*/
3236
toFilePart(): FilePart;
3337
}
@@ -138,6 +142,18 @@ export class CoderAgent<TOOLS extends ToolSet = {}> implements Agent<never, TOOL
138142
this.#organizationId = settings.organizationId;
139143
this.#workspaceFiles = settings.workspaceFiles;
140144

145+
// A file written to one workspace isn't visible to a chat bound to another.
146+
if (
147+
settings.workspaceFiles &&
148+
settings.workspaceId &&
149+
settings.workspaceFiles.workspaceId !== settings.workspaceId
150+
) {
151+
throw new CoderAgentError(
152+
`workspaceFiles.workspaceId (${settings.workspaceFiles.workspaceId}) does not match the ` +
153+
`agent's workspaceId (${settings.workspaceId}); the chat's tools would not see uploaded files.`,
154+
);
155+
}
156+
141157
this.#model = new CoderLanguageModel({
142158
client: this.#client,
143159
organizationId: settings.organizationId,

packages/agent/src/coder/client.ts

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ import { streamChatEvents, type WebSocketFactory } from "./ws.js";
1919
/** A file to upload as a chat attachment. */
2020
export interface ChatFileInput {
2121
content: FileContent;
22-
/** Media type. Required unless `content` is a Blob/File that carries its own `type`. */
22+
/**
23+
* Media type. Required unless `content` is a Blob/File with a non-empty `type`
24+
* — note `fs.openAsBlob()` returns a Blob with no type, so pass it explicitly there.
25+
*/
2326
mediaType?: string;
2427
/** Original filename, surfaced to the model and UI. Defaults to a File's `name`. */
2528
name?: string;
@@ -86,14 +89,7 @@ export class CoderChatClient {
8689

8790
const res = await this.#fetch(`${this.baseUrl}${path}`, init);
8891
if (!res.ok) {
89-
const text = await res.text().catch(() => "");
90-
let parsed: unknown;
91-
try {
92-
parsed = text.length > 0 ? JSON.parse(text) : undefined;
93-
} catch {
94-
parsed = undefined;
95-
}
96-
const errObj = (parsed ?? {}) as { message?: string; detail?: string };
92+
const errObj = (await this.#json<{ message?: string; detail?: string }>(res)) ?? {};
9793
throw new CoderApiError({
9894
status: res.status,
9995
method,
@@ -105,9 +101,9 @@ export class CoderChatClient {
105101
return res;
106102
}
107103

108-
/** Read a response body as JSON, tolerating an empty or malformed body (→ undefined). */
104+
/** Read a response body as JSON, tolerating an unreadable/empty/malformed body (→ undefined). */
109105
async #json<T>(res: Response): Promise<T | undefined> {
110-
const text = await res.text();
106+
const text = await res.text().catch(() => "");
111107
if (text.length === 0) return undefined;
112108
try {
113109
return JSON.parse(text) as T;
@@ -221,9 +217,12 @@ export class CoderChatClient {
221217
mediaType: file.mediaType,
222218
name: file.name,
223219
});
224-
if (!CHAT_ATTACHMENT_MEDIA_TYPES.has(resolved.mediaType)) {
220+
// Match the allowlist on the media type alone, ignoring parameters such as
221+
// `; charset=utf-8` that a Blob/File commonly carries.
222+
const mediaType = (resolved.mediaType.split(";")[0] ?? resolved.mediaType).trim().toLowerCase();
223+
if (!CHAT_ATTACHMENT_MEDIA_TYPES.has(mediaType)) {
225224
throw new CoderAgentError(
226-
`Media type "${resolved.mediaType}" is not allowed for chat attachments ` +
225+
`Media type "${mediaType}" is not allowed for chat attachments ` +
227226
`(allowed: ${[...CHAT_ATTACHMENT_MEDIA_TYPES].join(", ")}). ` +
228227
`Write other file types to a workspace instead.`,
229228
);
@@ -235,18 +234,34 @@ export class CoderChatClient {
235234
);
236235
}
237236

238-
const headers: Record<string, string> = { "Content-Type": resolved.mediaType };
237+
const headers: Record<string, string> = { "Content-Type": mediaType };
239238
if (resolved.name) {
240-
headers["Content-Disposition"] =
241-
`attachment; filename="${resolved.name.replace(/"/g, '\\"')}"`;
239+
// RFC 6266: a sanitized ASCII `filename` plus an RFC 5987 `filename*` that
240+
// preserves the exact name. Both are pure ASCII, so a non-Latin-1 name
241+
// (CJK, emoji, accents) can't trip fetch's ByteString header check, and
242+
// CR/LF/quote/backslash are encoded away rather than breaking the header.
243+
const ascii = resolved.name.replace(/[^\x20-\x7e]/g, "_").replace(/(["\\])/g, "\\$1");
244+
const utf8 = encodeURIComponent(resolved.name).replace(
245+
/['()*]/g,
246+
(c) => `%${c.charCodeAt(0).toString(16).toUpperCase()}`,
247+
);
248+
headers["Content-Disposition"] = `attachment; filename="${ascii}"; filename*=UTF-8''${utf8}`;
242249
}
243250
const res = await this.#send(
244251
"POST",
245252
`${API_PREFIX}/files?organization=${encodeURIComponent(organizationId)}`,
246253
{ body: resolved.body, headers, signal },
247254
);
248255
const parsed = await this.#json<UploadChatFileResponse>(res);
249-
return { id: parsed?.id ?? "", mediaType: resolved.mediaType, name: resolved.name };
256+
if (!parsed?.id) {
257+
throw new CoderApiError({
258+
status: res.status,
259+
method: "POST",
260+
path: `${API_PREFIX}/files`,
261+
message: "upload succeeded but the response contained no file id",
262+
});
263+
}
264+
return { id: parsed.id, mediaType, name: resolved.name };
250265
}
251266

252267
/** Download a chat file's bytes and media type by id. */

packages/agent/src/files.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,8 @@ export function resolveFileContent(
8282

8383
/** Decode a base64 string to bytes (Node `Buffer` or browser `atob`). */
8484
function base64ToBytes(b64: string): Uint8Array {
85-
if (typeof Buffer !== "undefined") return new Uint8Array(Buffer.from(b64, "base64"));
85+
// `Buffer` is already a `Uint8Array`; return it directly (no extra copy).
86+
if (typeof Buffer !== "undefined") return Buffer.from(b64, "base64");
8687
const bin = atob(b64);
8788
const bytes = new Uint8Array(bin.length);
8889
for (let i = 0; i < bin.length; i++) bytes[i] = bin.charCodeAt(i);

packages/agent/src/model/language-model.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -150,10 +150,13 @@ export class CoderLanguageModel implements LanguageModelV3 {
150150
let afterId: number | undefined;
151151

152152
if (action.kind === "new-turn") {
153-
const modelConfigId = await this.#resolveModelConfigId(signal);
154-
// Upload any file parts now (before the chat exists) and resolve them to
155-
// `file` input parts referencing their uploaded ids.
156-
const content = await this.#buildContent(action.content, signal);
153+
// Resolve the model config and upload any file parts concurrently — they
154+
// are independent round-trips. (Uploads run before the chat exists and
155+
// resolve file parts to `file` input parts referencing their uploaded ids.)
156+
const [modelConfigId, content] = await Promise.all([
157+
this.#resolveModelConfigId(signal),
158+
this.#buildContent(action.content, signal),
159+
]);
157160
if (!this.#chatId) {
158161
const req: CreateChatRequest = {
159162
organization_id: this.#config.organizationId,

packages/agent/src/model/prompt.ts

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,7 +48,9 @@ function coderFileId(providerOptions: unknown): string | undefined {
4848
const ns = (providerOptions as Record<string, CoderFileProviderOptions> | undefined)?.[
4949
CODER_PROVIDER_OPTIONS
5050
];
51-
return typeof ns?.fileId === "string" ? ns.fileId : undefined;
51+
// Require a non-empty id: an empty string is not a usable reference, so fall
52+
// back to uploading rather than emitting `file_id: ""`.
53+
return typeof ns?.fileId === "string" && ns.fileId.length > 0 ? ns.fileId : undefined;
5254
}
5355

5456
/**
Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
import { describe, expect, it } from "vitest";
2+
import { CoderChatClient } from "../../src/coder/client.js";
3+
import { CoderAgentError, CoderApiError } from "../../src/errors.js";
4+
5+
type Init = RequestInit & { headers: Record<string, string> };
6+
7+
/** A fake `fetch` that records calls and returns a scripted `Response`. */
8+
function fakeFetch(handler: () => Response) {
9+
const calls: { url: string; init: Init }[] = [];
10+
const fn = ((url: string, init: Init) => {
11+
calls.push({ url, init });
12+
return Promise.resolve(handler());
13+
}) as unknown as typeof globalThis.fetch;
14+
return { fn, calls };
15+
}
16+
17+
function client(fetchFn: typeof globalThis.fetch) {
18+
return new CoderChatClient({ baseUrl: "https://x", token: "t", fetch: fetchFn });
19+
}
20+
21+
describe("CoderChatClient.uploadChatFile", () => {
22+
it("uploads bytes to the org-scoped endpoint and returns the id", async () => {
23+
const { fn, calls } = fakeFetch(
24+
() => new Response(JSON.stringify({ id: "file-1" }), { status: 201 }),
25+
);
26+
const r = await client(fn).uploadChatFile("org-1", {
27+
content: new Uint8Array([1, 2, 3]),
28+
mediaType: "application/pdf",
29+
name: "report.pdf",
30+
});
31+
32+
expect(r).toEqual({ id: "file-1", mediaType: "application/pdf", name: "report.pdf" });
33+
expect(calls[0]?.url).toBe("https://x/api/experimental/chats/files?organization=org-1");
34+
expect(calls[0]?.init.headers["Content-Type"]).toBe("application/pdf");
35+
expect(calls[0]?.init.headers["Content-Disposition"]).toBe(
36+
"attachment; filename=\"report.pdf\"; filename*=UTF-8''report.pdf",
37+
);
38+
});
39+
40+
it("normalizes a parameterized media type for the allowlist check and the header", async () => {
41+
const { fn, calls } = fakeFetch(
42+
() => new Response(JSON.stringify({ id: "f" }), { status: 201 }),
43+
);
44+
const r = await client(fn).uploadChatFile("o", {
45+
content: new Uint8Array([1]),
46+
mediaType: "text/plain; charset=utf-8",
47+
});
48+
49+
expect(r.mediaType).toBe("text/plain");
50+
expect(calls[0]?.init.headers["Content-Type"]).toBe("text/plain");
51+
});
52+
53+
it("sanitizes the ASCII filename (escapes quotes, drops CR/LF) and adds filename*", async () => {
54+
const { fn, calls } = fakeFetch(
55+
() => new Response(JSON.stringify({ id: "f" }), { status: 201 }),
56+
);
57+
await client(fn).uploadChatFile("o", {
58+
content: new Uint8Array([1]),
59+
mediaType: "text/plain",
60+
name: 'he"llo\r\nworld',
61+
});
62+
63+
const cd = calls[0]?.init.headers["Content-Disposition"] ?? "";
64+
// CR/LF → "_" in the ASCII fallback, quote escaped; exact name preserved in filename*.
65+
expect(cd).toBe(
66+
'attachment; filename="he\\"llo__world"; filename*=UTF-8\'\'he%22llo%0D%0Aworld',
67+
);
68+
expect(cd).not.toMatch(/[\r\n]/);
69+
});
70+
71+
it("encodes a non-ASCII filename instead of throwing a ByteString error", async () => {
72+
const { fn, calls } = fakeFetch(
73+
() => new Response(JSON.stringify({ id: "f" }), { status: 201 }),
74+
);
75+
await client(fn).uploadChatFile("o", {
76+
content: new Uint8Array([1]),
77+
mediaType: "application/pdf",
78+
name: "報告書.pdf",
79+
});
80+
81+
const cd = calls[0]?.init.headers["Content-Disposition"] ?? "";
82+
// Pure ASCII header value (no code point > 0x7f) so fetch can't reject it.
83+
expect([...cd].every((ch) => ch.charCodeAt(0) <= 0x7f)).toBe(true);
84+
expect(cd).toContain("filename*=UTF-8''%E5%A0%B1%E5%91%8A%E6%9B%B8.pdf");
85+
});
86+
87+
it("throws when a 2xx response carries no file id (instead of returning an empty id)", async () => {
88+
const { fn } = fakeFetch(() => new Response("", { status: 201 }));
89+
await expect(
90+
client(fn).uploadChatFile("o", { content: new Uint8Array([1]), mediaType: "text/plain" }),
91+
).rejects.toThrow(/no file id/);
92+
});
93+
94+
it("rejects a non-allowlisted media type before issuing any request", async () => {
95+
let called = false;
96+
const { fn } = fakeFetch(() => {
97+
called = true;
98+
return new Response("{}", { status: 201 });
99+
});
100+
await expect(
101+
client(fn).uploadChatFile("o", {
102+
content: new Uint8Array([1]),
103+
mediaType: "application/zip",
104+
}),
105+
).rejects.toThrow(CoderAgentError);
106+
expect(called).toBe(false);
107+
});
108+
109+
it("surfaces a non-2xx upload as a CoderApiError", async () => {
110+
const { fn } = fakeFetch(
111+
() => new Response(JSON.stringify({ message: "too big" }), { status: 413 }),
112+
);
113+
await expect(
114+
client(fn).uploadChatFile("o", { content: new Uint8Array([1]), mediaType: "text/plain" }),
115+
).rejects.toThrow(CoderApiError);
116+
});
117+
});

0 commit comments

Comments
 (0)