Skip to content

Commit

Permalink
fix: prevent splitting multibyte characters in getPayload() (#939)
Browse files Browse the repository at this point in the history
If you expect that the payload is using utf8 or utf16 or any other multybyte encoding, then you have to concat the buffers first and in the final step you can serialize the buffer to a string with the desired encoding.

The issue will arise, when with some "luck" a multibyte character is split between two chunks.
  • Loading branch information
Uzlopak committed Dec 4, 2023
1 parent c328004 commit fe00834
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 8 deletions.
18 changes: 12 additions & 6 deletions src/middleware/node/get-payload.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,19 @@ export function getPayload(request: IncomingMessage): Promise<string> {
if (request.body) return Promise.resolve(request.body);

return new Promise((resolve, reject) => {
let data = "";
let data: Buffer[] = [];

request.setEncoding("utf8");

// istanbul ignore next
request.on("error", (error: Error) => reject(new AggregateError([error])));
request.on("data", (chunk: string) => (data += chunk));
request.on("end", () => resolve(data));
request.on("data", (chunk: Buffer) => data.push(chunk));
request.on("end", () =>
// setImmediate improves the throughput by reducing the pressure from
// the event loop
setImmediate(
resolve,
data.length === 1
? data[0].toString("utf8")
: Buffer.concat(data).toString("utf8"),
),
);
});
}
77 changes: 77 additions & 0 deletions test/integration/get-payload.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import EventEmitter from "node:events";
import { getPayload } from "../../src/middleware/node/get-payload.ts";

describe("getPayload", () => {
it("returns a promise", () => {
const request = new EventEmitter();
const promise = getPayload(request);

expect(promise).toBeInstanceOf(Promise);
});

it("resolves with a string when only receiving no chunk", async () => {
const request = new EventEmitter();
const promise = getPayload(request);

request.emit("end");

expect(await promise).toEqual("");
});

it("resolves with a string when only receiving one chunk", async () => {
const request = new EventEmitter();
const promise = getPayload(request);

request.emit("data", Buffer.from("foobar"));
request.emit("end");

expect(await promise).toEqual("foobar");
});

it("resolves with a string when receiving multiple chunks", async () => {
const request = new EventEmitter();
const promise = getPayload(request);

request.emit("data", Buffer.from("foo"));
request.emit("data", Buffer.from("bar"));
request.emit("end");

expect(await promise).toEqual("foobar");
});

it("rejects with an error", async () => {
const request = new EventEmitter();
const promise = getPayload(request);

request.emit("error", new Error("test"));

await expect(promise).rejects.toThrow("test");
});

it("resolves with a string with respecting the utf-8 encoding", async () => {
const request = new EventEmitter();
const promise = getPayload(request);

const doubleByteBuffer = Buffer.from("ݔ");
request.emit("data", doubleByteBuffer.subarray(0, 1));
request.emit("data", doubleByteBuffer.subarray(1, 2));
request.emit("end");

expect(await promise).toEqual("ݔ");
});

it("resolves with the body, if passed via the request", async () => {
const request = new EventEmitter();
// @ts-ignore body is not part of EventEmitter, which we are using
// to mock the request object
request.body = "foo";

const promise = getPayload(request);

// we emit data, to ensure that the body attribute is preferred
request.emit("data", "bar");
request.emit("end");

expect(await promise).toEqual("foo");
});
});
4 changes: 2 additions & 2 deletions test/integration/node-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -372,7 +372,7 @@ describe("createNodeMiddleware(webhooks)", () => {
});

test("Handles timeout", async () => {
jest.useFakeTimers();
jest.useFakeTimers({ doNotFake: ["setImmediate"] });

const webhooks = new Webhooks({
secret: "mySecret",
Expand Down Expand Up @@ -407,7 +407,7 @@ describe("createNodeMiddleware(webhooks)", () => {
});

test("Handles timeout with error", async () => {
jest.useFakeTimers();
jest.useFakeTimers({ doNotFake: ["setImmediate"] });

const webhooks = new Webhooks({
secret: "mySecret",
Expand Down

0 comments on commit fe00834

Please sign in to comment.