Skip to content

Commit

Permalink
Handle missing request body (#789)
Browse files Browse the repository at this point in the history
## Summary

Explicitly handle the missing request body case. This can happen when
the app doesn't have request body parsing middleware (e.g.
`express.json`).

Before these changes, the user would see an unhelpful "invalid
signature" error:
<img width="1046" alt="image"
src="https://github.com/user-attachments/assets/ac563ff8-dc0e-450c-8125-76274086a2c8"
/>

## Checklist
<!-- Tick these items off as you progress. -->
<!-- If an item isn't applicable, ideally please strikeout the item by
wrapping it in "~~"" and suffix it with "N/A My reason for skipping
this." -->
<!-- e.g. "- [ ] ~~Added tests~~ N/A Only touches docs" -->

- [ ] ~Added a [docs PR](https://github.com/inngest/website) that
references this PR~ N/A Bug fix
- [x] Added unit/integration tests
- [x] Added changesets if applicable

---------

Co-authored-by: Jack Williams <[email protected]>
Co-authored-by: Jack Williams <[email protected]>
  • Loading branch information
3 people authored Jan 9, 2025
1 parent 5e89885 commit 56067cd
Show file tree
Hide file tree
Showing 4 changed files with 134 additions and 8 deletions.
5 changes: 5 additions & 0 deletions .changeset/silent-carrots-develop.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"inngest": patch
---

Better handle missing request body
76 changes: 72 additions & 4 deletions packages/inngest/src/components/InngestCommHandler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -637,13 +637,30 @@ export class InngestCommHandler<
const handler = async (...args: Input) => {
const timer = new ServerTiming();

/**
* Used for testing, allow setting action overrides externally when
* calling the handler. Always search the final argument.
*/
const lastArg = args[args.length - 1] as unknown;
const actionOverrides =
typeof lastArg === "object" &&
lastArg !== null &&
"actionOverrides" in lastArg &&
typeof lastArg["actionOverrides"] === "object" &&
lastArg["actionOverrides"] !== null
? lastArg["actionOverrides"]
: {};

/**
* We purposefully `await` the handler, as it could be either sync or
* async.
*/
const rawActions = await timer
.wrap("handler", () => this.handler(...args))
.catch(rethrowError("Serve handler failed to run"));
const rawActions = {
...(await timer
.wrap("handler", () => this.handler(...args))
.catch(rethrowError("Serve handler failed to run"))),
...actionOverrides,
};

/**
* Map over every `action` in `rawActions` and create a new `actions`
Expand Down Expand Up @@ -701,6 +718,7 @@ export class InngestCommHandler<

return ret;
},
...actionOverrides,
};

const [env, expectedServerKind] = await Promise.all([
Expand Down Expand Up @@ -985,10 +1003,37 @@ export class InngestCommHandler<
method: string;
headers: Promise<Record<string, string>>;
}): Promise<ActionResponse> {
// This is when the request body is completely missing; it does not
// include an empty body. This commonly happens when the HTTP framework
// doesn't have body parsing middleware.
const isMissingBody = body === undefined;

try {
let url = await actions.url("starting to handle request");

if (method === "POST") {
if (isMissingBody) {
this.log(
"error",
"Missing body when executing, possibly due to missing request body middleware"
);

return {
status: 500,
headers: {
"Content-Type": "application/json",
},
body: stringify(
serializeError(
new Error(
"Missing request body when executing, possibly due to missing request body middleware"
)
)
),
version: undefined,
};
}

const validationResult = await signatureValidation;
if (!validationResult.success) {
return {
Expand Down Expand Up @@ -1203,6 +1248,28 @@ export class InngestCommHandler<
]);

if (inBandSyncRequested) {
if (isMissingBody) {
this.log(
"error",
"Missing body when syncing, possibly due to missing request body middleware"
);

return {
status: 500,
headers: {
"Content-Type": "application/json",
},
body: stringify(
serializeError(
new Error(
"Missing request body when syncing, possibly due to missing request body middleware"
)
)
),
version: undefined,
};
}

// Validation can be successful if we're in dev mode and did not
// actually validate a key. In this case, also check that we did indeed
// use a particular key to validate.
Expand Down Expand Up @@ -2070,7 +2137,8 @@ export interface ActionResponse<
* The version of the execution engine that was used to run this action.
*
* If the action didn't use the execution engine (for example, a GET request
* as a health check), this will be `undefined`.
* as a health check) or would have but errored before reaching it, this will
* be `undefined`.
*
* If the version should be entirely omitted from the response (for example,
* when sending preliminary headers when streaming), this will be `null`.
Expand Down
4 changes: 2 additions & 2 deletions packages/inngest/src/deno/fresh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ export const serve = (

const fn = handler.createHandler();

return function handleRequest(req: Request) {
return fn(req, Deno.env.toObject());
return function handleRequest(req: Request, ...other) {
return fn(req, Deno.env.toObject(), ...other);
};
};

Expand Down
57 changes: 55 additions & 2 deletions packages/inngest/src/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
/* eslint-disable @typescript-eslint/no-unsafe-call */
import { Inngest, InngestFunction } from "@local";
import {
HandlerResponse,
InngestCommHandler,
type ServeHandlerOptions,
} from "@local/components/InngestCommHandler";
Expand Down Expand Up @@ -260,7 +261,16 @@ export const testFramework = (
const run = async (
handlerOpts: Parameters<(typeof handler)["serve"]> | ServeHandler,
reqOpts: Parameters<typeof httpMocks.createRequest>,
env: Env = {}
env: Env = {},

/**
* Forced action overrides, directly overwriting any fields returned by the
* handler.
*
* This is useful to produce specific scenarios where actions like body
* parsing may be missing from a framework.
*/
actionOverrides?: Partial<HandlerResponse>
): Promise<HandlerStandardReturn> => {
const serveHandler = Array.isArray(handlerOpts)
? getServeHandler(handlerOpts)
Expand Down Expand Up @@ -302,6 +312,10 @@ export const testFramework = (
}

const args = opts?.transformReq?.(req, res, envToPass) ?? [req, res];
if (actionOverrides) {
args.push({ actionOverrides });
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any
const ret = await (serveHandler as (...args: any[]) => any)(...args);

Expand Down Expand Up @@ -847,12 +861,14 @@ export const testFramework = (
requestedSyncKind,
validSignature,
allowInBandSync,
actionOverrides,
}: {
serverMode: serverKind;
sdkMode: serverKind;
requestedSyncKind: syncKind | undefined;
validSignature: boolean | undefined;
allowInBandSync: boolean;
actionOverrides?: Partial<HandlerResponse>;
}
) => {
const signingKey = "123";
Expand Down Expand Up @@ -905,7 +921,8 @@ export const testFramework = (
[envKeys.InngestAllowInBandSync]: allowInBandSync
? "true"
: "false",
}
},
actionOverrides
);

if (typeof expectedResponse === "number") {
Expand Down Expand Up @@ -1033,11 +1050,47 @@ export const testFramework = (
});
});
});

describe("#789 with no body", () => {
Object.values(serverKind).forEach((serverMode) => {
Object.values(serverKind).forEach((sdkMode) => {
expectResponse(500, {
actionOverrides: { body: () => undefined },
serverMode,
sdkMode,
requestedSyncKind: syncKind.InBand,
validSignature: true,
allowInBandSync: true,
});
});
});
});
});
});
});

describe("POST (run function)", () => {
describe("#789 missing body", () => {
test("returns 500", async () => {
const client = createClient({ id: "test" });

const fn = client.createFunction(
{ name: "Test", id: "test" },
{ event: "demo/event.sent" },
() => "fn"
);

const ret = await run(
[{ client, functions: [fn] }],
[{ method: "POST" }],
{},
{ body: () => undefined }
);

expect(ret.status).toEqual(500);
});
});

describe("signature validation", () => {
const client = createClient({ id: "test" });

Expand Down

0 comments on commit 56067cd

Please sign in to comment.