Skip to content
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

Middleware changes #693

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 31 additions & 18 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ const webhooks = new Webhooks({
secret: "mysecret",
});

webhooks.onAny(({ id, name, payload }) => {
webhooks.onAny(({ id, name, payload, extraData }) => {
console.log(name, "event received");
});

Expand Down Expand Up @@ -223,7 +223,7 @@ The `verify` method can be imported as static method from [`@octokit/webhooks-me
### webhooks.verifyAndReceive()

```js
webhooks.verifyAndReceive({ id, name, payload, signature });
webhooks.verifyAndReceive({ id, name, payload, extraData, signature });
```

<table width="100%">
Expand Down Expand Up @@ -316,7 +316,7 @@ eventHandler
### webhooks.receive()

```js
webhooks.receive({ id, name, payload });
webhooks.receive({ id, name, payload, extraData });
```

<table width="100%">
Expand Down Expand Up @@ -370,6 +370,8 @@ Returns a promise. Runs all handlers set with [`webhooks.on()`](#webhookson) in

The `.receive()` method belongs to the `event-handler` module which can be used [standalone](src/event-handler/).

The `extraData` is an optional parameter, if it is set, it will be available in the `on` functions.

### webhooks.on()

```js
Expand Down Expand Up @@ -420,7 +422,7 @@ webhooks.on(eventNames, handler);
<strong>Required.</strong>
Method to be run each time the event with the passed name is received.
the <code>handler</code> function can be an async function, throw an error or
return a Promise. The handler is called with an event object: <code>{id, name, payload}</code>.
return a Promise. The handler is called with an event object: <code>{id, name, payload, extraData}</code>.
</td>
</tr>
</tbody>
Expand Down Expand Up @@ -449,7 +451,7 @@ webhooks.onAny(handler);
<strong>Required.</strong>
Method to be run each time any event is received.
the <code>handler</code> function can be an async function, throw an error or
return a Promise. The handler is called with an event object: <code>{id, name, payload}</code>.
return a Promise. The handler is called with an event object: <code>{id, name, payload, extraData}</code>.
</td>
</tr>
</tbody>
Expand Down Expand Up @@ -482,7 +484,7 @@ Asynchronous `error` event handler are not blocking the `.receive()` method from
<strong>Required.</strong>
Method to be run each time a webhook event handler throws an error or returns a promise that rejects.
The <code>handler</code> function can be an async function,
return a Promise. The handler is called with an error object that has a .event property which has all the information on the event: <code>{id, name, payload}</code>.
return a Promise. The handler is called with an error object that has a .event property which has all the information on the event: <code>{id, name, payload, extraData}</code>.
</td>
</tr>
</tbody>
Expand Down Expand Up @@ -579,21 +581,32 @@ createServer(middleware).listen(3000);
<td>
<code>path</code>
<em>
string
string | RegEx
</em>
</td>
<td>
Custom path to match requests against. Defaults to <code>/api/github/webhooks</code>.
</td>
</tr>
<tr>
<td>
<code>log</code>
<em>
object
</em>
</td>
<td>

Can be used as a regular expression;

```js
const middleware = createNodeMiddleware(webhooks, {
path: /^\/api\/github\/webhooks/,
});
```

Test the regex before usage, the `g` and `y` flags [makes it stateful](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp/test)!

</td>
</tr>
<tr>
<td>
<code>log</code>
<em>
object
</em>
</td>
<td>

Used for internal logging. Defaults to [`console`](https://developer.mozilla.org/en-US/docs/Web/API/console) with `debug` and `info` doing nothing.

Expand Down Expand Up @@ -721,7 +734,7 @@ A union of all possible events and event/action combinations supported by the ev

### `EmitterWebhookEvent`

The object that is emitted by `@octokit/webhooks` as an event; made up of an `id`, `name`, and `payload` properties.
The object that is emitted by `@octokit/webhooks` as an event; made up of an `id`, `name`, and `payload` properties, with an optional `extraData`.
An optional generic parameter can be passed to narrow the type of the `name` and `payload` properties based on event names or event/action combinations, e.g. `EmitterWebhookEvent<"check_run" | "code_scanning_alert.fixed">`.

## License
Expand Down
15 changes: 12 additions & 3 deletions src/middleware/node/middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,11 @@ export async function middleware(
);
return;
}

const isUnknownRoute = request.method !== "POST" || pathname !== options.path;
const pathMatch =
options.path instanceof RegExp
? options.path.test(pathname)
: pathname === options.path;
const isUnknownRoute = request.method !== "POST" || !pathMatch;
const isExpressMiddleware = typeof next === "function";
if (isUnknownRoute) {
if (isExpressMiddleware) {
Expand Down Expand Up @@ -72,7 +75,12 @@ export async function middleware(
didTimeout = true;
response.statusCode = 202;
response.end("still processing\n");
}, 9000).unref();
Copy link
Member

@wolfy1339 wolfy1339 Jun 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed for Node, so it exits even with a timeout running
octokit/octokit.js#2079

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read what unref is and why it is used, and I'm almost certain that we don't "need" that unref (we have a small timeout and also we clear in every paths). Also, v8 does not have unref, so cloudflare workers die BCS of it.

Probably we should use the below check instead of just adding/deleting it;

if (typeof timeout.unref === "function") {
  timeout.unref();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, v8 does not have unref, so cloudflare workers die BCS of it

are you sure? Node is built on v8

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I faced the "unref is not a function" error when I tried to run it on cloudflare workers so I'm sure. But a fixup commit is on the way.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this back with an if, and I left out the else from the branch coverage, to keep it 100%. I don't think it is worth the effort to mock the setInterval out.

}, 9000);

/* istanbul ignore else */
if (typeof timeout.unref === "function") {
timeout.unref();
}

try {
const payload = await getPayload(request);
Expand All @@ -82,6 +90,7 @@ export async function middleware(
name: eventName as any,
payload: payload as any,
signature: signatureSHA256,
extraData: request,
});
clearTimeout(timeout);

Expand Down
2 changes: 1 addition & 1 deletion src/middleware/node/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ type ServerResponse = any;
import { Logger } from "../../createLogger";

export type MiddlewareOptions = {
path?: string;
path?: string | RegExp;
log?: Logger;
onUnhandledRequest?: (
request: IncomingMessage,
Expand Down
4 changes: 4 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ export type EmitterWebhookEvent<
> = TEmitterEvent extends `${infer TWebhookEvent}.${infer TAction}`
? BaseWebhookEvent<Extract<TWebhookEvent, WebhookEventName>> & {
payload: { action: TAction };
} & {
extraData?: any;
}
: BaseWebhookEvent<Extract<TEmitterEvent, WebhookEventName>>;

Expand All @@ -20,6 +22,7 @@ export type EmitterWebhookEventWithStringPayloadAndSignature = {
name: EmitterWebhookEventName;
payload: string;
signature: string;
extraData?: any;
};

export type EmitterWebhookEventWithSignature = EmitterWebhookEvent & {
Expand All @@ -30,6 +33,7 @@ interface BaseWebhookEvent<TName extends WebhookEventName> {
id: string;
name: TName;
payload: WebhookEventMap[TName];
extraData?: any;
}

export interface Options<TTransformed = unknown> {
Expand Down
1 change: 1 addition & 0 deletions src/verify-and-receive.ts
Original file line number Diff line number Diff line change
Expand Up @@ -39,5 +39,6 @@ export async function verifyAndReceive(
typeof event.payload === "string"
? JSON.parse(event.payload)
: event.payload,
extraData: event.extraData,
});
}
111 changes: 111 additions & 0 deletions test/integration/node-middleware.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,117 @@ describe("createNodeMiddleware(webhooks)", () => {
server.close();
});

test("path match with regex", async () => {
expect.assertions(7);

const webhooks = new Webhooks({
secret: "mySecret",
});

const server = createServer(
createNodeMiddleware(webhooks, {
path: /^\/api\/github\/webhooks/,
})
).listen();

// @ts-expect-error complains about { port } although it's included in returned AddressInfo interface
const { port } = server.address();

webhooks.on("push", (event) => {
expect(event.id).toBe("123e4567-e89b-12d3-a456-426655440000");
});

const response1 = await fetch(
`http://localhost:${port}/api/github/webhooks/0001/testurl`,
{
method: "POST",
headers: {
"X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
"X-GitHub-Event": "push",
"X-Hub-Signature-256": signatureSha256,
},
body: pushEventPayload,
}
);

expect(response1.status).toEqual(200);
await expect(response1.text()).resolves.toBe("ok\n");

const response2 = await fetch(
`http://localhost:${port}/api/github/webhooks/0001/testurl`,
{
method: "POST",
headers: {
"X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
"X-GitHub-Event": "push",
"X-Hub-Signature-256": signatureSha256,
},
body: pushEventPayload,
}
);

expect(response2.status).toEqual(200);
await expect(response2.text()).resolves.toBe("ok\n");

const response3 = await fetch(`http://localhost:${port}/api/github/web`, {
method: "POST",
headers: {
"X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
"X-GitHub-Event": "push",
"X-Hub-Signature-256": signatureSha256,
},
body: pushEventPayload,
});

expect(response3.status).toEqual(404);

server.close();
});

test("original request passed by as intended", async () => {
expect.assertions(6);

const webhooks = new Webhooks({
secret: "mySecret",
});

const server = createServer(
createNodeMiddleware(webhooks, {
path: /^\/api\/github\/webhooks/,
})
).listen();

// @ts-expect-error complains about { port } although it's included in returned AddressInfo interface
const { port } = server.address();

webhooks.on("push", (event) => {
expect(event.id).toBe("123e4567-e89b-12d3-a456-426655440000");
const r = event.extraData;
expect(r).toBeDefined();
expect(r?.headers["my-custom-header"]).toBe("customHeader");
expect(r?.url).toBe(`/api/github/webhooks/0001/testurl`);
});

const response = await fetch(
`http://localhost:${port}/api/github/webhooks/0001/testurl`,
{
method: "POST",
headers: {
"X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
"X-GitHub-Event": "push",
"X-Hub-Signature-256": signatureSha256,
"my-custom-header": "customHeader",
},
body: pushEventPayload,
}
);

expect(response.status).toEqual(200);
await expect(response.text()).resolves.toBe("ok\n");

server.close();
});

test("request.body already parsed (e.g. Lambda)", async () => {
expect.assertions(3);

Expand Down