SEC-19 + COL-BOT-01: harden Discord verification + /unlink (#8)

* SEC-19: harden Discord verification (identical failures, attempt limits, id uniqueness)

Collapse both verification failure paths into one shared verification_failed_embed()
constructor so "not found" and "name mismatch" are byte-identical and can no longer be
used as an oracle for whether a student id holds an Active membership.

Remove the stale in-memory student_cache entirely: it was consulted before the only
query carrying membership_status = "Active", had no TTL and was never evicted, making it
a stale-membership bypass in a long-lived container. fetch_student now runs on every
verification attempt before any role grant.

Add a per-Discord-user failed-attempt counter in AppState
(Mutex<HashMap<UserId, (u32, Instant)>>). After 5 failures in 15 minutes an attempt is
refused with the generic embed and no database round trip. Each failure is logged to the
logs channel with the Discord user id and a timestamp only -- never the submitted name
or student id.

Add an application-level student-id uniqueness check in add_dsec_discord_table: if a
different discord_id already holds the student id, refuse with the generic embed and log
the conflict to the logs channel with both discord ids (not the student id).

The UNIQUE constraint DDL and duplicate sweep on live Supabase are an owner step (see
SECURITY.md); the emailed-OTP possession proof is noted as a follow-up TODO.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE

* 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

* SEC-19: fix 7 review defects in the verification hardening

Address adversarial-review findings on the verify path:

- Already-linked caller bypass (HIGH): add_dsec_discord_table no longer early-returns
  Linked on any existing row. It now checks the SUBMITTED id's owner first (idempotent
  only when that owner is the caller) and refuses a caller already linked to a DIFFERENT
  student id -- closing the path where a stale COR-03 partial-insert row let an account
  claim someone else's id.

- Check-then-insert race (HIGH): the insert now catches a UNIQUE(student_id) violation
  (SQLSTATE 23505), re-queries the owner and converts it to the generic refusal + audit.
  SECURITY.md now marks the live UNIQUE constraint (after the dup sweep) as a merge/deploy
  gate, not optional.

- PII leak in error prints (HIGH): raw supabase/reqwest errors (which embed
  student_id=eq.<id> and a 23505 Key detail) are never printed on the verify path. New
  redact_digits() masks 7+ digit runs; the interaction id is used as an opaque
  correlation ref. student_id_owner now selects only discord_id.

- Concurrent-attempt rate-limit bypass (HIGH): a per-UserId tokio Mutex (AppState.
  verify_locks) serializes a whole verification attempt so concurrent modal submits
  cannot each slip under the 5-in-15min limit. Requires tokio "sync" feature.

- Ownership conflict never counted (MED): a refused link now records exactly one failed
  attempt (success/infra-error record none), so a stolen-but-claimed credential can no
  longer loop the query set forever.

- Expired-interaction mutate-then-skip (MED): defer_ephemeral is now sent the instant the
  modal arrives, before any DB/logs work; every later reply edits the deferred response.

- Log oracle (MED): the failed-attempt audit line is one fixed generic string, so
  "not found" vs "name mismatch" are indistinguishable in the logs channel too.

Also: on_error no longer forwards a handled FrameworkError::Command to poise's builtin
(whose Command arm does a non-ephemeral ctx.say(raw_error), leaking DB error text); mutex
locks recover from poisoning instead of panicking; and the attempt/lock maps evict
stale/idle entries so they cannot grow unbounded.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE

* COL-BOT-01: fix /unlink auth, deferral and delete-failure audit (review)

Address adversarial-review findings on /unlink:

- Self-service-in-DM / cross-guild destructive (CRITICAL): the MANAGE_ROLES gate is not
  trusted for auth -- poise 0.6 treats a DM invoker as Permissions::all(), so it passed
  in a DM and let /unlink @self delete the row before any guild check. The command is now
  guild_only and, as the FIRST thing before any DB op, asserts ctx.guild_id() is the DSEC
  guild specifically (which also stops a moderator of another guild the bot is in from
  deleting DSEC rows). The gate is kept for command visibility only.

- Expired-interaction mutate-then-skip-audit (MED): defer_ephemeral is sent before any
  DB/HTTP work; the moderator reply is best-effort so a failed reply cannot skip the audit
  log, which now always runs.

- Delete failure skipped audit + could leak (MED): the row delete no longer uses `?`. A
  delete error is handled in-command with a sanitized ephemeral (no raw error) and a
  distinct failure audit line; the raw error is redacted (redact_digits) before it reaches
  stderr.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE

* SEC-19: close cross-guild verify exploit + 4 remaining review defects

MERGE-BLOCKER — verify flow was cross-guild exploitable: /verify was globally
registered and the button/modal handler accepted ANY guild, so a foreign guild's
copy of the button could reach live Supabase (insert a link row over real PII, then
fail the role grant) and hand an outsider a valid/invalid oracle while blocking the
real student. Fix: /verify is now guild_only, and handle_verify asserts the
interaction's guild == the configured DSEC guild as its FIRST action, before any
query — anything else is bounced with no DB round trip.

#3 fail-closed: after a 23505 the owner re-query now grants ONLY when the resolved
owner is the caller. A different owner is a conflict; an unresolved owner (winning
row vanished, or a violation from another constraint) is a new RefusedUnresolved
outcome — an infrastructure refusal, audited and never a silent grant.

#4 no submitted id in any log, in any format: normalise_student_id now reduces the
id to digits only, so no punctuated form (123-456-789) can survive into a query URL;
and redact_digits now masks separator-joined digit tokens (>=7 digits) as one unit,
not just contiguous runs. Two layers.

#6 a refused link always counts: record_failure moved INTO link_and_grant, before the
fallible Discord edit, so a lost/failed reply can no longer make a refusal count zero.

#7a verify defer failure now ABORTS before any DB/role mutation, so an expired modal
can never mutate state with no acknowledged interaction.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE

* COL-BOT-01: /unlink edits its deferred reply + disambiguates delete failures

#7b — one coherent reply: /unlink now takes ApplicationContext, defers via
defer_response(true), and EDITs that deferred ephemeral (create/edit at the serenity
layer). Previously ctx.send after a defer posted a followup in this poise version,
leaving the "thinking…" placeholder dangling. Defer failure now aborts before any DB
op (mirrors the verify fix).

new #A — no false "nothing changed": a DELETE whose HTTP call errors may still have
committed server-side. On a delete error /unlink now reads the row back: row present
-> genuinely failed, nothing changed; row gone -> the delete took effect, proceed to
role removal; read-back also fails -> report status UNKNOWN and do NOT touch the role,
telling the moderator to verify by hand. It never asserts "no change" on a bare error.

new #B — runbook SQL corrected: the bot stores student ids as digits only (strips the
leading s and punctuation), so the find-link query now searches '123456789', not
's123456789', with a note on the stored format.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE

* SEC-19: serialize /unlink against verify + floor supabase logging config-independently

1. Cross-command race: /unlink deleted the row and removed the role without the
   per-user lock that verification holds across its attempt, so the two could
   interleave into role-with-no-row (or the inverse) and /unlink could audit a removal
   the race had undone. Fix: /unlink now acquires the SAME per-user async lock
   (user_attempt_lock / verify_locks), keyed on the TARGET user's id, held across the
   delete + role removal — so it serializes against that user's own verification.
   user_attempt_lock is now pub(crate); it is a tokio Mutex (safe across awaits) and
   the std map guard is still released before the await, so no deadlock and no std
   lock held across an await.

2. "No submitted id in logs" made config-independent: supabase-lib-rs logs the full
   SELECT URL (student_id=eq.<id>) via tracing::debug! on target `supabase` (its [lib]
   name), which bypasses our redact_digits if an operator sets RUST_LOG=debug. The
   EnvFilter default now appends `supabase=info` via add_directive, which replaces any
   same-target directive from RUST_LOG — so supabase debug lines never emit even under
   RUST_LOG=debug or RUST_LOG=supabase=debug, while our own modules keep their level.

Left to the owner (documented deploy gate, non-blocking): the live UNIQUE(student_id)
constraint + dup sweep on Supabase.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE

---------

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

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"

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 = '<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) — 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.

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<bool, Error> {
+    let rows: Vec<serde_json::Value> = 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<String>) -> 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::<DiscordMemberRow>()
+        .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<String> = 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(())
+}

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> {

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,
+        ComponentInteraction, Context, CreateEmbed, CreateInteractionResponse,
-        CreateInteractionResponseMessage, GuildId, ModalInteraction, RoleId,
+        CreateInteractionResponseMessage, EditInteractionResponse, GuildId, ModalInteraction,
-        collector::ModalInteractionCollector,
+        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<HashMap<UserId, (u32, Instant)>>;
+
+/// 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<AsyncMutex<()>> {
+    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> {
+) -> Result<LinkOutcome, Error> {
-    if member_recorded(data, member_id).await? {
+    // 1. Who owns the SUBMITTED id right now? Always check before granting anything.
-        return Ok(());
+    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,
     });
-
+    let insert: supabase::Result<Vec<DiscordMemberRow>> = data
-    // `.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<DiscordMemberRow>
-    // 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<DiscordMemberRow> = 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.
+/// The Discord id currently linked to `student_id`, if any. Selects only `discord_id`
-async fn grant_verified_role(
+/// (minimum columns — SEC-19 #4).
+async fn student_id_owner(data: &Data, student_id: &str) -> Result<Option<String>, Error> {
+    #[derive(Deserialize)]
+    struct OwnerRow {
+        discord_id: String,
+    }
+    let rows: Vec<OwnerRow> = 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?;
+    let user_id = discord_member.user.id;
-    discord_member.add_role(ctx, verified_role_id).await?;
+    let member_id = user_id.to_string();
 
-    let mut embed = CreateEmbed::new().title("Verified ✅").description(format!(
+    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
             ));
-    if via_cache {
+            edit_reply(ctx, modal_submit, embed).await?;
-        embed = embed.footer(CreateEmbedFooter::new("⚡ via cache"));
+        }
+        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
+/// Reduce a submitted student id to digits only, so "s123456789", "S123-456-789" and
-/// "s123 456 789 " looks up as "123456789".
+/// "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
+    raw.chars().filter(|c| c.is_ascii_digit()).collect()
-        .chars()
-        .filter(|c| !c.is_whitespace())
-        .collect::<String>()
-        .to_lowercase();
-    lowered.strip_prefix('s').unwrap_or(&lowered).to_string()
 }
 
 /// 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<Option<StudentRow>, Error> {
     let student_data: Vec<StudentRow> = data
@@ -240,26 +412,138 @@ async fn member_recorded(data: &Data, user_id: &str) -> Result<bool, Error> {
     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;
 
-    let verify_result: Result<(), Error> = async {
+    // Acknowledge the modal submission within Discord's ~3s window BEFORE any database
-        let discord_member = GuildId::member(guild_id, ctx, user_id).await?;
+    // 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
-        let student_id = normalise_student_id(&modal_data.student_id);
+    // 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
-        if cached_name_matches(data, &student_id, &modal_data.name) {
+    // (insert a link row, grant a role) against an un-acknowledged interaction.
-            grant_verified_role(
+    if let Err(err) = modal_submit.defer_ephemeral(ctx).await {
-                ctx,
+        eprintln!(
-                data,
+            "[verify] defer failed for interaction {}: {}",
-                &modal_submit,
+            modal_submit.id,
-                &discord_member,
+            redact_digits(&err.to_string())
-                &student_id,
+        );
-                verified_role_id,
-                true,
-            )
-            .await?;
         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 {
+        // 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
+            record_failure(&data.state.verify_attempts, user_id);
-                .create_response(
+            log_verification_failure(ctx, data, user_id).await;
-                    ctx,
+            edit_reply(ctx, &modal_submit, verification_failed_embed()).await?;
-                    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?;
             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
+            record_failure(&data.state.verify_attempts, user_id);
-                .create_response(
+            log_verification_failure(ctx, data, user_id).await;
-                    ctx,
+            edit_reply(ctx, &modal_submit, verification_failed_embed()).await?;
-                    ephemeral_embed(CreateEmbed::new().title("Name mismatch ❌").description(
-                        "Your student ID is present, however the name does not match. Try again.",
-                    )),
-                )
-                .await?;
         }
 
         Ok(())
@@ -357,19 +644,23 @@ async fn handle_verify(
     .await;
 
     if let Err(err) = verify_result {
-        eprintln!("[verify] verification failed after modal submit: {err}");
+        // Never print the raw error on the verify path: supabase/reqwest errors embed
-        // Best-effort ephemeral error so the user does not see the generic
+        // the PostgREST URL (…student_id=eq.<id>) and a 23505 body echoes the id, both
-        // "This interaction failed" with no way forward.
+        // PII. Redact digit runs; the interaction id is the correlation ref (SEC-19 #4).
-        let _ = modal_submit
+        eprintln!(
-            .create_response(
+            "[verify] interaction {} failed: {}",
+            modal_submit.id,
+            redact_digits(&err.to_string())
+        );
+        let _ = edit_reply(
             ctx,
-                ephemeral_embed(
+            &modal_submit,
             CreateEmbed::new()
                 .title("Something went wrong")
-                        .description(
+                .description(format!(
-                            "A maintainer has been notified. Please try again in a minute.",
+                    "A maintainer has been notified. Please try again in a minute. (ref: {})",
-                        ),
+                    modal_submit.id
-                ),
+                )),
         )
         .await;
     }
@@ -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.<redacted> for <@<redacted>>"
+        );
+        // A 23505 key detail is masked.
+        assert_eq!(
+            redact_digits("Key (student_id)=(220123456) already exists"),
+            "Key (student_id)=(<redacted>) 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.<redacted>&x=1"
+        );
+        assert_eq!(redact_digits("id 123.456.789 seen"), "id <redacted> 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");
     }
 }

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.<id>`) 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<char> = 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("<redacted>");
+        } 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<HashMap<String, String>>,
+    // 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<HashMap<serenity::UserId, (u32, Instant)>>,
+    // 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<HashMap<serenity::UserId, Arc<tokio::sync::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,7 +204,17 @@ 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 {
+    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()
@@ -147,22 +225,33 @@ async fn on_error(error: poise::FrameworkError<'_, Data, Error>) {
                 )
                 .await;
         }
-    if let Err(e) = poise::builtins::on_error(error).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`:
+    // Initialise logging first, before anything can log. Default to `info`, then floor
-    // supabase-lib-rs logs generated query URLs (containing student IDs) and the
+    // the supabase crate at `info` *regardless of RUST_LOG*. supabase-lib-rs logs the
-    // service-account email at `debug`, so RUST_LOG must never be set to debug or
+    // generated query URL — `…student_id=eq.<id>` — and the service-account email via
-    // trace on the VPS. See OPS-04 and the README.
+    // `tracing::debug!` (its target is `supabase`, its [lib] name). That bypasses our
-    tracing_subscriber::fmt()
+    // own `redact_digits`, so an operator setting `RUST_LOG=debug` to debug something
-        .with_env_filter(
+    // else would leak student ids. `add_directive` replaces any same-target directive
-            tracing_subscriber::EnvFilter::try_from_default_env().unwrap_or_else(|_| "info".into()),
+    // 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
-        .init();
+    // 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| {