verify: does not gracefully handle invalid signatures

[mfulton26]

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/[email protected]?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/[email protected]/dist=es2019,mode=imports/optimized/@octokit/webhooks-methods.js:11:26) at verify (https://cdn.skypack.dev/-/@octokit/[email protected]/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)

[timrogers]

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!

[mfulton26]

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

[mfulton26]

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

[gr2m]

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.

[mfulton26]

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

[mfulton26]

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.

[mfulton26]

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

[gr2m]

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

[mfulton26]

initial Deno tests PR: https://github.com/octokit/webhooks-methods.js/pull/81

[wolfy1339]

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

[wolfy1339]

@mfulton26 is this still an issue?

[mfulton26]

I haven't worked with this since I reported the issue. I know Deno has made many improvements with their Node compatibility and now supports the npm: specifier so I doubt it is still an issue. Closing. Thank you.