From 81b519436cc4af1321cc2a3731e097f12710baf1 Mon Sep 17 00:00:00 2001 From: Clupai8o0 Date: Sun, 30 Aug 2026 18:25:17 +1000 Subject: [PATCH] SEC-19: serialize /unlink against verify + floor supabase logging config-independently MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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.) 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 Claude-Session: https://claude.ai/code/session_017XrE7F9ZuBWdQnS8CZvYDE --- src/commands/mods_only.rs | 12 +++++++++++- src/events/interaction_create.rs | 6 +++++- src/main.rs | 26 +++++++++++++++++--------- 3 files changed, 33 insertions(+), 11 deletions(-) diff --git a/src/commands/mods_only.rs b/src/commands/mods_only.rs index e42c1df..2405596 100644 --- a/src/commands/mods_only.rs +++ b/src/commands/mods_only.rs @@ -1,5 +1,6 @@ use crate::{ - AppState, ApplicationContext, Context, Error, events::interaction_create::DiscordMemberRow, + AppState, ApplicationContext, Context, Error, + events::interaction_create::{DiscordMemberRow, user_attempt_lock}, redact_digits, }; use poise::{CreateReply, serenity_prelude as serenity}; @@ -219,6 +220,15 @@ pub async fn unlink( 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; diff --git a/src/events/interaction_create.rs b/src/events/interaction_create.rs index 7c24054..afe01be 100644 --- a/src/events/interaction_create.rs +++ b/src/events/interaction_create.rs @@ -95,7 +95,11 @@ fn record_failure(attempts: &AttemptMap, user_id: UserId) { /// 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. -fn user_attempt_lock(data: &Data, user_id: UserId) -> Arc> { +/// +/// 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 diff --git a/src/main.rs b/src/main.rs index 784d9d3..95de5d6 100644 --- a/src/main.rs +++ b/src/main.rs @@ -235,15 +235,23 @@ async fn on_error(error: poise::FrameworkError<'_, Data, Error>) { #[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