mirror of
https://github.com/dsec-hub/dsec-discord-bot.git
synced 2026-09-22 07:44:26 +00:00
COL-BOT-01: add /unlink to undo hijacked verifications + manual runbook
Add a moderator-gated /unlink <user> 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE
This commit is contained in:
parent
959596aff5
commit
c41f478b1b
3 changed files with 178 additions and 1 deletions
75
SECURITY.md
Normal file
75
SECURITY.md
Normal file
|
|
@ -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 = '<discord user 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.
|
||||
|
|
@ -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<DiscordMemberRow> = 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<String> = 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(())
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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| {
|
||||
|
|
|
|||
Loading…
Reference in a new issue