Repository navigation
Conversation
❌ PR checklist incompleteThis PR cannot be merged until the following are addressed on its linked issue:
The fields live on the linked issue in the Shipping project (open the issue → right sidebar → Projects). After you set them, re-run this check (or push a commit) — issue/project changes do not re-trigger it automatically. Maintainers can bypass this check by adding the |
Code Review ✅ Approved🟡 Medium risk · Comparing undecryptable webhook secrets as stored could preserve unusable credentials and leave alerts unable to deliver until secrets are re-entered. Fixes alert edits being blocked while a webhook secret is waiting to be re-entered: saving no longer decrypts stored destinations for comparison and receiver counting, and decryption now happens only at send time, so edits and switch-offs succeed while unopened secrets are still never sent. No issues found. VerificationRules No rules evaluated Functional validation Not enabledAuto-approval Not enabledFor more verification details, comment OptionsDisplay: compact → Showing compact verification results. Comment with these commands to change the behavior for this request:
Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source |
Describe your changes:
Problem. Since PR #36939, a webhook secret opens only for the destination it was written for. A value the 2.1.0 step does not carry over is reported as "must be entered again". Entering it again failed with that same message, and so did every other edit of that alert, including switching it off. The only way out was to delete the alert and create it again.
Why. Saving an alert reads its stored destinations twice before using the new values: to count receivers against the limit, and to compare them and find what changed. Both decrypted the stored secrets, though neither needs them, so a value that does not open stopped the save.
What this PR changes.
DestinationValidationnow usesDestinationSecrets.decryptedOrAsStored, which keeps a value that does not open as it is stored.WebhookConfigRules.receiversOfreads the configuration as stored, as the email rules already do, so the count stays exact.ChannelDispatchdecrypts the destination itself before asking for its receivers, so what is sent does not change.What stays refused. A value that does not open is still never sent, on a test send or a real send. Moving a target while keeping a stored secret, and pasting ciphertext, are still refused. A destination changed while it still holds such a value gets a 400 naming the field, so OAuth2 credentials with two such values need both entered.
Linking: no issue was opened for this one, it follows up PR #36939 directly.
Type of change:
High-level design:
N/A, small change.
Tests:
Use cases covered
Unit tests
DestinationValidationTest,AlertLimitsTestandDestinationSecretsTest.Backend integration tests
WebhookSecretOwnershipIT#aValueLeftToEnterAgainStopsNoEditAndDeliversOnceEntered.WebhookSecretOwnershipIT,WebhookSecretsMigrationIT,AlertConfigLimitsIT,BoundedTestSendsITandProfileWebhookSecretsIT: 21 pass on MySQL and Elasticsearch. Postgres and the wider plan are pending in CI.Real upgrade check
The same change on top of a main build, upgraded from a real 2.0.7 database on MySQL: edits, re-entry, OAuth2, PUT without ids, receiver limits and real deliveries all behaved as above, with deliveries checked against the signature of the known secret.
Ingestion integration tests
Playwright (UI) tests