Skip to content

notifications (1/5): proto definitions and generated code#452

Open
illegalprime wants to merge 1 commit into
mainfrom
eden/notifications-1-proto
Open

notifications (1/5): proto definitions and generated code#452
illegalprime wants to merge 1 commit into
mainfrom
eden/notifications-1-proto

Conversation

@illegalprime

@illegalprime illegalprime commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

Stack 1/5

Adds the notifications service definitions and generated clients.

  • `notifications.proto`: Channel, Rule, Silence, History services
  • Generated Go (Connect handlers + messages) and TypeScript clients

Stack

  1. this PR — proto + generated code
  2. notification_history keyset listing
  3. notification:read / notification:manage permissions
  4. Grafana-backed service, handlers, server wiring
  5. notifications UI

🤖 Generated with Claude Code

@illegalprime illegalprime requested a review from a team as a code owner June 15, 2026 15:53
@github-actions github-actions Bot added javascript Pull requests that update javascript code client server shared labels Jun 15, 2026
@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown

🔐 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 (3f14d374f5c67e14b70014c14d582c2c27273fee...cd175c127b14bc0386b38e0f36e910d5d3111753, exact PR three-dot diff)
  • Model: gpt-5.5

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


Review Summary

Overall Risk: HIGH

Findings

[HIGH] Channel list/response types can expose stored notification secrets

  • Category: gRPC
  • Location: proto/notifications/v1/notifications.proto:89
  • Description: ListChannelsResponse returns full Channel objects, and Channel embeds WebhookConfig, SmtpConfig, and SlackConfig. Those config messages contain secret-bearing fields such as bearer_header, SMTP password, and Slack webhook_url. The presence of has_secret suggests redaction is intended, but the response schema still provides normal fields for the raw secrets.
  • Impact: A caller that can list or receive channels could exfiltrate Slack webhook URLs, SMTP passwords, and bearer tokens if handlers populate these fields. These values may also end up in frontend state or logs.
  • Recommendation: Split request/write config from response/read models. Return a redacted ChannelSummary that omits secret values and includes only safe metadata plus has_secret. Do not reuse secret-bearing config messages in list/create/update responses.

[HIGH] Test/channel config accepts arbitrary outbound targets without validation

  • Category: gRPC
  • Location: proto/notifications/v1/notifications.proto:120
  • Description: TestChannelRequest can carry arbitrary webhook, Slack webhook, or SMTP host/port data. The target fields have no URI, scheme, host, port, length, or private-network restrictions.
  • Impact: A straightforward implementation of TestChannel becomes an SSRF primitive from the server: probing internal services, loopback, link-local/cloud metadata endpoints, or sending attacker-controlled requests with configured headers.
  • Recommendation: Add strict validation and server-side canonicalization before any outbound request: require HTTPS for webhooks/Slack, reject userinfo and redirects to disallowed networks, block loopback/private/link-local/multicast targets, constrain SMTP ports/hosts, set short timeouts, and rate-limit tests.

[MEDIUM] Channel create/update schema allows invalid or conflicting channel configurations

  • Category: gRPC
  • Location: proto/notifications/v1/notifications.proto:92
  • Description: CreateChannelRequest and UpdateChannelRequest accept kind, webhook, smtp, and slack as independent fields. kind permits CHANNEL_KIND_UNSPECIFIED, and clients can send no config or multiple configs at once.
  • Impact: Handlers must guess which config is authoritative, creating room for dropped secrets, wrong validation, inconsistent persistence, or bypasses where a request’s kind does not match the supplied secret config.
  • Recommendation: Use a required oneof for channel config, or add Buf validate/CEL rules enforcing exactly one config and matching kind. Add enum.defined_only and not_in: [0] for kind.

[MEDIUM] Maintenance windows can be created without required scope/time invariants

  • Category: Reliability
  • Location: proto/notifications/v1/notifications.proto:208
  • Description: CreateMaintenanceWindowRequest and UpdateMaintenanceWindowRequest do not require scope, starts_at, or ends_at, and there is no validation that ends_at is after starts_at. MaintenanceWindowScope also allows an unspecified kind and multiple/missing target fields.
  • Impact: Invalid windows can accidentally silence alerts too broadly or indefinitely, depending on handler defaults. Missing message fields can also lead to handler panics if dereferenced without checks.
  • Recommendation: Mark scope and timestamps required, reject unspecified scope kinds, enforce exactly one target field for the selected scope, cap device_ids, and add a CEL rule requiring ends_at > starts_at plus any maximum-duration policy.

[MEDIUM] Notification history page size is unbounded

  • Category: Reliability
  • Location: proto/notifications/v1/notifications.proto:253
  • Description: ListNotificationsRequest.page_size only validates gte = 0; there is no upper bound.
  • Impact: An authenticated caller can request very large notification pages, forcing large DB reads and large protobuf/JSON responses. Notification history is naturally unbounded over time, so this can become a memory/CPU pressure point.
  • Recommendation: Add an lte cap consistent with other list APIs, and have the handler clamp 0 to a safe default.

Notes

No hand-written server handlers, SQL, frontend views, plugin code, Docker files, or infrastructure changes are present in this diff. I did not find cryptostealing/pool-hijack logic in the changed files. The main risk is that the new protobuf contract bakes in unsafe defaults before the RPC implementation lands.


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

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c64a5c2fd8

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread proto/notifications/v1/notifications.proto Outdated
@illegalprime illegalprime force-pushed the eden/notifications-1-proto branch from c64a5c2 to d032a2d Compare June 15, 2026 16:02

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d032a2dfba

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread proto/notifications/v1/notifications.proto
@illegalprime

Copy link
Copy Markdown
Contributor Author

Both Codex findings on this PR are resolved downstream in #455 (server layer), not here — they read as open only because Codex reviews this PR's proto diff in isolation:

No change needed on this PR.

@illegalprime

Copy link
Copy Markdown
Contributor Author

codex-connector triage:

  • [P1] Scope rule pauses per organizationfalse positive. PauseRule (service.go) does not flip Grafana's global isPaused; it "mutes a rule via a marker pause-silence" — buildPauseSilence(orgID, ruleID, …) is org-scoped, and pauseSilencedRules derives enabled per-org. So a pause by org A does not affect org B. No change.
  • [P2] Explicit secret-preservation semantics — handled in notifications (4/5): Grafana-backed service, handlers, and server wiring #455's service impl: requestHasNewSecret + carrySecretSettings carry the stored secret forward only when the destination is unchanged. The shared message shape is intentional (reads return redacted/empty secrets). No proto change.

@illegalprime illegalprime force-pushed the eden/notifications-1-proto branch from d032a2d to 7a2ceb2 Compare June 15, 2026 18:35

@flesher flesher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Where did we land on for the naming of "Silences" / how is it being reflected in the UI? I thought the general consensus was maintenance windows but forget where it ended up... Regardless we should try to align the api to whatever the UI calls them

@illegalprime

Copy link
Copy Markdown
Contributor Author

Great point. Thanks!

Adds the notifications.proto service definitions (Channel, Rule, Silence,
History) and the generated Go (Connect handlers + messages) and TypeScript
clients. Generated code only; no behavior is wired up yet.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@illegalprime illegalprime force-pushed the eden/notifications-1-proto branch from 7a2ceb2 to cd175c1 Compare June 15, 2026 19:24

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cd175c127b

ℹ️ About Codex in GitHub

Codex has been enabled to automatically 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 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

MaintenanceWindowScopeKind kind = 1;
string rule_id = 2;
string group_id = 3;
string site_id = 4;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Emit site labels before exposing site windows

When a SITE maintenance window is backed by the Grafana/Alertmanager rules in this repo, there is currently no site_id label for the silence to match: the default per-device rules only return organization_id and device_id from SQL and then add static labels like severity, rule_group, and template (server/monitoring/grafana/provisioning/alerting/proto-fleet-rules.yaml:24-31 and :76-81). A window created with this site_id can therefore appear active while offline/temperature alerts for that site keep firing, unless the implementation expands the site to device matchers or the provisioned rules start emitting site_id.

Useful? React with 👍 / 👎.

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

Labels

client javascript Pull requests that update javascript code server shared

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants