feat(security): encrypt NotificationChannel.config secrets at rest#198
feat(security): encrypt NotificationChannel.config secrets at rest#198TerrifiedBug merged 9 commits intomainfrom
Conversation
github-actions
Bot
added
feature
dependencies
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95626947de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| webhook: ["hmacSecret", "headers"], | ||
| slack: ["webhookUrl"], |
There was a problem hiding this comment.
Adding webhook.headers and slack.webhookUrl to the encrypted field map causes those values to be stored as v2: ciphertext, but listChannels still returns channel config values as-is for those keys. The UI edit flow reads them directly (formFromConfig) and then re-validates as URL/JSON-object on submit, so edited Slack channels and webhook channels with headers can fail updates unless users manually re-enter those values. This introduces a regression in the channel-edit workflow after encryption is enabled.
Useful? React with 👍 / 👎.
| const V2_PREFIX = "v2:"; | ||
|
|
||
| function isAlreadyEncrypted(value: unknown): boolean { | ||
| return typeof value === "string" && value.startsWith(V2_PREFIX); |
There was a problem hiding this comment.
The isAlreadyEncrypted heuristic treats any string beginning with v2: as ciphertext, so legitimate plaintext secrets like v2:token are skipped during encryption and stored unencrypted. At delivery/test time, decryptChannelConfig attempts to decrypt those values and throws, which breaks channel test/delivery for affected configs. This should use a verifiable ciphertext format check (or explicit metadata) rather than a bare prefix test.
Useful? React with 👍 / 👎.
Three regressions surfaced in Codex review of #198: 1. Retry delivery paths skipped decryption — both src/server/routers/ alert-deliveries.ts (manual retry) and src/server/services/retry-service.ts (background retry loop) now wrap channel.config in decryptChannelConfig before driver.deliver, matching deliverToChannels and testChannel. 2. listChannels returned encrypted webhookUrl/headers, breaking the edit dialog on slack/webhook channels. Decrypt the channel config before applying the existing redaction pass; truly-secret fields (hmacSecret, smtpPass, integrationKey) stay masked, while non-redacted fields needed by the form (webhookUrl, headers) reach the UI as plaintext. 3. Replaced the "v2:" idempotency-marker (which collided with crypto.ts's own ciphertext prefix and any user secret beginning "v2:") with a channel-secrets-specific wrapper "vfenc1:". On-disk format becomes vfenc1:v2:<base64>; isAlreadyEncrypted now keys off vfenc1: only.
The audit middleware sanitizer (src/server/middleware/audit.ts) only redacted exact-key matches for password/token/secret/etc. Channel-config secret fields slipped through unredacted, causing AuditLog.metadata to store plaintext webhook signing secrets, SMTP passwords, PagerDuty routing keys, and webhook URLs (which often embed auth tokens) on every notificationChannel.created / .updated / .deleted action. This undermined the encrypt-at-rest fix landed in PR #198, which closed plaintext storage in the channel.config column itself but left the audit-log side channel open. Changes: - Add hmacSecret, smtpPass, integrationKey, webhookUrl to SENSITIVE_KEYS. - Extract SENSITIVE_KEYS / sanitizeInput / computeDiff into a separate audit-sanitize module so they can be unit-tested without pulling in the full Prisma + NextAuth runtime via the middleware barrel. - Add unit tests covering top-level redaction, nested config redaction, arrays of channels, primitive passthrough, null/undefined, and presence of the new keys in the set.
…log (#200) * fix(audit): redact NotificationChannel secret config fields in audit log The audit middleware sanitizer (src/server/middleware/audit.ts) only redacted exact-key matches for password/token/secret/etc. Channel-config secret fields slipped through unredacted, causing AuditLog.metadata to store plaintext webhook signing secrets, SMTP passwords, PagerDuty routing keys, and webhook URLs (which often embed auth tokens) on every notificationChannel.created / .updated / .deleted action. This undermined the encrypt-at-rest fix landed in PR #198, which closed plaintext storage in the channel.config column itself but left the audit-log side channel open. Changes: - Add hmacSecret, smtpPass, integrationKey, webhookUrl to SENSITIVE_KEYS. - Extract SENSITIVE_KEYS / sanitizeInput / computeDiff into a separate audit-sanitize module so they can be unit-tested without pulling in the full Prisma + NextAuth runtime via the middleware barrel. - Add unit tests covering top-level redaction, nested config redaction, arrays of channels, primitive passthrough, null/undefined, and presence of the new keys in the set. * fix(settings): add Outbound Webhooks entry to settings sidebar nav PR #199 wired the page route and the settings-overview card but left settings-sidebar-nav.tsx untouched, so the page was unreachable from the sidebar. Add it to the Organization group alongside Service Accounts and AI (other team-level integrations) with the Webhook icon. Non-super-admin so team editors can manage event subscriptions, demo- hidden to match other integration entries.
1 participant
Summary
channel-secrets.tsservice with per-type sensitive-field map (webhook:hmacSecret+headers; slack:webhookUrl; pagerduty:integrationKey; email:smtpPass)createChannelandupdateChannel(with rotation + PRESERVE_IF_ABSENT preservation)deliverToChannelsdispatcher (both tracked + untracked paths) andtestChannelv2:prefix detection — re-running encryption is a no-oppnpm backfill:channel-secretsCloses the secret-encryption gap that persisted after the AlertWebhook → NotificationChannel migration (#197). Removes plaintext storage of webhook signing secrets, custom auth headers, PagerDuty routing keys, Slack webhook URLs (token in path), and SMTP passwords.
Out of scope
crypto.tsre-derives the key perencrypt/decryptcall. Hot-path concern for high-volume environments; out of scope for this change.Test plan
createChannelpersists ciphertext; non-sensitive fields stay plaintextupdateChannelrotates secret to fresh ciphertext; preserves existing v2: ciphertext when input lacks the fielddeliverToChannelsdecrypts beforedriver.deliver(both tracked and untracked paths)testChanneldecrypts beforedriver.testpnpm backfill:channel-secretsagainst dev DB; re-run to confirm idempotent skipMigration note
After merge, run the backfill script once against any environment with existing NotificationChannel rows to encrypt at-rest values. Idempotent — safe to re-run.