Skip to content

Commit 94e24a1

Browse files
fix: drop redundant decodeURIComponent and repair urlencoded schema validation (#267)
* fix: URLSearchParams refactor broke valid urlencoded webhooks; drop redundant decodeURIComponent The current `URL_ENCODED` branch of `parseRequestBody` has two bugs that combine to make every valid urlencoded GitHub webhook fail: 1. `bodySchema.parse(payloadParam)` is called with a `string | null` returned by `URLSearchParams.get("payload")`, but `bodySchema` is `z.object({ payload: z.string() })` — i.e. it expects an *object* with a `payload` key. zod throws `Invalid input: expected object, received string` on every valid payload, so the function returns `undefined`. The pre-existing fixture test (`fixtures/invalid-payload-urlencoded.txt`) hides this because it only exercises an invalid payload that's expected to be rejected with 403. 2. `JSON.parse(decodeURIComponent(payload))` calls `decodeURIComponent` a second time on a value `URLSearchParams.get` has already URL-decoded. This throws `URIError: URI malformed` whenever the decoded JSON contains a literal `%` that isn't part of a valid `%XX` escape — which is common in real PR titles, commit messages, and comments ("30% threshold", "set %USERPROFILE% to ~", "WHERE x LIKE '%foo%'", "printf(\"%s\", val)"). Fixes: - Wrap the `URLSearchParams.get("payload")` value into the `{ payload: string }` shape the schema expects, so schema validation actually runs against valid payloads. - Drop the redundant `decodeURIComponent(payload)` — `URLSearchParams.get` already URL-decodes once, which is exactly what GitHub's single-URL-encoded webhook body needs. Tests: 9 new cases in `lambda/proxy.test.ts` cover the percent-character regression patterns plus a control and a `%20`-preservation case, exercised both end-to-end through the handler and directly against `parseRequestBody`. * Apply suggestions from code review Co-authored-by: Dan Adajian <danadajian@gmail.com> --------- Co-authored-by: Dan Adajian <danadajian@gmail.com>
1 parent 9fcfd4a commit 94e24a1

2 files changed

Lines changed: 87 additions & 3 deletions

File tree

lambda/parse-request-body.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -27,9 +27,10 @@ export function parseRequestBody(
2727
return JSON.parse(body);
2828
case CONTENT_TYPES.URL_ENCODED:
2929
const params = new URLSearchParams(body);
30-
const payloadParam = params.get("payload");
31-
const { payload } = bodySchema.parse(payloadParam);
32-
return JSON.parse(decodeURIComponent(payload));
30+
const { payload } = bodySchema.parse({
31+
payload: params.get("payload"),
32+
});
33+
return JSON.parse(payload);
3334
}
3435
} catch (error) {
3536
console.error(`Error parsing request body: ${error}`);

lambda/proxy.test.ts

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ limitations under the License.
1212
*/
1313

1414
import { handler } from "./proxy";
15+
import { parseRequestBody } from "./parse-request-body";
1516
import type { AxiosRequestHeaders, AxiosResponse } from "axios";
1617
import { readFileSync } from "fs";
1718
import { Agent } from "https";
@@ -318,4 +319,86 @@ describe("proxy", () => {
318319
});
319320
expect(axiosPostMock).toHaveBeenCalled();
320321
});
322+
323+
it("should forward a urlencoded webhook whose JSON contains literal '%' characters", async () => {
324+
const payloadWithPercent = {
325+
...VALID_PUSH_PAYLOAD,
326+
head_commit: {
327+
...(VALID_PUSH_PAYLOAD as any).head_commit,
328+
message:
329+
"Roll out 30% threshold; reference %USERPROFILE% on Windows; WHERE x LIKE '%foo%'",
330+
},
331+
};
332+
const urlencodedBody =
333+
"payload=" + encodeURIComponent(JSON.stringify(payloadWithPercent));
334+
const destinationUrl = "https://approved.host/github-webhook/";
335+
const endpointId = encodeURIComponent(destinationUrl);
336+
const event: APIGatewayProxyWithLambdaAuthorizerEvent<any> = {
337+
...baseEvent,
338+
headers: {
339+
...baseEvent.headers,
340+
"content-type": "application/x-www-form-urlencoded",
341+
},
342+
body: urlencodedBody,
343+
pathParameters: { endpointId },
344+
};
345+
const result = await handler(event);
346+
expect(result).toEqual(expectedResponseObject);
347+
expect(axiosPostMock).toHaveBeenCalled();
348+
});
349+
});
350+
351+
describe("parseRequestBody — urlencoded payloads with literal '%' characters", () => {
352+
const headers = { "content-type": "application/x-www-form-urlencoded" };
353+
354+
const cases: Array<{ label: string; userContent: string }> = [
355+
{ label: "percentage in PR title", userContent: "30% threshold rollout" },
356+
{ label: "trailing percent", userContent: "rate: 5%" },
357+
{
358+
label: "Windows env var reference",
359+
userContent: "set %USERPROFILE% to ~",
360+
},
361+
{
362+
label: "SQL LIKE wildcard",
363+
userContent: "WHERE name LIKE '%foo%' AND status = 1",
364+
},
365+
{
366+
label: "C-style format string",
367+
userContent: 'printf("%s\\n", val);',
368+
},
369+
{ label: "Go format verb", userContent: 'log.Printf("%v", obj)' },
370+
{
371+
label: "bare % followed by non-hex",
372+
userContent: "look here: %g and %h",
373+
},
374+
];
375+
376+
for (const c of cases) {
377+
it(`parses a webhook whose JSON contains literal '%' (${c.label})`, () => {
378+
const json = JSON.stringify({
379+
action: "opened",
380+
pull_request: { title: c.userContent, body: c.userContent },
381+
});
382+
const body = "payload=" + encodeURIComponent(json);
383+
const result = parseRequestBody(body, headers);
384+
expect(result).toBeDefined();
385+
expect((result as any).pull_request.title).toBe(c.userContent);
386+
});
387+
}
388+
389+
it("still parses payloads with no '%' characters (control)", () => {
390+
const json = JSON.stringify({ action: "opened", number: 42 });
391+
const body = "payload=" + encodeURIComponent(json);
392+
const result = parseRequestBody(body, headers);
393+
expect(result).toBeDefined();
394+
expect((result as any).number).toBe(42);
395+
});
396+
397+
it("correctly preserves valid percent-escapes (%20 → space) inside JSON string values", () => {
398+
const literalText = "encoded as %20 example";
399+
const json = JSON.stringify({ comment: { body: literalText } });
400+
const body = "payload=" + encodeURIComponent(json);
401+
const result = parseRequestBody(body, headers);
402+
expect((result as any).comment.body).toBe(literalText);
403+
});
321404
});

0 commit comments

Comments
 (0)