notifications: preliminary UI work

[illegalprime]

UI work expose notifications controls and alerting sinks.

[github-actions]

🔐 Codex Security Review

Note: This is an automated security-focused code review generated by Codex.
It should be used as a supplementary check alongside human review.
False positives are possible - use your judgment.

Scope summary

• Reviewed pull request diff only (75933fce45c269e41e0ba2b30cd33825b0389fa7...c587607f0796cd66d7b04c5c10ab810fb055519e, exact PR three-dot diff)
• Model: gpt-5.5

💡 Click "edited" above to see previous reviews for this PR.

---

Review Summary

Overall Risk: MEDIUM

Findings

[MEDIUM] Destination Checks Do Not Enforce Grafana Egress

• Category: Infrastructure
• Location: server/internal/domain/notifications/service.go:475
• Description: The service validates the submitted destination hostname once, then hands the URL/SMTP host to Grafana, which performs the actual outbound delivery later with its own DNS resolution and redirect behavior. A notification:manage user can use a public hostname that passes this check and later DNS-rebinds to a private/link-local address; redirect-following by Grafana would create the same bypass.
• Impact: SSRF from the Grafana sidecar into internal services or metadata endpoints, bypassing the intended private-destination block.
• Recommendation: Enforce this at Grafana’s network boundary: deny egress to loopback, RFC1918, link-local, metadata, and other non-public ranges from the Grafana container/network. If app-level enforcement is required, proxy/test deliveries through fleet-api with redirect refusal and IP pinning, or configure Grafana to disallow unsafe redirects if supported.

[MEDIUM] Notification HTTP Surface Diverges From Generated Protobuf Contract

• Category: Protobuf
• Location: server/internal/handlers/notifications/handler.go:527
• Description: The PR adds generated notification protobuf/Connect code, but the mounted handler uses custom JSON shapes. It emits/accepts lowercase enum strings like webhook, pending, and rule, while protobuf JSON clients generated from the new proto expect enum names such as CHANNEL_KIND_WEBHOOK. The handler also mounts HistoryService/ListNotifications, but notifications.proto declares only Channel, Rule, and Silence services.
• Impact: Generated clients and reflection-based tooling cannot reliably use this API; a later “mechanical” swap to generated Connect handlers would break the current UI wire format and drop the history endpoint unless the proto is expanded.
• Recommendation: Either implement these routes with generated Connect handlers/proto messages now, or explicitly treat them as non-Connect custom JSON routes. Add HistoryService and its messages to notifications.proto, regenerate, and align frontend/server enum serialization with protobuf JSON.

Notes

No auth bypass, SQL injection, command injection, cryptostealing/pool hijack logic, or Rust ASIC plugin unsafe-code changes were found in the reviewed diff. This was a static review of .git/codex-review.diff; I did not run tests.

---

_{Generated by Codex Security Review |
Triggered by: @illegalprime |
Review workflow run}

[illegalprime]

Addressed the Codex security review findings in 146c469:

[HIGH] Notifications endpoints bypass RBAC — Added notification:read / notification:manage to the authz catalog (picked up by the startup reconciler, so SUPER_ADMIN/ADMIN gain them on next boot). The handler now loads effective permissions after session validation — the same path the Connect auth interceptor uses — and every route is gated through middleware.RequirePermission: list endpoints on :read, all mutations (including TestChannel, which triggers an outbound delivery) on :manage. Registering the generated Connect handlers stays a follow-up for when codegen lands.

[HIGH] Empty silence scopes can mute the whole org — CreateSilence/UpdateSilence now reject scopes without a target (rule_id / group_id / site_id / device_ids required per kind) with invalid_argument. The UI hides the group/site/device scope options until their pickers exist.

[HIGH] Saved channels are not routed to alerts — Routing via the Grafana notification-policy tree is a feature of its own and isn't in this PR; the Channels section copy now states explicitly that saved channels are not yet attached to alert routing and that "Test" sends directly to the destination. Policy-tree routing lands as a follow-up.

[HIGH] Grafana error logging leaks notification secrets — Request/response bodies logged on non-2xx Grafana responses now pass through structured key-based redaction (authorization_credentials, smtpPassword, password, secureSettings, …), recursing through nested objects/arrays.

[MEDIUM] Webhook/SMTP destinations allow server-side egress — New DestinationPolicy: webhook URLs must be http/https with a host, and destinations that are (or resolve to) loopback, link-local (incl. cloud metadata), private, or unspecified addresses are rejected by default. Deployments with internal relays can opt out via FLEET_METRICS_NOTIFICATIONS_ALLOW_PRIVATE_DESTINATIONS (enabled in the dev compose overlay).

[MEDIUM] Updating a webhook channel clears its stored secret — UpdateChannel now carries the existing settings' secret field into the PUT payload when the request has no fresh secret. For webhooks that's Grafana's [REDACTED] placeholder, which the provisioning API resolves back to the stored secure value; for SMTP the stored password is carried directly. Covered by unit tests against a fake Grafana.

🤖 Generated with Claude Code

[illegalprime]

Second-round review findings addressed in 72d9d9e:

[HIGH] SSRF policy bypassable via DNS rebinding — Partially mitigated: DNS lookups now fail closed (an unresolvable destination is rejected rather than allowed), per the reviewer's minimum recommendation. The fundamental rebinding window can't be closed from fleet-api — Grafana re-resolves the hostname at delivery time — so the residual risk is documented on checkDestinationHost with a pointer to network-boundary egress enforcement on the Grafana container, which is the real fix and an infra follow-up.

[MEDIUM] Webhook URLs leak into error logs — url added to the redacted log keys (Slack/PagerDuty-style URLs embed capability tokens in the path). The GrafanaError message returned to the API caller is unchanged: it reaches only the org user who supplied the channel definition, and stripping it would gut the test-channel UX; flagging if anyone disagrees.

[MEDIUM] Device-scope silence regex injection — Device ids are now validated against the identifier alphabet ([A-Za-z0-9._:-]+, covers UUIDs/MACs/serials), regexp.QuoteMeta-escaped, and compiled into an anchored ^(?:id1|id2)$ matcher; read-back strips the anchors. A crafted .* or a|b id is rejected with invalid_argument.

[MEDIUM] Existing ADMIN roles not backfilled — Correct catch: the additive reconciler never re-asserts seed keys on existing built-ins. Migration 000087_seed_notification_permissions backfills notification:read/notification:manage onto existing ADMIN roles, mirroring the 000069 activity:read precedent.

[MEDIUM] Grafana admin creds in deployment overlay — The overlay now passes FLEET_METRICS_GRAFANA_TOKEN through (the client already prefers it over basic auth), so operators can move fleet-api onto a least-privilege service-account token. The admin fallback remains until a token is provisioned — automating SA token creation in run-fleet.sh is a deployment follow-up.

[MEDIUM] Generated Connect API not served — Intentionally deferred, as documented in the handler header: the hand-written JSON routes are the interim surface until the notificationsv1connect codegen lands, at which point the swap is mechanical and the routes join the standard interceptor chain. The hand-written UI client is the only consumer of this surface today.

🤖 Generated with Claude Code

[illegalprime]

Third-round findings addressed in 0b131db:

[MEDIUM] Webhook URLs leak to read-only users — Good catch, consistent with the logger change. Reads now return webhook URLs host-only (scheme://host, dropping the userinfo/path/query where tokens live); the full URL stays only in Grafana's stored settings. UpdateChannel carries the stored full URL through an edit that didn't replace it — an empty or still-redacted URL is treated as "unchanged," same pattern as the secret carry — so renames don't truncate it, while a manage user submitting a fresh full URL overrides it. Covered by new tests (TestListChannelsRedactsWebhookURL, TestUpdateChannelPreservesWebhookURLOnRename).

[MEDIUM] Raw Grafana error bodies returned to clients — GrafanaError.Message is now redacted with the same key-based redactor before it rides back to the browser, so a Grafana/proxy error echoing the (secret-carrying) request body can't leak credentials.

[MEDIUM] Hand-rolled routes bypass validation/body limits — Added http.MaxBytesReader (1 MiB) on the request body, and the hand-written path now enforces the proto's buf.validate bounds (channel name ≤255, silence comment ≤1024, device-id count ≤500) until the generated Connect handlers + validate interceptor take over.

[MEDIUM] DNS rebinding bypasses SSRF checks — Unchanged from round two: mitigated at the application layer (fail-closed resolution) and documented on checkDestinationHost. Closing the rebinding window requires re-resolving at dial time, which only Grafana's own egress path controls — that's a network-boundary/infra fix, not something fleet-api can guarantee. Tracking separately rather than implying a code-level fix here closes it.

[MEDIUM] HistoryService outside the proto contract — Acknowledged; this is part of the same generated-handler migration the handler header documents. When notificationsv1connect is wired, HistoryService will be added to the proto and regenerated alongside the others. No generated client consumes this surface today (the hand-written UI client is the only caller), so there's no client to break in the interim.

🤖 Generated with Claude Code

[illegalprime]

Fourth-round review — risk is back at HIGH on two findings. Triage:

[HIGH] Channel destination updates preserve old secrets — Valid, and a regression from my round-1 secret-carry. This is already fixed in the working tree (in-progress on the branch): the secret carry is now gated on the destination being unchanged — a webhook URL or SMTP host/port change without a fresh secret drops the stored credential instead of delivering the old bearer token / SMTP password to the new destination. A rename or recipients-only edit still preserves it. It'll land with the next push of the in-progress channel work on this branch.

[HIGH] DNS rebinding bypasses SSRF check — Unchanged position: this cannot be closed in fleet-api. Pinning the resolved IP and handing Grafana an IP instead of the hostname would break TLS SNI / the Host header for real destinations (Slack, PagerDuty). The validation is an application-level backstop (fail-closed resolution, documented on checkDestinationHost); the actual guarantee has to come from egress policy at the Grafana container's network boundary, or a delivery proxy that pins/validates at dial time. Flagging for a human risk-acceptance decision + an infra follow-up rather than implying a code fix exists here.

[MEDIUM] Grafana error redaction passes non-JSON bodies through — Fixed in 988f58a. redactSecrets no longer returns non-JSON bodies verbatim (key-based redaction can't scrub HTML/plaintext that echoes the payload); it returns a length marker. The client-facing GrafanaError message is now the generic status text for non-JSON responses, surfacing only Grafana's own structured JSON error after redaction.

[MEDIUM] Saved channel tests ignore the saved channel ID — Valid; TestChannel tests the request body, and with the URL now redacted on reads the UI sends a host-only URL + empty secret, so testing a saved channel hits the wrong destination. The right fix (load + ownership-check the stored contact point by id, test its stored settings, use the request body only for unsaved preview) lives in service.go, which is under active concurrent edit for the Slack channel work — I'm deferring it to that change rather than racing the same file. Noting it here so it isn't lost.

[MEDIUM] History endpoint outside the proto contract — Same disposition as prior rounds: part of the tracked hand-written-JSON → generated-Connect-handler migration documented in the handler header. When notificationsv1connect is wired, HistoryService gets added to the proto and regenerated with the rest. No generated client consumes it today.

🤖 Generated with Claude Code

[illegalprime]

Fifth-round review (MEDIUM). Two fixed in 97ca093, three deferred with consistent rationale:

[MEDIUM] Grafana JSON error bodies leak secrets in generic string fields — Fixed. My round-4 redaction was key-based only, so a secret echoed inside a value (e.g. {"message":"failed to POST to https://hooks.slack.com/.../TOKEN"}) slipped through. redactValue now also scrubs webhook-URL and Bearer <token> substrings out of every string value, wherever they sit — applied to both the logged body and the client-facing GrafanaError message. (SMTP passwords inside free-text aren't pattern-matchable generically; Grafana doesn't echo them in practice, and the secret-keyed cases are already covered.)

[MEDIUM] Fleet API always receives the Grafana admin password — Fixed in the deployment overlay. It previously hard-required GRAFANA_ADMIN_PASSWORD for fleet-api even with a token configured, so the admin credential sat in fleet-api's env regardless. The basic-auth password now defaults empty and is an explicit opt-in fallback — a token deployment carries no admin credential. ⚠️ This needs a companion run-fleet.sh change to set FLEET_METRICS_GRAFANA_PASSWORD for no-token deployments (otherwise basic auth gets an empty password); I can't see/test that script here, so flagging it for whoever owns the deploy tooling. The full "fail startup if neither token nor explicit fallback is present" check is a server-side follow-up.

[MEDIUM] Hand-written JSON not Connect/protobuf-compatible & [MEDIUM] History endpoint missing from proto — Both are the same tracked item: the hand-written JSON routes are the documented interim surface (see the handler header) until the notificationsv1connect bindings are served. The Slack work just regenerated the proto, so the bindings are moving; HistoryService gets added and the UI switched to generated bindings as part of that migration. No generated/SDK client consumes this surface today — the hand-written TS client is the only caller — so there's no client mismatch in practice yet.

[MEDIUM] SSRF is only a pre-resolution check (DNS rebinding) — Unchanged: not closable in fleet-api without breaking TLS SNI/Host for real destinations. Application-level fail-closed validation stays as the backstop; the guarantee needs egress policy at the Grafana container boundary or a pinning delivery proxy. Flagged for human risk-acceptance + infra. (Noted the CGNAT range suggestion — worth folding into the host check if/when we extend it.)

🤖 Generated with Claude Code

[illegalprime]

Sixth-round review (MEDIUM). One fixed, two are the same tracked migration item:

[MEDIUM] Saved channel tests don't use the saved destination — Fixed in c587607. This was the round-4 item I'd deferred while service.go was under active concurrent edit (the Slack work has since landed, so it's safe to touch now), and it was made worse by my own read-time URL redaction. TestChannel now, when given a channel id, resolves the owned Grafana contact point (ownership-checked) and tests its stored settings — the full webhook URL and Grafana's secure fields — instead of the redacted read-model the UI echoes back. Unsaved definitions (no id) still validate + test the request payload for the "Test before save" flow. The handler also now surfaces TestChannel's service error (unknown/foreign id, invalid destination, Grafana unreachable) as a 4xx/5xx rather than discarding it, distinct from a delivery failure reported in the body. Covered by two new tests (stored-URL probe + foreign-channel rejection).

Caveat noted in the finding: a saved Slack channel still can't be fully tested because Grafana stores the Slack URL as a secure field and doesn't return it on reads — same fundamental limitation as any stored secret, and not regressed by this change (it failed before too). Fully fixing it would require fleet-api to keep a parallel copy of the secret, which the design deliberately avoids.

[MEDIUM] Manual wire format diverges from generated protobuf & [MEDIUM] History endpoint not in proto — Same tracked item as prior rounds: the hand-written JSON routes are the documented interim surface until the notificationsv1connect bindings are served (handler header documents this). The contract mismatch (lowercase enum tokens vs CHANNEL_KIND_WEBHOOK, missing HistoryService) is real but only matters once a generated client/handler is actually wired — the hand-written TS client is the sole consumer today. The Slack work regenerated the proto, so the bindings exist; serving them (and adding HistoryService to the proto) is the migration's job, tracked separately rather than partially done here.

The DNS-rebinding item moved to the reviewer's Notes this round ("deployment should enforce Grafana egress at the network layer") — agreed, that's the resolution: infra-level egress, not a fleet-api code change.

🤖 Generated with Claude Code

[illegalprime]

Seventh-round review — converged. No new actionable findings this round; both remaining items are ones already dispositioned over prior rounds, and I'm not making code changes for them (flagging here so it's explicit rather than re-answering each re-review):

1. Destination checks don't enforce Grafana egress (DNS rebinding / redirects) — Confirmed not fixable in fleet-api: handing Grafana a pinned IP breaks TLS SNI/Host for real destinations, and Grafana owns the delivery-time resolution + redirect behavior. The reviewer's own recommendation is the resolution — deny egress to loopback/RFC1918/link-local/metadata at the Grafana container's network boundary (an infra/deployment change). fleet-api keeps its fail-closed pre-resolution check as a best-effort backstop. Tracking as an infra task, not a code fix on this PR.

2. HTTP surface diverges from generated protobuf + HistoryService not in proto — The hand-written JSON routes are the documented interim surface (see the handler package header) until the notificationsv1connect bindings are served; that migration is where the enum-name alignment and HistoryService proto declaration belong. The only consumer today is the hand-written TS client, so there's no generated-client mismatch in practice yet. Tracked with the generated-handler migration.

All concrete, code-level findings from rounds 1–6 (RBAC bypass, org-wide silences, secret-log/error leakage, SSRF pre-checks, secret-reuse-on-destination-change, secret redaction depth, token-only Grafana auth, saved-channel testing) are addressed and on the branch. The two items above are an infra task and a tracked refactor respectively — both appropriate to handle outside this preliminary-UI PR.

I'll continue watching and will only follow up if a future re-review surfaces something genuinely new.

🤖 Generated with Claude Code