Signature differs from the payload's hash if the payload's serialization is not unique

[flavianh]

We've come across an interesting example. Take the following code sample:

<?php

class SubPayload implements JsonSerializable {
  public function jsonSerialize(): mixed
  {
    return [
      'id' => substr(str_shuffle(MD5(microtime())), 0, 10)
    ];
  }
}

$payload = [
  'subpayload' => new SubPayload(),
];

// string(34) "{"subpayload":{"id":"bc1b8bc704"}}"
var_dump(json_encode($payload));
// string(34) "{"subpayload":{"id":"70267cdf6b"}}"
var_dump(json_encode($payload));

You will see that the value after json_encode changes. We have an actual production piece of code that behaves similarly to this special payload.

The problem with laravel-webhook-server is the following:

$webhook = WebhookCall::create()
            ->url('/some/url')
            ->payload($payload)

// This function uses a side-effect to precompute the signature
$webhook->dispatch();

// This computed signature was done using a first `json_encode` which generated a first version of the serialized payload
$computedSignature = $webhook->callWebhookJob->headers[$webhook->signer->signatureHeaderName()];

// This function will serialize the payload again, generating another version that _does not match the signature_
$webhook->callWebhookJob->handle();

As an immediate correction, we serialized and unserialized our payload:

$webhook = WebhookCall::create()
            ->url('/some/url')
            ->payload(json_decode(json_encode($payload), true))

I see two possibilities moving forward.

1. Serializing once

To me, the crux of the issue is that the headers should be generated at the same moment as the body. I suggest to

• Move the function getAllHeaders to CallWebhookJob (https://github.com/spatie/laravel-webhook-server/blob/main/src/WebhookCall.php#L249-L262)
• Use it in the handle function and serialize the payload only once (https://github.com/spatie/laravel-webhook-server/blob/main/src/CallWebhookJob.php#L70-L72)

We would lose the headers in the job trail, but to me, this is not a huge loss.

2. Accepting only string payloads

Taking a step back, it doesn't seem like this library's job to serialize the payload. You could require the payload to be a string and generate the signature in the same way. Strings are still mutable so it would be worth implementing solution 1 too.

The drawback is that by doing this you would be breaking your current interface so it would require extra work to make sure that a backward-compatible mechanism remains.

We're ready at my company (Ocus) to help make those changes and submit a PR. Could you let us know which solution you prefer or if you'd like another one?

[flavianh]

@freekmurze Thanks for looking into this! I'm a bit confused about the current status of this item, as to me this is clearly a bug and not an "idea" (as per the category above).
In any case, I may be nitpicking. What's your opinion on the problem & solutions I exposed?

[freekmurze]

General, I don’t think that many people use time sensitive payloads, so I see this more as a feature request.

Can solution 1 be implemented without breaking changes?

[innoticFR]

I am not sure that time sensitive payload are so rare (eg. updated_at attribute between 2 attempts), but in any case, the signature should match the body and I would expect the signature calculation to be done once the body is "frozen".

The difficult part for the library is to preserve the compliance with 3rd parties that relies on the payload being an array (that may contain dynamic objects).

So moving signature in CallWebhookJob::handle() after the serialization makes sense.

In WebhookCall::prepareForDispatch() transmit signWebhook and signer variable to callWebhookJob

In callWebhookJob::handle:
['body' => $payload = json_encode($this->payload)]

and use $payload to calculate signature and merge that to $this->headers.