Repository navigation
Perf/decode response encodings #706
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,59 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { gunzip } from "zlib"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import { promisify } from "util"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Guards against decompression bombs; the largest npm packuments are ~32MB. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export const MAX_DECODED_BODY_BYTES = 256 * 1024 * 1024; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const gunzipAsync = promisify(gunzip); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param {Buffer} buffer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @returns {Promise<Buffer>} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| function decodeGzip(buffer) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return gunzipAsync(buffer, { maxOutputLength: MAX_DECODED_BODY_BYTES }); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Only gzip: npm's br responses are no smaller, and each decoder is attack surface. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @type {Record<string, (buffer: Buffer) => Promise<Buffer>>} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const decoders = { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| gzip: decodeGzip, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| "x-gzip": decodeGzip, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Decodes a response body off the event loop. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param {Buffer} body | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param {string | string[] | undefined} contentEncoding | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @returns {Promise<Buffer>} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export async function decodeBody(body, contentEncoding) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const encoding = String(contentEncoding ?? "identity").trim().toLowerCase(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (encoding === "identity" || encoding === "") { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return body; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if (!Object.hasOwn(decoders, encoding)) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| throw new Error(`Unsupported content-encoding: ${encoding}`); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return decoders[encoding](body); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * Narrows a client's Accept-Encoding to the offered encodings decodeBody supports. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @param {string | string[] | undefined} acceptEncoding | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| * @returns {string} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| export function supportedAcceptEncoding(acceptEncoding) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const offered = String(acceptEncoding ?? "") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .split(",") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .map((token) => token.trim()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| .filter(Boolean); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const supported = offered.filter((token) => | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Object.hasOwn(decoders, token.split(";")[0].trim().toLowerCase()) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return supported.length > 0 ? supported.join(", ") : "identity"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+50
to
+59
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔵 Low - Accept-Encoding wildcard with identity refusal is rewritten to an unacceptable encoding A client that sends Show fix
Suggested change
More info - Reply on this comment to give feedback or ignore the issue. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,81 @@ | ||
| import { describe, it } from "node:test"; | ||
| import assert from "node:assert"; | ||
| import zlib from "node:zlib"; | ||
| import { | ||
| decodeBody, | ||
| supportedAcceptEncoding, | ||
| MAX_DECODED_BODY_BYTES, | ||
| } from "./decodeBody.js"; | ||
|
|
||
| describe("decodeBody", () => { | ||
| const body = Buffer.from('{"name":"pkg"}'); | ||
|
|
||
| for (const [encoding, encode] of [ | ||
| ["gzip", zlib.gzipSync], | ||
| ["x-gzip", zlib.gzipSync], | ||
| ]) { | ||
| it(`decodes ${encoding}`, async () => { | ||
| const decoded = await decodeBody(encode(body), encoding); | ||
|
|
||
| assert.strictEqual(decoded.toString(), body.toString()); | ||
| }); | ||
| } | ||
|
|
||
| it("returns uncompressed bodies as the same buffer", async () => { | ||
| assert.strictEqual(await decodeBody(body, undefined), body); | ||
| assert.strictEqual(await decodeBody(body, "identity"), body); | ||
| }); | ||
|
|
||
| it("rejects encodings it cannot decode, including inherited property names", async () => { | ||
| await assert.rejects(decodeBody(body, "zstd"), /Unsupported/); | ||
| await assert.rejects(decodeBody(body, "constructor"), /Unsupported/); | ||
| await assert.rejects(decodeBody(body, "gzip, br"), /Unsupported/); | ||
| }); | ||
|
|
||
| it("does not decode br or deflate", async () => { | ||
| await assert.rejects( | ||
| decodeBody(zlib.brotliCompressSync(body), "br"), | ||
| /Unsupported/ | ||
| ); | ||
| await assert.rejects( | ||
| decodeBody(zlib.deflateSync(body), "deflate"), | ||
| /Unsupported/ | ||
| ); | ||
| }); | ||
|
|
||
| it("refuses to expand a body beyond the decoded size limit", async () => { | ||
| // Concatenated gzip members: a tiny body that decodes past the limit. | ||
| const oneMegabyte = 1024 * 1024; | ||
| const member = zlib.gzipSync(Buffer.alloc(oneMegabyte)); | ||
| const bomb = Buffer.concat( | ||
| Array(MAX_DECODED_BODY_BYTES / oneMegabyte + 1).fill(member) | ||
| ); | ||
|
|
||
| await assert.rejects(decodeBody(bomb, "gzip"), { | ||
| code: "ERR_BUFFER_TOO_LARGE", | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe("supportedAcceptEncoding", () => { | ||
| it("keeps only offered encodings that can be decoded", () => { | ||
| assert.strictEqual( | ||
| supportedAcceptEncoding("gzip, deflate, br, zstd, x-gzip"), | ||
| "gzip, x-gzip" | ||
| ); | ||
| }); | ||
|
|
||
| it("keeps quality values on the encodings it keeps", () => { | ||
| assert.strictEqual( | ||
| supportedAcceptEncoding("zstd;q=1.0, br;q=0.9, gzip;q=0.5"), | ||
| "gzip;q=0.5" | ||
| ); | ||
| }); | ||
|
|
||
| it("falls back to identity when nothing offered can be decoded", () => { | ||
| assert.strictEqual(supportedAcceptEncoding("zstd"), "identity"); | ||
| assert.strictEqual(supportedAcceptEncoding("br, deflate"), "identity"); | ||
| assert.strictEqual(supportedAcceptEncoding(undefined), "identity"); | ||
| assert.strictEqual(supportedAcceptEncoding("constructor"), "identity"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Medium - Over-limit gzip metadata bypasses minimum-age filtering
A registry can return a gzip metadata response whose decoded size exceeds the new 256 MiB limit. The decoder rejects it, but the response handler catches that error and forwards the original compressed bytes without invoking the metadata interceptor, so npm or pip receives unfiltered version/file metadata and can select releases that the minimum-age rewrite was meant to remove.
Show fix
Treat an over-limit decode as a failed upstream response and do not forward it to the package manager, or use a bounded streaming rewrite path that can safely enforce the limit while still applying the metadata checks. Add an integration test proving that an over-limit gzip response cannot reach the client as an unmodified metadata response.
More info - Reply on this comment to give feedback or ignore the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Partially fixed in commit 7f82be1.
This issue remains because packages/safe-chain/src/registryProxy/mitmRequestHandler.js:270-278 catches the MAX_DECODED_BODY_BYTES rejection from decodeBody and sends status/headers plus originalBuffer, allowing the over-limit compressed metadata to reach the client unmodified.