Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use webhook signing key instead of API key for webhook signature verification #819

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

damiantw
Copy link

When instantiating a Mailgun instance you are expected to provide an API key that is used to authenticate requests to the REST API.

// First, instantiate the SDK with your API credentials
$mg = Mailgun::create('key-example');

This API key was passed to the Api\Webhook instance that is instantiated via $mg->webhooks().

public function webhooks(): Api\Webhook
{
    return new Api\Webhook($this->httpClient, $this->requestBuilder, $this->hydrator, $this->apiKey);
}

https://github.com/mailgun/mailgun-php/blob/master/src/Mailgun.php#L142

This passed API key is used in the verifyWebhookSignature function.

This will result in improper verification results as the account webhook signing key should be used to compute the signature, not the REST API key.

As is, you must manually instantiate an Api\Webhook instance to provide a webhook signing key and properly validate webhook signatures using this library.

$webhook = new Webhook((new HttpClientConfigurator)->createConfiguredClient(), new RequestBuilder, new ModelHydrator,  'signing-key');
        
$webhook->verifyWebhookSignature($timestamp, $token, $signature); // This will work

This PR allows for passing a webhook signing key using the $mg->webhooks('signing-key') function.

@@ -139,9 +139,9 @@ public function tags(): Api\Tag
return new Api\Tag($this->httpClient, $this->requestBuilder, $this->hydrator);
}

public function webhooks(): Api\Webhook
public function webhooks(string $signingKey = ''): Api\Webhook

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public function webhooks(string $signingKey = ''): Api\Webhook
public function webhooks(string $signingKey): Api\Webhook

signing key isn't optional

@faizanakram99
Copy link

@Nyholm this fixes an important bug, signing key used to be equal to api key earlier but it is not the case at least since last 2 years, this is a breaking change though... the signature changes but not sure whats the best way here considering the existing code is broken, verification fails when using api key

@dpslwk
Copy link

dpslwk commented Aug 19, 2024

changing to
return new Api\Webhook($this->httpClient, $this->requestBuilder, $this->hydrator, $signingKey ?: $this->apiKey);
would allow $signingKey to be optional and keep the old behaviour
which would then not be a behaviour BC, but does still leave a question of if the signature change is classed as a BC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants