From e3c80ba550f5d399789e79eb5fe8414e469b069d Mon Sep 17 00:00:00 2001 From: NiallJoeMaher Date: Mon, 26 Oct 2026 19:47:00 +0000 Subject: [PATCH 1/2] Before going public: an AI tool gets read and write unless it names delete, and the review's other security fixes - An AI tool that names no scope now gets read and write: Better Auth stored every registered client with every scope, delete and the account included, and gave an authorize with no scope all of them. The guard narrows it, refuses the account scope for anything not ours, and the connector advertises read and write only. - Sign-in's next path refuses anything a browser reads as another site ("/\evil.com", control characters). - A public production server refuses ALLOW_PRIVATE_FETCH and AUTH_RATE_LIMIT=off, and page fetches also refuse NAT64, 6to4, 192.0.0.0/24 and 240.0.0.0/4. - "Send me one now" sends at most five digests a day. - Covers and images are kept only as JPEG, PNG, WebP, GIF or AVIF: an SVG can carry script. - The stdio bridge builds before it is published, and has a README, homepage and issues link. - .gitignore: Claude Code worktrees and personal settings, tsbuildinfo, coverage. Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01GZorAwPFkDvfWHxJNqzu9B Signed-off-by: NiallJoeMaher --- .gitignore | 6 ++++++ apps/web/app/api/v1/digest/send/route.ts | 12 +++++++++--- apps/web/app/signin/page.tsx | 3 ++- apps/web/instrumentation.ts | 4 ++++ apps/web/lib/auth-guard.test.ts | 13 +++++++++++++ apps/web/lib/auth.ts | 13 +++++++++++++ apps/web/lib/capture.ts | 7 +++++-- apps/web/lib/same-site.test.ts | 8 ++++++++ apps/web/lib/same-site.ts | 8 ++++++++ packages/extract/src/fetch.test.ts | 2 +- packages/extract/src/fetch.ts | 5 +++-- packages/mcp-stdio/README.md | 19 +++++++++++++++++++ packages/mcp-stdio/package.json | 5 ++++- 13 files changed, 95 insertions(+), 10 deletions(-) create mode 100644 apps/web/lib/same-site.test.ts create mode 100644 apps/web/lib/same-site.ts create mode 100644 packages/mcp-stdio/README.md diff --git a/.gitignore b/.gitignore index 42588b6..2d5a961 100644 --- a/.gitignore +++ b/.gitignore @@ -15,3 +15,9 @@ cdk.out/ apps/ios/ios/ apps/ios/.expo/ .vercel + +# Claude Code: worktrees and personal settings +.claude/worktrees/ +.claude/settings.local.json +*.tsbuildinfo +coverage/ diff --git a/apps/web/app/api/v1/digest/send/route.ts b/apps/web/app/api/v1/digest/send/route.ts index 8603a22..ca3f503 100644 --- a/apps/web/app/api/v1/digest/send/route.ts +++ b/apps/web/app/api/v1/digest/send/route.ts @@ -1,5 +1,11 @@ -import { json, route } from "@/lib/api"; +import { hitRateLimit } from "@pluck/db"; +import { ApiError, json, route } from "@/lib/api"; import { sendDigest } from "@/lib/library"; -// "Send me one now" from Settings: the digest as it would look today, whatever the day. -export const POST = route({ kind: "write" }, async ({ caller }) => json({ sent: await sendDigest(caller.userId, { force: true }) })); +// "Send me one now" from Settings: the digest as it would look today, whatever the day. Five a day: each is an email +// we pay for, and a key could otherwise send thousands. +export const POST = route({ kind: "write" }, async ({ caller }) => { + const r = await hitRateLimit(`digest-send:${caller.userId}`, 5, 24 * 60 * 60); + if (!r.ok) throw new ApiError("rate_limited", "That's five today. Try again tomorrow.", 429, { retryAfter: r.retryAfter }); + return json({ sent: await sendDigest(caller.userId, { force: true }) }); +}); diff --git a/apps/web/app/signin/page.tsx b/apps/web/app/signin/page.tsx index bfa9160..66deae6 100644 --- a/apps/web/app/signin/page.tsx +++ b/apps/web/app/signin/page.tsx @@ -4,6 +4,7 @@ import { cookies } from "next/headers"; import { redirect } from "next/navigation"; import { TRY_COOKIE } from "@/lib/api"; import { currentUser } from "@/lib/auth"; +import { samePath } from "@/lib/same-site"; import { SignInForm } from "./SignInForm"; export async function generateMetadata(): Promise { @@ -11,7 +12,7 @@ export async function generateMetadata(): Promise { } // Only same-site paths, so a crafted link cannot bounce people elsewhere after sign-in. -const safeNext = (n?: string) => (n && n.startsWith("/") && !n.startsWith("//") ? n : "/inbox"); +const safeNext = (n?: string) => samePath(n, "/inbox"); // Sent here by the OAuth provider (an AI tool connecting): after the email link signs you in, carry on // with the same authorization request. The signing fields only matter to this page, so they are dropped. diff --git a/apps/web/instrumentation.ts b/apps/web/instrumentation.ts index ec68988..79bfc08 100644 --- a/apps/web/instrumentation.ts +++ b/apps/web/instrumentation.ts @@ -9,6 +9,10 @@ export function register() { if (secret === "dev-only-secret-change-me-in-production-0123456789" || secret.length < 32) throw new Error("BETTER_AUTH_SECRET is the example one or shorter than 32 characters: set 32+ random bytes (openssl rand -base64 32)."); if (process.env.CRON_SECRET === "dev-cron-secret") throw new Error("CRON_SECRET is the example one: set a random value (openssl rand -hex 32)."); + // The two switches for tests (docs/self-host.md): on a public server one opens the private network to page fetches, + // the other lets anyone guess sign-in codes without limit. + if (process.env.ALLOW_PRIVATE_FETCH === "1") throw new Error("ALLOW_PRIVATE_FETCH is for local development and tests only: unset it."); + if (process.env.AUTH_RATE_LIMIT === "off") throw new Error("AUTH_RATE_LIMIT=off is for local development and tests only: unset it."); } // Errors thrown while rendering a page or running a route that did not catch them (route handlers in lib/api.ts diff --git a/apps/web/lib/auth-guard.test.ts b/apps/web/lib/auth-guard.test.ts index 2117823..7ac8276 100644 --- a/apps/web/lib/auth-guard.test.ts +++ b/apps/web/lib/auth-guard.test.ts @@ -31,6 +31,19 @@ test("only our own apps can ask for the account scope", async () => { expect((await register("Pluck for iPhone", "io.getpluck.app:/oauth", [`${BASE}/api/v1`], "offline_access library:read library:write account")).status).toBe(201); }); +test("an AI tool that names no scope gets read and write, never delete or the account", async () => { + const { client_id } = (await (await register("Example AI", "https://ai.example.test/cb")).json()) as { client_id: string }; + const authorize = (scope?: string) => + auth.handler(new Request(`${BASE}/api/auth/oauth2/authorize?${new URLSearchParams({ + response_type: "code", client_id, redirect_uri: "https://ai.example.test/cb", state: "s", code_challenge_method: "S256", + code_challenge: "E9Melhoa2OwvFrEMTJguCHaoeK1t8URWbuGJSstw-cM", resource: `${BASE}/mcp`, ...(scope ? { scope } : {}), + })}`, { headers: { "x-forwarded-for": `198.51.100.${++ip}` } })); + const scopeOf = (res: Response) => new URL(res.headers.get("location") ?? "", BASE).searchParams.get("scope"); + expect(scopeOf(await authorize())).toBe("offline_access library:read library:write"); // on to sign-in, narrowed + expect(scopeOf(await authorize("library:read library:delete"))).toBe("library:read library:delete"); // named, so the consent screen shows it + expect((await authorize("library:read account")).status).toBe(400); +}); + test("sign-in links are limited per address, whatever network asks: one a minute, six an hour", async () => { const email = `limit-${Date.now()}@example.test`; vi.useFakeTimers({ toFake: ["Date"] }); diff --git a/apps/web/lib/auth.ts b/apps/web/lib/auth.ts index 01a390f..832c3e9 100644 --- a/apps/web/lib/auth.ts +++ b/apps/web/lib/auth.ts @@ -28,6 +28,8 @@ export const API_RESOURCE = `${ORIGIN}/api/v1`; export const LIBRARY_SCOPES = ["library:read", "library:write", "library:delete"] as const; // Account settings a browser session has (connected AI tools, export, deleting it all), for our own apps only. export const ACCOUNT_SCOPE = "account"; +// What an AI tool gets when it names no scope, and all the connector advertises: never delete (CLAUDE.md) or the account. +export const AI_TOOL_SCOPES = ["offline_access", "library:read", "library:write"]; // Where the extension and the iPhone app take their sign-in back. Codes sent here never leave our apps. export const APP_REDIRECT = "io.getpluck.app:/oauth"; export const FIRST_PARTY_REDIRECTS = [`${ORIGIN}/extension/connected`, APP_REDIRECT]; @@ -70,6 +72,15 @@ const guard = createAuthMiddleware(async (ctx) => { if ((wantsApi || wantsAccount || /pluck/i.test(String(body.client_name ?? ""))) && !firstParty(body.redirect_uris)) throw new APIError("BAD_REQUEST", { error: "invalid_client_metadata", error_description: "Only Pluck's own apps can register for the Pluck API or use its name." }); } + // Better Auth stores every registered client with every scope Pluck has, whatever it asked for, and gives an authorize + // with no scope all of them. So the line is drawn here: an AI tool, or any client not ours, gets read and write unless + // it names delete itself, and never the account. + if (ctx.path === "/oauth2/authorize" && !firstParty([redirectOf(ctx)])) { + const asked = String(ctx.query?.scope ?? "").split(" ").filter(Boolean); + if (asked.includes(ACCOUNT_SCOPE)) + throw new APIError("BAD_REQUEST", { error: "invalid_scope", error_description: "The account scope is for Pluck's own apps." }); + if (!asked.length) return { context: { query: { ...ctx.query, scope: AI_TOOL_SCOPES.join(" ") } } }; + } const email = String((ctx.body as { email?: unknown } | undefined)?.email ?? "").trim().toLowerCase(); // The review address gets no link: the form asks for its password instead. if (ctx.path === "/sign-in/magic-link" && email === REVIEW?.email) return { password: true }; @@ -142,6 +153,8 @@ export const auth = betterAuth({ consentPage: "/oauth/consent", resource: MCP_RESOURCE, scopes: ["openid", "profile", "email", "offline_access", ...LIBRARY_SCOPES, ACCOUNT_SCOPE], + // Clients ask for what is advertised (the MCP spec says scopes_supported), so delete and the account stay unlisted. + advertisedMetadata: { scopes_supported: ["openid", "profile", "email", ...AI_TOOL_SCOPES] }, accessTokenExpiresIn: 60 * 60, refreshTokenExpiresIn: 90 * DAY, // CIMD first; dynamic registration as the fallback for clients that do not speak CIMD yet. diff --git a/apps/web/lib/capture.ts b/apps/web/lib/capture.ts index 2d40d2f..7876e08 100644 --- a/apps/web/lib/capture.ts +++ b/apps/web/lib/capture.ts @@ -24,10 +24,13 @@ const toResult = (x: Extracted): ExtractionResult => ({ canonicalUrl: x.canonicalUrl, lang: x.lang, wordCount: x.wordCount, markdown: x.markdown, extractor: x.extractor, }); +// Pictures only: an SVG can carry script, and a self-host may serve its blob store from its own domain. +const picture = (contentType: string) => /^image\/(jpeg|png|webp|gif|avif)\b/i.test(contentType); + // Raw page, file and cover to the blob store. Covers go through the same guarded fetch as pages. export async function storeBlobs(userId: string, itemId: string, x: Pick): Promise { const update: { coverKey?: string; htmlKey?: string } = {}; - if (x.raw) { + if (x.raw && (x.kind !== "image" || picture(x.raw.contentType))) { const key = x.kind === "pdf" ? keys.file(userId, itemId, "pdf") : x.kind === "image" ? keys.cover(userId, itemId) : keys.html(userId, itemId); await putObject(key, x.raw.body, x.raw.contentType); if (x.kind === "image") update.coverKey = key; @@ -36,7 +39,7 @@ export async function storeBlobs(userId: string, itemId: string, x: Pick { + for (const ok of ["/inbox", "/items/abc?x=1#top", "/lists/a/b", "/try/claim"]) expect(samePath(ok, "/inbox")).toBe(ok); + for (const bad of [undefined, "", "inbox", "//evil.com", "/\\evil.com", "/\t/evil.com", "/\\/evil.com", "https://evil.com", "/a\u0000b", "/a\nb", "\\\\evil.com"]) + expect(samePath(bad, "/inbox")).toBe("/inbox"); +}); diff --git a/apps/web/lib/same-site.ts b/apps/web/lib/same-site.ts new file mode 100644 index 0000000..27797fb --- /dev/null +++ b/apps/web/lib/same-site.ts @@ -0,0 +1,8 @@ +// Where a link may send someone after signing in: a path on this site and nowhere else. "/\evil.com" and "/\t/evil.com" +// are other sites to a browser (a backslash is a slash, a tab is dropped), so any backslash or control character is +// refused, and what is left must resolve to this origin. +export function samePath(next: string | undefined, fallback: string): string { + if (!next || !/^\/(?![/\\])/.test(next) || /[\\\u0000-\u001f\u007f]/.test(next)) return fallback; + const base = "https://pluck.invalid"; + return new URL(next, base).origin === base ? next : fallback; +} diff --git a/packages/extract/src/fetch.test.ts b/packages/extract/src/fetch.test.ts index f1e44c4..07b8d77 100644 --- a/packages/extract/src/fetch.test.ts +++ b/packages/extract/src/fetch.test.ts @@ -5,7 +5,7 @@ afterEach(() => { delete process.env.ALLOW_PRIVATE_FETCH; }); -test.each(["127.0.0.1", "10.1.2.3", "172.20.0.1", "192.168.1.1", "169.254.169.254", "::1", "fd00::1", "::ffff:127.0.0.1"])("%s is private", (ip) => { +test.each(["127.0.0.1", "10.1.2.3", "172.20.0.1", "192.168.1.1", "169.254.169.254", "192.0.0.8", "240.1.2.3", "::1", "fd00::1", "::ffff:127.0.0.1", "64:ff9b::7f00:1", "2002:7f00:1::"])("%s is private", (ip) => { expect(isPrivateAddress(ip)).toBe(true); }); diff --git a/packages/extract/src/fetch.ts b/packages/extract/src/fetch.ts index cbfa6cd..bf07852 100644 --- a/packages/extract/src/fetch.ts +++ b/packages/extract/src/fetch.ts @@ -10,9 +10,10 @@ export const BROWSER_UA = // Never fetch our own network on a user's behalf. Checked on the URL host and again at connect time, // so DNS answers and redirects cannot point the server at private addresses. const blocked = new BlockList(); -for (const [net, prefix] of [["0.0.0.0", 8], ["10.0.0.0", 8], ["100.64.0.0", 10], ["127.0.0.0", 8], ["169.254.0.0", 16], ["172.16.0.0", 12], ["192.168.0.0", 16], ["198.18.0.0", 15], ["224.0.0.0", 4]] as const) +for (const [net, prefix] of [["0.0.0.0", 8], ["10.0.0.0", 8], ["100.64.0.0", 10], ["127.0.0.0", 8], ["169.254.0.0", 16], ["172.16.0.0", 12], ["192.0.0.0", 24], ["192.168.0.0", 16], ["198.18.0.0", 15], ["224.0.0.0", 4], ["240.0.0.0", 4]] as const) blocked.addSubnet(net, prefix, "ipv4"); -for (const [net, prefix] of [["::", 128], ["::1", 128], ["fc00::", 7], ["fe80::", 10]] as const) +// NAT64 (64:ff9b::/96) and 6to4 (2002::/16) carry an IPv4 address inside, private ones included: refused whole. +for (const [net, prefix] of [["::", 128], ["::1", 128], ["64:ff9b::", 96], ["2002::", 16], ["fc00::", 7], ["fe80::", 10]] as const) blocked.addSubnet(net, prefix, "ipv6"); export function isPrivateAddress(ip: string): boolean { diff --git a/packages/mcp-stdio/README.md b/packages/mcp-stdio/README.md new file mode 100644 index 0000000..460b65b --- /dev/null +++ b/packages/mcp-stdio/README.md @@ -0,0 +1,19 @@ +# @pluck/mcp + +Your [Pluck](https://getpluck.io) library in any MCP client that speaks stdio, with an API key. Clients that connect to +a remote server (Claude, ChatGPT, Cursor) can use `https://getpluck.io/mcp` directly and need none of this. + +```sh +npx @pluck/mcp --key pk_live_… +``` + +Make a key in Pluck, Settings, AI tools. The key can also come from `PLUCK_API_KEY`. Running your own Pluck? Add +`--server https://pluck.example.com` (or set `PLUCK_SERVER`). + +In a client's config: + +```json +{ "mcpServers": { "pluck": { "command": "npx", "args": ["@pluck/mcp"], "env": { "PLUCK_API_KEY": "pk_live_…" } } } } +``` + +Requires Node 22. Apache-2.0. Source: [codu-code/pluck](https://github.com/codu-code/pluck/tree/main/packages/mcp-stdio). diff --git a/packages/mcp-stdio/package.json b/packages/mcp-stdio/package.json index 67c9e73..0672849 100644 --- a/packages/mcp-stdio/package.json +++ b/packages/mcp-stdio/package.json @@ -1,6 +1,8 @@ { "name": "@pluck/mcp", "repository": { "type": "git", "url": "https://github.com/codu-code/pluck.git", "directory": "packages/mcp-stdio" }, + "homepage": "https://getpluck.io/connect", + "bugs": "https://github.com/codu-code/pluck/issues", "version": "0.1.0", "description": "Use your Pluck library from any MCP client that speaks stdio, with an API key.", "license": "Apache-2.0", @@ -12,7 +14,8 @@ "engines": { "node": ">=22" }, "scripts": { "typecheck": "tsc -p .", - "build": "tsc -p tsconfig.build.json" + "build": "tsc -p tsconfig.build.json", + "prepublishOnly": "pnpm build" }, "dependencies": { "@modelcontextprotocol/client": "2.4.0", From ec460c3af01f3bf2929acc8cce53508ac85e4d77 Mon Sep 17 00:00:00 2001 From: NiallJoeMaher Date: Mon, 26 Oct 2026 19:47:43 +0000 Subject: [PATCH 2/2] Uploads and imports have a size the worker will read, a zip bomb is refused, a cover stops at 2 MB - @pluck/blob: getObject(key, { maxBytes }) refuses an object over the limit from its Content-Length, before the body is read (TooLarge). - A shared file over 50 MB marks the item failed and removes the blob; an import over 500 MB fails with that reason. The limits now live next to the schemas that declare them (MAX_FILE_BYTES, MAX_IMPORT_BYTES). - Email-in refuses a message over 10 MB before reading it. - Omnivore: entries over 20 MB, or metadata over 100 MB in all, are refused before fflate allocates them; a page too big to keep is fetched instead. - Cover PUT checks content-length first and still stops reading at 2 MB. - vitest resolves the web app's "@/" path so route handlers can be tested. The signed PUT URL still does not bind a length: the shipped iPhone app sends size 1 when it cannot read a file's size, and that upload would fail. Signed-off-by: NiallJoeMaher Co-Authored-By: Claude Opus 5.5 (1M context) Claude-Session: https://claude.ai/code/session_01GZorAwPFkDvfWHxJNqzu9B --- .../app/api/v1/items/[id]/cover/route.test.ts | 24 +++++++++++++++++++ apps/web/app/api/v1/items/[id]/cover/route.ts | 23 ++++++++++++++++-- apps/web/lib/library/account.ts | 13 ++++++---- apps/web/lib/library/email-in.ts | 12 +++++++--- apps/web/lib/library/files.ts | 16 +++++++++---- packages/blob/src/index.test.ts | 10 +++++++- packages/blob/src/index.ts | 9 ++++++- packages/extract/src/imports.test.ts | 11 +++++++++ packages/extract/src/imports.ts | 12 ++++++++-- packages/shared-schemas/src/items.ts | 3 ++- packages/shared-schemas/src/lists.ts | 3 ++- vitest.config.ts | 2 ++ 12 files changed, 119 insertions(+), 19 deletions(-) create mode 100644 apps/web/app/api/v1/items/[id]/cover/route.test.ts diff --git a/apps/web/app/api/v1/items/[id]/cover/route.test.ts b/apps/web/app/api/v1/items/[id]/cover/route.test.ts new file mode 100644 index 0000000..c005ee7 --- /dev/null +++ b/apps/web/app/api/v1/items/[id]/cover/route.test.ts @@ -0,0 +1,24 @@ +import { twoUsers } from "@pluck/db/testing"; +import { expect, test } from "vitest"; +import { newKey } from "@/lib/library"; +import { PUT } from "./route"; + +test("a cover over 2 MB is refused, by its content-length or, without one, while it is read", async () => { + const { a } = await twoUsers(); + const { key } = await newKey(a, "extension"); + const id = "0".repeat(24); // refused before the item is looked up + const put = (body: BodyInit, headers: Record = {}) => + PUT( + new Request(`http://x/api/v1/items/${id}/cover`, { + method: "PUT", body, headers: { authorization: `Bearer ${key}`, "content-type": "image/png", ...headers }, duplex: "half", + } as RequestInit), // why: `duplex` (needed for a stream body) is not in the DOM RequestInit type yet + { params: Promise.resolve({ id }) }, + ); + + const refused = async (res: Response) => [res.status, ((await res.json()) as { error: { message: string } }).error.message]; + + expect(await refused(await put("tiny", { "content-length": "2000001" }))).toEqual([400, "That image is over 2 MB."]); + const chunk = new Uint8Array(500_000); + const endless = new ReadableStream({ pull: (c) => c.enqueue(chunk) }); // no content-length + expect(await refused(await put(endless))).toEqual([400, "That image is over 2 MB."]); +}); diff --git a/apps/web/app/api/v1/items/[id]/cover/route.ts b/apps/web/app/api/v1/items/[id]/cover/route.ts index b1fd01c..644789a 100644 --- a/apps/web/app/api/v1/items/[id]/cover/route.ts +++ b/apps/web/app/api/v1/items/[id]/cover/route.ts @@ -11,12 +11,31 @@ export const GET = route<{ id: string }>({ kind: "read" }, async ({ req, caller, const COVER_TYPES = new Set(["image/jpeg", "image/png", "image/webp"]); const MAX_COVER = 2_000_000; +const tooBig = () => new ApiError("bad_request", "That image is over 2 MB.", 400); + +// Stops reading at the limit: content-length can be missing or wrong. +async function readUpTo(req: Request, max: number): Promise { + const chunks: Uint8Array[] = []; + let size = 0; + const reader = req.body?.getReader(); + while (reader) { + const { done, value } = await reader.read(); + if (done) break; + if ((size += value.byteLength) > max) { + await reader.cancel(); + return null; + } + chunks.push(value); + } + return Buffer.concat(chunks); +} // The extension sends a screenshot of the tab when the page has no image of its own. export const PUT = route<{ id: string }>({ kind: "write" }, async ({ req, caller, params }) => { const type = req.headers.get("content-type")?.split(";")[0]?.trim() ?? ""; if (!COVER_TYPES.has(type)) throw new ApiError("bad_request", "Send a JPEG, PNG or WebP image.", 400); - const body = new Uint8Array(await req.arrayBuffer()); - if (body.byteLength > MAX_COVER) throw new ApiError("bad_request", "That image is over 2 MB.", 400); + if (Number(req.headers.get("content-length")) > MAX_COVER) throw tooBig(); + const body = await readUpTo(req, MAX_COVER); + if (!body) throw tooBig(); return json({ item: await setCoverFromApp(caller.userId, params.id, body, type) }); }); diff --git a/apps/web/lib/library/account.ts b/apps/web/lib/library/account.ts index adcba1d..022bae1 100644 --- a/apps/web/lib/library/account.ts +++ b/apps/web/lib/library/account.ts @@ -1,10 +1,10 @@ -import { deletePrefix, getObject, putObject, signedPutUrl, signedUrl } from "@pluck/blob"; +import { TooLarge, deleteObject, deletePrefix, getObject, putObject, signedPutUrl, signedUrl } from "@pluck/blob"; import { aiConnections, applyExtraction, claimOnce, countItems, createApiKey, disconnectAi, claimImportStart, createImport, deleteAccountData, enqueue, exportData, finishExport, getImport, getMe, importLink, latestExport, recountTags, listApiKeys, revokeApiKey, startExport, updateImport, } from "@pluck/db"; import { extractFromHtml, parseImport, type ImportSource } from "@pluck/extract"; -import { normalizeUrl, type AiConnection, type ApiKey, type ImportStatus } from "@pluck/shared-schemas"; +import { MAX_IMPORT_BYTES, normalizeUrl, type AiConnection, type ApiKey, type ImportStatus } from "@pluck/shared-schemas"; import { strToU8, zipSync } from "fflate"; import { sendEmail } from "../email"; import { exportEmail, welcomeEmail } from "../emails"; @@ -82,9 +82,14 @@ export async function runImport(userId: string, importId: string): Promise if (!record || record.state !== "running") return; let entries; try { - entries = parseImport(record.source, new Uint8Array(await getObject(record.blobKey))); + entries = parseImport(record.source, new Uint8Array(await getObject(record.blobKey, { maxBytes: MAX_IMPORT_BYTES }))); } catch (e) { - await updateImport(importId, { state: "failed", error: "That file could not be read. Check it is the export from the right service." }); + const tooLarge = e instanceof TooLarge; + if (tooLarge) await deleteObject(record.blobKey); + await updateImport(importId, { + state: "failed", + error: tooLarge ? "That file is over 500 MB." : "That file could not be read. Check it is the export from the right service.", + }); log("warn", "import unreadable", { userId, importId, error: String(e) }); return; } diff --git a/apps/web/lib/library/email-in.ts b/apps/web/lib/library/email-in.ts index 5306482..d418558 100644 --- a/apps/web/lib/library/email-in.ts +++ b/apps/web/lib/library/email-in.ts @@ -1,4 +1,4 @@ -import { deleteObject, getObject, keys, listKeys, moveObject } from "@pluck/blob"; +import { TooLarge, deleteObject, getObject, keys, listKeys, moveObject } from "@pluck/blob"; import { aliasOf, claimOnce, createEmailItem, enqueue, hitRateLimit, regenerateAlias, setRawEmailKey, userForAlias } from "@pluck/db"; import { parseEmail } from "@pluck/extract"; import { log } from "../log"; @@ -31,13 +31,19 @@ export async function pollInbound(): Promise { } export async function ingestInbound(key: string): Promise<"created" | "duplicate" | "dropped"> { - const raw = await getObject(key); const drop = async (why: string) => { log("warn", "email-in dropped", { key, why }); await deleteObject(key); return "dropped" as const; }; - if (raw.length > MAX_BYTES) return drop("too large"); + // Refused before it is read: SES itself accepts messages up to 40 MB. + let raw: Buffer; + try { + raw = await getObject(key, { maxBytes: MAX_BYTES }); + } catch (e) { + if (e instanceof TooLarge) return drop("too large"); + throw e; + } const mail = await parseEmail(raw).catch(() => null); if (!mail) return drop("unreadable"); // retrying will not make it parse if (mail.spam) return drop("spam or virus verdict"); diff --git a/apps/web/lib/library/files.ts b/apps/web/lib/library/files.ts index ba1d86a..be0d772 100644 --- a/apps/web/lib/library/files.ts +++ b/apps/web/lib/library/files.ts @@ -1,7 +1,7 @@ -import { getObject, keys, signedPutUrl } from "@pluck/blob"; -import { applyExtraction, createFileItem, enqueue, getItem, setBlobKeys } from "@pluck/db"; +import { TooLarge, deleteObject, getObject, keys, signedPutUrl } from "@pluck/blob"; +import { applyExtraction, createFileItem, enqueue, getItem, recordExtractionFailure, setBlobKeys } from "@pluck/db"; import { extractFile } from "@pluck/extract"; -import { FILE_TYPES, type CreateFileItemBody, type Item } from "@pluck/shared-schemas"; +import { FILE_TYPES, MAX_FILE_BYTES, type CreateFileItemBody, type Item } from "@pluck/shared-schemas"; const keyFor = (userId: string, itemId: string, contentType: CreateFileItemBody["contentType"]) => contentType === "application/pdf" ? keys.file(userId, itemId, "pdf") : keys.file(userId, itemId, FILE_TYPES[contentType]); @@ -21,7 +21,15 @@ export async function finishFile(userId: string, itemId: string, key: string, co const item = await getItem(userId, itemId); if (!item || item.extraction.state !== "pending") return; // shortcut: an upload that never arrives leaves the item pending once the job gives up (five tries, about 15 minutes); mark it failed if that shows up. - const body = await getObject(key); // missing yet: throws, and the job tries again later + let body: Buffer; + try { + body = await getObject(key, { maxBytes: MAX_FILE_BYTES }); // missing yet: throws, and the job tries again later + } catch (e) { + if (!(e instanceof TooLarge)) throw e; + await recordExtractionFailure(userId, itemId, "That file is over 50 MB.", true); + await deleteObject(key); + return; + } const x = await extractFile({ body, contentType, fileName: name }); if (x.kind === "image") await setBlobKeys(userId, itemId, { coverKey: key }); await applyExtraction(userId, itemId, { diff --git a/packages/blob/src/index.test.ts b/packages/blob/src/index.test.ts index 5cfc424..9f635f4 100644 --- a/packages/blob/src/index.test.ts +++ b/packages/blob/src/index.test.ts @@ -1,5 +1,5 @@ import { expect, test } from "vitest"; -import { deletePrefix, getObject, putObject, signedUrl } from "./index.ts"; +import { TooLarge, deletePrefix, getObject, putObject, signedUrl } from "./index.ts"; test("put, read back, sign and delete against MinIO", async () => { const prefix = `test/${Date.now()}/`; @@ -9,3 +9,11 @@ test("put, read back, sign and delete against MinIO", async () => { expect(await res.text()).toBe("hello"); expect(await deletePrefix(prefix)).toBe(1); }); + +test("getObject refuses an object over maxBytes", async () => { + const prefix = `test/${Date.now()}-max/`; + await putObject(`${prefix}a.txt`, "hello", "text/plain"); + expect((await getObject(`${prefix}a.txt`, { maxBytes: 5 })).toString()).toBe("hello"); + await expect(getObject(`${prefix}a.txt`, { maxBytes: 4 })).rejects.toBeInstanceOf(TooLarge); + await deletePrefix(prefix); +}); diff --git a/packages/blob/src/index.ts b/packages/blob/src/index.ts index dcc112e..9d0807e 100644 --- a/packages/blob/src/index.ts +++ b/packages/blob/src/index.ts @@ -33,8 +33,15 @@ export async function putObject(key: string, body: Buffer | string, contentType: await s3().send(new PutObjectCommand({ Bucket: bucket(), Key: key, Body: body, ContentType: contentType })); } -export async function getObject(key: string): Promise { +export class TooLarge extends Error {} + +// `maxBytes` refuses an object before its body is read: a signed PUT URL takes whatever size is sent to it. +export async function getObject(key: string, { maxBytes = Infinity } = {}): Promise { const res = await s3().send(new GetObjectCommand({ Bucket: bucket(), Key: key })); + if ((res.ContentLength ?? 0) > maxBytes) { + await res.Body?.transformToWebStream().cancel(); + throw new TooLarge(`${key} is ${res.ContentLength} bytes, over ${maxBytes}`); + } return Buffer.from(await res.Body!.transformToByteArray()); } diff --git a/packages/extract/src/imports.test.ts b/packages/extract/src/imports.test.ts index 82d5fa5..faed776 100644 --- a/packages/extract/src/imports.test.ts +++ b/packages/extract/src/imports.test.ts @@ -1,5 +1,6 @@ import { readFileSync } from "node:fs"; import { join } from "node:path"; +import { strToU8, zipSync } from "fflate"; import { expect, test } from "vitest"; import { parseCsv, parseImport } from "./imports.ts"; @@ -35,3 +36,13 @@ test("Omnivore: metadata plus the kept page when the zip has it", () => { expect(e[0]!.html?.()).toContain("Every printed photograph"); expect(e[1]!.html).toBeNull(); }); + +test("Omnivore: a zip that unpacks too large is refused before it is unpacked", () => { + const meta = strToU8(JSON.stringify([{ url: "https://example.com/a", slug: "a" }])); + const big = new Uint8Array(21_000_000); // zeros: a few kilobytes zipped + const e = parseImport("omnivore", zipSync({ "metadata_0.json": meta, "content/a.html": big })); + expect(e[0]!.html).toBeNull(); // a page too big to keep is fetched instead + expect(() => parseImport("omnivore", zipSync({ "metadata_0.json": big }))).toThrow(/too large/); + const many = Object.fromEntries(Array.from({ length: 6 }, (_, i) => [`metadata_${i}.json`, big.subarray(0, 19_000_000)])); + expect(() => parseImport("omnivore", zipSync(many))).toThrow(/too large/); +}); diff --git a/packages/extract/src/imports.ts b/packages/extract/src/imports.ts index 695e3eb..b52c980 100644 --- a/packages/extract/src/imports.ts +++ b/packages/extract/src/imports.ts @@ -82,16 +82,24 @@ function bookmarksHtml(text: string): ImportEntry[] { }); } +// fflate unpacks each file into a buffer of the size the zip claims and never grows it, so checking that size +// before unpacking is what stops a zip bomb. +const MAX_ZIP_ENTRY = 20_000_000; +const MAX_ZIP_METADATA = 100_000_000; + // Omnivore: a zip of metadata_*.json arrays, with the kept page at content/.html when present. function omnivore(zip: Uint8Array): ImportEntry[] { // Unzip only the metadata; note where each kept page is. The import runs in batches and parses the file // each time, so a page is decompressed only when its batch asks for it. const pages = new Map(); // slug → file name + let total = 0; const files = unzipSync(zip, { filter: (f) => { const slug = f.name.match(/(?:^|\/)content\/(.+)\.html$/)?.[1]; - if (slug) pages.set(slug, f.name); - return /(^|\/)metadata_[^/]*\.json$/.test(f.name); + if (slug && f.originalSize <= MAX_ZIP_ENTRY) pages.set(slug, f.name); // a bigger page is fetched instead + if (!/(^|\/)metadata_[^/]*\.json$/.test(f.name)) return false; + if (f.originalSize > MAX_ZIP_ENTRY || (total += f.originalSize) > MAX_ZIP_METADATA) throw new Error("the zip unpacks too large"); + return true; }, }); const read = (name: string) => () => strFromU8(unzipSync(zip, { filter: (f) => f.name === name })[name]!); diff --git a/packages/shared-schemas/src/items.ts b/packages/shared-schemas/src/items.ts index e29e01f..42d2f21 100644 --- a/packages/shared-schemas/src/items.ts +++ b/packages/shared-schemas/src/items.ts @@ -186,10 +186,11 @@ export type SearchResults = z.infer; // A file shared from the phone (the share extension): the client uploads it to the returned URL. export const FILE_TYPES = { "application/pdf": "pdf", "image/jpeg": "jpg", "image/png": "png", "image/heic": "heic", "image/webp": "webp", "image/gif": "gif" } as const; +export const MAX_FILE_BYTES = 50_000_000; // also the most the worker will read back export const CreateFileItemBody = z.object({ name: z.string().trim().min(1).max(255), contentType: z.enum(Object.keys(FILE_TYPES) as [keyof typeof FILE_TYPES, ...(keyof typeof FILE_TYPES)[]]), - size: z.number().int().min(1).max(50_000_000), + size: z.number().int().min(1).max(MAX_FILE_BYTES), source: Source.optional(), }); export type CreateFileItemBody = z.infer; diff --git a/packages/shared-schemas/src/lists.ts b/packages/shared-schemas/src/lists.ts index 407b662..ad30940 100644 --- a/packages/shared-schemas/src/lists.ts +++ b/packages/shared-schemas/src/lists.ts @@ -159,7 +159,8 @@ export const ImportStatus = z.object({ state: z.enum(["waiting", "running", "done", "failed"]), error: z.string().nullable(), createdAt: z.string(), }); export type ImportStatus = z.infer; -export const CreateImportBody = z.object({ source: ImportSourceEnum, size: z.number().int().min(1).max(500_000_000) }); +export const MAX_IMPORT_BYTES = 500_000_000; // also the most the worker will read back +export const CreateImportBody = z.object({ source: ImportSourceEnum, size: z.number().int().min(1).max(MAX_IMPORT_BYTES) }); // An AI tool the person connected over OAuth, shown in Settings with a Disconnect button. export const AiConnection = z.object({ clientId: z.string(), name: z.string(), scopes: z.array(z.string()), connectedAt: z.string() }); diff --git a/vitest.config.ts b/vitest.config.ts index 65ee7d0..39dfc50 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,6 +1,8 @@ import { defineConfig } from "vitest/config"; export default defineConfig({ + // The web app's "@/" path, so its route handlers can be tested as they are. + resolve: { alias: [{ find: /^@\//, replacement: new URL("./apps/web/", import.meta.url).pathname }] }, test: { include: ["{apps,packages,infra,scripts}/**/*.test.{ts,tsx}"], exclude: ["**/node_modules/**", "**/.next/**", "**/cdk.out/**"],