feat: deprecate onUnhandledRequest and accepting object payloads fo…

…r for `webhooks.verify()` and `webhooks.verifyAndReceive()` (#790)
octokit · Jan 4, 2023 · c9d6a9d · c9d6a9d
1 parent bd7974d
commit c9d6a9d
Show file tree

Hide file tree

Showing 9 changed files with 136 additions and 67 deletions.
diff --git a/README.md b/README.md
@@ -191,7 +191,7 @@ webhooks.verify(eventPayload, signature);
           eventPayload
         </code>
         <em>
-          (Object or String)
+          (Object [deprecated] or String)
         </em>
       </td>
       <td>
@@ -262,7 +262,7 @@ webhooks.verifyAndReceive({ id, name, payload, signature });
           payload
         </code>
         <em>
-          Object or String
+          Object (deprecated) or String
         </em>
       </td>
       <td>
@@ -601,7 +601,7 @@ Used for internal logging. Defaults to [`console`](https://developer.mozilla.org
     </tr>
     <tr>
       <td>
-        <code>onUnhandledRequest</code>
+        <code>onUnhandledRequest (deprecated)</code>
         <em>
           function
         </em>

diff --git a/src/index.ts b/src/index.ts
@@ -19,13 +19,21 @@ import {
 export { createNodeMiddleware } from "./middleware/node/index";
 export { emitterEventNames } from "./generated/webhook-names";
 
+export type Iverify = {
+  /** @deprecated Passing a JSON payload object to `verify()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`. */
+  (eventPayload: object, signature: string): Promise<boolean>;
+  (eventPayload: string, signature: string): Promise<boolean>;
+};
+export type IverifyAndReceive = {
+  /** @deprecated Passing a JSON payload object to `verifyAndReceive()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`. */
+  (options: EmitterWebhookEventWithSignature): Promise<void>;
+  (options: EmitterWebhookEventWithStringPayloadAndSignature): Promise<void>;
+};
+
 // U holds the return value of `transform` function in Options
 class Webhooks<TTransformed = unknown> {
   public sign: (payload: string | object) => Promise<string>;
-  public verify: (
-    eventPayload: string | object,
-    signature: string
-  ) => Promise<boolean>;
+  public verify: Iverify;
   public on: <E extends EmitterWebhookEventName>(
     event: E | E[],
     callback: HandlerFunction<E, TTransformed>
@@ -37,11 +45,7 @@ class Webhooks<TTransformed = unknown> {
     callback: RemoveHandlerFunction<E, TTransformed>
   ) => void;
   public receive: (event: EmitterWebhookEvent) => Promise<void>;
-  public verifyAndReceive: (
-    options:
-      | EmitterWebhookEventWithStringPayloadAndSignature
-      | EmitterWebhookEventWithSignature
-  ) => Promise<void>;
+  public verifyAndReceive: IverifyAndReceive;
 
   constructor(options: Options<TTransformed> & { secret: string }) {
     if (!options || !options.secret) {
@@ -56,13 +60,31 @@ class Webhooks<TTransformed = unknown> {
     };
 
     this.sign = sign.bind(null, options.secret);
-    this.verify = verify.bind(null, options.secret);
+    this.verify = (eventPayload: object | string, signature: string) => {
+      if (typeof eventPayload === "object") {
+        console.error(
+          "[@octokit/webhooks] Passing a JSON payload object to `verify()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`"
+        );
+      }
+      return verify(options.secret, eventPayload, signature);
+    };
     this.on = state.eventHandler.on;
     this.onAny = state.eventHandler.onAny;
     this.onError = state.eventHandler.onError;
     this.removeListener = state.eventHandler.removeListener;
     this.receive = state.eventHandler.receive;
-    this.verifyAndReceive = verifyAndReceive.bind(null, state);
+    this.verifyAndReceive = (
+      options:
+        | EmitterWebhookEventWithStringPayloadAndSignature
+        | EmitterWebhookEventWithSignature
+    ) => {
+      if (typeof options.payload === "object") {
+        console.error(
+          "[@octokit/webhooks] Passing a JSON payload object to `verifyAndReceive()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`"
+        );
+      }
+      return verifyAndReceive(state, options);
+    };
   }
 }
 

diff --git a/src/middleware/node/index.ts b/src/middleware/node/index.ts
@@ -12,9 +12,15 @@ export function createNodeMiddleware(
     log = createLogger(),
   }: MiddlewareOptions = {}
 ) {
+  const deprecateOnUnhandledRequest = (request: any, response: any) => {
+    console.error(
+      "[@octokit/webhooks] `onUnhandledRequest()` is deprecated and will be removed in a future release of `@octokit/webhooks`"
+    );
+    return onUnhandledRequest(request, response);
+  };
   return middleware.bind(null, webhooks, {
     path,
-    onUnhandledRequest,
+    onUnhandledRequest: deprecateOnUnhandledRequest,
     log,
   } as Required<MiddlewareOptions>);
 }
diff --git a/src/middleware/node/types.ts b/src/middleware/node/types.ts
@@ -9,6 +9,7 @@ import { Logger } from "../../createLogger";
 export type MiddlewareOptions = {
   path?: string;
   log?: Logger;
+  /** @deprecated `onUnhandledRequest()` is deprecated and will be removed in a future release of `@octokit/webhooks` */
   onUnhandledRequest?: (
     request: IncomingMessage,
     response: ServerResponse

diff --git a/src/to-normalized-json-string.ts b/src/to-normalized-json-string.ts
@@ -1,5 +1,5 @@
 /**
- * GitHub sends its JSON with an indentation of 2 spaces and a line break at the end
+ * GitHub sends its JSON with no indentation and no line break at the end
  */
 export function toNormalizedJsonString(payload: object) {
   const payloadString = JSON.stringify(payload);

diff --git a/src/types.ts b/src/types.ts
@@ -21,7 +21,7 @@ export type EmitterWebhookEventWithStringPayloadAndSignature = {
   payload: string;
   signature: string;
 };
-
+/** @deprecated */
 export type EmitterWebhookEventWithSignature = EmitterWebhookEvent & {
   signature: string;
 };

diff --git a/test/integration/node-middleware.test.ts b/test/integration/node-middleware.test.ts
@@ -168,43 +168,6 @@ describe("createNodeMiddleware(webhooks)", () => {
     server.close();
   });
 
-  test("custom non-found handler", async () => {
-    const webhooks = new Webhooks({
-      secret: "mySecret",
-    });
-
-    const server = createServer(
-      createNodeMiddleware(webhooks, {
-        onUnhandledRequest(_request, response) {
-          response.writeHead(404);
-          response.end("nope");
-        },
-      })
-    ).listen();
-
-    // @ts-expect-error complains about { port } although it's included in returned AddressInfo interface
-    const { port } = server.address();
-
-    const response = await fetch(
-      `http://localhost:${port}/api/github/webhooks`,
-      {
-        method: "PUT",
-        headers: {
-          "X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
-          "X-GitHub-Event": "push",
-          "X-Hub-Signature-256": signatureSha256,
-        },
-        body: "invalid",
-      }
-    );
-
-    expect(response.status).toEqual(404);
-
-    await expect(response.text()).resolves.toEqual("nope");
-
-    server.close();
-  });
-
   test("Handles missing headers", async () => {
     const webhooks = new Webhooks({
       secret: "mySecret",

diff --git a/test/integration/webhooks.test.ts b/test/integration/webhooks.test.ts
@@ -2,7 +2,7 @@ import { readFileSync } from "fs";
 
 import { sign } from "@octokit/webhooks-methods";
 
-import { Webhooks, EmitterWebhookEvent } from "../../src";
+import { Webhooks } from "../../src";
 import { toNormalizedJsonString } from "../../src/to-normalized-json-string";
 
 const pushEventPayloadString = readFileSync(
@@ -32,16 +32,6 @@ describe("Webhooks", () => {
     await webhooks.sign(pushEventPayloadString);
   });
 
-  test("webhooks.verify(payload, signature) with object payload", async () => {
-    const secret = "mysecret";
-    const webhooks = new Webhooks({ secret });
-
-    await webhooks.verify(
-      JSON.parse(pushEventPayloadString),
-      await sign({ secret, algorithm: "sha256" }, pushEventPayloadString)
-    );
-  });
-
   test("webhooks.verify(payload, signature) with object payload containing special characters", async () => {
     const secret = "mysecret";
     const webhooks = new Webhooks({ secret });
@@ -84,7 +74,7 @@ describe("Webhooks", () => {
   test("webhooks.verifyAndReceive(event) with incorrect signature", async () => {
     const webhooks = new Webhooks({ secret: "mysecret" });
 
-    const pingPayload = {} as EmitterWebhookEvent<"ping">["payload"];
+    const pingPayload = "{}";
     await expect(async () =>
       webhooks.verifyAndReceive({
         id: "1",

diff --git a/test/unit/deprecation.test.ts b/test/unit/deprecation.test.ts
@@ -1,3 +1,90 @@
+import { createServer } from "http";
+import { readFileSync } from "fs";
+
+import fetch from "node-fetch";
+import { sign } from "@octokit/webhooks-methods";
+
+import { Webhooks, createNodeMiddleware } from "../../src";
+
+const pushEventPayloadString = readFileSync(
+  "test/fixtures/push-payload.json",
+  "utf-8"
+);
+let signatureSha256: string;
 describe("Deprecations", () => {
-  test("there are currently no deprecations", () => {});
+  beforeAll(async () => {
+    signatureSha256 = await sign(
+      { secret: "mySecret", algorithm: "sha256" },
+      pushEventPayloadString
+    );
+  });
+  test("onUnhandledRequest", async () => {
+    const spy = jest.spyOn(console, "error");
+    const webhooks = new Webhooks({
+      secret: "mySecret",
+    });
+
+    const server = createServer(
+      createNodeMiddleware(webhooks, {
+        onUnhandledRequest(_request, response) {
+          response.writeHead(404);
+          response.end("nope");
+        },
+      })
+    ).listen();
+
+    // @ts-expect-error complains about { port } although it's included in returned AddressInfo interface
+    const { port } = server.address();
+
+    await fetch(`http://localhost:${port}/api/github/webhooks`, {
+      method: "PUT",
+      headers: {
+        "X-GitHub-Delivery": "123e4567-e89b-12d3-a456-426655440000",
+        "X-GitHub-Event": "push",
+        "X-Hub-Signature-256": signatureSha256,
+      },
+      body: "invalid",
+    });
+
+    expect(spy).toBeCalledWith(
+      "[@octokit/webhooks] `onUnhandledRequest()` is deprecated and will be removed in a future release of `@octokit/webhooks`"
+    );
+    spy.mockClear();
+    server.close();
+  });
+
+  test("webhooks.verify(payload, signature) with object payload", async () => {
+    const spy = jest.spyOn(console, "error");
+    const secret = "mysecret";
+    const webhooks = new Webhooks({ secret });
+
+    await webhooks.verify(
+      JSON.parse(pushEventPayloadString),
+      await sign({ secret, algorithm: "sha256" }, pushEventPayloadString)
+    );
+    expect(spy).toBeCalledWith(
+      "[@octokit/webhooks] Passing a JSON payload object to `verify()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`"
+    );
+    spy.mockClear();
+  });
+
+  test("webhooks.verifyAndReceive(payload, signature) with object payload", async () => {
+    const spy = jest.spyOn(console, "error");
+    const secret = "mysecret";
+    const webhooks = new Webhooks({ secret });
+
+    await webhooks.verifyAndReceive({
+      id: "123e456",
+      name: "push",
+      payload: JSON.parse(pushEventPayloadString),
+      signature: await sign(
+        { secret, algorithm: "sha256" },
+        pushEventPayloadString
+      ),
+    });
+    expect(spy).toBeCalledWith(
+      "[@octokit/webhooks] Passing a JSON payload object to `verifyAndReceive()` is deprecated and the functionality will be removed in a future release of `@octokit/webhooks`"
+    );
+    spy.mockClear();
+  });
 });