From 65d869e0ae4f6985d8dae602d997ec01aee2f076 Mon Sep 17 00:00:00 2001 From: Morris Jencen Chavez Date: Sun, 20 Sep 2026 19:15:24 +0000 Subject: [PATCH] fix: distinguish endpoint secret decryption failures from ordinary delivery failures A naive APP_KEY rotation (replacing the key instead of moving the old value into APP_PREVIOUS_KEYS) permanently breaks decryption of every Endpoint.secret_key. Previously this DecryptException was caught by SendWebhook's generic catch(Throwable) block and recorded as an ordinary delivery failure, indistinguishable from a downed customer endpoint, so the real cause (an APP_KEY misconfiguration) went undetected while retries burned through backoff for a condition they could never fix. SendWebhook now catches DecryptException specifically, logs it at critical level, and records a delivery response_body that names the actual cause. DEPLOYMENT.md documents the correct APP_KEY rotation procedure using APP_PREVIOUS_KEYS. --- DEPLOYMENT.md | 16 ++++ app/Jobs/SendWebhook.php | 22 +++++ .../SendWebhookSecretDecryptFailureTest.php | 84 +++++++++++++++++++ 3 files changed, 122 insertions(+) create mode 100644 tests/Feature/SendWebhookSecretDecryptFailureTest.php diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index 973d893..9324970 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -343,6 +343,22 @@ server { - Regular backups - Use connection pooling +### APP_KEY Rotation + +`Endpoint.secret_key` (the HMAC signing secret used for every webhook delivery) is encrypted at rest using `APP_KEY`. **Never rotate `APP_KEY` by simply generating a new value and replacing the old one** — every `Endpoint::secret_key` becomes permanently undecryptable the moment the old key is gone, since it's the only copy of the plaintext secret anywhere. The failure is silent: deliveries start throwing decryption errors that look identical to ordinary endpoint downtime, and burn through retries for a condition retries can never fix. + +To rotate `APP_KEY` safely: + +1. Move the **current** `APP_KEY` value into `APP_PREVIOUS_KEYS` (comma-separated if it already holds prior keys), so Laravel can still decrypt values encrypted under it. +2. Generate and set the **new** `APP_KEY` (`php artisan key:generate --ansi`). +3. Re-save every `Endpoint` row so `secret_key` is re-encrypted under the new key (touching the `encrypted` cast re-encrypts on save), e.g.: + ```bash + php artisan tinker --execute="App\Models\Endpoint::withTrashed()->each(fn (\$e) => \$e->save());" + ``` +4. Once every row has been re-saved, remove the old key from `APP_PREVIOUS_KEYS`. + +Until step 3 completes, keep the old key in `APP_PREVIOUS_KEYS` — removing it early re-creates the same permanent-decryption-failure problem this runbook exists to avoid. + ## 📊 Monitoring and Health Checks ### Health Check Endpoint diff --git a/app/Jobs/SendWebhook.php b/app/Jobs/SendWebhook.php index 3c172b3..4853d84 100644 --- a/app/Jobs/SendWebhook.php +++ b/app/Jobs/SendWebhook.php @@ -4,6 +4,7 @@ use App\Models\Delivery; use App\Rules\SafeWebhookUrl; +use Illuminate\Contracts\Encryption\DecryptException; use Illuminate\Contracts\Queue\ShouldQueue; use Illuminate\Foundation\Queue\Queueable; use Illuminate\Queue\InteractsWithQueue; @@ -195,6 +196,27 @@ public function handle(): void $this->handleFailedDelivery(); } } + } catch (DecryptException $e) { + // Thrown when $endpoint->secret_key can't be decrypted under the + // current APP_KEY/APP_PREVIOUS_KEYS — almost always the result of + // an APP_KEY rotation performed without preserving the old key in + // APP_PREVIOUS_KEYS. Flagged distinctly (critical, not error) and + // with an unambiguous message so this looks nothing like ordinary + // endpoint downtime; see DEPLOYMENT.md's APP_KEY rotation runbook. + Log::critical('Webhook delivery failed: unable to decrypt endpoint secret_key. This usually means APP_KEY was rotated without preserving the previous key in APP_PREVIOUS_KEYS.', [ + 'delivery_id' => $this->delivery->id, + 'endpoint_id' => $endpoint->id, + ]); + + $this->delivery->update([ + 'status' => 'failed', + 'response_code' => null, + 'response_body' => 'Delivery failed: unable to decrypt endpoint secret key. This usually indicates APP_KEY was rotated without preserving the previous key in APP_PREVIOUS_KEYS.', + 'delivered_at' => null, + 'next_retry_at' => null, + ]); + + $this->handleFailedDelivery(); } catch (Throwable $e) { Log::error('Webhook delivery exception', [ 'delivery_id' => $this->delivery->id, diff --git a/tests/Feature/SendWebhookSecretDecryptFailureTest.php b/tests/Feature/SendWebhookSecretDecryptFailureTest.php new file mode 100644 index 0000000..92e2a96 --- /dev/null +++ b/tests/Feature/SendWebhookSecretDecryptFailureTest.php @@ -0,0 +1,84 @@ +for($user)->create(); + $endpoint = Endpoint::factory()->for($user)->create(['url' => 'http://8.8.8.8/webhook', 'is_active' => true]); + + return Delivery::factory()->create([ + 'event_id' => $event->id, + 'endpoint_id' => $endpoint->id, + 'status' => 'pending', + 'attempt_count' => 0, + 'next_retry_at' => null, + ]); + } + + /** + * Rotates APP_KEY at runtime and rebuilds the 'encrypter' singleton so + * the new config actually takes effect, mirroring what happens when an + * operator changes APP_KEY and restarts the app. + */ + private function rotateAppKey(string $newKey, array $previousKeys = []): void + { + config(['app.key' => $newKey, 'app.previous_keys' => $previousKeys]); + $this->app->forgetInstance('encrypter'); + Crypt::clearResolvedInstance('encrypter'); + } + + public function test_decrypt_exception_reading_endpoint_secret_is_caught_and_flagged_distinctly(): void + { + $delivery = $this->makeDelivery(User::factory()->withPersonalTeam()->create()); + + // Simulate a naive APP_KEY rotation: the secret was encrypted under + // the old key, but the new key is set with no APP_PREVIOUS_KEYS, so + // it can no longer be decrypted. + $this->rotateAppKey('base64:'.base64_encode(Str::random(32))); + + $job = (new SendWebhook($delivery->fresh()))->withFakeQueueInteractions(); + $job->handle(); + + $job->assertNotReleased(); + + $delivery->refresh(); + $this->assertSame('failed', $delivery->status); + $this->assertNull($delivery->response_code); + $this->assertStringContainsString('decrypt', $delivery->response_body); + $this->assertStringContainsString('APP_PREVIOUS_KEYS', $delivery->response_body); + + // Unlike a permanent 4xx rejection, this isn't inherently + // unrecoverable (fixing APP_PREVIOUS_KEYS could make the next + // attempt succeed), so it should still be scheduled for retry. + $this->assertNotNull($delivery->next_retry_at); + } + + public function test_endpoint_secret_still_decrypts_after_app_key_rotation_via_previous_keys(): void + { + $originalKey = config('app.key'); + + $endpoint = Endpoint::factory()->for(User::factory()->withPersonalTeam()->create())->create(); + $plainSecret = $endpoint->secret_key; + + // Rotate to a new key the proper way: the old key is preserved in + // APP_PREVIOUS_KEYS, so values encrypted under it remain readable. + $this->rotateAppKey('base64:'.base64_encode(Str::random(32)), [$originalKey]); + + $this->assertSame($plainSecret, $endpoint->fresh()->secret_key); + } +}