From dab0f6cfe8dca8c4cefe69d18129da2b93ba88f9 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 17:25:08 +1000 Subject: [PATCH 1/4] fix(image-write): add a per-session cumulative volume cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per-message caps (MAX_IMAGE_DATA_LENGTH + the 10 MB decode limit) bound a single image write but nothing stops a compromised webview from looping image-write with distinct PNG-magic-prefixed payloads — each content- addresses to a NEW /assets/ file, filling the disk silently while every per-message cap is individually satisfied. Add the count/volume bound every other webview->host channel already has: a generous 512 MiB cumulative byte budget scoped to the editor session (one per panel). Once crossed it rejects further writes and warns the user once; reopening the document starts a fresh budget. Real paste flows never approach it. --- src/extension/image/image-write-budget.ts | 53 +++++++++++++++++++ src/extension/image/image-write-service.ts | 14 +++++ src/extension/image/image-write-wiring.ts | 16 ++++++ .../image/image-write-budget.test.ts | 35 ++++++++++++ .../image/image-write-service.test.ts | 23 ++++++++ .../image/image-write-wiring.test.ts | 43 +++++++++++++++ 6 files changed, 184 insertions(+) create mode 100644 src/extension/image/image-write-budget.ts create mode 100644 test/extension/image/image-write-budget.test.ts diff --git a/src/extension/image/image-write-budget.ts b/src/extension/image/image-write-budget.ts new file mode 100644 index 00000000..ce12c408 --- /dev/null +++ b/src/extension/image/image-write-budget.ts @@ -0,0 +1,53 @@ +// Per-session cumulative volume cap for the image-WRITE path. +// +// The per-message caps in image-write-service (the MAX_IMAGE_DATA_LENGTH string +// bound + the 10 MB decode limit) bound a SINGLE write; they do nothing against +// a compromised webview — the exact adversary image-write-wiring gates against — +// that loops `image-write` with distinct PNG-magic-prefixed random payloads. +// Each such payload content-addresses to a NEW /assets/ file, so the +// per-message caps are individually satisfied while the disk fills silently. +// +// This is the count/volume bound every other webview→host channel already has +// (MAX_LINT_DIAGNOSTICS etc.): a generous cumulative byte ceiling scoped to the +// editor session (one budget per panel). Once crossed it rejects further writes +// and warns the user ONCE (a per-write toast would itself be a notification-flood +// vector). Real paste flows — even bulk multi-image paste — never approach it. + +/** Cumulative image bytes a single editor session may write to ./assets/. + * 512 MiB ≈ 50 full-size (10 MB) images — orders of magnitude beyond any real + * paste session, yet a hard stop against an unbounded write loop. A session is + * one open panel; reopening the document starts a fresh budget. */ +export const SESSION_IMAGE_WRITE_BUDGET_BYTES = 512 * 1024 * 1024; + +/** User-facing warning shown ONCE when the session budget is first exceeded. */ +export const SESSION_IMAGE_WRITE_BUDGET_TOAST = + "Quoll: image paste limit reached for this document — reopen it to insert more images."; + +export interface SessionVolumeBudget { + /** Charge `byteLength` against the session budget. Returns true (and records + * the spend) when the write may proceed; false once the cumulative cap would + * be exceeded — in which case `onExceeded` fires exactly once across the + * session's lifetime, no matter how many further writes are attempted. */ + reserve(byteLength: number): boolean; +} + +export function createSessionVolumeBudget( + budgetBytes: number, + onExceeded: () => void +): SessionVolumeBudget { + let spent = 0; + let warned = false; + return { + reserve(byteLength) { + if (spent + byteLength > budgetBytes) { + if (!warned) { + warned = true; + onExceeded(); + } + return false; + } + spent += byteLength; + return true; + }, + }; +} diff --git a/src/extension/image/image-write-service.ts b/src/extension/image/image-write-service.ts index 1f3e702e..285cd8ed 100644 --- a/src/extension/image/image-write-service.ts +++ b/src/extension/image/image-write-service.ts @@ -29,6 +29,13 @@ export type ImageWriteDeps = { showError: (message: string) => void; /** Post the image-write-result; `null` path ⇒ ok:false. */ postResult: (requestId: string, relativePath: string | null) => void; + /** Charge the session cumulative-volume budget for a to-be-written image of + * `byteLength` bytes; returns false once the session cap is exceeded. Checked + * AFTER validation (only bytes that would actually hit disk are counted) and + * the budget owns its own one-time user warning, so this path posts ok:false + * WITHOUT a showError. Omitted ⇒ unbounded (the service's historical + * behaviour); the Panel wiring supplies the real per-session budget. */ + reserveBudget?: (byteLength: number) => boolean; }; /** Validate + write a base64 image and post the result. Never throws — every @@ -64,6 +71,13 @@ export async function handleImageWrite( deps.postResult(requestId, null); return; } + // Session cumulative-volume gate: count only validated bytes (those that would + // reach disk), AFTER the per-message caps above. The budget surfaces its own + // one-time warning, so this rejection just clears the webview's pending entry. + if (deps.reserveBudget && !deps.reserveBudget(decision.bytes.length)) { + deps.postResult(requestId, null); + return; + } try { const relativePath = await deps.writeImage(decision.filename, decision.bytes); deps.postResult(requestId, relativePath); diff --git a/src/extension/image/image-write-wiring.ts b/src/extension/image/image-write-wiring.ts index 89cfc132..893ac144 100644 --- a/src/extension/image/image-write-wiring.ts +++ b/src/extension/image/image-write-wiring.ts @@ -18,6 +18,11 @@ import { Uri, workspace } from "vscode"; import type { HostToWebview } from "../../shared/protocol.js"; import { buildImageWriteResultMessage } from "../session/document-message.js"; +import { + createSessionVolumeBudget, + SESSION_IMAGE_WRITE_BUDGET_BYTES, + SESSION_IMAGE_WRITE_BUDGET_TOAST, +} from "./image-write-budget.js"; import { handleImageWrite } from "./image-write-service.js"; export interface ImageWriteWiringDeps { @@ -35,6 +40,10 @@ export interface ImageWriteWiringDeps { * (harness.writeImageFileOverride) — read PER WRITE (it can be set after * resolve). null routes to workspace.fs.writeFile. */ readonly writeFileOverride: () => ((uri: Uri, content: Uint8Array) => Thenable) | null; + /** Session cumulative-volume ceiling in bytes. Defaults to + * SESSION_IMAGE_WRITE_BUDGET_BYTES; overridable only so tests can trip the cap + * without allocating half a gigabyte. */ + readonly budgetBytes?: number; } export interface ImageWriteWiring { @@ -44,12 +53,19 @@ export interface ImageWriteWiring { } export function createImageWriteWiring(deps: ImageWriteWiringDeps): ImageWriteWiring { + // One budget per wiring instance = one per panel = one per editor session. It + // owns its own one-time warning (a per-write toast would be a flood vector). + const budget = createSessionVolumeBudget( + deps.budgetBytes ?? SESSION_IMAGE_WRITE_BUDGET_BYTES, + () => deps.showError(SESSION_IMAGE_WRITE_BUDGET_TOAST) + ); return { handle(requestId: string, data: string): void { void handleImageWrite( { canWrite: deps.canWrite, showError: deps.showError, + reserveBudget: (byteLength) => budget.reserve(byteLength), postResult: (id, relativePath) => deps.post(buildImageWriteResultMessage(id, relativePath)), // writeImage creates /assets/ then writes — the explicit diff --git a/test/extension/image/image-write-budget.test.ts b/test/extension/image/image-write-budget.test.ts new file mode 100644 index 00000000..86408089 --- /dev/null +++ b/test/extension/image/image-write-budget.test.ts @@ -0,0 +1,35 @@ +import { describe, expect, it, vi } from "vitest"; + +import { createSessionVolumeBudget } from "../../../src/extension/image/image-write-budget.js"; + +describe("createSessionVolumeBudget", () => { + it("allows writes up to (and including) the budget, then rejects", () => { + const onExceeded = vi.fn(); + const budget = createSessionVolumeBudget(100, onExceeded); + + expect(budget.reserve(60)).toBe(true); // spent 60 + expect(budget.reserve(40)).toBe(true); // spent 100 — exactly at the cap + expect(budget.reserve(1)).toBe(false); // 101 > 100 — over budget + expect(onExceeded).toHaveBeenCalledOnce(); + }); + + it("warns exactly once no matter how many over-budget writes follow", () => { + const onExceeded = vi.fn(); + const budget = createSessionVolumeBudget(10, onExceeded); + + expect(budget.reserve(8)).toBe(true); // spent 8 + expect(budget.reserve(5)).toBe(false); // 13 > 10 — first over-budget, warns + expect(budget.reserve(5)).toBe(false); // still over — no second warning + expect(onExceeded).toHaveBeenCalledOnce(); + }); + + it("does not charge the budget for a rejected write (a rejected spend cannot exhaust it)", () => { + const onExceeded = vi.fn(); + const budget = createSessionVolumeBudget(10, onExceeded); + + expect(budget.reserve(8)).toBe(true); // spent 8 + expect(budget.reserve(5)).toBe(false); // 13 > 10 — rejected, NOT charged + expect(budget.reserve(2)).toBe(true); // 8 + 2 = 10 still fits + expect(budget.reserve(1)).toBe(false); // 11 > 10 + }); +}); diff --git a/test/extension/image/image-write-service.test.ts b/test/extension/image/image-write-service.test.ts index e6aa977c..cf9e424e 100644 --- a/test/extension/image/image-write-service.test.ts +++ b/test/extension/image/image-write-service.test.ts @@ -62,4 +62,27 @@ describe("handleImageWrite", () => { expect(writeImage).not.toHaveBeenCalled(); expect(postResult).toHaveBeenCalledWith("r1", null); }); + + it("rejects a validated write when the session budget denies it (no showError — the budget warns)", async () => { + const reserveBudget = vi.fn(() => false); + const { deps, writeImage, postResult, showError } = makeDeps({ reserveBudget }); + await handleImageWrite(deps, "r1", pngBase64); + // The image passed every per-message gate — the budget is charged with the + // validated byte length, and only then does it deny the write. + expect(reserveBudget).toHaveBeenCalledWith(PNG.length); + expect(writeImage).not.toHaveBeenCalled(); + expect(postResult).toHaveBeenCalledWith("r1", null); + // The budget owns its one-time warning; the service must NOT toast here. + expect(showError).not.toHaveBeenCalled(); + }); + + it("writes normally when the session budget allows it", async () => { + const reserveBudget = vi.fn(() => true); + const { deps, writeImage, postResult } = makeDeps({ reserveBudget }); + await handleImageWrite(deps, "r1", pngBase64); + expect(reserveBudget).toHaveBeenCalledWith(PNG.length); + expect(writeImage).toHaveBeenCalledTimes(1); + const [filename] = writeImage.mock.calls[0]; + expect(postResult).toHaveBeenCalledWith("r1", `./assets/${filename}`); + }); }); diff --git a/test/extension/image/image-write-wiring.test.ts b/test/extension/image/image-write-wiring.test.ts index e879fdb4..b64586fd 100644 --- a/test/extension/image/image-write-wiring.test.ts +++ b/test/extension/image/image-write-wiring.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it, vi } from "vitest"; import { workspace } from "vscode"; +import { SESSION_IMAGE_WRITE_BUDGET_TOAST } from "../../../src/extension/image/image-write-budget.js"; import { createImageWriteWiring } from "../../../src/extension/image/image-write-wiring.js"; // A minimal valid PNG (8-byte signature) — decideImageWrite sniffs the magic @@ -76,6 +77,48 @@ describe("createImageWriteWiring", () => { createDirSpy.mockRestore(); }); + it("enforces the per-session cumulative volume cap: writes past the budget are rejected with one warning", async () => { + // PNG_BYTES is 8 bytes; a budget of 8 admits exactly the first write, then + // denies every subsequent one for the life of this wiring (= the session). + const write = vi.fn(async () => {}); + const post = vi.fn(); + const showError = vi.fn(); + const wiring = createImageWriteWiring({ + documentUri, + canWrite: () => true, + showError, + post, + writeFileOverride: () => write, + budgetBytes: PNG_BYTES.length, + }); + + // 1st write — fits the budget exactly, written + ok:true. + wiring.handle("req-1", PNG_BASE64); + await flush(); + // 2nd write — over budget, rejected without touching disk, ok:false + warning. + wiring.handle("req-2", PNG_BASE64); + await flush(); + // 3rd write — still over budget, still rejected, but NO second warning. + wiring.handle("req-3", PNG_BASE64); + await flush(); + + expect(write).toHaveBeenCalledOnce(); + expect(post).toHaveBeenCalledWith( + expect.objectContaining({ type: "image-write-result", requestId: "req-1", ok: true }) + ); + expect(post).toHaveBeenCalledWith( + expect.objectContaining({ type: "image-write-result", requestId: "req-2", ok: false }) + ); + expect(post).toHaveBeenCalledWith( + expect.objectContaining({ type: "image-write-result", requestId: "req-3", ok: false }) + ); + // Exactly one budget warning across the two rejected writes. + const budgetWarnings = showError.mock.calls.filter( + ([msg]) => msg === SESSION_IMAGE_WRITE_BUDGET_TOAST + ); + expect(budgetWarnings).toHaveLength(1); + }); + it("re-reads writeFileOverride per call (late-bound override, not captured at construction)", async () => { // The wiring reads deps.writeFileOverride() fresh inside the write closure on // every handle() — the e2e harness sets writeImageFileOverride AFTER the panel From 8b8ebdd7131fc71c28c8a8656c98a015a3bbb235 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 17:40:56 +1000 Subject: [PATCH 2/4] fix(image-write): make budget required, refund on write failure, harden Review-cycle fixes for the session volume cap: - Make the budget a required dep (one SessionVolumeBudget object) so the pre-cap 'unbounded' state is no longer a silent default of ImageWriteDeps. - Add release(): refund a reservation when the write fails, so a run of transient FS failures can't exhaust the session cap and lock out pastes. - Guard reserve()/release() against NaN/negative input, which would otherwise poison or rewind the running total (disabling the cap). - Correct the module header (this is the first session-cumulative bound, not MAX_LINT_DIAGNOSTICS precedent) and the reserve/rollback comments. - Add tests: per-panel budget isolation, default-budget normal-paste flow, release-on-failure, early-exit paths skipping the charge, domain guards. - Bump version 0.1.59 -> 0.1.60 + CHANGELOG entry for the user-visible cap. --- CHANGELOG.md | 6 ++ package.json | 2 +- src/extension/image/image-write-budget.ts | 43 ++++++++++-- src/extension/image/image-write-service.ts | 31 ++++++--- src/extension/image/image-write-wiring.ts | 2 +- .../image/image-write-budget.test.ts | 26 +++++++ .../image/image-write-service.test.ts | 58 ++++++++++++---- .../image/image-write-wiring.test.ts | 67 +++++++++++++++++++ 8 files changed, 202 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a92ea984..73b2a05b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All notable changes to Quoll are documented here. +## 0.1.60 — 2026-07-29 + +### Fixed + +- Pasting images into a single document is now capped at a generous total size per editing session, guarding against runaway disk use from a misbehaving source. The limit is far above any normal paste workflow; if it is ever reached, further image pastes are declined with a one-time notice and resume when you reopen the document. + ## 0.1.59 — 2026-07-29 ### Fixed diff --git a/package.json b/package.json index de78c2eb..74d3fc8e 100644 --- a/package.json +++ b/package.json @@ -27,7 +27,7 @@ "gfm", "MEO" ], - "version": "0.1.59", + "version": "0.1.60", "publisher": "mtskf", "type": "module", "engines": { diff --git a/src/extension/image/image-write-budget.ts b/src/extension/image/image-write-budget.ts index ce12c408..18a2bfc4 100644 --- a/src/extension/image/image-write-budget.ts +++ b/src/extension/image/image-write-budget.ts @@ -7,11 +7,15 @@ // Each such payload content-addresses to a NEW /assets/ file, so the // per-message caps are individually satisfied while the disk fills silently. // -// This is the count/volume bound every other webview→host channel already has -// (MAX_LINT_DIAGNOSTICS etc.): a generous cumulative byte ceiling scoped to the -// editor session (one budget per panel). Once crossed it rejects further writes -// and warns the user ONCE (a per-write toast would itself be a notification-flood -// vector). Real paste flows — even bulk multi-image paste — never approach it. +// Other webview→host channels are already count-bounded PER MESSAGE (e.g. +// MAX_LINT_DIAGNOSTICS caps diagnostics on each inbound lint message), but none +// keeps a running session-lifetime total — this budget is the FIRST such bound +// in the codebase, added because the image-write path is uniquely able to turn a +// message loop into unbounded disk growth. It is a generous cumulative byte +// ceiling scoped to the editor session (one budget per panel). Once crossed it +// rejects further writes and warns the user ONCE (a per-write toast would itself +// be a notification-flood vector). Real paste flows — even bulk multi-image +// paste — never approach it. /** Cumulative image bytes a single editor session may write to ./assets/. * 512 MiB ≈ 50 full-size (10 MB) images — orders of magnitude beyond any real @@ -27,8 +31,30 @@ export interface SessionVolumeBudget { /** Charge `byteLength` against the session budget. Returns true (and records * the spend) when the write may proceed; false once the cumulative cap would * be exceeded — in which case `onExceeded` fires exactly once across the - * session's lifetime, no matter how many further writes are attempted. */ + * session's lifetime, no matter how many further writes are attempted. + * Reserve BEFORE the async write so concurrent (fire-and-forget) writes can't + * each independently pass the check and overshoot the cap; `release` the + * reservation if that write then fails (see below). */ reserve(byteLength: number): boolean; + /** Refund a prior `reserve(byteLength)` whose write did not reach disk (an FS + * failure after the reservation). Keeps the running total counting only bytes + * actually written, so a run of transient write failures can't exhaust the + * session cap. No-op below zero. Never un-warns: a released reservation was, + * by definition, admitted — it never tripped `onExceeded`. */ + release(byteLength: number): void; +} + +// Guard the numeric domain the running total assumes (non-negative integer). +// The sole production caller passes a `Uint8Array.length`, always safe — but a +// NaN would poison `spent` permanently (every `NaN > cap` is false, silently +// disabling the cap: the exact failure this module exists to prevent), and a +// negative would rewind it. Fail loud on the contract violation instead. +function assertByteLength(byteLength: number): void { + if (!Number.isInteger(byteLength) || byteLength < 0) { + throw new RangeError( + `SessionVolumeBudget: byteLength must be a non-negative integer, got ${byteLength}` + ); + } } export function createSessionVolumeBudget( @@ -39,6 +65,7 @@ export function createSessionVolumeBudget( let warned = false; return { reserve(byteLength) { + assertByteLength(byteLength); if (spent + byteLength > budgetBytes) { if (!warned) { warned = true; @@ -49,5 +76,9 @@ export function createSessionVolumeBudget( spent += byteLength; return true; }, + release(byteLength) { + assertByteLength(byteLength); + spent = Math.max(0, spent - byteLength); + }, }; } diff --git a/src/extension/image/image-write-service.ts b/src/extension/image/image-write-service.ts index 285cd8ed..caced811 100644 --- a/src/extension/image/image-write-service.ts +++ b/src/extension/image/image-write-service.ts @@ -5,6 +5,7 @@ import { MAX_IMAGE_DATA_LENGTH } from "../../shared/protocol.js"; import { decideImageWrite, type ImageRejectReason } from "./image-ingest.js"; +import type { SessionVolumeBudget } from "./image-write-budget.js"; export function imageRejectToast(reason: ImageRejectReason): string { switch (reason) { @@ -29,13 +30,15 @@ export type ImageWriteDeps = { showError: (message: string) => void; /** Post the image-write-result; `null` path ⇒ ok:false. */ postResult: (requestId: string, relativePath: string | null) => void; - /** Charge the session cumulative-volume budget for a to-be-written image of - * `byteLength` bytes; returns false once the session cap is exceeded. Checked - * AFTER validation (only bytes that would actually hit disk are counted) and - * the budget owns its own one-time user warning, so this path posts ok:false - * WITHOUT a showError. Omitted ⇒ unbounded (the service's historical - * behaviour); the Panel wiring supplies the real per-session budget. */ - reserveBudget?: (byteLength: number) => boolean; + /** Session cumulative-volume budget (the DoS cap this service enforces). + * REQUIRED, not optional: a security-load-bearing dep must never default to + * "unbounded" — callers that genuinely want no cap pass an all-permitting + * budget explicitly, so "unbounded" is a visible decision, not a silent one. + * reserve() is charged AFTER validation (only bytes that would reach disk), + * and released again if the write then fails, so the running total counts + * only bytes actually written. The budget owns its own one-time warning, so + * a budget rejection posts ok:false WITHOUT a showError. */ + budget: SessionVolumeBudget; }; /** Validate + write a base64 image and post the result. Never throws — every @@ -71,10 +74,12 @@ export async function handleImageWrite( deps.postResult(requestId, null); return; } - // Session cumulative-volume gate: count only validated bytes (those that would - // reach disk), AFTER the per-message caps above. The budget surfaces its own - // one-time warning, so this rejection just clears the webview's pending entry. - if (deps.reserveBudget && !deps.reserveBudget(decision.bytes.length)) { + // Session cumulative-volume gate: reserve the validated byte count (only bytes + // that would reach disk), AFTER the per-message caps above and BEFORE the async + // write so concurrent fire-and-forget writes can't each overshoot the cap. The + // budget surfaces its own one-time warning, so this rejection just clears the + // webview's pending entry. + if (!deps.budget.reserve(decision.bytes.length)) { deps.postResult(requestId, null); return; } @@ -82,6 +87,10 @@ export async function handleImageWrite( const relativePath = await deps.writeImage(decision.filename, decision.bytes); deps.postResult(requestId, relativePath); } catch (err) { + // The write failed — no bytes reached disk, so refund the reservation. + // Otherwise a run of transient FS failures would exhaust the session cap and + // lock out legitimate pastes until the document is reopened. + deps.budget.release(decision.bytes.length); console.error("[quoll] image write failed", err); deps.showError( "Quoll: failed to write the image file. See the extension host log for details." diff --git a/src/extension/image/image-write-wiring.ts b/src/extension/image/image-write-wiring.ts index 893ac144..7fe4da8f 100644 --- a/src/extension/image/image-write-wiring.ts +++ b/src/extension/image/image-write-wiring.ts @@ -65,7 +65,7 @@ export function createImageWriteWiring(deps: ImageWriteWiringDeps): ImageWriteWi { canWrite: deps.canWrite, showError: deps.showError, - reserveBudget: (byteLength) => budget.reserve(byteLength), + budget, postResult: (id, relativePath) => deps.post(buildImageWriteResultMessage(id, relativePath)), // writeImage creates /assets/ then writes — the explicit diff --git a/test/extension/image/image-write-budget.test.ts b/test/extension/image/image-write-budget.test.ts index 86408089..290d737a 100644 --- a/test/extension/image/image-write-budget.test.ts +++ b/test/extension/image/image-write-budget.test.ts @@ -32,4 +32,30 @@ describe("createSessionVolumeBudget", () => { expect(budget.reserve(2)).toBe(true); // 8 + 2 = 10 still fits expect(budget.reserve(1)).toBe(false); // 11 > 10 }); + + it("release refunds a reservation so a failed write does not exhaust the budget", () => { + const onExceeded = vi.fn(); + const budget = createSessionVolumeBudget(10, onExceeded); + + expect(budget.reserve(10)).toBe(true); // spent 10 — full + budget.release(10); // the write failed — refund + expect(budget.reserve(10)).toBe(true); // room again + expect(onExceeded).not.toHaveBeenCalled(); // a refunded write never warned + }); + + it("release never drives spend below zero", () => { + const budget = createSessionVolumeBudget(10, vi.fn()); + budget.release(5); // nothing reserved yet — clamps at 0, not -5 + expect(budget.reserve(10)).toBe(true); // full budget still available + }); + + it("rejects a NaN or negative byteLength instead of poisoning the running total", () => { + const budget = createSessionVolumeBudget(10, vi.fn()); + expect(() => budget.reserve(Number.NaN)).toThrow(RangeError); + expect(() => budget.reserve(-1)).toThrow(RangeError); + expect(() => budget.reserve(1.5)).toThrow(RangeError); + expect(() => budget.release(Number.NaN)).toThrow(RangeError); + // The cap is still intact after the rejected inputs (spent untouched). + expect(budget.reserve(10)).toBe(true); + }); }); diff --git a/test/extension/image/image-write-service.test.ts b/test/extension/image/image-write-service.test.ts index cf9e424e..a7afab27 100644 --- a/test/extension/image/image-write-service.test.ts +++ b/test/extension/image/image-write-service.test.ts @@ -6,12 +6,23 @@ import { MAX_IMAGE_DATA_LENGTH } from "../../../src/shared/protocol.js"; const PNG = new Uint8Array([0x89, 0x50, 0x4e, 0x47, 0x0d, 0x0a, 0x1a, 0x0a, 0x00]); const pngBase64 = Buffer.from(PNG).toString("base64"); +// An all-permitting budget stub — the default so tests that don't exercise the +// cap read cleanly. Tests that care pass a spy via `overrides.budget`. +const unboundedBudget = () => ({ reserve: vi.fn(() => true), release: vi.fn() }); + function makeDeps(overrides: Partial[0]> = {}) { const writeImage = vi.fn(async (filename: string) => `./assets/${filename}`); const showError = vi.fn(); const postResult = vi.fn(); return { - deps: { canWrite: () => true, writeImage, showError, postResult, ...overrides }, + deps: { + canWrite: () => true, + writeImage, + showError, + postResult, + budget: unboundedBudget(), + ...overrides, + }, writeImage, showError, postResult, @@ -29,30 +40,45 @@ describe("handleImageWrite", () => { expect(showError).not.toHaveBeenCalled(); }); - it("rejects on a read-only document without writing", async () => { - const { deps, writeImage, postResult, showError } = makeDeps({ canWrite: () => false }); + it("rejects on a read-only document without writing OR charging the budget", async () => { + const budget = unboundedBudget(); + const { deps, writeImage, postResult, showError } = makeDeps({ + canWrite: () => false, + budget, + }); await handleImageWrite(deps, "r1", pngBase64); expect(writeImage).not.toHaveBeenCalled(); expect(postResult).toHaveBeenCalledWith("r1", null); expect(showError).toHaveBeenCalledTimes(1); + // The read-only guard precedes (and short-circuits) the budget charge — pins + // that ordering so a future reorder that charges before the guard is caught. + expect(budget.reserve).not.toHaveBeenCalled(); }); - it("rejects a non-image (svg) without writing", async () => { - const { deps, writeImage, postResult } = makeDeps(); + it("rejects a non-image (svg) without writing OR charging the budget", async () => { + const budget = unboundedBudget(); + const { deps, writeImage, postResult } = makeDeps({ budget }); const svg = Buffer.from("").toString("base64"); await handleImageWrite(deps, "r1", svg); expect(writeImage).not.toHaveBeenCalled(); expect(postResult).toHaveBeenCalledWith("r1", null); + // A per-message-cap rejection (unsupported type) must skip the budget entirely. + expect(budget.reserve).not.toHaveBeenCalled(); }); - it("surfaces an error and posts null when writeImage rejects", async () => { + it("surfaces an error, posts null, AND refunds the budget when writeImage rejects", async () => { const writeImage = vi.fn(async () => { throw new Error("disk full"); }); - const { deps, postResult, showError } = makeDeps({ writeImage }); + const budget = unboundedBudget(); + const { deps, postResult, showError } = makeDeps({ writeImage, budget }); await handleImageWrite(deps, "r1", pngBase64); expect(postResult).toHaveBeenCalledWith("r1", null); expect(showError).toHaveBeenCalledTimes(1); + // A failed write reached no disk, so its reservation is released — otherwise + // a run of transient FS failures would exhaust the session cap. + expect(budget.reserve).toHaveBeenCalledWith(PNG.length); + expect(budget.release).toHaveBeenCalledWith(PNG.length); }); it("rejects an over-cap data string without decoding or writing", async () => { @@ -64,25 +90,29 @@ describe("handleImageWrite", () => { }); it("rejects a validated write when the session budget denies it (no showError — the budget warns)", async () => { - const reserveBudget = vi.fn(() => false); - const { deps, writeImage, postResult, showError } = makeDeps({ reserveBudget }); + const budget = { reserve: vi.fn(() => false), release: vi.fn() }; + const { deps, writeImage, postResult, showError } = makeDeps({ budget }); await handleImageWrite(deps, "r1", pngBase64); // The image passed every per-message gate — the budget is charged with the // validated byte length, and only then does it deny the write. - expect(reserveBudget).toHaveBeenCalledWith(PNG.length); + expect(budget.reserve).toHaveBeenCalledWith(PNG.length); expect(writeImage).not.toHaveBeenCalled(); expect(postResult).toHaveBeenCalledWith("r1", null); + // A denied reservation was never charged, so nothing to release. + expect(budget.release).not.toHaveBeenCalled(); // The budget owns its one-time warning; the service must NOT toast here. expect(showError).not.toHaveBeenCalled(); }); - it("writes normally when the session budget allows it", async () => { - const reserveBudget = vi.fn(() => true); - const { deps, writeImage, postResult } = makeDeps({ reserveBudget }); + it("writes normally when the session budget allows it (and does not release on success)", async () => { + const budget = { reserve: vi.fn(() => true), release: vi.fn() }; + const { deps, writeImage, postResult } = makeDeps({ budget }); await handleImageWrite(deps, "r1", pngBase64); - expect(reserveBudget).toHaveBeenCalledWith(PNG.length); + expect(budget.reserve).toHaveBeenCalledWith(PNG.length); expect(writeImage).toHaveBeenCalledTimes(1); const [filename] = writeImage.mock.calls[0]; expect(postResult).toHaveBeenCalledWith("r1", `./assets/${filename}`); + // A successful write keeps its charge — no refund. + expect(budget.release).not.toHaveBeenCalled(); }); }); diff --git a/test/extension/image/image-write-wiring.test.ts b/test/extension/image/image-write-wiring.test.ts index b64586fd..5564479a 100644 --- a/test/extension/image/image-write-wiring.test.ts +++ b/test/extension/image/image-write-wiring.test.ts @@ -119,6 +119,73 @@ describe("createImageWriteWiring", () => { expect(budgetWarnings).toHaveLength(1); }); + it("scopes the budget per wiring instance: exhausting one panel's budget does not starve another", async () => { + // Two independent wirings (= two panels/sessions). The design claims one + // budget per panel; a regression that hoisted the budget to module scope + // would let panel A's flood reject panel B's first write — this pins it. + const makeWiring = () => { + const write = vi.fn(async () => {}); + const post = vi.fn(); + const wiring = createImageWriteWiring({ + documentUri, + canWrite: () => true, + showError: vi.fn(), + post, + writeFileOverride: () => write, + budgetBytes: PNG_BYTES.length, // admits exactly one write + }); + return { wiring, write, post }; + }; + + const a = makeWiring(); + const b = makeWiring(); + + // Exhaust panel A: 1st fits, 2nd is over budget. + a.wiring.handle("a-1", PNG_BASE64); + a.wiring.handle("a-2", PNG_BASE64); + await flush(); + // Panel B's FIRST write must still succeed — its budget is untouched. + b.wiring.handle("b-1", PNG_BASE64); + await flush(); + + expect(a.write).toHaveBeenCalledOnce(); // A got exactly one write + expect(b.write).toHaveBeenCalledOnce(); // B not starved by A's flood + expect(b.post).toHaveBeenCalledWith( + expect.objectContaining({ type: "image-write-result", requestId: "b-1", ok: true }) + ); + }); + + it("uses a generous default budget when budgetBytes is omitted (normal multi-image paste is unaffected)", async () => { + // Omitting budgetBytes falls back to SESSION_IMAGE_WRITE_BUDGET_BYTES. A + // handful of real (distinct) images must all write and none trip the cap or + // warn — the Done-when "normal single/multi-image paste flows unaffected". + const write = vi.fn(async () => {}); + const post = vi.fn(); + const showError = vi.fn(); + const wiring = createImageWriteWiring({ + documentUri, + canWrite: () => true, + showError, + post, + writeFileOverride: () => write, + // budgetBytes omitted → real SESSION_IMAGE_WRITE_BUDGET_BYTES default. + }); + + // Distinct payloads so each content-addresses to a new file (a real paste + // batch), all far under the 512 MiB default. + for (let i = 0; i < 5; i++) { + const bytes = Buffer.concat([PNG_BYTES, Buffer.from([i])]); + wiring.handle(`req-${i}`, bytes.toString("base64")); + } + await flush(); + + expect(write).toHaveBeenCalledTimes(5); + const warnings = showError.mock.calls.filter( + ([msg]) => msg === SESSION_IMAGE_WRITE_BUDGET_TOAST + ); + expect(warnings).toHaveLength(0); + }); + it("re-reads writeFileOverride per call (late-bound override, not captured at construction)", async () => { // The wiring reads deps.writeFileOverride() fresh inside the write closure on // every handle() — the e2e harness sets writeImageFileOverride AFTER the panel From 894fa3f5e66be47b3eb352dff85916259b25e9f0 Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 17:54:10 +1000 Subject: [PATCH 3/4] fix(image-write): drop budget refund (fail-safe) + narrow write try-scope MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cycle-2 review fixes: - Remove release()/refund from SessionVolumeBudget. workspace.fs.writeFile is not atomic, so a failed write can still leave a partial content-addressed file; refunding the reservation would let an attacker loop failing writes and grow the disk without bound while every charge is returned. Never refunding keeps total disk growth bounded by the budget — the fail-safe choice for a disk-fill cap. Cost (a session with many genuine FS failures may hit the cap early; reopen to reset) is negligible against 512 MiB. - Narrow handleImageWrite's try to the write alone and post the success result outside it, so a throwing postResult can no longer be misread as a write failure (wrong toast + double post). - Test: over-cap-data-string path now asserts the budget is not charged, like its sibling early-exit tests; add an end-to-end no-refund exhaustion test. --- src/extension/image/image-write-budget.ts | 24 +++++----- src/extension/image/image-write-service.ts | 28 ++++++----- .../image/image-write-budget.test.ts | 20 +++----- .../image/image-write-service.test.ts | 48 +++++++++++++------ 4 files changed, 68 insertions(+), 52 deletions(-) diff --git a/src/extension/image/image-write-budget.ts b/src/extension/image/image-write-budget.ts index 18a2bfc4..da80c616 100644 --- a/src/extension/image/image-write-budget.ts +++ b/src/extension/image/image-write-budget.ts @@ -33,15 +33,19 @@ export interface SessionVolumeBudget { * be exceeded — in which case `onExceeded` fires exactly once across the * session's lifetime, no matter how many further writes are attempted. * Reserve BEFORE the async write so concurrent (fire-and-forget) writes can't - * each independently pass the check and overshoot the cap; `release` the - * reservation if that write then fails (see below). */ + * each independently pass the check and overshoot the cap. + * + * There is deliberately NO release/refund counterpart: a charge is never + * reversed, even when the subsequent write fails. This is the fail-SAFE + * choice for a disk-fill cap — `workspace.fs.writeFile` gives no atomicity + * guarantee, so a failed write can still leave a partial content-addressed + * file on disk; refunding it would let an attacker loop failing writes and + * grow the disk without bound while every reservation is returned. Counting + * the attempt keeps total disk growth bounded by the budget. The only cost is + * that a session hitting many genuine FS failures may reach the cap early + * (reopen the document to reset) — negligible for real use against a 512 MiB + * ceiling. */ reserve(byteLength: number): boolean; - /** Refund a prior `reserve(byteLength)` whose write did not reach disk (an FS - * failure after the reservation). Keeps the running total counting only bytes - * actually written, so a run of transient write failures can't exhaust the - * session cap. No-op below zero. Never un-warns: a released reservation was, - * by definition, admitted — it never tripped `onExceeded`. */ - release(byteLength: number): void; } // Guard the numeric domain the running total assumes (non-negative integer). @@ -76,9 +80,5 @@ export function createSessionVolumeBudget( spent += byteLength; return true; }, - release(byteLength) { - assertByteLength(byteLength); - spent = Math.max(0, spent - byteLength); - }, }; } diff --git a/src/extension/image/image-write-service.ts b/src/extension/image/image-write-service.ts index caced811..b90f1a5c 100644 --- a/src/extension/image/image-write-service.ts +++ b/src/extension/image/image-write-service.ts @@ -34,10 +34,11 @@ export type ImageWriteDeps = { * REQUIRED, not optional: a security-load-bearing dep must never default to * "unbounded" — callers that genuinely want no cap pass an all-permitting * budget explicitly, so "unbounded" is a visible decision, not a silent one. - * reserve() is charged AFTER validation (only bytes that would reach disk), - * and released again if the write then fails, so the running total counts - * only bytes actually written. The budget owns its own one-time warning, so - * a budget rejection posts ok:false WITHOUT a showError. */ + * reserve() is charged AFTER validation (only bytes past every per-message + * gate) and BEFORE the write; the charge is never refunded, so total disk + * growth stays bounded by the budget even when a write fails and leaves a + * partial file (see SessionVolumeBudget.reserve). The budget owns its own + * one-time warning, so a budget rejection posts ok:false WITHOUT a showError. */ budget: SessionVolumeBudget; }; @@ -75,26 +76,29 @@ export async function handleImageWrite( return; } // Session cumulative-volume gate: reserve the validated byte count (only bytes - // that would reach disk), AFTER the per-message caps above and BEFORE the async - // write so concurrent fire-and-forget writes can't each overshoot the cap. The + // past every per-message gate above) BEFORE the async write so concurrent + // fire-and-forget writes can't each overshoot the cap. The charge is not + // refunded on failure (see SessionVolumeBudget.reserve — a failed write can + // still leave a partial file, so counting the attempt keeps disk bounded). The // budget surfaces its own one-time warning, so this rejection just clears the // webview's pending entry. if (!deps.budget.reserve(decision.bytes.length)) { deps.postResult(requestId, null); return; } + // Scope the try to the write ALONE: a throw here means the write failed. The + // success-path post is deliberately OUTSIDE it so a postResult that throws is + // not misread as a write failure (wrong toast + double post). + let relativePath: string; try { - const relativePath = await deps.writeImage(decision.filename, decision.bytes); - deps.postResult(requestId, relativePath); + relativePath = await deps.writeImage(decision.filename, decision.bytes); } catch (err) { - // The write failed — no bytes reached disk, so refund the reservation. - // Otherwise a run of transient FS failures would exhaust the session cap and - // lock out legitimate pastes until the document is reopened. - deps.budget.release(decision.bytes.length); console.error("[quoll] image write failed", err); deps.showError( "Quoll: failed to write the image file. See the extension host log for details." ); deps.postResult(requestId, null); + return; } + deps.postResult(requestId, relativePath); } diff --git a/test/extension/image/image-write-budget.test.ts b/test/extension/image/image-write-budget.test.ts index 290d737a..9499ef82 100644 --- a/test/extension/image/image-write-budget.test.ts +++ b/test/extension/image/image-write-budget.test.ts @@ -33,20 +33,13 @@ describe("createSessionVolumeBudget", () => { expect(budget.reserve(1)).toBe(false); // 11 > 10 }); - it("release refunds a reservation so a failed write does not exhaust the budget", () => { - const onExceeded = vi.fn(); - const budget = createSessionVolumeBudget(10, onExceeded); - - expect(budget.reserve(10)).toBe(true); // spent 10 — full - budget.release(10); // the write failed — refund - expect(budget.reserve(10)).toBe(true); // room again - expect(onExceeded).not.toHaveBeenCalled(); // a refunded write never warned - }); - - it("release never drives spend below zero", () => { + it("never refunds a charge: a spend stays spent for the life of the session", () => { + // No release/refund counterpart exists — a charged write is permanent even + // if the write later fails, so total disk growth stays bounded by the cap. const budget = createSessionVolumeBudget(10, vi.fn()); - budget.release(5); // nothing reserved yet — clamps at 0, not -5 - expect(budget.reserve(10)).toBe(true); // full budget still available + expect(budget.reserve(10)).toBe(true); // spent 10 — full + expect(budget.reserve(1)).toBe(false); // still full; the charge did not lapse + expect("release" in budget).toBe(false); // no refund API to reopen the cap }); it("rejects a NaN or negative byteLength instead of poisoning the running total", () => { @@ -54,7 +47,6 @@ describe("createSessionVolumeBudget", () => { expect(() => budget.reserve(Number.NaN)).toThrow(RangeError); expect(() => budget.reserve(-1)).toThrow(RangeError); expect(() => budget.reserve(1.5)).toThrow(RangeError); - expect(() => budget.release(Number.NaN)).toThrow(RangeError); // The cap is still intact after the rejected inputs (spent untouched). expect(budget.reserve(10)).toBe(true); }); diff --git a/test/extension/image/image-write-service.test.ts b/test/extension/image/image-write-service.test.ts index a7afab27..72a3e3b3 100644 --- a/test/extension/image/image-write-service.test.ts +++ b/test/extension/image/image-write-service.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it, vi } from "vitest"; +import { createSessionVolumeBudget } from "../../../src/extension/image/image-write-budget.js"; import { handleImageWrite } from "../../../src/extension/image/image-write-service.js"; import { MAX_IMAGE_DATA_LENGTH } from "../../../src/shared/protocol.js"; @@ -8,7 +9,7 @@ const pngBase64 = Buffer.from(PNG).toString("base64"); // An all-permitting budget stub — the default so tests that don't exercise the // cap read cleanly. Tests that care pass a spy via `overrides.budget`. -const unboundedBudget = () => ({ reserve: vi.fn(() => true), release: vi.fn() }); +const unboundedBudget = () => ({ reserve: vi.fn(() => true) }); function makeDeps(overrides: Partial[0]> = {}) { const writeImage = vi.fn(async (filename: string) => `./assets/${filename}`); @@ -66,7 +67,7 @@ describe("handleImageWrite", () => { expect(budget.reserve).not.toHaveBeenCalled(); }); - it("surfaces an error, posts null, AND refunds the budget when writeImage rejects", async () => { + it("surfaces an error and posts null when writeImage rejects, and keeps the charge (no refund)", async () => { const writeImage = vi.fn(async () => { throw new Error("disk full"); }); @@ -74,23 +75,46 @@ describe("handleImageWrite", () => { const { deps, postResult, showError } = makeDeps({ writeImage, budget }); await handleImageWrite(deps, "r1", pngBase64); expect(postResult).toHaveBeenCalledWith("r1", null); + // Exactly one result post — the success post is outside the try, so a write + // failure does not double-post. + expect(postResult).toHaveBeenCalledTimes(1); expect(showError).toHaveBeenCalledTimes(1); - // A failed write reached no disk, so its reservation is released — otherwise - // a run of transient FS failures would exhaust the session cap. + // The charge stands even though the write failed: a failed write can still + // leave a partial file, so the budget must count the attempt (no refund API). expect(budget.reserve).toHaveBeenCalledWith(PNG.length); - expect(budget.release).toHaveBeenCalledWith(PNG.length); + expect(budget).not.toHaveProperty("release"); }); - it("rejects an over-cap data string without decoding or writing", async () => { - const { deps, writeImage, postResult } = makeDeps(); + it("a run of failed writes deterministically exhausts the session cap (fail-safe, no refund)", async () => { + // Real (small) budget so the no-refund contract is exercised end-to-end: two + // failed writes still consume the cap, so the third is rejected before the + // write is even attempted — this is what bounds disk under a write-failure + // flood. PNG is 9 bytes; budget = 2 writes' worth. + const writeImage = vi.fn(async () => { + throw new Error("disk full"); + }); + const budget = createSessionVolumeBudget(PNG.length * 2, vi.fn()); + const { deps } = makeDeps({ writeImage, budget }); + await handleImageWrite(deps, "r1", pngBase64); // charged, write fails + await handleImageWrite(deps, "r2", pngBase64); // charged, write fails — cap full + await handleImageWrite(deps, "r3", pngBase64); // over budget — rejected pre-write + expect(writeImage).toHaveBeenCalledTimes(2); // r3 never reached the write + }); + + it("rejects an over-cap data string without decoding, writing, OR charging the budget", async () => { + const budget = unboundedBudget(); + const { deps, writeImage, postResult } = makeDeps({ budget }); const huge = "a".repeat(MAX_IMAGE_DATA_LENGTH + 1); await handleImageWrite(deps, "r1", huge); expect(writeImage).not.toHaveBeenCalled(); expect(postResult).toHaveBeenCalledWith("r1", null); + // The over-cap early exit precedes the budget charge — pins that an + // undecoded, oversized payload never touches the session budget. + expect(budget.reserve).not.toHaveBeenCalled(); }); it("rejects a validated write when the session budget denies it (no showError — the budget warns)", async () => { - const budget = { reserve: vi.fn(() => false), release: vi.fn() }; + const budget = { reserve: vi.fn(() => false) }; const { deps, writeImage, postResult, showError } = makeDeps({ budget }); await handleImageWrite(deps, "r1", pngBase64); // The image passed every per-message gate — the budget is charged with the @@ -98,21 +122,17 @@ describe("handleImageWrite", () => { expect(budget.reserve).toHaveBeenCalledWith(PNG.length); expect(writeImage).not.toHaveBeenCalled(); expect(postResult).toHaveBeenCalledWith("r1", null); - // A denied reservation was never charged, so nothing to release. - expect(budget.release).not.toHaveBeenCalled(); // The budget owns its one-time warning; the service must NOT toast here. expect(showError).not.toHaveBeenCalled(); }); - it("writes normally when the session budget allows it (and does not release on success)", async () => { - const budget = { reserve: vi.fn(() => true), release: vi.fn() }; + it("writes normally when the session budget allows it", async () => { + const budget = { reserve: vi.fn(() => true) }; const { deps, writeImage, postResult } = makeDeps({ budget }); await handleImageWrite(deps, "r1", pngBase64); expect(budget.reserve).toHaveBeenCalledWith(PNG.length); expect(writeImage).toHaveBeenCalledTimes(1); const [filename] = writeImage.mock.calls[0]; expect(postResult).toHaveBeenCalledWith("r1", `./assets/${filename}`); - // A successful write keeps its charge — no refund. - expect(budget.release).not.toHaveBeenCalled(); }); }); From ca1e882d214cca68310856fcbe2d79ca00ca5f2a Mon Sep 17 00:00:00 2001 From: Mitsuki Fukunaga Date: Wed, 29 Jul 2026 18:00:08 +1000 Subject: [PATCH 4/4] docs(image-write): note the accepted pre-write-failure over-charge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Record why the budget is reserved before the whole write (createDirectory + writeFile): a pre-write setup failure is charged despite writing no bytes, but that is deliberate — reserving here keeps budget rejection cleanly separate from I/O failures, and the over-charge is immaterial (createDirectory only fails on a broken FS, which blocks pastes regardless of the cap). Comment-only. --- src/extension/image/image-write-service.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/extension/image/image-write-service.ts b/src/extension/image/image-write-service.ts index b90f1a5c..82045986 100644 --- a/src/extension/image/image-write-service.ts +++ b/src/extension/image/image-write-service.ts @@ -82,6 +82,14 @@ export async function handleImageWrite( // still leave a partial file, so counting the attempt keeps disk bounded). The // budget surfaces its own one-time warning, so this rejection just clears the // webview's pending entry. + // + // The charge brackets the WHOLE write (in the wiring, writeImage does an + // idempotent createDirectory then writeFile), so a pre-write setup failure is + // charged too even though it wrote no bytes. Accepted deliberately: reserving + // here keeps the budget-rejection path cleanly separate from I/O failures, and + // the over-charge is immaterial — createDirectory only fails on a broken FS, + // which blocks all pastes regardless of the cap (a persistent failure is not a + // budget problem), while a transient blip barely dents a 512 MiB ceiling. if (!deps.budget.reserve(decision.bytes.length)) { deps.postResult(requestId, null); return;