Repository: octokit/webhooks.js (ts)

Issue:
Only accept string payloads for `webhooks.verify()` and `webhooks.verifyAndReceive()`
**What’s missing?**

We have recurring problems with some event payloads where the code returns a "signature does not match" error, mostly reported by Probot users who are probably the biggest share of users of `@octokit/webhooks`.

I think we should stop making assumptions about how a JSON payload is stringified by GitHub and only accept the raw request string, as all other webhooks SDKs do that I know of, e.g. [`stripe`](https://github.com/stripe/stripe-node#webhook-signing)

I'd consider passing the raw request body string as a best practice today, and we enforce best practices in the @octokit modules.

The challenge that this will bring is that the raw request body is not always easily accessible by server frameworks or serverless environments, so we should document how to do it with e.g. `express`, AWS Lambda, Vercel, Begin, Azure Functions, Google Cloud Functions, Cloudflare Workers, and invite users to add examples for other platforms.

**Alternatives you tried**

n/a

Deprecate `onUnhandledRequest` middleware option
Followup to #740, we should deprecate the `onUnhandledRequest` middleware option, and release that PR that is currently sitting in the `beta` branch

Pull request:
feat: deprecate `onUnhandledRequest` and accepting object payloads for  for `webhooks.verify()` and `webhooks.verifyAndReceive()`
<!-- Please refer to our contributing docs for any questions on submitting a pull request -->


<!-- Issues are required for both bug fixes and features. -->
Resolves #784
Resolves #589
See also #775

----

## Behavior

### Before the change?
<!-- Please describe the current behavior that you are modifying. -->

* The `onUnhandledRequest` option was not deprecated
* One could pass an object to the API and due to how Node works, it could change the object slightly so it wouldn't validate

### After the change?
<!-- Please describe the behavior or changes that are being added by this PR. -->

* Deprecate the `onUnhandledRequest` option
* Users will be required to pass a string version of the JSON object


### Other information
<!-- Any other information that is important to this PR  -->

* 

----

## Additional info

### Pull request checklist
- [x] Tests for the changes have been added (for bug fixes / features)
- [x] Docs have been reviewed and added / updated if needed (for bug fixes / features)
- [x] Added the appropriate label for the given change

### Does this introduce a breaking change?
<!-- If this introduces a breaking change make sure to note it here any what the impact might be -->

Please see our docs on [breaking changes](https://github.com/octokit/.github/blob/master/community/breaking_changes.md) to help!

- [ ] Yes (Please add the `Type: Breaking change` label)
- [x] No

If `Yes`, what's the impact:  

* N/A


### Pull request type

<!-- Please do not submit updates to dependencies unless it fixes an issue. --> 
<!-- Please try to limit your pull request to one type, submit multiple pull requests if needed. --> 

Please add the corresponding label for change this PR introduces:
- Bugfix: `Type: Bug`
- Feature/model/API additions: `Type: Feature`
- Updates to docs or samples: `Type: Documentation`
- Dependencies/code cleanup: `Type: Maintenance`

----



Patch:
diff --git a/README.md b/README.md
index e3fc6a7..9efae41 100644
--- 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
index 08868a6..06fba3a 100644
--- 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
index 06312ba..46398af 100644
--- 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
index 81c4e0e..48aa66f 100644
--- 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
index d20c519..ca551ca 100644
--- 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
index a2eedbe..138374d 100644
--- 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;
 };


Test patch:
diff --git a/test/integration/node-middleware.test.ts b/test/integration/node-middleware.test.ts
index d0d69f4..3ffeaab 100644
--- 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
index 97337f1..9316f9c 100644
--- 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
index e14b277..af7584a 100644
--- 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();
+  });
 });