Rework notifications

[abarz722]

No description provided.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
86.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[tpurschke]

Ninth-round review — PR #5285

Review setup: deep review under the fwo-review-pr guardrails, with separate correctness/quality and security passes, plus the schema-change, RBAC/auth and installer guidance. Model: Claude Opus 5 (1M context) (claude-opus-5[1m]), extended thinking on, default reasoning effort; this session exposes no finer-grained reasoning-level label.

Head 24e23bc5, base develop. This is the same head round eight reviewed — there are no new commits since. Round eight closed F1–F18, so this round is a fresh independent pass over the whole diff rather than a fix verification. GitHub reports no submitted reviews and no inline review threads.

Findings

#	Criticality	Confidence	Status	Subject
F19	🟥 high	🟢 high	🔴 new	Daily check re-sends the initial interface-request notification every day
F20	🟥 high	🟢 high	🔴 new	log_only notifications never update last_sent, so they are re-logged on every run
F21	🟥 high	🟡 medium	🔴 new	Workflow Requester recipient is resolved by display name before DN
F14	🟥 high	—	✅ fixed	Multiline plain-text notification bodies are accepted
F15	🟥 high	—	✅ fixed	Workflow-role callers are always rejected by the send route
F16	🟧 medium	🟢 high	✅ fixed	Preserve successful last_sent updates in mixed delivery batches (re-verified)
F17	🟧 medium	🟢 high	✅ fixed	Reject inactive replacement interfaces (re-verified)
F18	🟧 medium	🟢 high	✅ fixed	Rejected interface requests cannot trigger request notifications again (re-verified)
F22	🟧 medium	🟢 high	🔴 new	Notification links rendered as HTML into plain-text emails
F23	🟧 medium	🟡 medium	🔴 new	RenderLink interpolates URL and link name into HTML without encoding
F24	🟧 medium	🟡 medium	🔴 new	Unanswered-request reminder body has no migration target
F25	🟧 medium	🟡 medium	🔴 new	Notification authorization can succeed on empty-string name equality
F3	🟧 medium (was high)	—	🤝 accepted	Fresh-install notification seeds are intentionally omitted
F13	🟧 medium	—	🤝 accepted	One-off upgrade migration will not receive an unstable integration test
F26	🟨 low	🟢 high	🔴 new	[Obsolete] messages name the wrong version and the wrong notification client
F27	🟨 low	🟢 high	🔴 new	Inline array arguments in a new test file

Table completeness: the finding count exceeds the ten-row limit, so the oldest fixed findings — F1, F2, F4–F12 — are omitted per the skill's table rule. All open, new and accepted findings are shown, as are the most recent fixed ones (F14–F18). The omitted rows and their evidence remain in the round-eight comment. No severity assigned in an earlier round was lowered here.

Recommendations

Fix before merge: F19 and F20. Both are upgrade-facing regressions in the feature this PR ships — F19 makes every open interface request generate a duplicate email daily after upgrade, and F20 makes the advertised log_only mode write duplicate audit rows without bound. F21 should be fixed in the same pass; it is a small change with a wrong-recipient consequence.

Worth fixing before release: F22 and F24, which both make the migrated (i.e. default post-upgrade) configuration behave worse than the 9.5.2 configuration it replaces. F25 is a cheap hardening of a hand-rolled authorization check.

Nice to have: F23 (pre-existing injection class that this PR extends), F26 and F27.

F19 — the daily check re-sends the initial request notification

DailyCheckJob.CheckUnansweredInterfaceRequests iterates every InterfaceRequest notification without filtering on the deadline:

foreach (var notification in notificationService.Notifications)   // DailyCheckJob.cs:254

NotificationService.Notifications is filtered only by Active, and getNotifications.graphql selects purely by notification_client, so the list contains both the Deadline = None initial request entry and the Deadline = RequestDate reminder entry — the two the settings page labels initial_request and reminder.

For the initial entry the guards all pass:

• GetInterfaceRequestCutOffPeriod (:304-312) sums InitialOffsetAfterDeadline, RepeatOffsetAfterDeadline and RepetitionsAfterDeadline, all NULL on that row, giving 0, so GetOpenTickets returns every open new_interface ticket.
• NotificationScheduleHelper.IsNotificationDue returns true unconditionally for Deadline == None (NotificationScheduleHelper.cs:25-28), so LastSent never suppresses it.

9.5.3.sql creates exactly that row from the legacy modReqEmailBody config (deadline is inserted as 'None'). So after upgrade, the "Interface requested" mail goes out again to the owner responsibles, for every still-open request, on every daily-check run.

Both notification endpoints already do the filtering this job is missing — NotificationController.SendInterfaceDecommission and SendInterfaceRequest each apply .Where(notification => notification.Deadline == NotificationDeadline.None). The daily job needs the mirror of that: restrict to NotificationDeadline.RequestDate.

F20 — log_only never advances last_sent

SendNotificationWithResult records a notification as processed only when it was actually delivered:

NotificationDeliveryResult deliveryResult = await SendEmail(...);
if (deliveryResult == NotificationDeliveryResult.Delivered)   // NotificationService.cs:153-156
{
    AddCheckedNotificationId(notification.Id);
}

A log_only notification returns Suppressed (:249-253), so its id never reaches CheckedNotificationIds and UpdateNotificationsLastSent never writes its last_sent. Every later scheduler run then re-evaluates it as due:

• the RequestDate fast path is notification.LastSent == null || notification.LastSent.Value.Date < DateTime.Now.Date (NotificationScheduleHelper.cs:30-34) — always true with a null LastSent;
• IsTimeToSend(null, notifDate) is likewise always true, so IsNotificationDueAfterDeadline's repetition series never advances past its current step.

Each run inserts a fresh notification_log row for the same notification and the same deadline. The table has an index but no retention path, so in log_only mode — which is advertised in whats_new_facts — the audit log grows with duplicates indefinitely, and the repetition count configured by the admin is not honoured.

The same bookkeeping appears in SendBundledNotifications (:188-192, gated on ShouldSend) and in ActionHandlerSendEmail.SendActionNotifications, which adds the id only on Delivered. Suggested direction: record the id whenever the notification was processed (delivered or suppressed by logging mode), keeping NoRecipients and Failed excluded as F16 established.

F21 — Requester is matched by display name before DN

CollectEmailAddressesFromScopedUser gained a name-keyed lookup that runs before the authoritative DN resolution:

UiUser? cachedUser = uiUsers.FirstOrDefault(user =>          // EmailHelper.cs:550-556
    !string.IsNullOrWhiteSpace(userName)
    && string.Equals(user.Name, userName, StringComparison.OrdinalIgnoreCase)
    && !string.IsNullOrWhiteSpace(user.Email));
if (cachedUser != null)
{
    return [cachedUser.Email!];
}
return await CollectEmailAddressesFromUserOrGroup(dn);

userName is supplied by ActionHandlerSendEmail:

await emailHelper.Init(ScopedUserTo, ScopedUserCc, ScopedUserBcc, ScopedUserEmailTo, ScopedUserEmailCc,
    ScopedUserEmailBcc, wfHandler.ActTicket.Requester?.Name);   // ActionHandlerSendEmail.cs:76-77

and uiUsers is the unfiltered AuthQueries.getUserEmails result (EmailHelper.cs:70) — every UI user across every connected LDAP. FWO supports several LDAP connections and several tenants, so two directories holding the same uiuser_username is an ordinary configuration, not a contrived one. When that happens FirstOrDefault picks an arbitrary match and the workflow email — carrying ticket content — is delivered to an unrelated user, potentially in another tenant.

The DN is the unique key and is already available in the same call. Resolve on Dn (the file's other lookups correctly use DistName.DnEquals, :634 and :726) and fall back to the name only when no DN is known.

F22 — HTML links in plain-text bodies

renderHtmlLinks is passed as a constant, independently of the notification layout:

body = NotificationPlaceholderResolver.ReplaceNotificationPlaceholders(body, placeholderValues, renderHtmlLinks: true);   // NotificationService.cs:472

while the message itself is sent with the format the layout dictates:

bool sent = await MailKitMailer.SendAsync(mail, emailConnection, notification.Layout == NotificationLayout.HtmlInBody, new());   // NotificationService.cs:360

EmailHelper.SendWorkflowActionEmail has the same pair (:151 against :162). RenderLink honours the flag and emits <a target="_blank" href="…">…</a>.

9.5.3.sql creates the migrated InterfaceRequest, InterfaceDecomm and RuleRecertification rows with layout = 'SimpleText', so @@INTERFACE_LINK@@ and @@NEW_INTERFACE_LINK@@ expand to literal anchor markup inside a text/plain body. This is a regression against 9.5.2, where these emails were always sent as HTML — the old EmailHelper.SendEmail defaulted mailFormatHtml to true. Derive renderHtmlLinks from notification.Layout at both call sites.

F23 — RenderLink does not encode its inputs

return $"<a target=\"_blank\" href=\"{url}\">{displayText}: {linkName}</a>";   // NotificationPlaceholderResolver.cs:174

Neither url nor linkName is HTML-encoded. linkName is the interface name — requestTask?.Title ?? connection.Name in NotificationController.BuildInterfaceRequestPlaceholderValues — which a modeller controls, and url embeds owner.ExtAppId, which comes from imported app data. An interface named x"><a href="http://evil">click</a> injects arbitrary markup into an HTML notification delivered to the owner responsibles.

For fairness: this is not introduced here. DailyCheckJob.ConstructLink in develop built the same unencoded anchor. But this PR moves that code into a shared resolver and applies it to more placeholders (NEW_INTERFACE_LINK) and more paths (decommission, workflow actions), so it is the right moment to HTML-encode the interpolated values.

F24 — the reminder body has nowhere to migrate to

9.5.3.sql reads modUnansweredReqEmailBody into request_config.reminder_body, but the only consumer is:

update_reminder_bodies AS ( UPDATE notification n SET email_body = ...
  WHERE n.notification_client = 'InterfaceRequest'
    AND n.deadline = 'RequestDate'
    AND reminder_notification_seed.notification_count > 0 ... )

which requires an InterfaceRequest notification with deadline = 'RequestDate' to already exist. Nothing under roles/database/files/sql/ ever creates one — insert_initial_notification only writes the deadline = 'None' row, and no creation or earlier upgrade script seeds a reminder. The CTE therefore cannot match on any real upgrade.

Meanwhile the key is no longer reachable from the UI: subscribeDailyCheckConfigChanges.graphql drops its subscription and SettingsModellingNotifications.razor no longer renders the modUnansweredReqEmailBody field. Net effect after upgrade: the configured reminder text is stranded in the config table and the unanswered-request reminder has no body until an admin creates the reminder notification by hand. Either insert the reminder row in the migration (as is done for the initial one) or document the manual step in the release notes.

F25 — authorization can succeed on empty-string equality

string callerName = caller.FindFirstValue("unique_name") ?? caller.Identity?.Name ?? "";
...
if (allowRequestCreator && ((callerId > 0 && ticket?.Requester?.DbId == callerId)
    || string.Equals(ticket?.Requester?.Name, callerName, StringComparison.OrdinalIgnoreCase)
    || string.Equals(connection.Creator, callerName, StringComparison.OrdinalIgnoreCase)))
{
    return true;
}

callerName falls back to "". UiUser.Name defaults to "" and modelling_connection.creator can hold '', so if the JWT ever lacks unique_name and Identity.Name, either comparison succeeds and any authenticated modeller passes the owner-scope check for that connection — bypassing the x-hasura-editable-owners test below it.

This needs a missing claim to be reachable, so it is a latent trap rather than an exploitable path today. Guard both comparisons with !string.IsNullOrWhiteSpace(callerName), and prefer the x-hasura-user-id comparison that is already there.

F26 — inaccurate obsolete messages

• ConfigData.cs:171: [Obsolete("Migrated to notification subjects in version 9.5.5.")] — this release is 9.5.3 (inventory/group_vars/all.yml).
• ConfigData.cs:454-468: the modDecomm* properties are marked [Obsolete("Use notification entries with NotificationClient.AppDecomm instead.")], but 9.5.3.sql migrates them to notification_client = 'InterfaceDecomm', the new enum member added in FwoNotification.cs:16. AppDecomm is a different client.

Documentation-only, but these messages are what the next maintainer will follow.

F27 — inline array arguments in a new test

ModellingConnectionHandlerDecommissionTest.cs:186 and :210 introduce inline arrays:

ReturnIdWrapper wrapper = new() { ReturnIds = new ReturnId[] { new ReturnId { InsertedId = connId } } };

CODING_GUIDELINES.md asks for a private static readonly field instead. UiEditNotificationsTest.cs:942-943, added in the same PR, already uses the compliant form. These are the only two occurrences on lines this PR adds.

Validation

Run in a detached worktree at 24e23bc5, so the checks below describe this head exactly:

• ✅ dotnet build --configuration Debug roles/FWO.sln — 0 warnings, 0 errors. Notably, the new [Obsolete] attributes produce no CS0618.
• ✅ dotnet format roles/FWO.sln --verify-no-changes --no-restore — clean.
• ✅ git diff --check upstream/develop...24e23bc5 — clean.
• ✅ 350 focused notification / email-helper / daily-check / decommission tests passed, 0 failed.
• ⏸️ Full unit suite not re-run this round; round eight reported 5,933 passed with three environment-dependent failures outside the changed area.
• ⏸️ Disposable-VM installer validation not executed — the required FWO MCP VM capabilities are not available in this session. 9.5.3.sql is verified by inspection only, which is why F19, F22 and F24 are stated as migration-outcome reasoning rather than observed upgrade behaviour.

Residual risk. F19, F20 and F24 are all properties of the migrated configuration and none of them is covered by a test: there is no test that runs 9.5.3.sql (F13, accepted) and none that drives CheckUnansweredInterfaceRequests with both an initial and a reminder InterfaceRequest notification present. The 350 green tests do not contradict these findings — they exercise the components, not the post-upgrade combination. A test that seeds both notification rows and asserts which one the daily job picks would close F19 and F20 together.

Review metadata

• Depth / capability: deep, once. Performed directly against the checklists in fwo-review-pr — both a correctness/quality pass and a separate security pass (auth and Hasura permission changes, injection boundaries, secrets in logs and errors, REST input validation, TLS/deserialization, tenant isolation on the new notification_log table and the new MonitorEmailLog page).
• Delegation: 3 of 6 permitted sub-agent dispatches, all on a reduced tier — PR review history, the coding-guideline mechanical scan, and the localization/help/What's-New/test-presence scan. The coding-guideline dispatch returned no usable evidence, so those checks were redone inline on the primary model; the file-size, inline-array and whitespace results above are from that inline run. Every defect judgment, the security pass, all ratings, the numbering and this comment are primary-model work.
• Usage budget: this session exposes no readable usage or cost meter, and no per-request token accounting was located on disk, so the token figure could not be measured and none is reported. The observable proxy limits were enforced instead and met: 3 of 6 dispatches, one deep pass, reads confined to the diff plus its direct callers, data contracts and tests, and no pass re-run.