fix(notify): address 5 post-merge review findings#2517
Conversation
High - Slack webhook stored plaintext: Encrypt URL with AES-256-GCM (Web Crypto) in edge endpoint before storing as webhookEnvelope. Matches relay's decrypt format. NOTIFICATION_ENCRYPTION_KEY must be set on Vercel. High - Variant scoping lost in publish/relay path: api/notify accepts + forwards variant in published event. breaking-news-alerts includes SITE_VARIANT in POST body. Relay filters rules by r.variant === event.variant. Medium - New channel not added to existing alert rule: Connect handlers (Telegram/email/Slack) call saveRuleWithNewChannel() so the newly linked channel is immediately in the rule's channels. Medium - Cloud sync misses removeItem (deletes not propagated): Patch Storage.prototype.removeItem alongside setItem so watchlist resets and layout resets trigger a cloud upload. Medium - Dead channel auto-deactivation was log-only: Add deactivateChannelForUser internalMutation + /relay/deactivate HTTP action (RELAY_SHARED_SECRET protected). Relay calls it on Telegram 403/400 and Slack 404/410.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Greptile SummaryThis PR resolves five post-merge findings from the notification delivery feature. It closes real gaps: Slack webhooks are now AES-256-GCM encrypted before storage, variant scoping is propagated end-to-end, newly linked channels are immediately added to the active alert rule, Key changes:
Confidence Score: 5/5Safe to merge; both remaining findings are P2 quality/efficiency issues that don't affect data correctness or security. All five original findings are addressed. The two new observations (missing key-length check and repeated Telegram saves) are non-critical: the key-length issue is a deployment-configuration concern that fails loudly at the relay, and the repeated saves are idempotent with no data-corruption risk. No P0 or P1 issues were found.
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client as Browser (breaking-news-alerts)
participant Notify as /api/notify (Edge)
participant Upstash as Upstash Pub/Sub
participant Relay as notification-relay.cjs
participant Convex as Convex DB
Client->>Notify: POST {eventType, payload, severity, variant}
Notify->>Upstash: PUBLISH wm:events:notify {…, variant}
Upstash-->>Relay: message event
Relay->>Convex: getByEnabled alertRules
Convex-->>Relay: rules[]
Note over Relay: filter by eventType, severity,<br/>and variant (if both set)
Relay->>Convex: GET /relay/channels {userId}
Convex-->>Relay: channels[]
alt Telegram 403/400
Relay->>Convex: POST /relay/deactivate {userId, telegram}
Convex->>Convex: deactivateChannelForUser → verified=false
else Slack 404/410
Relay->>Convex: POST /relay/deactivate {userId, slack}
Convex->>Convex: deactivateChannelForUser → verified=false
end
Relay->>Relay: sendTelegram / sendSlack / sendEmail
Reviews (1): Last reviewed commit: "fix(notify): address 5 review findings (..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| getChannelsData().then((data) => { | ||
| const tg = data.channels.find(c => c.channelType === 'telegram'); | ||
| if (tg?.verified || expired) { | ||
| if (tg?.verified) saveRuleWithNewChannel('telegram'); | ||
| reloadNotifSection(); | ||
| } |
There was a problem hiding this comment.
saveRuleWithNewChannel fires on every subsequent poll tick after pairing succeeds
clearNotifPoll() is only called when expired is true. Once tg?.verified becomes true the condition stays true for every remaining 3-second tick, so saveRuleWithNewChannel('telegram') (and reloadNotifSection()) are invoked repeatedly until the countdown hits zero — potentially 30+ redundant saveAlertRules API calls for a freshly-linked Telegram channel.
Clearing the poll on the success path prevents the repeated calls:
| getChannelsData().then((data) => { | |
| const tg = data.channels.find(c => c.channelType === 'telegram'); | |
| if (tg?.verified || expired) { | |
| if (tg?.verified) saveRuleWithNewChannel('telegram'); | |
| reloadNotifSection(); | |
| } | |
| if (tg?.verified || expired) { | |
| if (tg?.verified) { clearNotifPoll(); saveRuleWithNewChannel('telegram'); } | |
| reloadNotifSection(); | |
| } |
| async function encryptSlackWebhook(webhookUrl: string): Promise<string> { | ||
| const rawKey = process.env.NOTIFICATION_ENCRYPTION_KEY; | ||
| if (!rawKey) throw new Error('NOTIFICATION_ENCRYPTION_KEY not set'); | ||
| const keyBytes = Uint8Array.from(atob(rawKey), (c) => c.charCodeAt(0)); | ||
| const key = await crypto.subtle.importKey('raw', keyBytes, 'AES-GCM', false, ['encrypt']); | ||
| const iv = crypto.getRandomValues(new Uint8Array(12)); | ||
| const encoded = new TextEncoder().encode(webhookUrl); | ||
| const result = new Uint8Array(await crypto.subtle.encrypt({ name: 'AES-GCM', iv, tagLength: 128 }, key, encoded)); | ||
| // Web Crypto returns ciphertext || tag (tag is last 16 bytes) | ||
| const ciphertext = result.slice(0, -16); | ||
| const tag = result.slice(-16); | ||
| const payload = new Uint8Array(12 + 16 + ciphertext.length); | ||
| payload.set(iv, 0); | ||
| payload.set(tag, 12); | ||
| payload.set(ciphertext, 28); | ||
| const binary = Array.from(payload, (b) => String.fromCharCode(b)).join(''); | ||
| return `v1:${btoa(binary)}`; | ||
| } |
There was a problem hiding this comment.
encryptSlackWebhook decodes the base64 key and imports it without checking it is exactly 32 bytes. The relay's crypto.cjs has an explicit guard, so if an operator accidentally sets a 16-byte or 24-byte key on Vercel, the edge function will happily encrypt with AES-128/192-GCM while the relay will throw on every decrypt attempt. The result is silent Slack delivery failure for all users, with no error surfaced on the write path.
Adding the same check here surfaces the misconfiguration at link-time (a 503 back to the client) rather than silently at alert delivery time:
const keyBytes = Uint8Array.from(atob(rawKey), (c) => c.charCodeAt(0));
if (keyBytes.length !== 32) throw new Error('NOTIFICATION_ENCRYPTION_KEY must be 32 bytes');
const key = await crypto.subtle.importKey('raw', keyBytes, 'AES-GCM', false, ['encrypt']);
1 participant
Summary
Fixes 5 findings from a review of the notification delivery feature (Phases 2-4).
High
Slack webhook stored plaintext: The edge endpoint now encrypts the raw URL with AES-256-GCM (Web Crypto API, matching the relay's
crypto.cjsdecrypt format) before storing aswebhookEnvelopein Convex.NOTIFICATION_ENCRYPTION_KEYmust be set on Vercel.Variant scoping lost:
/api/notifynow accepts and forwardsvariantin the published Upstash event.breaking-news-alertsincludesSITE_VARIANTin the POST body. The relay filters alert rules byr.variant === event.variant.Medium
New channel not added to existing rule: Connect handlers (Telegram/email/Slack) now call
saveRuleWithNewChannel()immediately after linking, so the newly connected channel is included in the active rule'schannelsarray without requiring a manual toggle.Cloud sync misses
removeItem(deletes not propagated):cloud-prefs-sync.tsnow patchesStorage.prototype.removeItemalongsidesetItem, so watchlist resets and layout resets trigger a cloud upload.Dead channel auto-deactivation was log-only: Added
deactivateChannelForUserinternalMutationin Convex + a/relay/deactivateHTTP action (protected byRELAY_SHARED_SECRET). Relay calls it when Telegram returns 403/400 or Slack returns 404/410 instead of just logging.Post-Deploy Monitoring & Validation
webhookEnvelopestarts withv1:(not a raw URL)tech.worldmonitor.app, confirm relay only delivers to rules withvariant=techverified: falsein Convex