feat(chats): align EditChat form + add per-channel Remove buttons (#1271)

[GonzoSpire]

Summary

Closes #1271. Two rough edges in the unified EditChat form shipped in #1262:

1. Field alignment — EnableMessages switch and MessagesDelay input used col-4 while every sibling row used col-6, causing them to render shifted left. Both changed to col-6 in EditChat.cshtml:73,83.

2. Per-channel Remove buttons — previously the only destructive action was the header "Remove chat" link, which deletes the whole Chat. Now each configured channel (Telegram / Slack / Mattermost) has its own Remove button in the footer button group on the right, next to the existing Send-test buttons. Each button clears only its channel; the Chat row, folder bindings, and other channels stay intact.

Backend

• ChatUpdate gains ClearTelegramBinding / ClearSlackWebhook / ClearMattermostWebhook nullable-bool flags. The existing ?? coalescing in Chat.ApplyUpdate swallows nulls (a null update value means "don't change"), so submitting an empty webhook through EditChat cannot clear a saved value today — these explicit flags route around that.
• Chat.TelegramType and Chat.AuthorizationTime move from init to internal set so ApplyUpdate can null them on clear (existing object initializers in ChatsManager.TryConnect and ChatsManagerTests still compile via InternalsVisibleTo). Chat.ToEntity() already gated the Telegram block on TelegramType.HasValue, so the cleared fields drop from the entity on round-trip.
• Three new NotificationsController endpoints (RemoveTelegramBinding / ClearSlackWebhook / ClearMattermostWebhook), each [HttpPost] + [TelegramRoleFilterById(nameof(id), ProductRoleEnum.ProductManager)] (same auth as RemoveChat). Routes through ChatsManager.TryUpdate so the Updated event fires for NotificationsCenter/SlackNotificationChannel subscribers and _telegramChatIds index is rekeyed by the existing override.

Frontend (EditChat.cshtml)

• Alignment fix at lines 73, 83 (col-4 → col-6).
• Header "Remove" link relabeled to "Remove chat" to clarify vs. the new per-channel buttons.
• Footer button group extended with Remove Telegram / Remove Slack / Remove Mattermost buttons (channel-grouped with their Send-test sibling; btn-outline-danger to distinguish from btn-outline-secondary). Each is gated by isTelegramBound / Model.HasSlack / Model.HasMattermost so it only renders for configured channels.
• Single removeChannel(channelName, actionUrl, confirmText) JS helper mirrors the existing removeChat() pattern: showConfirmationModal() → AJAX POST → window.location.reload().
• HasMattermost already existed on ChatViewModel.cs:55 — issue body was wrong about needing to add it.

Test plan

• dotnet build src/server/HSMServer/HSMServer.csproj — 0 errors (33 pre-existing warnings)
• dotnet test --filter ChatsManagerTests — 12/12 pass (8 existing + 4 new):
  • TryUpdate_ClearSlackWebhook_FlagSetsFieldToNull
  • TryUpdate_ClearMattermostWebhook_FlagSetsFieldToNull
  • TryUpdate_ClearTelegramBinding_FlagClearsFieldsAndRekeysIndex
  • Chat_AfterClearTelegramBinding_ToEntityOmitsTelegramFields
• Full HSMServer.Core.Tests suite — 506/508 pass (the 2 failures are AgentInstallerBundleTests.StaticUninstallScript_MatchesGenerated, unrelated to this PR — they fail only when running from isolated -o /tmp/... output because RepoFile() can't walk up to the repo root; pass in normal CI)
• Playwright modify_folder_chat.spec.ts — 3 tests (existing 2 + new EditChat: per-channel Remove clears Slack webhook without deleting the chat). CI must confirm.
• Manual smoke (run server locally):
  • Open EditChat for a Slack-only chat → observe EnableMessages switch and MessagesDelay input align horizontally with Name/Description/SlackWebhookUrl/MattermostWebhookUrl
  • Click "Remove Slack" → confirm modal → form reloads with empty webhook field, "Remove Slack" button gone, chat row still present in Chats list
  • Open EditChat for a Telegram-bound chat → "Remove Telegram" clears Telegram binding; re-invite via bot works afterward
  • Header "Remove chat" still deletes the whole record

Out of scope (noted in issue)

• Accumulator flush on channel clear — when a webhook is cleared, the per-channel ChannelAccumulator may still hold buffered alerts. Chat.SlackAccumulator/TelegramAccumulator return null when the channel is cleared (Chat.cs:49-51), so dispatch already skips them; no incorrect delivery. Buffer is memory-only and bounded by the aggregation period. Defer to a follow-up.
• Mattermost notification channel implementation — the Remove-Mattermost button is wired and gated behind HasMattermost so it appears only if legacy/test data has the field set; stays hidden in normal use until the channel ships.

🤖 Generated with Claude Code

[github-actions]

PR #1272 Review — Per-channel Remove buttons & tabbed EditChat editor

Reviewed origin/master...feature/1271-chat-editor-alignment-channel-remove.

Overall this is a clean, well-tested change. The core mechanism — negative-clear flags on ChatUpdate that route around the ?? current merge semantics, plus internal set on the Telegram fields so ApplyUpdate can null them — is coherent and the inline comments explaining why are excellent. The Telegram-chat-id index rekeying on clear is correctly handled by the existing TryUpdate override (Chat.cs clears TelegramChatId → old-vs-new comparison in ChatsManager.TryUpdate removes the stale index entry), and the unit tests pin exactly the behaviors that would silently break (ToEntity omission, index rekey). Authorization is consistent with the surrounding endpoints. I found no blocking correctness bug.

Comments below, most important first.

---

1. Stored XSS via @Model.Name interpolated into JS template literals (security)

EditChat.cshtml interpolates @Model.Name into backtick template literals in the new code:

function removeChannel(channelName, actionUrl, confirmText) {
    showConfirmationModal(
        `Removing ${channelName} from chat '@Model.Name'`,   // line 235
        ...
// and lines 269, 277, 285:
`Telegram binding will be removed from chat '@Model.Name'. ...`

Razor's default HTML-encoder escapes < > & ' " but not `, $, {, }. Inside a JS template literal those are exactly the metacharacters that matter: a chat named ${alert(document.cookie)} (or one containing a backtick) breaks out and executes arbitrary JS in the viewer's session when the handler runs. For Telegram-bound chats the Name is synced from the bot (i.e. from an externally-controlled group title), which makes this reachable by someone other than the person opening the page.

Note this is not introduced by the PR — the pre-existing removeChat() (lines 214–216) has the same shape — but this PR adds four new instances of the pattern, and the PR itself ships an "XSS lock-down" autotest for a sibling view, so the codebase is clearly treating this class of bug seriously.

The repo already has the idiomatic fix in use elsewhere (@Json.Serialize(...) / @Html.Raw(JsonSerializer.Serialize(...)) in Views/Dashboards/*.cshtml and Views/Home/_MetaInfo.cshtml). Suggest hoisting the name once:

const chatName = @Json.Serialize(Model.Name);
// then: `Removing ${channelName} from chat '${chatName}'`

and using chatName in all five spots (including the existing removeChat, while you're here).

---

2. Mattermost tab: "Remove" button with no way to add (consistency)

The Mattermost input is disabled with "Mattermost delivery is not yet implemented," and there is no other UI path to set MattermostWebhookUrl. Yet the PR adds a full ClearMattermostWebhook endpoint, the #removeMattermost button (gated on Model.HasMattermost), and a unit test. Since HasMattermost can never become true through the app, this is dead UI/endpoint surface today. That's fine if it's intentional groundwork, but worth a one-line note in the PR/commit so a future reader doesn't hunt for the missing "add Mattermost" flow. (The unit test still has value as a regression pin.)

3. CSRF posture (informational)

The three new [HttpPost] endpoints have no antiforgery validation and the AJAX calls send no token. This matches the rest of the app (RemoveChat and every other POST here behave identically, and no global antiforgery filter is configured), so it's not a regression — flagging only so it's on record that these state-changing POSTs inherit the existing app-wide gap.

4. Minor nits

• NotificationsController.cs: double blank lines introduced (before ClearChannel, and after ClearMattermostWebhook). Cosmetic.
• The new endpoints bind Guid id from the query string implicitly; the adjacent SendTestSlackMessage uses an explicit [FromQuery] Guid id. Both work for simple types, but matching the neighbor with [FromQuery] reads more intentionally.
• A clear request whose target field is already null still returns Ok, writes to the DB, and fires the Updated event. Harmless (idempotent), just a redundant round-trip.

---

Verdict: Solid, well-covered change. Recommend addressing #1 (the JS-string encoding) before merge since the fix is small and the pattern is already in the codebase; #2–#4 are non-blocking.