diff --git a/Cargo.toml b/Cargo.toml index 3542690..aac0b0a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -9,7 +9,7 @@ poise = "0.6.1" reqwest = "0.12.24" serde_json = "1.0.145" serenity = "0.12" -tokio = { version = "1.21.2", features = ["macros", "rt-multi-thread"] } +tokio = { version = "1.21.2", features = ["macros", "rt-multi-thread", "sync"] } tracing-subscriber = { version = "0.3.20", features = ["env-filter"] } supabase-lib-rs = "0.5.3" serde = "1.0.228" \ No newline at end of file diff --git a/SECURITY.md b/SECURITY.md new file mode 100644 index 0000000..d2e281a --- /dev/null +++ b/SECURITY.md @@ -0,0 +1,83 @@ +# 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. The bot stores the id as **digits only** (it +strips the leading `s` and any punctuation before saving), so search the bare digits — +`'123456789'`, not `'s123456789'`: + +```sql +select * from dsec_discord_members where student_id = '123456789'; +``` + +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) — MERGE / DEPLOY GATE + +**This is a deploy gate, not optional.** `dsec_discord_members.student_id` MUST have a +`UNIQUE` constraint so one student id cannot be claimed by two Discord accounts. Run +the duplicate sweep and add the constraint (below) as part of shipping this change. + +The application also refuses the second claimant and catches a concurrent-insert unique +violation (SQLSTATE 23505), converting it to a generic refusal — but the check-then- +insert has an inherent race, so the database constraint is what actually guarantees +uniqueness. Until the constraint exists, two accounts verifying the same unused id at +the exact same moment can both succeed. The constraint 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..2405596 100644 --- a/src/commands/mods_only.rs +++ b/src/commands/mods_only.rs @@ -1,4 +1,8 @@ -use crate::{Context, Error}; +use crate::{ + AppState, ApplicationContext, Context, Error, + events::interaction_create::{DiscordMemberRow, user_attempt_lock}, + redact_digits, +}; use poise::{CreateReply, serenity_prelude as serenity}; /// Send message to logs channel @@ -138,3 +142,242 @@ pub async fn embed( Ok(()) } + +/// Whether a `dsec_discord_members` row exists for `discord_id`. Used as a read-back +/// to disambiguate a delete whose HTTP call errored (SEC-19 new #A). +async fn link_row_exists(state: &AppState, discord_id: &str) -> Result { + let rows: Vec = state + .supabase + .database() + .from("dsec_discord_members") + .select("discord_id") + .eq("discord_id", discord_id) + .execute() + .await?; + Ok(!rows.is_empty()) +} + +/// Build an ephemeral edit for the deferred `/unlink` response. +fn unlink_reply(title: &str, description: impl Into) -> serenity::EditInteractionResponse { + serenity::EditInteractionResponse::new().embed( + serenity::CreateEmbed::new() + .title(title) + .description(description), + ) +} + +/// 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. +/// +/// Uses `ApplicationContext` so it can `defer_response` and then EDIT that one deferred +/// ephemeral (poise's `ctx.send` after a defer posts a *followup* in this version, +/// leaving the "thinking…" placeholder dangling — SEC-19 #7b). +/// +/// AuthZ note: the `MANAGE_ROLES` gate is for command visibility only and is NOT +/// trusted for authorization — poise 0.6 treats a DM invoker as `Permissions::all()`, +/// so the gate passes in a DM. The command is `guild_only` and, before any database +/// work, asserts it is running in the DSEC guild specifically, which also stops a +/// moderator of some *other* guild the bot is in from deleting DSEC rows (SEC-19 #1). +#[poise::command(slash_command, guild_only, required_permissions = "MANAGE_ROLES")] +pub async fn unlink( + ctx: ApplicationContext<'_>, + #[description = "The member whose verification link should be removed"] user: serenity::User, +) -> Result<(), Error> { + let state = &ctx.data.state; + let serenity_ctx = ctx.serenity_context; + let interaction = ctx.interaction; + + // AuthZ, before anything else and before any DB op: must be the DSEC guild. + if interaction.guild_id != Some(state.guild_id) { + interaction + .create_response( + serenity_ctx, + serenity::CreateInteractionResponse::Message( + serenity::CreateInteractionResponseMessage::new() + .embed( + serenity::CreateEmbed::new() + .title("Unavailable here") + .description("This command can only be used in the DSEC server."), + ) + .ephemeral(true), + ), + ) + .await?; + return Ok(()); + } + + // Acknowledge within Discord's ~3s window before any DB/HTTP work. If the defer + // itself fails the interaction is dead — ABORT before any mutation (SEC-19 #7a). + if let Err(err) = ctx.defer_response(true).await { + eprintln!("[unlink] defer failed: {}", redact_digits(&err.to_string())); + return Ok(()); + } + + // Serialize against the TARGET user's own verification (SEC-19): this holds the + // same per-user async lock verification uses, keyed on the user being unlinked, so + // an in-flight verify for them cannot interleave with our delete + role removal and + // leave a role-with-no-row (or the inverse). Acquired AFTER defer so waiting on a + // running verify never eats the ack window; the tokio guard is held across the DB + // and Discord calls below. + let target_lock = user_attempt_lock(ctx.data, user.id); + let _target_guard = target_lock.lock().await; + + let user_id = user.id.to_string(); + let moderator = &interaction.user; + + // 1. Delete the link row FIRST. `.returning(...)` is required (COR-03 empty-204). + // On a delete error we must NOT assert "nothing changed": the DELETE may have + // committed before a response/body failure. Read the row back to disambiguate, + // and report UNKNOWN only if the read-back also fails (SEC-19 #A). Errors are + // redacted before printing and never shown to the user (SEC-19 #9). + let link_status: &str = match state + .supabase + .database() + .delete("dsec_discord_members") + .eq("discord_id", &user_id) + .returning("student_id,discord_id") + .execute::() + .await + { + Ok(rows) if rows.is_empty() => "no link row existed", + Ok(_) => "deleted", + Err(err) => { + eprintln!( + "[unlink] delete failed for target {}: {}", + user.id, + redact_digits(&err.to_string()) + ); + match link_row_exists(state, &user_id).await { + // Row still present: the delete genuinely did not happen. + Ok(true) => { + let _ = interaction + .edit_response( + serenity_ctx, + unlink_reply( + "Unlink failed", + "The database returned an error and the link row is still present (confirmed by read-back). Nothing changed. Try again, or remove it by hand — see SECURITY.md.", + ), + ) + .await; + let _ = log_embed( + serenity_ctx, + state.logs_channel_id, + Some("Unlink FAILED (no change)".to_string()), + None, + Some(format!( + "Moderator <@{}> (id `{}`) ran /unlink on <@{}> (id `{}`): the delete errored and a read-back confirms the row is still present. No changes made.", + moderator.id, moderator.id, user.id, user.id + )), + None, + None, + None, + None, + Some(true), + ) + .await; + return Ok(()); + } + // Row gone: the delete actually committed; fall through to role removal. + Ok(false) => "deleted (confirmed by read-back after a write error)", + // Read-back also failed: genuinely UNKNOWN — do not touch the role. + Err(err2) => { + eprintln!( + "[unlink] read-back after delete error failed for target {}: {}", + user.id, + redact_digits(&err2.to_string()) + ); + let _ = interaction + .edit_response( + serenity_ctx, + unlink_reply( + "Unlink status UNKNOWN", + "A database error occurred and a follow-up read could not confirm whether the link row was removed. It MAY already be gone. Verify by hand before relying on this — see SECURITY.md. The role was not touched.", + ), + ) + .await; + let _ = log_embed( + serenity_ctx, + state.logs_channel_id, + Some("Unlink UNKNOWN (database error)".to_string()), + None, + Some(format!( + "Moderator <@{}> (id `{}`) ran /unlink on <@{}> (id `{}`): the delete errored and a read-back also failed. The link row may or may not be removed; manual verification required. Role not touched.", + moderator.id, moderator.id, user.id, user.id + )), + None, + None, + None, + None, + Some(true), + ) + .await; + return Ok(()); + } + } + } + }; + + // 2. Then remove the verified role. We reach here only when the row is gone or + // never existed. Report clearly if THIS half fails so a human can finish it. + // Role errors are Discord API errors (no student id), safe to show the mod. + let role_id = state.verified_role_id; + let mut role_removed = false; + let mut role_error: Option = None; + match state.guild_id.member(serenity_ctx, user.id).await { + Ok(member) => match member.remove_role(serenity_ctx, role_id).await { + Ok(()) => role_removed = true, + Err(err) => role_error = Some(err.to_string()), + }, + Err(err) => role_error = Some(err.to_string()), + } + + // Report to the moderator by EDITING the deferred response (exactly one ephemeral, + // not a followup — SEC-19 #7b). Best-effort so a failed reply cannot skip the audit. + let mut summary = format!("Link row: {link_status}.\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") + )); + } + let _ = interaction + .edit_response(serenity_ctx, unlink_reply("Unlink", summary)) + .await; + + // Audit to the logs channel, naming the moderator — runs regardless of the reply. + let _ = log_embed( + serenity_ctx, + state.logs_channel_id, + Some("Verification link removed (/unlink)".to_string()), + None, + Some(format!( + "Moderator <@{}> (id `{}`) ran /unlink on <@{}> (id `{}`). Link row: {link_status}. Verified role: {}.", + moderator.id, + moderator.id, + user.id, + user.id, + if role_removed { + "removed" + } else { + "NOT removed — needs manual follow-up" + }, + )), + None, + None, + None, + None, + Some(true), + ) + .await; + + Ok(()) +} diff --git a/src/commands/verification.rs b/src/commands/verification.rs index f5e27b6..b972964 100644 --- a/src/commands/verification.rs +++ b/src/commands/verification.rs @@ -22,8 +22,15 @@ pub struct VerificationModal { } /// Embed message with verify button to verify membership +/// +/// `guild_only`: the verify button drives a handler that mutates live PII and grants +/// the DSEC role, so the button must never be posted into a DM. The handler itself +/// additionally asserts it is running in the configured DSEC guild before any query +/// (SEC-19 merge-blocker) — a foreign guild's copy of this button reaches Supabase +/// through nothing. #[poise::command( slash_command, + guild_only, required_permissions = "MANAGE_MESSAGES | MANAGE_THREADS" )] pub async fn verify(ctx: ApplicationContext<'_>) -> Result<(), Error> { diff --git a/src/events/interaction_create.rs b/src/events/interaction_create.rs index 5fa1ea5..afe01be 100644 --- a/src/events/interaction_create.rs +++ b/src/events/interaction_create.rs @@ -1,14 +1,21 @@ -use std::time::Duration; +use std::collections::HashMap; +use std::sync::{Arc, Mutex}; +use std::time::{Duration, Instant}; +use tokio::sync::Mutex as AsyncMutex; use crate::{ Data, Error, - commands::verification::{StudentRow, VerificationModal}, + commands::{ + mods_only::log_embed, + verification::{StudentRow, VerificationModal}, + }, + redact_digits, }; use ::serenity::{ all::{ - ComponentInteraction, Context, CreateEmbed, CreateEmbedFooter, CreateInteractionResponse, - CreateInteractionResponseMessage, GuildId, ModalInteraction, RoleId, - collector::ModalInteractionCollector, + ComponentInteraction, Context, CreateEmbed, CreateInteractionResponse, + CreateInteractionResponseMessage, EditInteractionResponse, GuildId, ModalInteraction, + RoleId, UserId, collector::ModalInteractionCollector, }, model::guild::Member, }; @@ -31,6 +38,101 @@ fn ephemeral_embed(embed: CreateEmbed) -> CreateInteractionResponse { ) } +/// The single verification-failure embed (SEC-19). +/// +/// Every failure path — student id not on the roster, name mismatch, rate-limit +/// refusal, and a student id already claimed by a different Discord account — +/// renders this exact embed. Because there is one constructor the responses cannot +/// drift apart, so the flow can no longer be used as an oracle for whether a given +/// student id holds an Active DSEC membership. +fn verification_failed_embed() -> CreateEmbed { + CreateEmbed::new().title("Verification failed").description( + "We couldn't verify that name and student ID. Check both and try again — it can take up to a week after signing up for your membership to appear.", + ) +} + +/// After this many failed attempts inside `ATTEMPT_WINDOW`, a user is refused with +/// no database round trip. Kept deliberately generous — a real member fixing a typo +/// in a hyphenated name must not be locked out — and the counter only moves on a +/// genuine failure (SEC-19). +const MAX_FAILURES: u32 = 5; +const ATTEMPT_WINDOW: Duration = Duration::from_secs(15 * 60); + +/// Per-Discord-user failed-verification counter: `(failures, window_start)`. +type AttemptMap = Mutex>; + +/// Whether `user_id` has already failed `MAX_FAILURES` times within the current +/// window. A window that has fully elapsed is treated as no failures. +/// +/// Recovers a poisoned lock rather than panicking: a poisoned `verify_attempts` +/// mutex must not turn every future verification into a panic (the map holds only +/// counters, never invariant-critical state). +fn is_rate_limited(attempts: &AttemptMap, user_id: UserId) -> bool { + let attempts = attempts.lock().unwrap_or_else(|p| p.into_inner()); + matches!( + attempts.get(&user_id), + Some((count, window_start)) + if *count >= MAX_FAILURES && window_start.elapsed() < ATTEMPT_WINDOW + ) +} + +/// Record one failed verification attempt for `user_id`. Fully-elapsed windows are +/// evicted first, which both resets a returning user's window and bounds the map so +/// it cannot grow without limit. +fn record_failure(attempts: &AttemptMap, user_id: UserId) { + let mut attempts = attempts.lock().unwrap_or_else(|p| p.into_inner()); + attempts.retain(|_, (_, window_start)| window_start.elapsed() < ATTEMPT_WINDOW); + match attempts.get_mut(&user_id) { + // Any surviving entry is within the window (retain kept it), so increment. + Some(entry) => entry.0 += 1, + None => { + attempts.insert(user_id, (1, Instant::now())); + } + } +} + +/// Get (or create) the per-user attempt lock. The std mutex guarding the map is +/// released before the caller awaits the returned tokio lock, so no std guard is +/// ever held across an await. Entries no attempt is using any more (only the map +/// still references them) are pruned so the map cannot grow without bound. +/// +/// Shared with `/unlink`, which acquires the lock keyed on its TARGET user so a +/// concurrent verify for that user cannot interleave with the delete + role removal +/// (SEC-19). Callers hold the returned tokio guard across their whole operation. +pub(crate) fn user_attempt_lock(data: &Data, user_id: UserId) -> Arc> { + let mut locks = data + .state + .verify_locks + .lock() + .unwrap_or_else(|p| p.into_inner()); + locks.retain(|_, lock| Arc::strong_count(lock) > 1); + Arc::clone( + locks + .entry(user_id) + .or_insert_with(|| Arc::new(AsyncMutex::new(()))), + ) +} + +/// Edit the deferred ephemeral response for a modal submission (SEC-19 #7: we +/// `defer_ephemeral` the moment the modal arrives, so every later reply is an edit). +async fn edit_reply( + ctx: &Context, + modal_submit: &ModalInteraction, + embed: CreateEmbed, +) -> Result<(), Error> { + modal_submit + .edit_response(ctx, EditInteractionResponse::new().embed(embed)) + .await?; + Ok(()) +} + +/// Whether a supabase error is a Postgres unique-constraint violation (SQLSTATE +/// 23505). Used only as a boolean signal — the error string embeds the student id +/// (`Key (student_id)=(…)`) and must never be logged (SEC-19 #4). +fn is_unique_violation(err: &supabase::Error) -> bool { + err.to_string().contains("23505") +} + /// Show the verification modal and wait for (and parse) the submission. /// /// Returns `None` when the user let the modal time out or the submission @@ -71,27 +173,61 @@ async fn collect_verification_modal( } } -/// Add to dsec_discord_members table (skips insert if already recorded). +/// Outcome of trying to link a Discord account to a student id. +enum LinkOutcome { + /// The row now exists for this Discord id (freshly inserted, or already present + /// with exactly this student id — an idempotent re-verify). + Linked, + /// The submitted student id is already held by a *different* Discord account; + /// nothing was written. Carries that other Discord id for the conflict audit. + RefusedIdClaimed { existing_discord_id: String }, + /// This caller's Discord account is already linked to a *different* student id, so + /// it may not claim another. The stale-row / hijack path (SEC-19 #2). + RefusedCallerLinked, + /// A `UNIQUE(student_id)` violation fired but the owner could not be resolved + /// afterwards (the winning row vanished, or the violation came from a different + /// constraint). This is an infrastructure refusal — never a success (SEC-19 #3). + RefusedUnresolved, +} + +/// Add to dsec_discord_members table. +/// +/// The ownership of the *submitted* id is always checked before anything is written +/// (SEC-19 #2): an idempotent re-verify is allowed only when this caller already owns +/// exactly that id. A caller already linked to a *different* id is refused, closing +/// the path where a stale COR-03 partial-insert row let an account claim someone +/// else's id. A concurrent-insert `UNIQUE` violation (23505) is caught and converted +/// to the same refusal (SEC-19 #3) — the constraint itself is an owner/deploy step. async fn add_dsec_discord_table( data: &Data, student_id: &str, member_id: &String, -) -> Result<(), Error> { - if member_recorded(data, member_id).await? { - return Ok(()); +) -> Result { + // 1. Who owns the SUBMITTED id right now? Always check before granting anything. + if let Some(existing_discord_id) = student_id_owner(data, student_id).await? { + if &existing_discord_id == member_id { + return Ok(LinkOutcome::Linked); // idempotent: caller already holds this id + } + return Ok(LinkOutcome::RefusedIdClaimed { + existing_discord_id, + }); } + // 2. Submitted id is unowned. If this caller already holds a DIFFERENT id, refuse: + // a linked account trying to claim a new student id is the hijack / stale-row path. + if member_recorded(data, member_id).await? { + return Ok(LinkOutcome::RefusedCallerLinked); + } + + // 3. Insert. `.returning(...)` makes supabase-lib-rs send `Prefer: + // return=representation`; without it PostgREST answers a POST with an empty + // `return=minimal` body that fails to deserialise, aborting before the role + // grant even though the row was written (COR-03). let new_member = serde_json::json!({ "student_id": student_id, "discord_id": member_id, }); - - // `.returning(...)` makes supabase-lib-rs send `Prefer: return=representation`. - // Without it PostgREST defaults a POST to `return=minimal` — a 201 with an - // empty body — and deserialising that empty body into Vec - // failed, aborting before the role grant even though the row was written. - // That is why verification failed on every member's first attempt (COR-03). - let _: Vec = data + let insert: supabase::Result> = data .state .supabase .database() @@ -99,35 +235,92 @@ async fn add_dsec_discord_table( .values(new_member)? .returning("student_id,discord_id") .execute() - .await?; + .await; - Ok(()) + match insert { + Ok(_) => Ok(LinkOutcome::Linked), + // A UNIQUE(student_id) violation means another account inserted the same id + // between our check and our insert. Re-query the owner and refuse — never + // surface the raw 23505 body, which echoes the student id (SEC-19 #3, #4). + // This must FAIL CLOSED: only a re-query that proves the winning row is ours + // grants the role. A different owner is a conflict; an owner that cannot be + // resolved (row already gone, or a violation from a different constraint) is an + // infrastructure refusal — never a silent success (SEC-19 #3). + Err(err) if is_unique_violation(&err) => match student_id_owner(data, student_id).await? { + Some(owner) if &owner == member_id => Ok(LinkOutcome::Linked), + Some(owner) => Ok(LinkOutcome::RefusedIdClaimed { + existing_discord_id: owner, + }), + None => Ok(LinkOutcome::RefusedUnresolved), + }, + Err(err) => Err(err.into()), + } } -/// Assign the verified role and send the success response. -async fn grant_verified_role( +/// The Discord id currently linked to `student_id`, if any. Selects only `discord_id` +/// (minimum columns — SEC-19 #4). +async fn student_id_owner(data: &Data, student_id: &str) -> Result, Error> { + #[derive(Deserialize)] + struct OwnerRow { + discord_id: String, + } + let rows: Vec = data + .state + .supabase + .database() + .from("dsec_discord_members") + .select("discord_id") + .eq("student_id", student_id) + .execute() + .await?; + Ok(rows.into_iter().next().map(|row| row.discord_id)) +} + +/// A name-matched attempt: try to link the account and grant the role. +/// +/// On any refusal (ownership conflict, caller already linked to a different id, or an +/// unresolved unique-violation) this records exactly one failed attempt **before** the +/// fallible Discord reply, so a refusal always counts even if the edit is lost (SEC-19 +/// #6); a successful grant counts none. The user sees the generic embed on every +/// refusal, and all replies edit the deferred ephemeral (SEC-19 #7). +async fn link_and_grant( ctx: &Context, data: &Data, modal_submit: &ModalInteraction, discord_member: &Member, student_id: &str, verified_role_id: RoleId, - via_cache: bool, ) -> Result<(), Error> { - add_dsec_discord_table(data, student_id, &discord_member.user.id.to_string()).await?; - discord_member.add_role(ctx, verified_role_id).await?; + let user_id = discord_member.user.id; + let member_id = user_id.to_string(); - let mut embed = CreateEmbed::new().title("Verified ✅").description(format!( - "You have been assigned the <@&{}> role!", - verified_role_id - )); - if via_cache { - embed = embed.footer(CreateEmbedFooter::new("⚡ via cache")); + match add_dsec_discord_table(data, student_id, &member_id).await? { + LinkOutcome::Linked => { + discord_member.add_role(ctx, verified_role_id).await?; + let embed = CreateEmbed::new().title("Verified ✅").description(format!( + "You have been assigned the <@&{}> role!", + verified_role_id + )); + edit_reply(ctx, modal_submit, embed).await?; + } + LinkOutcome::RefusedIdClaimed { + existing_discord_id, + } => { + record_failure(&data.state.verify_attempts, user_id); + log_link_conflict(ctx, data, &member_id, &existing_discord_id).await; + edit_reply(ctx, modal_submit, verification_failed_embed()).await?; + } + LinkOutcome::RefusedCallerLinked => { + record_failure(&data.state.verify_attempts, user_id); + log_caller_already_linked(ctx, data, &member_id).await; + edit_reply(ctx, modal_submit, verification_failed_embed()).await?; + } + LinkOutcome::RefusedUnresolved => { + record_failure(&data.state.verify_attempts, user_id); + log_unresolved_conflict(ctx, data, &member_id).await; + edit_reply(ctx, modal_submit, verification_failed_embed()).await?; + } } - - modal_submit - .create_response(ctx, ephemeral_embed(embed)) - .await?; Ok(()) } @@ -141,15 +334,13 @@ fn normalise_name(raw: &str) -> String { .join(" ") } -/// Trim, lower-case, strip a leading "s", and drop spaces so a pasted -/// "s123 456 789 " looks up as "123456789". +/// Reduce a submitted student id to digits only, so "s123456789", "S123-456-789" and +/// "123 456 789" all look up as "123456789". Keeping *only* digits (rather than just +/// stripping spaces and a leading "s") is deliberate: the normalised value is what +/// goes into the PostgREST query, so this guarantees no separator-punctuated form of +/// a submitted id can survive into an error URL and thence a log line (SEC-19 #4). fn normalise_student_id(raw: &str) -> String { - let lowered: String = raw - .chars() - .filter(|c| !c.is_whitespace()) - .collect::() - .to_lowercase(); - lowered.strip_prefix('s').unwrap_or(&lowered).to_string() + raw.chars().filter(|c| c.is_ascii_digit()).collect() } /// Whether the submitted name matches the roster name closely enough to be the @@ -193,25 +384,6 @@ fn name_matches(roster: &str, submitted: &str) -> bool { true } -/// Whether the cached name for `student_id` matches the submitted `name`. -fn cached_name_matches(data: &Data, student_id: &str, name: &str) -> bool { - let cache = data - .state - .student_cache - .lock() - .expect("Failed to get cache"); - match cache.get(student_id) { - Some(cached_name) => name_matches(cached_name, name), - None => false, - } -} - -/// Store the resolved student name in the cache (normalised for comparison). -fn cache_student(data: &Data, student_id: &str, full_name: &str) { - let mut cache = data.state.student_cache.lock().unwrap(); - cache.insert(student_id.to_string(), normalise_name(full_name)); -} - /// Look up a student by id in the database. async fn fetch_student(data: &Data, student_id: &str) -> Result, Error> { let student_data: Vec = data @@ -240,26 +412,138 @@ async fn member_recorded(data: &Data, user_id: &str) -> Result { Ok(!rows.is_empty()) } +/// Post an audit line to the logs channel: a fixed title, the Discord user id and a +/// timestamp only — NEVER the submitted name or student id. The logs channel is read +/// by humans and persists forever, so nothing a member typed into the modal may go +/// here (SEC-19). `title` and `body` are always fixed literals. +async fn log_verify_audit(ctx: &Context, data: &Data, title: &str, body: String) { + let _ = log_embed( + ctx, + data.state.logs_channel_id, + Some(title.to_string()), + None, + Some(body), + None, + None, + None, + None, + Some(true), + ) + .await; +} + +/// Log a failed verification attempt. The description is one fixed generic string: +/// "student id not found" and "name mismatch" must be indistinguishable in the log +/// too, so it cannot become a mod-visible membership oracle (SEC-19 #8). +async fn log_verification_failure(ctx: &Context, data: &Data, user_id: UserId) { + log_verify_audit( + ctx, + data, + "Verification failed", + format!("User <@{user_id}> (id `{user_id}`) — a verification attempt failed."), + ) + .await; +} + +/// Log that a user was refused because they are already at the attempt limit. +async fn log_rate_limited(ctx: &Context, data: &Data, user_id: UserId) { + log_verify_audit( + ctx, + data, + "Verification rate-limited", + format!("User <@{user_id}> (id `{user_id}`) — too many attempts; refused without a database query."), + ) + .await; +} + +/// Log that a caller already linked to a *different* student id tried to claim a new +/// one. Carries only the caller's Discord id — never a student id (SEC-19 #2). +async fn log_caller_already_linked(ctx: &Context, data: &Data, member_id: &str) { + log_verify_audit( + ctx, + data, + "Verification refused: account already linked", + format!( + "<@{member_id}> (id `{member_id}`) is already linked to a different student id and tried to claim another; refused." + ), + ) + .await; +} + +/// Log a unique-violation whose owner could not be resolved: the insert hit a +/// `UNIQUE` constraint but a follow-up owner lookup found no row, so the attempt was +/// refused rather than granted. Worth a mod's eye — it can indicate a race or a +/// constraint firing for a reason we did not expect (SEC-19 #3). +async fn log_unresolved_conflict(ctx: &Context, data: &Data, member_id: &str) { + log_verify_audit( + ctx, + data, + "Verification refused: unresolved unique violation", + format!( + "<@{member_id}> (id `{member_id}`) hit a unique-constraint violation whose owner could not be resolved; refused (not granted). Investigate if this recurs." + ), + ) + .await; +} + +/// Log a student-id link conflict to the logs channel with BOTH Discord ids and no +/// student id (SEC-19): someone tried to verify with a student id already linked to +/// a different Discord account. +async fn log_link_conflict( + ctx: &Context, + data: &Data, + attempting_discord_id: &str, + existing_discord_id: &str, +) { + log_verify_audit( + ctx, + data, + "Verification refused: student id already linked", + format!( + "<@{attempting_discord_id}> (id `{attempting_discord_id}`) tried to verify with a student id already linked to <@{existing_discord_id}> (id `{existing_discord_id}`)." + ), + ) + .await; +} + +// TODO(SEC-19 follow-up): name + student id is not proof of ownership — both are +// semi-public, so anyone who knows a classmate's name and id can verify as them. +// The real fix is a possession proof: email a one-time code to the address on the +// roster and require it back. dsec-app already owns OTP machinery, so the cheap +// version is this bot calling dsec-api rather than growing its own email sender. +// That is feature-sized work, tracked separately, not a patch to this handler. +// +// Owner-only DB step (NOT done here, needs a maintainer on live Supabase): sweep +// for duplicates, then add `UNIQUE` on dsec_discord_members.student_id. See +// SECURITY.md. The uniqueness check below is the application-level safety net until +// that constraint exists. +// /// Handle a click on the "verify" button: collect the modal, then verify the -/// submitted student id/name against the cache and database. +/// submitted student id/name against the database on every attempt. async fn handle_verify( ctx: &Context, component_interaction: &ComponentInteraction, data: &Data, ) -> Result<(), Error> { - let Some(guild_id) = component_interaction.guild_id else { + // AuthZ (SEC-19 merge-blocker): this handler queries live Supabase, inserts a link + // row over real PII, and grants the DSEC role — so it must run in the configured + // DSEC guild specifically, not merely "some guild". The bot may be in other guilds; + // a foreign `/verify` button must never reach the database. Bounce anything else + // before any query. (The button carries no state, so this is the only gate.) + let guild_id = data.state.guild_id; + if component_interaction.guild_id != Some(guild_id) { component_interaction .create_response( ctx, ephemeral_embed( CreateEmbed::new() .title("Unable to perform action") - .description("Action can only be performed in the DSEC server"), + .description("Verification can only be performed in the DSEC server."), ), ) .await?; return Ok(()); - }; + } let verified_role_id = data.state.verified_role_id; @@ -288,68 +572,71 @@ async fn handle_verify( return Ok(()); }; - // From here on we hold the modal-submit token, so the slower member fetch and - // database work below is no longer racing the button's ack window. Wrap that - // work so any failure (e.g. a database error) still sends the user an - // ephemeral message rather than leaving a dead "This interaction failed" - // interaction — poise's on_error cannot reach this modal submission (COR-03). let user_id = component_interaction.user.id; + // Acknowledge the modal submission within Discord's ~3s window BEFORE any database + // or logs-channel work, so a slow query can never leave a dead "This interaction + // failed" and a mutation can never happen with no ack (SEC-19 #7). Every later + // reply edits this deferred ephemeral response. If the defer itself fails the + // interaction is already dead — ABORT before any query so we never mutate state + // (insert a link row, grant a role) against an un-acknowledged interaction. + if let Err(err) = modal_submit.defer_ephemeral(ctx).await { + eprintln!( + "[verify] defer failed for interaction {}: {}", + modal_submit.id, + redact_digits(&err.to_string()) + ); + return Ok(()); + } + + // Serialize all verification work for THIS user, held across the whole attempt, so + // concurrent modal submits cannot each slip under the attempt limit or the + // uniqueness checks (SEC-19 #5). Acquired AFTER defer so waiting on it never eats + // the ack window; different users never contend. + let attempt_lock = user_attempt_lock(data, user_id); + let _attempt_guard = attempt_lock.lock().await; + + // Any failure inside still edits the deferred response rather than leaving the user + // stuck — poise's on_error cannot reach this modal submission (COR-03). let verify_result: Result<(), Error> = async { - let discord_member = GuildId::member(guild_id, ctx, user_id).await?; - - let student_id = normalise_student_id(&modal_data.student_id); - - if cached_name_matches(data, &student_id, &modal_data.name) { - grant_verified_role( - ctx, - data, - &modal_submit, - &discord_member, - &student_id, - verified_role_id, - true, - ) - .await?; + // Cap failed attempts before ANY database work (SEC-19 #3): a user already over + // the limit gets the generic embed and no query runs. + if is_rate_limited(&data.state.verify_attempts, user_id) { + log_rate_limited(ctx, data, user_id).await; + edit_reply(ctx, &modal_submit, verification_failed_embed()).await?; return Ok(()); } + let student_id = normalise_student_id(&modal_data.student_id); + + // `fetch_student` is the only query carrying `membership_status = "Active"`, + // and it runs on every attempt before any role grant (SEC-19): no cache + // shortcut can admit a member whose membership has since lapsed. let Some(student) = fetch_student(data, &student_id).await? else { - modal_submit - .create_response( - ctx, - ephemeral_embed( - CreateEmbed::new().title("Student ID not found!").description( - "Your student ID is not found.\nIt takes up to **a week** for your membership to be updated in the database since sign up.\nTry again later.", - ), - ), - ) - .await?; + record_failure(&data.state.verify_attempts, user_id); + log_verification_failure(ctx, data, user_id).await; + edit_reply(ctx, &modal_submit, verification_failed_embed()).await?; return Ok(()); }; - cache_student(data, &student_id, &student.full_name); - if name_matches(&student.full_name, &modal_data.name) { - grant_verified_role( + // Fetch the guild member only once we know we may grant the role. + // `link_and_grant` records its own failure (before its fallible reply) on a + // refused link, so a refusal always counts even if the edit is lost (#6). + let discord_member = GuildId::member(guild_id, ctx, user_id).await?; + link_and_grant( ctx, data, &modal_submit, &discord_member, &student_id, verified_role_id, - false, ) .await?; } else { - modal_submit - .create_response( - ctx, - ephemeral_embed(CreateEmbed::new().title("Name mismatch ❌").description( - "Your student ID is present, however the name does not match. Try again.", - )), - ) - .await?; + record_failure(&data.state.verify_attempts, user_id); + log_verification_failure(ctx, data, user_id).await; + edit_reply(ctx, &modal_submit, verification_failed_embed()).await?; } Ok(()) @@ -357,21 +644,25 @@ async fn handle_verify( .await; if let Err(err) = verify_result { - eprintln!("[verify] verification failed after modal submit: {err}"); - // Best-effort ephemeral error so the user does not see the generic - // "This interaction failed" with no way forward. - let _ = modal_submit - .create_response( - ctx, - ephemeral_embed( - CreateEmbed::new() - .title("Something went wrong") - .description( - "A maintainer has been notified. Please try again in a minute.", - ), - ), - ) - .await; + // Never print the raw error on the verify path: supabase/reqwest errors embed + // the PostgREST URL (…student_id=eq.) and a 23505 body echoes the id, both + // PII. Redact digit runs; the interaction id is the correlation ref (SEC-19 #4). + eprintln!( + "[verify] interaction {} failed: {}", + modal_submit.id, + redact_digits(&err.to_string()) + ); + let _ = edit_reply( + ctx, + &modal_submit, + CreateEmbed::new() + .title("Something went wrong") + .description(format!( + "A maintainer has been notified. Please try again in a minute. (ref: {})", + modal_submit.id + )), + ) + .await; } Ok(()) @@ -438,5 +729,54 @@ mod tests { assert_eq!(normalise_student_id("s123456789 "), "123456789"); assert_eq!(normalise_student_id("S123456789"), "123456789"); assert_eq!(normalise_student_id(" 123 456 789 "), "123456789"); + // Punctuated forms are reduced to digits only, so no separator survives into a + // query URL / log line (SEC-19 #4). + assert_eq!(normalise_student_id("s123-456-789"), "123456789"); + assert_eq!(normalise_student_id("123.456.789"), "123456789"); + } + + #[test] + fn rate_limits_after_max_failures() { + let attempts: AttemptMap = Mutex::new(HashMap::new()); + let user = UserId::new(1); + + // A fresh user is never limited. + assert!(!is_rate_limited(&attempts, user)); + + // The first MAX_FAILURES attempts are allowed through (they still hit the DB). + for _ in 0..MAX_FAILURES { + assert!(!is_rate_limited(&attempts, user)); + record_failure(&attempts, user); + } + + // The next attempt (the 6th, with MAX_FAILURES == 5) is refused with no query. + assert!(is_rate_limited(&attempts, user)); + + // A different user is unaffected. + assert!(!is_rate_limited(&attempts, UserId::new(2))); + } + + #[test] + fn redacts_ids_but_keeps_short_numbers() { + // Student id (9 digits) and Discord snowflake (19) are masked. + assert_eq!( + redact_digits("student_id=eq.123456789 for <@1234567890123456789>"), + "student_id=eq. for <@>" + ); + // A 23505 key detail is masked. + assert_eq!( + redact_digits("Key (student_id)=(220123456) already exists"), + "Key (student_id)=() already exists" + ); + // Punctuated ids are masked as one unit, not left half-visible (SEC-19 #4). + assert_eq!( + redact_digits("student_id=eq.123-456-789&x=1"), + "student_id=eq.&x=1" + ); + assert_eq!(redact_digits("id 123.456.789 seen"), "id seen"); + // Short runs (< 7 digits, e.g. the SQLSTATE code or v1.2.3) are preserved. + assert_eq!(redact_digits("code 23505"), "code 23505"); + assert_eq!(redact_digits("version v1.2.3"), "version v1.2.3"); + assert_eq!(redact_digits("no digits here"), "no digits here"); } } diff --git a/src/main.rs b/src/main.rs index a9da5ac..95de5d6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -1,10 +1,63 @@ use dotenv::dotenv; use poise::serenity_prelude as serenity; -use std::{collections::HashMap, sync::Mutex}; +use std::{ + collections::HashMap, + sync::{Arc, Mutex}, + time::Instant, +}; use supabase::prelude::Client; mod commands; mod events; +/// Mask any "numeric token" that contains 7+ ASCII digits, so student ids (9 digits) +/// and Discord snowflakes (17-19 digits) never reach a log line. A numeric token is a +/// run of digits optionally joined by single interior separators (`-` `.` `_`), so a +/// punctuated id — `123-456-789`, `123.456.789` — is masked as one unit, not left half +/// visible (SEC-19 #4). Supabase/reqwest error text embeds the PostgREST URL +/// (`…student_id=eq.`) and a `23505` body echoes the id in a `Key (…)=(…)` detail, +/// so any error printed on a request path must pass through here first. The 7-digit +/// floor keeps short diagnostic codes (SQLSTATE like `23505`, HTTP status, `v1.2.3`) +/// intact. Submitted student ids are already reduced to pure digits before any query, +/// so this is defence in depth; names never appear on these error paths, and +/// correlation ids are printed separately, never through this function. +pub(crate) fn redact_digits(input: &str) -> String { + const MIN_DIGITS: usize = 7; + const CONNECTORS: [char; 3] = ['-', '.', '_']; + let chars: Vec = input.chars().collect(); + let mut out = String::with_capacity(input.len()); + let mut i = 0; + while i < chars.len() { + if !chars[i].is_ascii_digit() { + out.push(chars[i]); + i += 1; + continue; + } + // Consume a numeric token: digits, plus a separator only when it sits directly + // between two digits. + let start = i; + let mut digit_count = 0usize; + while i < chars.len() { + if chars[i].is_ascii_digit() { + digit_count += 1; + i += 1; + } else if CONNECTORS.contains(&chars[i]) + && i + 1 < chars.len() + && chars[i + 1].is_ascii_digit() + { + i += 1; + } else { + break; + } + } + if digit_count >= MIN_DIGITS { + out.push_str(""); + } else { + out.extend(&chars[start..i]); + } + } + out +} + #[derive(Debug)] pub struct Data { pub state: AppState, @@ -18,7 +71,21 @@ type ApplicationContext<'a> = poise::ApplicationContext<'a, Data, Error>; #[derive(Debug)] pub struct AppState { pub supabase: Client, - pub student_cache: Mutex>, + // SEC-19: per-Discord-user failed-verification counter. `(failures, window_start)` + // keyed by user id; once `failures` hits the limit inside the window the verify + // handler refuses the attempt with no database round trip. The old + // `student_cache` was removed: it was consulted *before* the only query carrying + // the `membership_status = "Active"` filter, had no TTL and was never evicted, so + // in a long-lived container it was a stale-membership bypass. One query per verify + // is not a performance problem. + pub verify_attempts: Mutex>, + // SEC-19: one async lock per Discord user, held across a whole verification + // attempt so concurrent modal submits from the same user serialize and cannot each + // slip under the attempt limit / uniqueness checks. A tokio Mutex (not std) because + // the guard is held across awaits; the std Mutex here only guards the brief + // get-or-insert of the map and is never held across an await. Idle entries are + // pruned on access so the map cannot grow without bound. + pub verify_locks: Mutex>>>, // Parsed once at boot. Re-reading these per event means a config typo takes // down a handler at some random future moment instead of failing the deploy. pub guild_id: serenity::GuildId, @@ -98,7 +165,8 @@ impl AppState { Ok(Self { supabase: client, - student_cache: Mutex::new(HashMap::new()), + verify_attempts: Mutex::new(HashMap::new()), + verify_locks: Mutex::new(HashMap::new()), guild_id: serenity::GuildId::new(guild_id), honeypot_channel_id: serenity::ChannelId::new(honeypot_channel_id), leetcode_channel_id: serenity::ChannelId::new(leetcode_channel_id), @@ -136,33 +204,54 @@ async fn event_handler( /// too, but nothing surfaces it (no tracing subscriber, no log shipping — OPS-04), /// so an explicit handler is set (COR-03). async fn on_error(error: poise::FrameworkError<'_, Data, Error>) { - if let poise::FrameworkError::Command { ctx, .. } = &error { - let _ = ctx - .send( - poise::CreateReply::default() - .content( - "Something went wrong — a maintainer has been notified. Please try again in a minute.", - ) - .ephemeral(true), - ) - .await; - } - if let Err(e) = poise::builtins::on_error(error).await { - eprintln!("[on_error] failed while handling a framework error: {e}"); + match error { + poise::FrameworkError::Command { ctx, error, .. } => { + // Print a redacted line ourselves and reply with a fixed generic ephemeral. + // We deliberately do NOT forward this to `poise::builtins::on_error`: its + // `Command` arm does a non-ephemeral `ctx.say(raw_error)`, which would leak + // Supabase/DB error text (including student ids) into the channel (SEC-19). + eprintln!( + "[on_error] command '{}' failed: {}", + ctx.command().name, + redact_digits(&error.to_string()) + ); + let _ = ctx + .send( + poise::CreateReply::default() + .content( + "Something went wrong — a maintainer has been notified. Please try again in a minute.", + ) + .ephemeral(true), + ) + .await; + } + other => { + if let Err(e) = poise::builtins::on_error(other).await { + eprintln!("[on_error] failed while handling a framework error: {e}"); + } + } } } #[tokio::main] async fn main() { - // Initialise logging first, before anything can log. Default to `info`: - // supabase-lib-rs logs generated query URLs (containing student IDs) and the - // service-account email at `debug`, so RUST_LOG must never be set to debug or - // trace on the VPS. See OPS-04 and the README. - tracing_subscriber::fmt() - .with_env_filter( - tracing_subscriber::EnvFilter::try_from_default_env().unwrap_or_else(|_| "info".into()), - ) - .init(); + // Initialise logging first, before anything can log. Default to `info`, then floor + // the supabase crate at `info` *regardless of RUST_LOG*. supabase-lib-rs logs the + // generated query URL — `…student_id=eq.` — and the service-account email via + // `tracing::debug!` (its target is `supabase`, its [lib] name). That bypasses our + // own `redact_digits`, so an operator setting `RUST_LOG=debug` to debug something + // else would leak student ids. `add_directive` replaces any same-target directive + // parsed from RUST_LOG, so this wins over `RUST_LOG=debug` and even an explicit + // `RUST_LOG=supabase=debug` (only a hyper-specific `supabase::database=debug` could + // override it). Our own modules keep their normal level. See OPS-04 and the README. + let env_filter = tracing_subscriber::EnvFilter::try_from_default_env() + .unwrap_or_else(|_| tracing_subscriber::EnvFilter::new("info")) + .add_directive( + "supabase=info" + .parse() + .expect("static tracing directive `supabase=info` is valid"), + ); + tracing_subscriber::fmt().with_env_filter(env_filter).init(); dotenv().ok(); // load env @@ -186,6 +275,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| {