verify: does not gracefully handle invalid signatures

Question

verify: does not gracefully handle invalid signatures

mfulton26 opened this issue 3 years ago · 12 comments

What happened?

An invalid signature was sent in a request and my server returned a 500, Internal Server Error, due to verify rejecting with a TypeError:

script.ts
import { verify } from "https://cdn.skypack.dev/@octokit/webhooks-methods@3.0.0?dts"; await verify("secret", "eventPayload", "signature");

run command
NO_COLOR=true deno run script.ts \|& pbcopy

stderr

error: Uncaught TypeError: Cannot read properties of null (reading 'map')
  const integers = pairs.map(function(s) {
                         ^
    at hexToUInt8Array (https://cdn.skypack.dev/-/@octokit/webhooks-methods@v3.0.0-DdQWLXQcURxxVAmkrhzk/dist=es2019,mode=imports/optimized/@octokit/webhooks-methods.js:11:26)
    at verify (https://cdn.skypack.dev/-/@octokit/webhooks-methods@v3.0.0-DdQWLXQcURxxVAmkrhzk/dist=es2019,mode=imports/optimized/@octokit/webhooks-methods.js:7:70)
    at async file:///path/to/script.ts:3:1

What did you expect to happen?

For verify to resolve to false.

What the problem might be

verify does not validate the format of the signature before attempting to iterate over character pairs (e.g. /^[\dA-F]{64}$/i)

Answer 1 · 2022-08-30T13:45:09.000Z

Thanks for reporting this!

Just to clarify, do the example values you provided in your issue trigger this error? (I haven't had a chance to test yet.)

Would you be open to making a PR to fix this? I'd be happy to review your contribution!

Answer 2 · 2022-08-30T16:13:35.000Z

Thanks for reporting this!

Just to clarify, do the example values you provided in your issue trigger this error? (I haven't had a chance to test yet.)

Would you be open to making a PR to fix this? I'd be happy to review your contribution!

yes

I think any signature that starts with something other 2 or more non-hex characters will reproduce

Answer 3 · 2022-08-30T16:14:29.000Z

I'll see if I have time to make a PR sometime today or tomorrow

Answer 4 · 2022-08-30T23:40:34.000Z

Looks like a Deno error to me, I cannot reproduce it with Node. Tested with Node v18.8.0 and v16.17.0.

Unfortunately the libraries are not native ESM yet, Deno is using a built version of the code base served from skypack.dev, the build step might also be the culprit here.

Answer 5 · 2022-08-31T00:10:18.000Z

I see there are some browser tests. We can confirm if it is broken by adding additional test cases there first.

Answer 6 · 2022-08-31T00:13:45.000Z

The browser version is completely independent of the node implementation. In fact, I think the browser version has some other behavior differences too. e.g. I have to strip "sha256=" from the signature before passing it into verify for the browser/web version too work.

Answer 7 · 2022-08-31T00:21:11.000Z

It might be worthwhile to write the browser/web tests in Deno. Any concerns with me dropping puppeteer in favor of Deno here?

Answer 8 · 2022-08-31T00:22:59.000Z

why not both? Maybe start a PR with Deno tests, without removing the existing tests, and we take it from there?

Answer 9 · 2022-08-31T22:42:11.000Z

initial Deno tests PR: #81

Answer 10 · 2024-02-17T21:20:44.000Z

I think the proper thing to do is to catch these errors and handle them in your own code.

We are switching to outputting ESM in the next major version, which should help out with these things