diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 34f095f887..fd40b579af 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -2145,7 +2145,10 @@ export { createCostEventSchema, createFinanceEventSchema, updateBudgetSchema, + ASSET_NAMESPACE_MAX_LENGTH, + ASSET_NAMESPACE_RULE, createAssetImageMetadataSchema, + sanitizeAssetNamespace, createCompanyInviteSchema, createOpenClawInvitePromptSchema, acceptInviteSchema, diff --git a/packages/shared/src/validators/asset.test.ts b/packages/shared/src/validators/asset.test.ts new file mode 100644 index 0000000000..cc6b4a18b1 --- /dev/null +++ b/packages/shared/src/validators/asset.test.ts @@ -0,0 +1,156 @@ +import { describe, expect, it } from "vitest"; +import { + ASSET_NAMESPACE_MAX_LENGTH, + createAssetImageMetadataSchema, + sanitizeAssetNamespace, +} from "./asset.js"; + +function parseNamespace(namespace: string) { + return createAssetImageMetadataSchema.safeParse({ namespace }); +} + +describe("createAssetImageMetadataSchema", () => { + it("accepts a plain slug namespace", () => { + expect(parseNamespace("goals/drafts").success).toBe(true); + }); + + it("accepts identity-provider user ids", () => { + const namespaces = [ + "profiles/oidc:example|user-1", + "profiles/jane.example@example.com", + "profiles/auth0|507f1f77bcf86cd799439011", + "profiles/https:__id.example.com_users_1", + ]; + for (const namespace of namespaces) { + expect(parseNamespace(namespace), namespace).toMatchObject({ success: true }); + } + }); + + it("accepts filenames that contain a dot", () => { + expect(parseNamespace("agents/agent-1/instructions/SKILL.md").success).toBe(true); + }); + + it("treats the namespace as optional", () => { + expect(createAssetImageMetadataSchema.parse({})).toEqual({}); + }); + + it("trims surrounding whitespace", () => { + expect(createAssetImageMetadataSchema.parse({ namespace: " goals " })).toEqual({ + namespace: "goals", + }); + }); + + it("rejects namespaces with characters outside the accepted set", () => { + const namespaces = ["profiles/bad name!", "profiles/user#1", "profiles/user\\1", "profiles/user?x=1"]; + for (const namespace of namespaces) { + expect(parseNamespace(namespace).success, namespace).toBe(false); + } + }); + + it("rejects empty and whitespace-only namespaces", () => { + expect(parseNamespace("").success).toBe(false); + expect(parseNamespace(" ").success).toBe(false); + }); + + it("rejects namespaces longer than the maximum length", () => { + expect(parseNamespace("a".repeat(ASSET_NAMESPACE_MAX_LENGTH)).success).toBe(true); + expect(parseNamespace("a".repeat(ASSET_NAMESPACE_MAX_LENGTH + 1)).success).toBe(false); + }); + + it("rejects dot path segments", () => { + const namespaces = ["profiles/../secrets", "../secrets", "profiles/./self", "profiles/..", "."]; + for (const namespace of namespaces) { + expect(parseNamespace(namespace).success, namespace).toBe(false); + } + }); + + it("names the namespace field in the failure message", () => { + const parsed = parseNamespace("profiles/bad name!"); + expect(parsed.success).toBe(false); + expect(parsed.error?.issues[0]?.message).toContain("namespace"); + expect(parsed.error?.issues[0]?.path).toEqual(["namespace"]); + }); +}); + +describe("sanitizeAssetNamespace", () => { + it("keeps identity-provider user ids unchanged", () => { + const namespaces = [ + "profiles/oidc:example|user-1", + "profiles/jane.example@example.com", + "agents/agent-1/instructions/SKILL.md", + ]; + for (const namespace of namespaces) { + expect(sanitizeAssetNamespace(namespace), namespace).toBe(namespace); + } + }); + + it("replaces characters outside the accepted set with a dash", () => { + expect(sanitizeAssetNamespace("profiles/bad name")).toBe("profiles/bad-name"); + expect(sanitizeAssetNamespace("profiles/user#1")).toBe("profiles/user-1"); + }); + + it("collapses repeated dashes", () => { + expect(sanitizeAssetNamespace("profiles/a b")).toBe("profiles/a-b"); + expect(sanitizeAssetNamespace("profiles/a???b")).toBe("profiles/a-b"); + }); + + it("removes control characters", () => { + expect(sanitizeAssetNamespace("profiles/user\u0000\u0007id")).toBe("profiles/user-id"); + }); + + it("trims whitespace around every segment", () => { + expect(sanitizeAssetNamespace(" profiles / user-1 ")).toBe("profiles/user-1"); + }); + + it("drops empty segments", () => { + expect(sanitizeAssetNamespace("profiles//user-1/")).toBe("profiles/user-1"); + expect(sanitizeAssetNamespace("/profiles/user-1")).toBe("profiles/user-1"); + }); + + it("drops the . and .. segments", () => { + expect(sanitizeAssetNamespace("profiles/../secrets")).toBe("profiles/secrets"); + expect(sanitizeAssetNamespace("profiles/./user-1")).toBe("profiles/user-1"); + expect(sanitizeAssetNamespace("../../etc/passwd")).toBe("etc/passwd"); + }); + + it("keeps a segment of three or more dots, which the schema accepts", () => { + expect(sanitizeAssetNamespace("profiles/...")).toBe("profiles/..."); + expect(sanitizeAssetNamespace("profiles/....")).toBe("profiles/...."); + expect(parseNamespace("profiles/...").success).toBe(true); + }); + + it("caps the result at the maximum length", () => { + const long = `profiles/${"a".repeat(400)}`; + const sanitized = sanitizeAssetNamespace(long); + expect(sanitized).toBeDefined(); + expect(sanitized?.length).toBe(ASSET_NAMESPACE_MAX_LENGTH); + }); + + it("does not leave a dot segment behind after a cut", () => { + const long = `${"a".repeat(ASSET_NAMESPACE_MAX_LENGTH - 3)}/..b`; + expect(sanitizeAssetNamespace(long)).toBe("a".repeat(ASSET_NAMESPACE_MAX_LENGTH - 3)); + }); + + it("returns undefined when no segment survives", () => { + expect(sanitizeAssetNamespace("")).toBeUndefined(); + expect(sanitizeAssetNamespace(" ")).toBeUndefined(); + expect(sanitizeAssetNamespace("///")).toBeUndefined(); + expect(sanitizeAssetNamespace("../..")).toBeUndefined(); + }); + + it("returns a value that the schema accepts", () => { + const inputs = [ + "profiles/oidc:example|user-1", + "profiles/bad name!", + "profiles/../secrets", + `profiles/${"a".repeat(400)}`, + `${"a".repeat(ASSET_NAMESPACE_MAX_LENGTH - 3)}/..b`, + "profiles/user\u0000id", + ]; + for (const input of inputs) { + const sanitized = sanitizeAssetNamespace(input); + expect(sanitized, input).toBeDefined(); + expect(parseNamespace(sanitized as string).success, `${input} -> ${sanitized}`).toBe(true); + } + }); +}); diff --git a/packages/shared/src/validators/asset.ts b/packages/shared/src/validators/asset.ts index 4283e31ad6..93b9a747b0 100644 --- a/packages/shared/src/validators/asset.ts +++ b/packages/shared/src/validators/asset.ts @@ -1,14 +1,81 @@ import { z } from "zod"; +/** Maximum length of an asset namespace, in characters. */ +export const ASSET_NAMESPACE_MAX_LENGTH = 120; + +/** + * Characters that an asset namespace can contain. + * + * The set is wider than a plain slug because namespaces embed user ids. A + * hosted deployment takes user ids from its identity layer without a change, + * and an OIDC subject often contains ":", "|", "." or "@". + */ +const ASSET_NAMESPACE_PATTERN = /^[a-zA-Z0-9/_.:@|-]+$/; + +/** Characters that a single namespace segment cannot contain. */ +const DISALLOWED_SEGMENT_CHARS = /[^a-zA-Z0-9_.:@|-]+/g; + +/** Human-readable statement of the namespace rule. Reused in API errors. */ +export const ASSET_NAMESPACE_RULE = `"namespace" must be 1-${ASSET_NAMESPACE_MAX_LENGTH} characters of letters, numbers, or / _ - . : @ |, and cannot contain "." or ".." path segments`; + +/** True when the segment is a relative path step, which is traversal. */ +function isDotSegment(segment: string): boolean { + return segment === "." || segment === ".."; +} + +/** + * True when the segment is safe to keep in a namespace. + * + * The schema and the sanitizer share `isDotSegment`, so the sanitizer never + * drops a segment that the schema accepts. A segment of three or more dots is + * an ordinary directory name, and it survives. + */ +function isUsableSegment(segment: string): boolean { + return segment.length > 0 && !isDotSegment(segment); +} + +function hasNoDotSegments(namespace: string): boolean { + return !namespace.split("/").some(isDotSegment); +} + export const createAssetImageMetadataSchema = z.object({ namespace: z .string() .trim() .min(1) - .max(120) - .regex(/^[a-zA-Z0-9/_-]+$/) + .max(ASSET_NAMESPACE_MAX_LENGTH) + .regex(ASSET_NAMESPACE_PATTERN, ASSET_NAMESPACE_RULE) + .refine(hasNoDotSegments, { message: ASSET_NAMESPACE_RULE }) .optional(), }); export type CreateAssetImageMetadata = z.infer; +/** + * Make a namespace that `createAssetImageMetadataSchema` accepts. + * + * Each "/"-separated segment keeps its accepted characters. Other characters + * become "-", repeated dashes collapse, and empty, "." and ".." segments go + * away. The result is at most `ASSET_NAMESPACE_MAX_LENGTH` characters. + * + * Returns undefined when no segment survives. Callers then send no namespace + * and the server applies its default. + */ +export function sanitizeAssetNamespace(namespace: string): string | undefined { + const cleaned = namespace + .split("/") + .map((segment) => segment.trim().replace(DISALLOWED_SEGMENT_CHARS, "-").replace(/-{2,}/g, "-")) + .filter(isUsableSegment) + .join("/"); + if (cleaned.length <= ASSET_NAMESPACE_MAX_LENGTH) { + return cleaned.length > 0 ? cleaned : undefined; + } + + // A cut can leave an empty, "." or ".." tail segment, so filter again. + const truncated = cleaned + .slice(0, ASSET_NAMESPACE_MAX_LENGTH) + .split("/") + .filter(isUsableSegment) + .join("/"); + return truncated.length > 0 ? truncated : undefined; +} diff --git a/packages/shared/src/validators/index.ts b/packages/shared/src/validators/index.ts index 08dbb03be3..f5637f0cbc 100644 --- a/packages/shared/src/validators/index.ts +++ b/packages/shared/src/validators/index.ts @@ -701,7 +701,10 @@ export { } from "./finance.js"; export { + ASSET_NAMESPACE_MAX_LENGTH, + ASSET_NAMESPACE_RULE, createAssetImageMetadataSchema, + sanitizeAssetNamespace, type CreateAssetImageMetadata, } from "./asset.js"; diff --git a/server/src/__tests__/assets.test.ts b/server/src/__tests__/assets.test.ts index fba6b09af2..724f8d42a3 100644 --- a/server/src/__tests__/assets.test.ts +++ b/server/src/__tests__/assets.test.ts @@ -174,6 +174,68 @@ describe("POST /api/companies/:companyId/assets/images", () => { }); }); + it("accepts namespaces that hold identity-provider user ids", async () => { + const png = createStorageService("image/png"); + const app = await createApp(png); + + createAssetMock.mockResolvedValue(createAsset()); + + const namespace = "profiles/oidc:example|jane.example@example.com"; + const res = await requestApp(app, (baseUrl) => + request(baseUrl) + .post("/api/companies/company-1/assets/images") + .field("namespace", namespace) + .attach("file", Buffer.from("png"), "avatar.png"), + ); + + expect(res.status, JSON.stringify(res.body)).toBe(201); + expect(png.__calls.putFileInputs[0]).toMatchObject({ + companyId: "company-1", + namespace: `assets/${namespace}`, + originalFilename: "avatar.png", + contentType: "image/png", + }); + }); + + it("rejects namespaces with characters outside the accepted set", async () => { + const png = createStorageService("image/png"); + const app = await createApp(png); + + createAssetMock.mockResolvedValue(createAsset()); + + const res = await requestApp(app, (baseUrl) => + request(baseUrl) + .post("/api/companies/company-1/assets/images") + .field("namespace", "profiles/bad name!") + .attach("file", Buffer.from("png"), "avatar.png"), + ); + + expect(res.status).toBe(400); + expect(res.body.error).toContain("namespace"); + expect(res.body.details?.[0]?.path).toEqual(["namespace"]); + expect(png.__calls.putFileInputs).toHaveLength(0); + expect(createAssetMock).not.toHaveBeenCalled(); + }); + + it("rejects namespaces that hold a dot path segment", async () => { + const png = createStorageService("image/png"); + const app = await createApp(png); + + createAssetMock.mockResolvedValue(createAsset()); + + const res = await requestApp(app, (baseUrl) => + request(baseUrl) + .post("/api/companies/company-1/assets/images") + .field("namespace", "profiles/../secrets") + .attach("file", Buffer.from("png"), "avatar.png"), + ); + + expect(res.status).toBe(400); + expect(res.body.error).toContain("namespace"); + expect(png.__calls.putFileInputs).toHaveLength(0); + expect(createAssetMock).not.toHaveBeenCalled(); + }); + it("allows supported non-image attachments outside the company logo flow", async () => { const text = createStorageService("text/plain"); const app = await createApp(text); diff --git a/server/src/routes/assets.ts b/server/src/routes/assets.ts index db8de4b030..8a4aea02d3 100644 --- a/server/src/routes/assets.ts +++ b/server/src/routes/assets.ts @@ -3,7 +3,7 @@ import multer from "multer"; import createDOMPurify from "dompurify"; import { JSDOM } from "jsdom"; import type { Db } from "@paperclipai/db"; -import { createAssetImageMetadataSchema } from "@paperclipai/shared"; +import { ASSET_NAMESPACE_RULE, createAssetImageMetadataSchema } from "@paperclipai/shared"; import type { StorageService } from "../storage/types.js"; import { assetService, logActivity } from "../services/index.js"; import { isAllowedContentType, MAX_ATTACHMENT_BYTES } from "../attachment-types.js"; @@ -133,7 +133,10 @@ export function assetRoutes(db: Db, storage: StorageService) { const parsedMeta = createAssetImageMetadataSchema.safeParse(req.body ?? {}); if (!parsedMeta.success) { - res.status(400).json({ error: "Invalid image metadata", details: parsedMeta.error.issues }); + res.status(400).json({ + error: `Invalid image metadata: ${ASSET_NAMESPACE_RULE}`, + details: parsedMeta.error.issues, + }); return; } diff --git a/ui/src/api/assets.ts b/ui/src/api/assets.ts index 6fcf323f4d..bf61818870 100644 --- a/ui/src/api/assets.ts +++ b/ui/src/api/assets.ts @@ -1,4 +1,4 @@ -import type { AssetImage } from "@paperclipai/shared"; +import { sanitizeAssetNamespace, type AssetImage } from "@paperclipai/shared"; import { api } from "./client"; export const assetsApi = { @@ -11,8 +11,11 @@ export const assetsApi = { const safeFile = new File([buffer], file.name, { type: file.type }); const form = new FormData(); - if (namespace && namespace.trim().length > 0) { - form.append("namespace", namespace.trim()); + // Callers build namespaces from ids and filenames that can hold characters + // the API rejects. Clean the namespace here so every caller is covered. + const safeNamespace = namespace ? sanitizeAssetNamespace(namespace) : undefined; + if (safeNamespace) { + form.append("namespace", safeNamespace); } form.append("file", safeFile); return api.postForm(`/companies/${companyId}/assets/images`, form); diff --git a/ui/src/pages/ProfileSettings.test.tsx b/ui/src/pages/ProfileSettings.test.tsx index 72a9425936..17270809f7 100644 --- a/ui/src/pages/ProfileSettings.test.tsx +++ b/ui/src/pages/ProfileSettings.test.tsx @@ -4,6 +4,7 @@ import { act } from "react"; import { createRoot } from "react-dom/client"; import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { sanitizeAssetNamespace } from "@paperclipai/shared"; import { ProfileSettings } from "./ProfileSettings"; const mockAuthApi = vi.hoisted(() => ({ @@ -130,4 +131,61 @@ describe("ProfileSettings", () => { root.unmount(); }); }); + + it("uploads an avatar for a user id that comes from an identity provider", async () => { + const userId = "oidc:example|jane.example@example.com"; + mockAuthApi.getSession.mockResolvedValue({ + session: { id: "session-1", userId }, + user: { + id: userId, + name: "Jane Example", + email: "jane@example.com", + image: "https://example.com/jane.png", + }, + }); + + const root = createRoot(container); + const queryClient = new QueryClient({ + defaultOptions: { queries: { retry: false } }, + }); + + await act(async () => { + root.render( + + + , + ); + }); + await flushReact(); + await flushReact(); + + const avatarInput = container.querySelector('input[type="file"]') as HTMLInputElement | null; + expect(avatarInput).not.toBeNull(); + + const file = new File(["avatar"], "avatar.png", { type: "image/png" }); + Object.defineProperty(avatarInput, "files", { + configurable: true, + value: [file], + }); + + await act(async () => { + avatarInput?.dispatchEvent(new Event("change", { bubbles: true })); + }); + await flushReact(); + await flushReact(); + + const namespace = `profiles/${userId}`; + expect(mockAssetsApi.uploadImage).toHaveBeenCalledWith("company-1", file, namespace); + // The namespace goes to the API without a change, so the avatar arrives + // under the identity of the user. + expect(sanitizeAssetNamespace(namespace)).toBe(namespace); + expect(mockAuthApi.updateProfile).toHaveBeenCalledWith({ + name: "Jane Example", + image: "/api/assets/asset-1/content", + }); + + await act(async () => { + root.unmount(); + }); + }); });