Skip to content

fix(alerts): let an alert be edited while a webhook secret waits to be entered again - #37269

Open
manerow wants to merge 1 commit into
mainfrom
fix/alert-secret-re-entry
Open

manerow wants to merge 1 commit into
mainfrom
fix/alert-secret-re-entry

Conversation

@manerow

@manerow manerow commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

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.

  • Comparing: DestinationValidation now uses DestinationSecrets.decryptedOrAsStored, which keeps a value that does not open as it is stored.
  • Counting: WebhookConfigRules.receiversOf reads the configuration as stored, as the email rules already do, so the count stays exact.
  • Sending: ChannelDispatch decrypts the destination itself before asking for its receivers, so what is sent does not change.
  • Decision record: an amendment states that only sending opens a stored secret.

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:

  • Bug fix

High-level design:

N/A, small change.

Tests:

Use cases covered

  • An alert holding a value left for re-entry can be edited and switched off, and still does not send.
  • Entering the value again is accepted, and the next send uses it.
  • Receiver limits count the same with or without a value that opens.

Unit tests

  • Added to DestinationValidationTest, AlertLimitsTest and DestinationSecretsTest.
  • Without the production change, 3 of the new tests fail with "must be entered again". All 573 alerting unit tests pass with it.

Backend integration tests

  • Added WebhookSecretOwnershipIT#aValueLeftToEnterAgainStopsNoEditAndDeliversOnceEntered.
  • Ran WebhookSecretOwnershipIT, WebhookSecretsMigrationIT, AlertConfigLimitsIT, BoundedTestSendsIT and ProfileWebhookSecretsIT: 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

  • Not applicable.

Playwright (UI) tests

  • Not applicable.

@manerow
manerow requested a review from a team as a code owner November 6, 2026 12:56
@manerow
manerow requested a review from pmbrull November 6, 2026 12:56
@github-actions

github-actions Bot commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

❌ PR checklist incomplete

This PR cannot be merged until the following are addressed on its linked issue:

  • No GitHub issue is linked. Link an issue in the Development section of the PR (or add Fixes #12345 to the description). For a same-org cross-repo issue, add Fixes open-metadata/<repo>#123 to the description.

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 skip-pr-checks label.

@github-actions github-actions Bot added backend safe to test Add this label to run secure Github workflows on PRs labels Nov 6, 2026
@gitar-bot

gitar-bot Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
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.

Verification

Rules No rules evaluated

Functional validation Not enabled

Set up

Auto-approval Not enabled

Set up

For more verification details, comment gitar display:verbose.

Options

Display: compact → Showing compact verification results.

Comment with these commands to change the behavior for this request:

Compact
gitar display:verbose         

Was this helpful? React with 👍 / 👎 | Powered by Gitar — free for open source

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backend safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant