feat(notifications): per-recipient mute + List-Unsubscribe (closes #297) by cristim · Pull Request #828 · LeanerCloud/CUDly

cristim · 2026-05-28T19:08:15Z

Summary

Adds migration 000061 (muted_recipients table keyed on (recipient_email, scope)) for per-recipient, per-scope opt-out persistence.
Adds GET /api/notifications/unsubscribe?token=...&email=...&scope=... (AuthPublic) that verifies an HMAC-SHA256 signed token, upserts the mute row, and returns a minimal HTML confirmation page (RFC 8058 one-click flow).
Wires mute checking and List-Unsubscribe / List-Unsubscribe-Post headers into SendPurchaseApprovalRequest; the SES send is skipped silently when the recipient is muted, and the headers are attached when unsubscribeBaseURL is configured.

Design notes

Token mechanism: HMAC-SHA256(NOTIFICATION_MUTE_SECRET, lower(email)+"|"+scope) in pkg/common.DeriveMuteToken; compared with hmac.Equal (constant time). Dev/test fallback key used when env var is unset.
Mute is fail-open: a DB error during the mute check is logged and the email is sent, so a transient outage never blocks approval notifications.
Scope whitelist enforced at the handler boundary (purchase_approvals / ri_exchange_approvals).
PII: emails are logged only as us***@example.com (redacted).

Test plan

go test github.com/LeanerCloud/CUDly/internal/email/... github.com/LeanerCloud/CUDly/internal/api/... github.com/LeanerCloud/CUDly/pkg/common/... passes (1867 tests)
TestUnsubscribeHandler_Success - valid signed token stores mute row, returns HTML page
TestUnsubscribeHandler_ForgedToken_Returns401 - bad token rejected
TestSendPurchaseApprovalRequest_MutedRecipient_NoSESCall - muted address skips SES call
TestSendPurchaseApprovalRequest_NotMuted_SendsEmail - normal path unchanged
TestSendPurchaseApprovalRequest_MuteCheckError_FailOpen - DB error doesn't block send
RFC 8058 compliance: List-Unsubscribe carries angle-bracketed HTTPS URL; List-Unsubscribe-Post: List-Unsubscribe=One-Click present when base URL configured

Summary by CodeRabbit

New Features
- Added RFC 8058 one-click unsubscribe (POST) plus confirmation (GET) for notification emails.
- Added signed unsubscribe tokens with email/scope validation and an unsubscribe confirmation page.
- Added per-recipient notification muting for approval emails, including CC filtering, with unsubscribe header support.
Configuration
- Added NOTIFICATION_MUTE_SECRET for generating signed one-click unsubscribe links.
Bug Fixes
- Improved safety of unsubscribe pages by applying a restrictive content policy for the confirmation response.
Tests
- Expanded coverage for routing, token validation, error cases, mute suppression, and unsubscribe header behavior.

coderabbitai · 2026-05-28T19:08:24Z

Important

Review skipped

Review was skipped due to path filters

⛔ Files ignored due to path filters (1)

frontend/package-lock.json is excluded by !**/package-lock.json

CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including **/dist/** will override the default block on the dist directory, by removing the pattern from both the lists.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4cd60fdb-dcbc-470b-939a-4cf533f40245

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds end-to-end notification muting and RFC 8058 one-click unsubscribe support through signed tokens, PostgreSQL persistence, email transport integration, public API routes, and application wiring.

Changes

Notification Mute and One-Click Unsubscribe

Layer / File(s)	Summary
Mute token primitives `pkg/common/tokens.go`, `.env.example`	Defines notification scopes, HMAC token derivation and verification, and the mute-secret configuration entry.
Mute persistence contracts and stores `internal/config/...`, `internal/mocks/stores.go`, `internal/scheduler/`, `internal/analytics/`, `internal/server/test_helpers_test.go`	Adds idempotent mute upsert and case-insensitive lookup methods with PostgreSQL and test-store implementations.
Email suppression and unsubscribe headers `internal/email/*`	Adds recipient mute filtering, RFC 8058 URL/header generation, SES and SMTP support, approval-email integration, and tests.
Public unsubscribe endpoint `internal/api/*`	Adds public GET/POST routes, request and token validation, mute persistence, confirmation HTML, and route tests.
Application sender wiring `internal/server/app.go`, `internal/server/app_mute_wiring_test.go`	Connects the mute store and dashboard URL to concrete email senders and tests suppression and headers.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant EmailClient
  participant Router
  participant unsubscribeHandler
  participant PostgresStore
  EmailClient->>Router: GET or POST unsubscribe request
  Router->>unsubscribeHandler: dispatch request
  unsubscribeHandler->>PostgresStore: UpsertNotificationMute
  PostgresStore-->>unsubscribeHandler: persistence result
  unsubscribeHandler-->>EmailClient: HTML confirmation

sequenceDiagram
  participant PurchaseApproval
  participant MuteChecker
  participant EmailSender
  PurchaseApproval->>MuteChecker: check recipient and CC addresses
  MuteChecker-->>PurchaseApproval: mute statuses
  PurchaseApproval->>EmailSender: send filtered message with RFC 8058 headers

Possibly related PRs

LeanerCloud/CUDly#298: Adds multipart purchase-approval email sending paths used by this change.
LeanerCloud/CUDly#1036: Modifies overlapping token-bearing approval-notification email flows.

Suggested labels: type/security

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 61.90% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: per-recipient notification muting and List-Unsubscribe support.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch fix/297-wave9

_{Comment @coderabbitai help to get the list of available commands.}

cristim · 2026-05-28T19:08:25Z

@coderabbitai review

coderabbitai · 2026-05-28T19:08:30Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-05-30T10:22:18Z

@coderabbitai full review

coderabbitai · 2026-05-30T10:22:23Z

✅ Actions performed

Full review triggered.

…828) UpsertNotificationMute and IsNotificationMuted were added to config.StoreInterface (issue #297) but the stub implementations in five test files were not updated, causing go vet to fail with interface satisfaction errors. Affected mocks: - internal/analytics/collector_test.go (mockConfigStore) - internal/purchase/mocks_test.go (MockConfigStore) - internal/scheduler/scheduler_overrides_test.go (mockOverrideStore) - internal/scheduler/scheduler_suppressions_test.go (mockSuppressionStore) - internal/scheduler/scheduler_test.go (MockConfigStore) - internal/server/test_helpers_test.go (mockConfigStoreForHealth)

cristim · 2026-06-01T22:29:23Z

@coderabbitai review

coderabbitai · 2026-06-01T22:29:27Z

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

cristim · 2026-06-04T14:04:02Z

@coderabbitai review

coderabbitai · 2026-06-04T14:04:11Z

Rate Limit Exceeded

@cristim have exceeded the limit for the number of chat messages per hour. Please wait 39 minutes and 24 seconds before sending another message.

cristim · 2026-06-05T14:49:38Z

@coderabbitai review

coderabbitai · 2026-06-05T14:49:45Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…828) UpsertNotificationMute and IsNotificationMuted were added to config.StoreInterface (issue #297) but the stub implementations in five test files were not updated, causing go vet to fail with interface satisfaction errors. Affected mocks: - internal/analytics/collector_test.go (mockConfigStore) - internal/purchase/mocks_test.go (MockConfigStore) - internal/scheduler/scheduler_overrides_test.go (mockOverrideStore) - internal/scheduler/scheduler_suppressions_test.go (mockSuppressionStore) - internal/scheduler/scheduler_test.go (MockConfigStore) - internal/server/test_helpers_test.go (mockConfigStoreForHealth)

cristim · 2026-06-08T07:06:39Z

@coderabbitai full review

coderabbitai · 2026-06-08T07:07:39Z

✅ Action performed

Full review finished.

…828) UpsertNotificationMute and IsNotificationMuted were added to config.StoreInterface (issue #297) but the stub implementations in five test files were not updated, causing go vet to fail with interface satisfaction errors. Affected mocks: - internal/analytics/collector_test.go (mockConfigStore) - internal/purchase/mocks_test.go (MockConfigStore) - internal/scheduler/scheduler_overrides_test.go (mockOverrideStore) - internal/scheduler/scheduler_suppressions_test.go (mockSuppressionStore) - internal/scheduler/scheduler_test.go (MockConfigStore) - internal/server/test_helpers_test.go (mockConfigStoreForHealth)

cristim · 2026-06-09T01:02:34Z

Rebased onto the current feat/multicloud-web-frontend head. Resolved three additive mock conflicts caused by the base branch's refactor that moved the shared MockConfigStore into internal/mocks: kept the base type-alias form in the api/purchase/scheduler test files and added the two new UpsertNotificationMute / IsNotificationMuted methods to the shared internal/mocks/stores.go, plus added both methods to the hand-written mockConfigStoreForHealth in internal/server/test_helpers_test.go (keeping both that file's revocation methods and the new mute methods). Renumbered the notification-mute migration from 000061 to 000076 (the next free slot; base tops at 000073 and 000074/000075 are taken by in-flight PRs #1011/#1094) for both the .up.sql and .down.sql, and updated the matching "migration 000061" reference in the config.StoreInterface doc comment. go build ./..., go test ./internal/... (4711 passed) and go test ./pkg/... (443 passed) are all green.

@coderabbitai review

coderabbitai · 2026-06-09T01:02:48Z

@cristim Thanks for the detailed rebase summary! The migration renumbering to 000076, the mock additions to internal/mocks/stores.go and internal/server/test_helpers_test.go, and the doc-comment update all look like clean, well-reasoned conflict resolutions. Triggering a fresh review now.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

…828) UpsertNotificationMute and IsNotificationMuted were added to config.StoreInterface (issue #297) but the stub implementations in five test files were not updated, causing go vet to fail with interface satisfaction errors. Affected mocks: - internal/analytics/collector_test.go (mockConfigStore) - internal/purchase/mocks_test.go (MockConfigStore) - internal/scheduler/scheduler_overrides_test.go (mockOverrideStore) - internal/scheduler/scheduler_suppressions_test.go (mockSuppressionStore) - internal/scheduler/scheduler_test.go (MockConfigStore) - internal/server/test_helpers_test.go (mockConfigStoreForHealth)

cristim · 2026-07-10T22:10:50Z

@coderabbitai review

coderabbitai · 2026-07-10T22:10:55Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Migration 000078 collided with PR #828. Main's highest is 000081; #1277 reserves 000082; 000083 is the next free slot for this PR. Renamed via git mv (up + down); no Go code referenced the number directly.

* feat(settings): configurable EC2 RI OfferingClass (closes #694) Add a GlobalConfig.OfferingClass field ("convertible" | "standard") that controls which EC2 Reserved Instance offering class is purchased. Empty / absent values default to "convertible" to preserve pre-694 behaviour. Backend: - pkg/common/types.go: OfferingClass field on PurchaseOptions - internal/config/types.go: OfferingClass on GlobalConfig - internal/config/store_postgres.go: read/write offering_class column ($20) - internal/purchase/execution.go: load GlobalConfig and propagate OfferingClass into PurchaseOptions before the fan-out - providers/aws/services/ec2/client.go: resolveOfferingClassType() fails loudly on unknown values (feedback_empty_string_vs_error.md); empty string maps to convertible so the DB default is safe - internal/database/postgres/migrations/000064_ec2_ri_offering_class: ADD COLUMN offering_class TEXT NOT NULL DEFAULT 'convertible' Frontend: - frontend/src/index.html: fieldset with <select> (Convertible / Standard) - frontend/src/settings.ts: load, save, and reset handlers - frontend/src/api/types.ts + types.ts: offering_class optional field Tests: - TestResolveOfferingClassType: unknown values error, "" = convertible - TestDescribeInputFromQuery_OfferingClass: empty / explicit convertible / standard - All findOfferingID call sites updated to 4-arg signature - store_postgres_pgxmock_test.go: +offering_class column in mock rows - settings.test.ts: offering_class: 'convertible' in saveGlobalSettings expected payload * fix(settings): validate OfferingClass on PUT + assert it reaches the EC2 SDK call (refs #694) - Add validateOfferingClass() to GlobalConfig.Validate() so an invalid offering_class is rejected at PUT time with a clear error rather than silently persisting and only failing at purchase time. Accepts "" | "convertible" | "standard" (case-sensitive, matching resolveOfferingClassType). - Add ValidOfferingClasses exported var for reference/documentation. - Add six table-driven test cases in TestGlobalConfig_Validate covering empty (valid), both valid values, wrong-case "Convertible", "STANDARD", and an unknown value. - Add capturingMockEC2Client embedding MockEC2Client that records the last DescribeReservedInstancesOfferingsInput to enable SDK-level assertions. - Add TestFindOfferingID_OfferingClassReachesSDKCall: three subtests ("", "convertible", "standard") each calling findOfferingID and asserting that the captured OfferingClass on the outbound SDK call equals the expected types.OfferingClassType. This test fails if the wiring from offeringClassStr through resolveOfferingClassType to describeInputFromQuery regresses. * refactor(config): extract validateScheduleAndNotifications to reduce cyclomatic complexity Validate had complexity 11 after adding the OfferingClass check in #694. Extract the CollectionSchedule + NotificationDaysBefore + GracePeriodDays checks into validateScheduleAndNotifications, keeping Validate under 10. * fix(test): avoid mutex copy in TestFindOfferingID_OfferingClassReachesSDKCall Construct capturingMockEC2Client directly instead of copying *MockEC2Client to silence the go vet copylocks warning (MockEC2Client embeds mock.Mock which contains sync.Mutex). * fix(purchase): fail EC2 RI recs on GlobalConfig load error instead of defaulting OfferingClass When GetGlobalConfig returns an error, the old code logged it and continued with an empty OfferingClass, which resolveOfferingClassType maps to "convertible". This caused a transient DB failure to silently buy the wrong (and more expensive, irreversible) RI class instead of the operator-configured "standard". Violates the no-silent-fallbacks-on-money-paths rule. Fix: change processPurchaseRecommendations to return (float64, float64, []string, error) and propagate a hard error on GetGlobalConfig failure. Both call sites (executeSingleAccount and executeForAccount) check the new error return and abort without making any cloud purchase. Regression test (HOLE 1): TestProcessPurchaseRecommendations_GlobalConfigError_FailsInsteadOfDefaulting confirms the function returns an error and never calls the provider factory when GetGlobalConfig fails. Verified red on pre-fix code, green after. Also add TestSaveGlobalConfig_OfferingClassBindsAt21 (HOLE 2): a pgxmock test against the real PostgresStore.SaveGlobalConfig verifying offering_class binds as the 21st positional arg. The hand-maintained testablePostgresStore in store_postgres_mock_test.go omits this field entirely, so no fast test previously guarded the 21-placeholder write query. * refactor(purchase): extract applyAccountOutcome to drop executeForAccount below gocyclo 10 The gocyclo pre-commit hook (-over 10) failed: executeForAccount was at cyclomatic complexity 11. Extract the per-account status-resolution switch (partially_completed / failed / completed stamping, #642) and the committed gate (#1014) into a cohesive applyAccountOutcome helper, dropping the function below the threshold without a //nolint or a threshold bump. Behavior is unchanged: same status transitions, same error-note appending, same committed semantics (anyRecPurchased is now evaluated once and reused for the partial flag). The OfferingClass typed-enum validation and no-silent-fallback behavior are untouched. * fix(test): update SQL parameter positions after laddering_enabled shift Rebasing onto main added laddering_enabled as $21 in global_config INSERT, shifting offering_class from $21 to $22. Update the pgxmock and coverage tests to reflect the new column count and binding positions. * style(purchase,config): clear new-from-rev lint on OfferingClass diff Resolve the three golangci-lint findings that --new-from-rev=origin/main attributes to the #694 OfferingClass changes: - errcheck: replace the silent `_ =` discard of SavePurchaseExecution on the processPurchaseRecommendations error path with the existing saveExecutionStatusBestEffort helper, which persists and logs the audit-save failure instead of dropping it. - gocritic unnamedResult: name the four results of processPurchaseRecommendations (its return set grew to include error); switch the trailing assignment from := to = accordingly. - misspell: pre-694 behaviour -> behavior in the ValidOfferingClasses doc. No behavior change to the purchase path; base-debt Lint/Security failures are pre-existing on main and out of scope. * chore(migration): renumber ec2_ri_offering_class from 000078 to 000083 Migration 000078 collided with PR #828. Main's highest is 000081; #1277 reserves 000082; 000083 is the next free slot for this PR. Renamed via git mv (up + down); no Go code referenced the number directly. * chore(migration): renumber ec2_ri_offering_class from 000083 to 000090 Migration 000083 is now taken by ladder_execution_enabled (merged to main while this PR was in review). Next free slot after 000089_rename_cancelled_to_canceled is 000090. * fix(rebase): restore saveExecutionStatusBestEffort; update param-23 guard test Two issues introduced when rebasing onto main (which added ladder_execution_enabled in parallel): 1. saveExecutionStatusBestEffort helper was dropped during conflict resolution of the applyAccountOutcome refactor commit; restored from the pre-rebase branch. 2. TestSaveGlobalConfig_OfferingClassBindsAt22 expected 22 args; with ladder_execution_enabled now at $22, offering_class moved to $23. Renamed the test to TestSaveGlobalConfig_OfferingClassBindsAt23 and added the $22 AnyArg placeholder.

…828) UpsertNotificationMute and IsNotificationMuted were added to config.StoreInterface (issue #297) but the stub implementations in five test files were not updated, causing go vet to fail with interface satisfaction errors. Affected mocks: - internal/analytics/collector_test.go (mockConfigStore) - internal/purchase/mocks_test.go (MockConfigStore) - internal/scheduler/scheduler_overrides_test.go (mockOverrideStore) - internal/scheduler/scheduler_suppressions_test.go (mockSuppressionStore) - internal/scheduler/scheduler_test.go (MockConfigStore) - internal/server/test_helpers_test.go (mockConfigStoreForHealth)

cristim · 2026-07-19T18:15:22Z

@coderabbitai review

coderabbitai · 2026-07-19T18:15:39Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Migration 000061 adds muted_recipients table keyed on (email, scope). GET /api/notifications/unsubscribe verifies an HMAC-signed token and upserts the mute row (AuthPublic, mirrors approve/cancel pattern). SendPurchaseApprovalRequest checks the mute table before each send and attaches List-Unsubscribe + List-Unsubscribe-Post headers (RFC 8058) when an unsubscribe base URL is configured. Token signing uses HMAC-SHA256 over (lower(email)|scope) via NOTIFICATION_MUTE_SECRET.

…828) UpsertNotificationMute and IsNotificationMuted were added to config.StoreInterface (issue #297) but the stub implementations in five test files were not updated, causing go vet to fail with interface satisfaction errors. Affected mocks: - internal/analytics/collector_test.go (mockConfigStore) - internal/purchase/mocks_test.go (MockConfigStore) - internal/scheduler/scheduler_overrides_test.go (mockOverrideStore) - internal/scheduler/scheduler_suppressions_test.go (mockSuppressionStore) - internal/scheduler/scheduler_test.go (MockConfigStore) - internal/server/test_helpers_test.go (mockConfigStoreForHealth)

…roduction sender (refs #297) The per-recipient mute + List-Unsubscribe feature was dead in production: the sender returned by email.NewSenderFromEnvironment was never decorated with the mute checker or the unsubscribe base URL, so the bare sender's mute checker was always nil and no List-Unsubscribe header was ever emitted. The SMTP transport bypassed the mute logic entirely. - app.go: decorate the factory-produced sender via a new decorateSenderWithMute helper that type-switches on *email.Sender / *email.SMTPSender and calls WithMuteChecker(configStore) + WithUnsubscribeBaseURL(trimmed DASHBOARD_URL). configStore (config.StoreInterface) satisfies email.MuteChecker via IsNotificationMuted. The base URL reuses the same DASHBOARD_URL the email templates already use, trimmed to match resolveOIDCIssuerURL. - email/mute.go: extract the transport-agnostic mute + List-Unsubscribe decision logic (isRecipientMuted, filterMutedRecipients, unsubscribeURLFor, unsubscribeHeaderValuesFor) into shared free functions so the SES and SMTP paths cannot diverge; *Sender delegates to them. - email/smtp_sender.go: add muteChecker + unsubscribeBaseURL fields and WithMuteChecker/WithUnsubscribeBaseURL mirroring *Sender, and apply the mute skip + CC filter + RFC 8058 List-Unsubscribe headers in SendPurchaseApprovalRequest. - Wiring tests exercise the real factory/decoration path (decorateSenderWithMute on a factory-shaped sender) and the SMTP transport: a muted recipient is suppressed and the List-Unsubscribe header is emitted with the wired base URL. Both fail against the pre-wiring behavior (nil checker, no base URL).

…ibe path buildSMTPMessageMultipartWithHeaders used a fixed boundary string instead of mimeRandBoundary(), unlike its sibling buildSMTPMessageMultipart. A body containing the literal boundary causes MIME corruption. Also fix two US-spelling violations (behaviour->behavior, Centralising->Centralizing) in pkg/common/tokens.go introduced by this PR.

Address CodeRabbit review findings on the per-recipient mute feature: - Fail closed on a missing mute secret. ResolveMuteSecret returns the NOTIFICATION_MUTE_SECRET bytes when set, the dev key only in non-production, and ErrMuteSecretMissing in production. DeriveMuteToken no longer silently signs with a well-known fallback on an empty key (returns ""), and VerifyMuteToken fails closed so a missing secret can never accept a forged token. The unsubscribe handler returns a 500 (not a 401) when the production secret is absent, and the send path emits no List-Unsubscribe header rather than a tokenless or forgeable one. - Suppress List-Unsubscribe when CC recipients share the envelope. The header token is bound to the primary recipient only; sending it to a CC list let any CC recipient one-click-mute the primary address. Both the SES and SMTP paths now omit the header whenever a CC list exists. - Redact the recipient email in the SMTP approval-delivery debug logs. - Correct the UpsertNotificationMute interface doc to match the implemented refresh-on-conflict semantics (muted_at = NOW()).

NewApplication no longer has a config store at startup (initConfigStore was refactored on main to return only dbConfig+secretResolver for lazy DB init). Move the decorateSenderWithMute call into reinitializeAfterConnect where pgStore is live, so the mute checker is wired once the DB connection is established rather than at cold-start with a nil store.

…ons PR Address four linter findings in the mute/unsubscribe feature: - smtp_sender: use %q instead of escaped-quote "%s" (sprintfQuotedString) - sender: remove unused mailtoURL return from buildUnsubscribeURL (unparam) - templates: remove dead sendPurchaseApprovalRequestVia function (unused) - migration: renumber 000078_muted_recipients to 000091 to resolve conflict with 000078_monthly_summary_nested_rollup already on main

cristim · 2026-07-19T18:23:27Z

@coderabbitai review

coderabbitai · 2026-07-19T18:23:39Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

- accept and validate RFC 8058 POST requests using signed query tuples - apply RI exchange mute scopes and unsubscribe headers in SES and SMTP - require an explicit notification mute secret in every environment

coderabbitai

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/api/handler_notifications.go`:
- Around line 86-127: Update internal/api/handler_notifications.go:86-127 in
unsubscribeHandler to branch on the validated HTTP method, allowing
UpsertNotificationMute and the unsubscribed response only for POST while
rendering a non-mutating confirmation form for GET; update
internal/api/handler_notifications_test.go:28-55 to assert GET does not mutate
and add GET coverage while preserving POST mutation assertions; make no change
to internal/api/router.go:323-327, since its shared GET/POST routing remains
valid.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

Push a commit to this branch (recommended)
Create a new PR with the fixes

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 98c3be3e-6354-4266-84fb-afb01d852a62

📥 Commits

Reviewing files that changed from the base of the PR and between 3e04c1d and aef1850.

📒 Files selected for processing (8)

.env.example
internal/analytics/collector_test.go
internal/api/handler_notifications.go
internal/api/handler_notifications_test.go
internal/api/middleware.go
internal/api/router.go
internal/config/interfaces.go
internal/config/store_postgres.go

🚧 Files skipped from review as they are similar to previous changes (3)

internal/config/interfaces.go
internal/api/middleware.go
internal/analytics/collector_test.go

coderabbitai · 2026-07-19T23:54:42Z

+func (h *Handler) unsubscribeHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, _ map[string]string) (any, error) {
+	if err := validateOneClickUnsubscribeBody(req); err != nil {
+		return nil, err
+	}
+	token, email, scope, err := unsubscribeRequestParams(req)
+	if err != nil {
+		return nil, err
+	}
+
+	// Resolve the HMAC key with the fail-closed policy: a missing
+	// NOTIFICATION_MUTE_SECRET is a server misconfiguration, not a client error,
+	// and must never silently verify against a well-known key.
+	key, err := common.ResolveMuteSecret()
+	if err != nil {
+		logging.Errorf("notifications/unsubscribe: %v", err)
+		return nil, fmt.Errorf("unsubscribe is not configured: %w", err)
+	}
+	if !common.VerifyMuteToken(key, email, scope, token) {
+		logging.Warnf("notifications/unsubscribe: invalid token for scope=%s", scope)
+		return nil, NewClientError(401, "invalid or expired unsubscribe token")
+	}
+
+	if err := h.config.UpsertNotificationMute(ctx, email, scope, token); err != nil {
+		logging.Errorf("notifications/unsubscribe: store error: %v", err)
+		return nil, fmt.Errorf("could not save unsubscribe preference: %w", err)
+	}
+
+	logging.Infof("notifications/unsubscribe: muted scope=%s for %s", scope, redactEmailLocal(email))
+
+	var buf strings.Builder
+	if err := unsubscribeConfirmTmpl.Execute(&buf, struct{ ScopeLabel string }{
+		ScopeLabel: scopeLabel(scope),
+	}); err != nil {
+		return nil, fmt.Errorf("render unsubscribe page: %w", err)
+	}
+
+	return &rawResponse{
+		contentType: "text/html; charset=utf-8",
+		body:        buf.String(),
+		csp:         mutePageCSP,
+	}, nil
+}


🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

GET-triggers-mutation bug spans handler, routing, and its own test coverage. The root cause is in unsubscribeHandler, which mutates unconditionally regardless of HTTP method; router.go routes both verbs to it unchanged (no fix needed there), and the test suite currently asserts the buggy behavior as correct.

internal/api/handler_notifications.go#L86-L127: gate h.config.UpsertNotificationMute (and the "you have been unsubscribed" response) behind a validated POST; render a non-mutating confirmation/landing page for GET.

internal/api/router.go#L323-L327: no change required — once the handler branches on method internally, the existing GET+POST routing to the same handler is fine.

internal/api/handler_notifications_test.go#L28-L55: update TestUnsubscribeHandler_Success (and add a GET-specific test) to assert GET does not call UpsertNotificationMute and instead returns a confirmation form; keep the existing POST-based mutation assertions in TestRouter_UnsubscribePOSTRoute_MutesSignedRecipientAndScope.

📍 Affects 3 files

internal/api/handler_notifications.go#L86-L127 (this comment)

internal/api/router.go#L323-L327

internal/api/handler_notifications_test.go#L28-L55

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/api/handler_notifications.go` around lines 86 - 127, Update internal/api/handler_notifications.go:86-127 in unsubscribeHandler to branch on the validated HTTP method, allowing UpsertNotificationMute and the unsubscribed response only for POST while rendering a non-mutating confirmation form for GET; update internal/api/handler_notifications_test.go:28-55 to assert GET does not mutate and add GET coverage while preserving POST mutation assertions; make no change to internal/api/router.go:323-327, since its shared GET/POST routing remains valid.

… unnamedResult golangci v2.10.1 flagged two unused methods on *Sender (isMuted, filterMutedAddresses) and an unnamedResult in renderRIExchangePendingApproval. The two methods were thin wrappers around package-level functions that are already used directly; removing them avoids the dead code. The return tuple is now named (textBody, htmlBody string, err error) and the first assignment changes from := to = accordingly.

cristim · 2026-07-21T15:55:55Z

Rebased onto main and pushed a fix commit for the three golangci-lint v2.10.1 failures:

removed unused (*Sender).isMuted and (*Sender).filterMutedAddresses wrapper methods (both were thin delegators to package-level functions that are called directly; golangci unused linter flagged them)
named the return tuple of renderRIExchangePendingApproval as (textBody, htmlBody string, err error) and changed the first assignment from := to = accordingly (gocritic unnamedResult)

All 370 email tests pass locally. CI watch armed.

@coderabbitai review

coderabbitai · 2026-07-21T15:56:06Z

@cristim Thanks for the clear lint-fix summary. Triggering a fresh review of the latest commit.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

npm audit flagged brace-expansion <1.1.16 as high severity DoS. This was disclosed 2026-07-21 and affects all PR branches equally. Lockfile-only change; no production API surface altered.

cristim added triaged Item has been triaged priority/p3 Polish / idea / may never ship severity/low Minor harm urgency/eventually No deadline impact/few Limited audience effort/m Days type/feat New capability labels May 28, 2026

cristim force-pushed the fix/297-wave9 branch from 07567a8 to 8a627a7 Compare June 8, 2026 04:19

cristim force-pushed the fix/297-wave9 branch from 8a627a7 to 743b8b9 Compare June 9, 2026 01:02

cristim changed the base branch from feat/multicloud-web-frontend to main June 9, 2026 15:45

cristim force-pushed the fix/297-wave9 branch from 743b8b9 to 3e04c1d Compare June 19, 2026 21:22

cristim force-pushed the fix/297-wave9 branch from 0b082f0 to 798ab9a Compare July 10, 2026 22:10

cristim force-pushed the fix/297-wave9 branch from 798ab9a to 1b2c622 Compare July 17, 2026 18:29

cristim force-pushed the fix/297-wave9 branch from 1b2c622 to 65e9e1b Compare July 19, 2026 18:11

cristim added 7 commits July 19, 2026 20:19

cristim force-pushed the fix/297-wave9 branch from 65e9e1b to 8df363a Compare July 19, 2026 18:21

fix(notifications): harden one-click unsubscribe delivery

aef1850

- accept and validate RFC 8058 POST requests using signed query tuples - apply RI exchange mute scopes and unsubscribe headers in SES and SMTP - require an explicit notification mute secret in every environment

coderabbitai Bot reviewed Jul 19, 2026

View reviewed changes

fix(deps): bump brace-expansion to 1.1.16 (GHSA-3jxr-9vmj-r5cp)

f49cb41

npm audit flagged brace-expansion <1.1.16 as high severity DoS. This was disclosed 2026-07-21 and affects all PR branches equally. Lockfile-only change; no production API surface altered.

cristim merged commit 6573d76 into main Jul 21, 2026
20 checks passed

cristim deleted the fix/297-wave9 branch July 27, 2026 11:09

Uh oh!

Conversation

cristim commented May 28, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Design notes

Test plan

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Walkthrough

Changes

Sequence Diagram(s)

❌ Failed checks (1 warning)

Uh oh!

cristim commented May 28, 2026

Uh oh!

coderabbitai Bot commented May 28, 2026

Uh oh!

cristim commented May 30, 2026

Uh oh!

coderabbitai Bot commented May 30, 2026

Uh oh!

cristim commented Jun 1, 2026

Uh oh!

coderabbitai Bot commented Jun 1, 2026

Uh oh!

cristim commented Jun 4, 2026

Uh oh!

coderabbitai Bot commented Jun 4, 2026

Rate Limit Exceeded

Uh oh!

cristim commented Jun 5, 2026

Uh oh!

coderabbitai Bot commented Jun 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 8, 2026

Uh oh!

coderabbitai Bot commented Jun 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jun 9, 2026

Uh oh!

coderabbitai Bot commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 10, 2026

Uh oh!

coderabbitai Bot commented Jul 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 19, 2026

Uh oh!

coderabbitai Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cristim commented Jul 19, 2026

Uh oh!

coderabbitai Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

cristim commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

cristim commented May 28, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented May 28, 2026 •

edited

Loading

coderabbitai Bot commented Jun 5, 2026 •

edited

Loading

coderabbitai Bot commented Jun 8, 2026 •

edited

Loading

coderabbitai Bot commented Jun 9, 2026 •

edited

Loading

coderabbitai Bot commented Jul 10, 2026 •

edited

Loading

coderabbitai Bot commented Jul 19, 2026 •

edited

Loading

coderabbitai Bot commented Jul 19, 2026 •

edited

Loading

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading