Skip to content

Commit c9d6a9d

Browse files
authored
feat: deprecate onUnhandledRequest and accepting object payloads for for webhooks.verify() and webhooks.verifyAndReceive() (#790)
1 parent bd7974d commit c9d6a9d

File tree

9 files changed

+136
-67
lines changed

9 files changed

+136
-67
lines changed

README.md

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ webhooks.verify(eventPayload, signature);
191191
eventPayload
192192
</code>
193193
<em>
194-
(Object or String)
194+
(Object [deprecated] or String)
195195
</em>
196196
</td>
197197
<td>
@@ -262,7 +262,7 @@ webhooks.verifyAndReceive({ id, name, payload, signature });
262262
payload
263263
</code>
264264
<em>
265-
Object or String
265+
Object (deprecated) or String
266266
</em>
267267
</td>
268268
<td>
@@ -601,7 +601,7 @@ Used for internal logging. Defaults to [`console`](https://developer.mozilla.org
601601
</tr>
602602
<tr>
603603
<td>
604-
<code>onUnhandledRequest</code>
604+
<code>onUnhandledRequest (deprecated)</code>
605605
<em>
606606
function
607607
</em>

src/index.ts

Lines changed: 33 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -19,13 +19,21 @@ import {
1919
export { createNodeMiddleware } from "./middleware/node/index";
2020
export { emitterEventNames } from "./generated/webhook-names";
2121

22+
export type Iverify = {
23+
/** @deprecated Passing a JSON payload object to `verify()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`. */
24+
(eventPayload: object, signature: string): Promise<boolean>;
25+
(eventPayload: string, signature: string): Promise<boolean>;
26+
};
27+
export type IverifyAndReceive = {
28+
/** @deprecated Passing a JSON payload object to `verifyAndReceive()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`. */
29+
(options: EmitterWebhookEventWithSignature): Promise<void>;
30+
(options: EmitterWebhookEventWithStringPayloadAndSignature): Promise<void>;
31+
};
32+
2233
// U holds the return value of `transform` function in Options
2334
class Webhooks<TTransformed = unknown> {
2435
public sign: (payload: string | object) => Promise<string>;
25-
public verify: (
26-
eventPayload: string | object,
27-
signature: string
28-
) => Promise<boolean>;
36+
public verify: Iverify;
2937
public on: <E extends EmitterWebhookEventName>(
3038
event: E | E[],
3139
callback: HandlerFunction<E, TTransformed>
@@ -37,11 +45,7 @@ class Webhooks<TTransformed = unknown> {
3745
callback: RemoveHandlerFunction<E, TTransformed>
3846
) => void;
3947
public receive: (event: EmitterWebhookEvent) => Promise<void>;
40-
public verifyAndReceive: (
41-
options:
42-
| EmitterWebhookEventWithStringPayloadAndSignature
43-
| EmitterWebhookEventWithSignature
44-
) => Promise<void>;
48+
public verifyAndReceive: IverifyAndReceive;
4549

4650
constructor(options: Options<TTransformed> & { secret: string }) {
4751
if (!options || !options.secret) {
@@ -56,13 +60,31 @@ class Webhooks<TTransformed = unknown> {
5660
};
5761

5862
this.sign = sign.bind(null, options.secret);
59-
this.verify = verify.bind(null, options.secret);
63+
this.verify = (eventPayload: object | string, signature: string) => {
64+
if (typeof eventPayload === "object") {
65+
console.error(
66+
"[@octokit/webhooks] Passing a JSON payload object to `verify()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`"
67+
);
68+
}
69+
return verify(options.secret, eventPayload, signature);
70+
};
6071
this.on = state.eventHandler.on;
6172
this.onAny = state.eventHandler.onAny;
6273
this.onError = state.eventHandler.onError;
6374
this.removeListener = state.eventHandler.removeListener;
6475
this.receive = state.eventHandler.receive;
65-
this.verifyAndReceive = verifyAndReceive.bind(null, state);
76+
this.verifyAndReceive = (
77+
options:
78+
| EmitterWebhookEventWithStringPayloadAndSignature
79+
| EmitterWebhookEventWithSignature
80+
) => {
81+
if (typeof options.payload === "object") {
82+
console.error(
83+
"[@octokit/webhooks] Passing a JSON payload object to `verifyAndReceive()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`"
84+
);
85+
}
86+
return verifyAndReceive(state, options);
87+
};
6688
}
6789
}
6890

src/middleware/node/index.ts

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,15 @@ export function createNodeMiddleware(
1212
log = createLogger(),
1313
}: MiddlewareOptions = {}
1414
) {
15+
const deprecateOnUnhandledRequest = (request: any, response: any) => {
16+
console.error(
17+
"[@octokit/webhooks] `onUnhandledRequest()` is deprecated and will be removed in a future release of `@octokit/webhooks`"
18+
);
19+
return onUnhandledRequest(request, response);
20+
};
1521
return middleware.bind(null, webhooks, {
1622
path,
17-
onUnhandledRequest,
23+
onUnhandledRequest: deprecateOnUnhandledRequest,
1824
log,
1925
} as Required<MiddlewareOptions>);
2026
}

src/middleware/node/types.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { Logger } from "../../createLogger";
99
export type MiddlewareOptions = {
1010
path?: string;
1111
log?: Logger;
12+
/** @deprecated `onUnhandledRequest()` is deprecated and will be removed in a future release of `@octokit/webhooks` */
1213
onUnhandledRequest?: (
1314
request: IncomingMessage,
1415
response: ServerResponse

src/to-normalized-json-string.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/**
2-
* GitHub sends its JSON with an indentation of 2 spaces and a line break at the end
2+
* GitHub sends its JSON with no indentation and no line break at the end
33
*/
44
export function toNormalizedJsonString(payload: object) {
55
const payloadString = JSON.stringify(payload);

src/types.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ export type EmitterWebhookEventWithStringPayloadAndSignature = {
2121
payload: string;
2222
signature: string;
2323
};
24-
24+
/** @deprecated */
2525
export type EmitterWebhookEventWithSignature = EmitterWebhookEvent & {
2626
signature: string;
2727
};

test/integration/node-middleware.test.ts

Lines changed: 0 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -168,43 +168,6 @@ describe("createNodeMiddleware(webhooks)", () => {
168168
server.close();
169169
});
170170

171-
test("custom non-found handler", async () => {
172-
const webhooks = new Webhooks({
173-
secret: "mySecret",
174-
});
175-
176-
const server = createServer(
177-
createNodeMiddleware(webhooks, {
178-
onUnhandledRequest(_request, response) {
179-
response.writeHead(404);
180-
response.end("nope");
181-
},
182-
})
183-
).listen();
184-
185-
// @ts-expect-error complains about { port } although it's included in returned AddressInfo interface
186-
const { port } = server.address();
187-
188-
const response = await fetch(
189-
`http://localhost:${port}/api/github/webhooks`,
190-
{
191-
method: "PUT",
192-
headers: {
193-
"X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
194-
"X-GitHub-Event": "push",
195-
"X-Hub-Signature-256": signatureSha256,
196-
},
197-
body: "invalid",
198-
}
199-
);
200-
201-
expect(response.status).toEqual(404);
202-
203-
await expect(response.text()).resolves.toEqual("nope");
204-
205-
server.close();
206-
});
207-
208171
test("Handles missing headers", async () => {
209172
const webhooks = new Webhooks({
210173
secret: "mySecret",

test/integration/webhooks.test.ts

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import { readFileSync } from "fs";
22

33
import { sign } from "@octokit/webhooks-methods";
44

5-
import { Webhooks, EmitterWebhookEvent } from "../../src";
5+
import { Webhooks } from "../../src";
66
import { toNormalizedJsonString } from "../../src/to-normalized-json-string";
77

88
const pushEventPayloadString = readFileSync(
@@ -32,16 +32,6 @@ describe("Webhooks", () => {
3232
await webhooks.sign(pushEventPayloadString);
3333
});
3434

35-
test("webhooks.verify(payload, signature) with object payload", async () => {
36-
const secret = "mysecret";
37-
const webhooks = new Webhooks({ secret });
38-
39-
await webhooks.verify(
40-
JSON.parse(pushEventPayloadString),
41-
await sign({ secret, algorithm: "sha256" }, pushEventPayloadString)
42-
);
43-
});
44-
4535
test("webhooks.verify(payload, signature) with object payload containing special characters", async () => {
4636
const secret = "mysecret";
4737
const webhooks = new Webhooks({ secret });
@@ -84,7 +74,7 @@ describe("Webhooks", () => {
8474
test("webhooks.verifyAndReceive(event) with incorrect signature", async () => {
8575
const webhooks = new Webhooks({ secret: "mysecret" });
8676

87-
const pingPayload = {} as EmitterWebhookEvent<"ping">["payload"];
77+
const pingPayload = "{}";
8878
await expect(async () =>
8979
webhooks.verifyAndReceive({
9080
id: "1",

test/unit/deprecation.test.ts

Lines changed: 88 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,90 @@
1+
import { createServer } from "http";
2+
import { readFileSync } from "fs";
3+
4+
import fetch from "node-fetch";
5+
import { sign } from "@octokit/webhooks-methods";
6+
7+
import { Webhooks, createNodeMiddleware } from "../../src";
8+
9+
const pushEventPayloadString = readFileSync(
10+
"test/fixtures/push-payload.json",
11+
"utf-8"
12+
);
13+
let signatureSha256: string;
114
describe("Deprecations", () => {
2-
test("there are currently no deprecations", () => {});
15+
beforeAll(async () => {
16+
signatureSha256 = await sign(
17+
{ secret: "mySecret", algorithm: "sha256" },
18+
pushEventPayloadString
19+
);
20+
});
21+
test("onUnhandledRequest", async () => {
22+
const spy = jest.spyOn(console, "error");
23+
const webhooks = new Webhooks({
24+
secret: "mySecret",
25+
});
26+
27+
const server = createServer(
28+
createNodeMiddleware(webhooks, {
29+
onUnhandledRequest(_request, response) {
30+
response.writeHead(404);
31+
response.end("nope");
32+
},
33+
})
34+
).listen();
35+
36+
// @ts-expect-error complains about { port } although it's included in returned AddressInfo interface
37+
const { port } = server.address();
38+
39+
await fetch(`http://localhost:${port}/api/github/webhooks`, {
40+
method: "PUT",
41+
headers: {
42+
"X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
43+
"X-GitHub-Event": "push",
44+
"X-Hub-Signature-256": signatureSha256,
45+
},
46+
body: "invalid",
47+
});
48+
49+
expect(spy).toBeCalledWith(
50+
"[@octokit/webhooks] `onUnhandledRequest()` is deprecated and will be removed in a future release of `@octokit/webhooks`"
51+
);
52+
spy.mockClear();
53+
server.close();
54+
});
55+
56+
test("webhooks.verify(payload, signature) with object payload", async () => {
57+
const spy = jest.spyOn(console, "error");
58+
const secret = "mysecret";
59+
const webhooks = new Webhooks({ secret });
60+
61+
await webhooks.verify(
62+
JSON.parse(pushEventPayloadString),
63+
await sign({ secret, algorithm: "sha256" }, pushEventPayloadString)
64+
);
65+
expect(spy).toBeCalledWith(
66+
"[@octokit/webhooks] Passing a JSON payload object to `verify()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`"
67+
);
68+
spy.mockClear();
69+
});
70+
71+
test("webhooks.verifyAndReceive(payload, signature) with object payload", async () => {
72+
const spy = jest.spyOn(console, "error");
73+
const secret = "mysecret";
74+
const webhooks = new Webhooks({ secret });
75+
76+
await webhooks.verifyAndReceive({
77+
id: "123e456",
78+
name: "push",
79+
payload: JSON.parse(pushEventPayloadString),
80+
signature: await sign(
81+
{ secret, algorithm: "sha256" },
82+
pushEventPayloadString
83+
),
84+
});
85+
expect(spy).toBeCalledWith(
86+
"[@octokit/webhooks] Passing a JSON payload object to `verifyAndReceive()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`"
87+
);
88+
spy.mockClear();
89+
});
390
});

0 commit comments

Comments
 (0)