fix(security): AVATAR_PROXY_MAX_BYTES read like a cap and gated only caching

openwiki-facts/source-facts.json

@@ -44,87 +44,87 @@
         "methods": [
           "POST"
         ],
-        "evidence": "src/lib/local-api.js:945",
+        "evidence": "src/lib/local-api.js:985",
         "mutation": true
       },
       {
         "path": "/functions/tokentracker-wrapped",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:979",
+        "evidence": "src/lib/local-api.js:1019",
         "mutation": false
       },
       {
         "path": "/functions/tokentracker-usage-summary",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:990",
+        "evidence": "src/lib/local-api.js:1030",
         "mutation": false
       },
       {
         "path": "/functions/tokentracker-usage-daily",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:1075",
+        "evidence": "src/lib/local-api.js:1115",
         "mutation": false
       },
       {
         "path": "/functions/tokentracker-usage-heatmap",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:1086",
+        "evidence": "src/lib/local-api.js:1126",
         "mutation": false
       },
       {
         "path": "/functions/tokentracker-usage-model-breakdown",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:1154",
+        "evidence": "src/lib/local-api.js:1194",
         "mutation": false
       },
       {
         "path": "/functions/tokentracker-usage-category-breakdown",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:1233",
+        "evidence": "src/lib/local-api.js:1273",
         "mutation": false
       },
       {
         "path": "/functions/tokentracker-project-usage-summary",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:1279",
+        "evidence": "src/lib/local-api.js:1319",
         "mutation": false
       },
       {
         "path": "/functions/tokentracker-user-status",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:1356",
+        "evidence": "src/lib/local-api.js:1396",
         "mutation": false
       },
       {
         "path": "/functions/tokentracker-usage-hourly",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:1366",
+        "evidence": "src/lib/local-api.js:1406",
         "mutation": false
       },
       {
         "path": "/functions/tokentracker-usage-monthly",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:1376",
+        "evidence": "src/lib/local-api.js:1416",
         "mutation": false
       },
       {
@@ -133,15 +133,15 @@
           "GET",
           "POST"
         ],
-        "evidence": "src/lib/local-api.js:1410",
+        "evidence": "src/lib/local-api.js:1450",
         "mutation": true
       },
       {
         "path": "/functions/tokentracker-usage-limits",
         "methods": [
           "GET"
         ],
-        "evidence": "src/lib/local-api.js:1560",
+        "evidence": "src/lib/local-api.js:1600",
         "mutation": false
       }
     ]

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,

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);
+});