‘verify’ should not detect the algorithm from the unverified signature

[andersk]

Although HMAC-SHA1 is still believed to be secure at this time, if the point of GitHub’s migration from HMAC-SHA1 to HMAC-SHA256 was to guard against potential future weaknesses in HMAC-SHA1, that point is entirely negated when an attacker can force a signature to be treated as HMAC-SHA1 simply by starting it with sha1=.

webhooks-methods.js/src/node/verify.ts

Lines 7 to 9 in 3fdcecf

	const getAlgorithm = (signature: string) => {
	return signature.startsWith("sha256=") ? "sha256" : "sha1";
	};

To fix this, the verify API needs to be changed to take the expected algorithm as a parameter, rather than detecting it from the unverified signature string.

[karfau]

Sadly this has not been addressed yet.
According to this answer which seems to be updated it is advised to "walk away but not run". So I assume there is still no rush...

By the way: The code recommended on the GitHub documentation page that the OP linked, already no longer considers the sha from the signature and assumes sha256, looks like it works in all current runtimes and browsers and is short enough to copy paste it, so you can own it, instead of importing a library for it that you have to trust passing your secret to.

Anyways, at least here is an idea how to provide a way to control the required algorithm from the outside it in a non breaking way:
change verify from this

webhooks-methods.js/src/web.ts

Lines 78 to 89 in 75feb5e

	export async function verify(
	secret: string,
	eventPayload: string,
	signature: string,
	) {
	if (!secret || !eventPayload || !signature) {
	throw new TypeError(
	"[@octokit/webhooks-methods] secret, eventPayload & signature required",
	);
	}
	const algorithm = getAlgorithm(signature);

to

 export async function verify( 
   secret: string, 
   eventPayload: string, 
   signature: string,
   algorithm = getAlgorithm(signature || ''),
 ) { 
   if (!secret || !eventPayload || !signature) { 
     throw new TypeError( 
       "[@octokit/webhooks-methods] secret, eventPayload & signature required", 
     ); 
   }

This way the option to control it becomes visible in the signature and existing code will not break, since it still falls back to sha1.

If you don't fear the breaking change you could already flip the default by dropping the getAlgorithm method from verify while still allowing users to opt in to sha1 when needed:

 export async function verify( 
   secret: string, 
   eventPayload: string, 
   signature: string,
   algorithm: 'sha256' | 'sha1' = 'sha256',
 ) { 
   if (!secret || !eventPayload || !signature) { 
     throw new TypeError( 
       "[@octokit/webhooks-methods] secret, eventPayload & signature required", 
     ); 
   }

[gr2m]

The reason we kept support for sha1 was to not break existing GitHub Enterprise versions. I'm sure all the one currently supported send out the sha256 header now, so we should remove support for sha1. Pull request welcome!