From 0868765680bb34001ec2e2fed6383cd76d750ec9 Mon Sep 17 00:00:00 2001 From: "itarun.p" Date: Sat, 25 Jul 2026 22:03:47 +0700 Subject: [PATCH] fix(security): AVATAR_PROXY_MAX_BYTES read like a cap and gated only caching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #110 item 7. Pre-existing; found while closing the round-3 port finding on #109, and deliberately left out of that PR. The whole upstream body was buffered with arrayBuffer() first, and the constant then only decided whether the result entered avatarProxyCache. 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. readCappedAvatarBody() reads at most maxBytes and stops there. Two gates, because either alone is soft: content-length is a claim the peer makes and can omit or lie about, so check the claim AND count the bytes. Returning from the for-await calls the iterator's return(), which cancels the stream. Measured: a 5120-byte body against a 1024-byte cap now reads 1280 bytes — one chunk past the limit, which is inherent since a chunk cannot be known to overrun until it has arrived. Not "stops exactly at the cap"; bounded at cap + one chunk. Behaviour change: an over-cap avatar is now REFUSED with 413 rather than served and merely not cached. 512 KiB is far above any real avatar, and the dashboard falls back to its local icon on a failed image load. 6 tests, including that an over-cap content-length is refused with the body never touched, that a lying content-length does not get past the byte count, that a body exactly at the cap is allowed, and that the no-stream path is bounded too. ci:local exit 0: 889 root tests, 256 dashboard. --- openwiki-facts/source-facts.json | 26 ++++---- src/lib/local-api.js | 57 ++++++++++++++--- test/avatar-proxy-size-cap.test.js | 99 ++++++++++++++++++++++++++++++ 3 files changed, 161 insertions(+), 21 deletions(-) create mode 100644 test/avatar-proxy-size-cap.test.js 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); +});