Actually, looking at how this gets implemented, it might be easy enough to be left outside this package (choice is up to maintainers):

$sig = 'sha1='.hash_hmac('sha1', $requestBody, $secret);

return hash_equals($sig, $signature);

I actually like the idea! I had to implement (copy the code after the first time) the same check a few times. So I think a utility class for this check can easily be added and help developers in the future. The code I created in the past

list($userSignatureAlgo, $userSignatureHash) = explode('=', $userSignature);
$signature = hash_hmac($userSignatureAlgo, $userBody, $secret);

if (!hash_equals($signature, $userSignatureHash)) {
     throw new InvalidSignatureException('Signatures didn\'t match!');
}

So i'm 👍 on including it here in a small helper class. Any thoughts @Nyholm?

@acrobat your solution lets the input control the hashing algo, with no validation of the provided value. That's exactly how all JWT implementation out there (in all languages) faced a security issue allowing to bypass the signature check (as they could control the algo used to validate it).
Github says they always use sha1. So until they migrate to a different algo, it is much safer to accept only that one (and the day they support another one, the logic choosing the algo should only allow using sha1 and the new one, not any algo known by PHP).

and for the throwing, I would not make it part of the validation logic, as it makes it harder in case they want a different behavior for this case.

@stof Then just catch the exception

@acrobat your solution lets the input control the hashing algo, with no validation of the provided value. That's exactly how all JWT implementation out there (in all languages) faced a security issue allowing to bypass the signature check (as they could control the algo used to validate it).
Github says they always use sha1. So until they migrate to a different algo, it is much safer to accept only that one (and the day they support another one, the logic choosing the algo should only allow using sha1 and the new one, not any algo known by PHP).

Hmm 🤔 you are indeed right, thanks for pointing that out!

Can you start a PR for it @stof? So we can discuss further changes there (like throwing an exception or not), thanks!

@m1guelpf having an API turning a boolean into an exception that you need to catch again, to either not throw or throw a different exception suited to your needs ? Why not keeping a utility returning the boolean and letting users decide what they want to do with it ?
This SDK has no idea how the code surrounding the signature check looks like, as it is not a framework for building github webhooks.

@acrobat the question is whether the 2 LOC of #741 (comment) are worth building a utility for that (or just documenting them in the README for people wondering the equivalent of the Ruby code provided in the github doc).

For what it is worth, none of the official Octokit SDKs for node.js, Ruby, .NET or go are providing it (I haven't check the ObjC one). The only place I found this logic is in https://github.com/octokit/webhooks.js which is a framework for implementing webhook-based services (used at the core of probot).
So I'm not even sure this should be part of this SDK.

and if we decide to put it in this package, where should it live ? It does not really fit into the existing architecture, as it is not about accessing an API.

yeah, that's true, it doesn't fit in the current structure... 🤔

Note that when I opened the initial issue, I hadn't looked for the implementation of computing the signature yet. I was fearing it would require using the openssl APIs (which are easy to misuse). But the fact that it is just hash_hmac('sha1', $requestBody, $secret) makes it dead easy.

@stof Maybe just a docs entry with a minimul code example is better then?