From c41f478b1bede53b70aec6a52f0d8b0c8d30ee74 Mon Sep 17 00:00:00 2001 From: Clupai8o0 Date: Sun, 30 Aug 2026 17:11:08 +1000 Subject: [PATCH] COL-BOT-01: add /unlink to undo hijacked verifications + manual runbook Add a moderator-gated /unlink slash command (required_permissions = "MANAGE_ROLES", same gate as embed()). It deletes the dsec_discord_members row FIRST, then removes the verified role -- that order avoids leaving a member un-roled but still linked, the state that permanently breaks /member_info. The delete uses .returning(...) so PostgREST returns the row body (the COR-03 empty-204 gotcha) and so we can tell whether a link actually existed. It replies ephemerally with what it did and reports clearly if the role-removal half fails so a human can finish it, then writes a log_embed entry to the logs channel naming the acting moderator. Registered in main.rs. Add SECURITY.md: the moderator runbook for undoing a link (via /unlink and by hand), who holds the Supabase credentials, the fact that deleting the row does NOT revoke the Discord role (why /unlink does both), and the owner-only SEC-19 UNIQUE-constraint step. A member-facing /unverify is deliberately not added. The /member_info leave/trim/remove policy call is an owner decision and is intentionally not implemented here. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE --- SECURITY.md | 75 +++++++++++++++++++++++++++ src/commands/mods_only.rs | 103 +++++++++++++++++++++++++++++++++++++- src/main.rs | 1 + 3 files changed, 178 insertions(+), 1 deletion(-) create mode 100644 SECURITY.md diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..d41c944 --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,75 @@ +# Security runbook + +Operational notes for moderators and maintainers. This file is for the people who +run the bot, not for contributors setting up a dev environment (that is `README.md`). + +## Undoing a verification link (`/unlink`) + +The bot links a Discord account to a student id in the `dsec_discord_members` table +when someone verifies. Because verification only checks a **name + student id** pair — +both semi-public — a person can verify as someone else. When that happens, undo it. + +### Preferred: the `/unlink` command + +Run `/unlink @member` in the server. It requires the **Manage Roles** permission and: + +1. deletes the member's `dsec_discord_members` row, **then** +2. removes the verified role from them, +3. replies to you privately (ephemeral) with what it did, and +4. writes a log entry to the logs channel naming you as the moderator who ran it. + +It does the delete first and the role removal second **on purpose**: the reverse order +can leave someone un-roled but still linked, which permanently breaks `/member_info` +for them. If the role removal half fails, the reply and the log both say so — finish it +by hand (next section) and the row is already gone. + +### Manual removal (when `/unlink` cannot be used) + +You need the Supabase project credentials for this. **Who holds them:** the club +committee — ask in the committee Discord. (Historically the bot maintainer; TODO: name +the current holder here.) Do **not** paste real credentials into a chat or a ticket. + +Find the link row(s) for a student id: + +```sql +select * from dsec_discord_members where student_id = 's123456789'; +``` + +Delete a specific link by hand: + +```sql +delete from dsec_discord_members where discord_id = ''; +``` + +**Deleting the row does NOT revoke the Discord role.** Removing the row and stripping +the verified role are two separate actions — that is exactly why `/unlink` does both. +After a manual delete, also remove the verified role from the member in Discord (Server +Settings → Members, or right-click the member → Roles), or they keep their access with +no link. + +## Owner-only database step (SEC-19) + +`dsec_discord_members.student_id` should have a `UNIQUE` constraint so one student id +cannot be claimed by two Discord accounts. The application already refuses the second +claimant, but the durable fix is the constraint. It is **not** applied by any migration +in this repo — it touches live data and must be run by a maintainer. + +Sweep for existing duplicates first; the DDL fails if any exist: + +```sql +select student_id, count(*) from dsec_discord_members +group by student_id having count(*) > 1; +``` + +Resolve any duplicates by hand (decide which Discord account keeps each link), then: + +```sql +alter table dsec_discord_members add constraint dsec_discord_members_student_id_key unique (student_id); +``` + +There is no staging Supabase project — do the sweep and the `ALTER` with a second +maintainer watching. Expect it to start rejecting inserts that used to succeed. + +> The real fix for the weak identity check is a possession proof — email a one-time +> code to the roster address (dsec-app already owns OTP machinery; the bot would call +> dsec-api). That is feature-sized work tracked under SEC-19, not covered here. diff --git a/src/commands/mods_only.rs b/src/commands/mods_only.rs index 799fe13..814c84a 100644 --- a/src/commands/mods_only.rs +++ b/src/commands/mods_only.rs @@ -1,4 +1,4 @@ -use crate::{Context, Error}; +use crate::{Context, Error, events::interaction_create::DiscordMemberRow}; use poise::{CreateReply, serenity_prelude as serenity}; /// Send message to logs channel @@ -138,3 +138,104 @@ pub async fn embed( Ok(()) } + +/// Remove a member's verification link (COL-BOT-01). +/// +/// This is the moderator-gated undo for a hijacked verification. It deletes the +/// `dsec_discord_members` row FIRST and only then strips the verified role, because +/// the reverse order can leave a member un-roled but still linked — the exact state +/// that permanently breaks `/member_info` for them. If the role removal fails the +/// row is already gone, so the reply and the log both flag that a human must strip +/// the role by hand. A member-facing `/unverify` is deliberately NOT provided: a +/// self-service unlink would let a hijacker cover their tracks. +#[poise::command(slash_command, required_permissions = "MANAGE_ROLES")] +pub async fn unlink( + ctx: Context<'_>, + #[description = "The member whose verification link should be removed"] user: serenity::User, +) -> Result<(), Error> { + let state = &ctx.data().state; + let user_id = user.id.to_string(); + + // 1. Delete the link row FIRST. `.returning(...)` is required: without it + // PostgREST answers a DELETE with an empty 204 body that fails to deserialise + // (the COR-03 gotcha), and the returned rows also tell us whether a link + // actually existed. If this errors we stop before touching the role, so we + // never leave the member un-roled but still linked. + let deleted: Vec = state + .supabase + .database() + .delete("dsec_discord_members") + .eq("discord_id", &user_id) + .returning("student_id,discord_id") + .execute() + .await?; + let had_link = !deleted.is_empty(); + + // 2. Then remove the verified role. Report clearly if THIS half fails so a human + // can finish it — the row is already gone, so nothing else is inconsistent. + let role_id = state.verified_role_id; + let mut role_removed = false; + let mut role_error: Option = None; + match ctx.guild_id() { + Some(guild_id) => match guild_id.member(ctx.serenity_context(), user.id).await { + Ok(member) => match member.remove_role(ctx.serenity_context(), role_id).await { + Ok(()) => role_removed = true, + Err(err) => role_error = Some(err.to_string()), + }, + Err(err) => role_error = Some(err.to_string()), + }, + None => role_error = Some("command was not run in a guild".to_string()), + } + + // Ephemeral report to the moderator. + let mut summary = if had_link { + String::from("Deleted the verification link row.\n") + } else { + String::from("No verification link row existed (nothing to delete).\n") + }; + if role_removed { + summary.push_str("Removed the verified role."); + } else { + summary.push_str(&format!( + "⚠️ Could NOT remove the verified role — a human must remove <@&{role_id}> from <@{user_id}> by hand. Reason: {}.", + role_error.as_deref().unwrap_or("unknown error") + )); + } + + ctx.send( + CreateReply::default() + .embed(serenity::CreateEmbed::new().title("Unlink").description(summary)) + .ephemeral(true), + ) + .await?; + + // Log to the logs channel, naming the moderator who ran the command. + let moderator = ctx.author(); + log_embed( + ctx.serenity_context(), + state.logs_channel_id, + Some("Verification link removed (/unlink)".to_string()), + None, + Some(format!( + "Moderator <@{}> (id `{}`) ran /unlink on <@{}> (id `{}`). Link row: {}. Verified role: {}.", + moderator.id, + moderator.id, + user.id, + user.id, + if had_link { "deleted" } else { "none found" }, + if role_removed { + "removed" + } else { + "NOT removed — needs manual follow-up" + }, + )), + None, + None, + None, + None, + Some(true), + ) + .await?; + + Ok(()) +} diff --git a/src/main.rs b/src/main.rs index eb261fe..5b6fe20 100644 --- a/src/main.rs +++ b/src/main.rs @@ -193,6 +193,7 @@ async fn main() { commands::weather::weather(), commands::verification::verify(), commands::mods_only::embed(), + commands::mods_only::unlink(), commands::member_info::member_info(), ], event_handler: |ctx, event, framework, data| {