diff --git a/openwiki-facts/source-facts.json b/openwiki-facts/source-facts.json index 653b664f..0fe21f52 100644 --- a/openwiki-facts/source-facts.json +++ b/openwiki-facts/source-facts.json @@ -44,7 +44,7 @@ "methods": [ "POST" ], - "evidence": "src/lib/local-api.js:945", + "evidence": "src/lib/local-api.js:985", "mutation": true }, { @@ -52,7 +52,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:979", + "evidence": "src/lib/local-api.js:1019", "mutation": false }, { @@ -60,7 +60,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:990", + "evidence": "src/lib/local-api.js:1030", "mutation": false }, { @@ -68,7 +68,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1075", + "evidence": "src/lib/local-api.js:1115", "mutation": false }, { @@ -76,7 +76,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1086", + "evidence": "src/lib/local-api.js:1126", "mutation": false }, { @@ -84,7 +84,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1154", + "evidence": "src/lib/local-api.js:1194", "mutation": false }, { @@ -92,7 +92,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1233", + "evidence": "src/lib/local-api.js:1273", "mutation": false }, { @@ -100,7 +100,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1279", + "evidence": "src/lib/local-api.js:1319", "mutation": false }, { @@ -108,7 +108,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1356", + "evidence": "src/lib/local-api.js:1396", "mutation": false }, { @@ -116,7 +116,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1366", + "evidence": "src/lib/local-api.js:1406", "mutation": false }, { @@ -124,7 +124,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1376", + "evidence": "src/lib/local-api.js:1416", "mutation": false }, { @@ -133,7 +133,7 @@ "GET", "POST" ], - "evidence": "src/lib/local-api.js:1410", + "evidence": "src/lib/local-api.js:1450", "mutation": true }, { @@ -141,7 +141,7 @@ "methods": [ "GET" ], - "evidence": "src/lib/local-api.js:1560", + "evidence": "src/lib/local-api.js:1600", "mutation": false } ] diff --git a/src/lib/local-api.js b/src/lib/local-api.js index a8eef768..1bce4b69 100644 --- a/src/lib/local-api.js +++ b/src/lib/local-api.js @@ -25,6 +25,40 @@ const avatarProxyCache = new Map(); const AVATAR_REDIRECT_BLOCKED = Symbol("avatar-redirect-blocked"); const AVATAR_MAX_REDIRECTS = 3; +// Reads at most `maxBytes`, and STOPS READING at the limit. Returns null when the +// response is over it. +// +// `AVATAR_PROXY_MAX_BYTES` read like a download cap and was not one: the whole +// upstream body was buffered with `arrayBuffer()` first, and the constant then +// only decided whether the result entered the cache. An oversized response was +// still read into memory in full and still written to the client, so an +// allowlisted host serving a very large image/* drove loopback-server memory +// with no ceiling. +// +// Two gates, because either alone is soft: `content-length` is a claim the peer +// makes and can lie about or omit, and counting bytes without it means the +// transfer has already started. Check the claim, then count anyway. +async function readCappedAvatarBody(response, maxBytes) { + const declared = Number(response.headers.get("content-length")); + if (Number.isFinite(declared) && declared > maxBytes) return null; + // A HEAD response, or a stub without a stream. `arrayBuffer()` is bounded here + // by the content-length check above and by the length check below. + if (!response.body) { + const buffered = Buffer.from(await response.arrayBuffer()); + return buffered.length > maxBytes ? null : buffered; + } + const chunks = []; + let total = 0; + for await (const chunk of response.body) { + total += chunk.length; + // Returning from a for-await calls the iterator's return(), which cancels + // the stream — the download stops here rather than running to completion. + if (total > maxBytes) return null; + chunks.push(Buffer.from(chunk)); + } + return Buffer.concat(chunks); +} + // One check for the URL the caller asked for AND for every redirect hop, so the // two can't drift apart. Port is part of the address: `https://gravatar.com:8443/` // shares the hostname but is a different service, so leaving the port free turns @@ -920,15 +954,21 @@ function createLocalApiHandler({ queuePath }) { json(res, { error: "Not an image" }, 415); return true; } - const body = Buffer.from(await upstream.arrayBuffer()); - if (body.length <= AVATAR_PROXY_MAX_BYTES) { - // Simple LRU: drop oldest if over capacity. - if (avatarProxyCache.size >= AVATAR_PROXY_MAX_ENTRIES) { - const oldestKey = avatarProxyCache.keys().next().value; - if (oldestKey) avatarProxyCache.delete(oldestKey); - } - avatarProxyCache.set(cacheKey, { body, contentType, fetchedAt: now }); + const body = await readCappedAvatarBody(upstream, AVATAR_PROXY_MAX_BYTES); + if (body === null) { + // Refused rather than served-but-not-cached, which is what the old + // length check amounted to. An avatar this size is not an avatar, and + // the dashboard falls back to its local icon on a failed image load. + json(res, { error: "Avatar too large" }, 413); + return true; + } + // Within the cap by construction, so caching is unconditional. + // Simple LRU: drop oldest if over capacity. + if (avatarProxyCache.size >= AVATAR_PROXY_MAX_ENTRIES) { + const oldestKey = avatarProxyCache.keys().next().value; + if (oldestKey) avatarProxyCache.delete(oldestKey); } + avatarProxyCache.set(cacheKey, { body, contentType, fetchedAt: now }); res.writeHead(200, { "Content-Type": contentType, "Cache-Control": "public, max-age=3600", @@ -1586,6 +1626,7 @@ module.exports = { // closes is only observable across a redirect hop, which no handler-level // test reaches. fetchAvatarFollowingAllowlist, + readCappedAvatarBody, isAllowedAvatarTarget, AVATAR_REDIRECT_BLOCKED, resolveQueuePath, diff --git a/test/avatar-proxy-size-cap.test.js b/test/avatar-proxy-size-cap.test.js new file mode 100644 index 00000000..66f95b3e --- /dev/null +++ b/test/avatar-proxy-size-cap.test.js @@ -0,0 +1,99 @@ +const assert = require("node:assert/strict"); +const { test } = require("node:test"); + +const { readCappedAvatarBody } = require("../src/lib/local-api"); + +const CAP = 1024; + +// A response whose body records how much of it was actually pulled, so a test can +// tell "refused after reading everything" from "stopped reading". +function response({ chunks = [], contentLength = null } = {}) { + const pulled = []; + let index = 0; + // highWaterMark 0 so nothing is pulled until the reader asks. The default + // strategy pre-fetches a chunk when the stream is constructed, which would + // make this stub measure ReadableStream's buffering rather than what the + // function under test reads. + const body = new ReadableStream( + { + pull(controller) { + if (index >= chunks.length) { + controller.close(); + return; + } + const chunk = chunks[index++]; + pulled.push(chunk.length); + controller.enqueue(chunk); + }, + }, + { highWaterMark: 0 }, + ); + return { + headers: { get: (name) => (name.toLowerCase() === "content-length" ? contentLength : null) }, + body, + pulled, + async arrayBuffer() { + throw new Error("arrayBuffer() must not be used when a stream is available"); + }, + }; +} + +const kb = (n, byte = 0x61) => Buffer.alloc(n, byte); + +test("a declared content-length over the cap is refused before any body is read", async () => { + const res = response({ chunks: [kb(64)], contentLength: String(CAP + 1) }); + assert.equal(await readCappedAvatarBody(res, CAP), null); + assert.deepEqual(res.pulled, [], "the body must not be touched once the claim is over the cap"); +}); + +test("a body that exceeds the cap mid-stream stops the download", async () => { + // The point of the change. The old code buffered the whole response with + // arrayBuffer() and only then compared its length, so an oversized image was + // read into memory in full — and served — with no ceiling. + const chunks = Array.from({ length: 20 }, () => kb(256)); + const res = response({ chunks }); + assert.equal(await readCappedAvatarBody(res, CAP), null); + assert.ok( + res.pulled.length < chunks.length, + `must stop early, pulled ${res.pulled.length}/${chunks.length} chunks`, + ); + assert.ok( + res.pulled.reduce((a, b) => a + b, 0) <= CAP + 256, + "must not read far past the cap", + ); +}); + +test("a lying content-length does not get past the byte count", async () => { + // content-length is a claim the peer makes. Checking it is worth doing because + // it costs nothing; trusting it is not. + const res = response({ chunks: [kb(512), kb(512), kb(512)], contentLength: "10" }); + assert.equal(await readCappedAvatarBody(res, CAP), null); +}); + +test("a body within the cap is returned whole", async () => { + const res = response({ chunks: [kb(100, 0x61), kb(100, 0x62)], contentLength: "200" }); + const body = await readCappedAvatarBody(res, CAP); + assert.equal(body.length, 200); + assert.equal(body.subarray(0, 100).toString(), "a".repeat(100)); + assert.equal(body.subarray(100).toString(), "b".repeat(100)); +}); + +test("a body exactly at the cap is allowed, not off by one", async () => { + const res = response({ chunks: [kb(CAP)] }); + const body = await readCappedAvatarBody(res, CAP); + assert.equal(body?.length, CAP); +}); + +test("a response with no stream is still bounded", async () => { + // HEAD responses, and any stub without a body, take the buffered path. It has + // to enforce the same limit rather than fall through. + const noStream = (bytes) => ({ + headers: { get: () => null }, + body: null, + async arrayBuffer() { + return kb(bytes); + }, + }); + assert.equal(await readCappedAvatarBody(noStream(CAP + 1), CAP), null); + assert.equal((await readCappedAvatarBody(noStream(10), CAP)).length, 10); +});