mirror of
https://github.com/dsec-hub/dsec-discord-bot.git
synced 2026-09-22 07:44:26 +00:00
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
This commit is contained in:
parent
327a4962d1
commit
81b519436c
3 changed files with 33 additions and 11 deletions
|
|
@ -1,5 +1,6 @@
|
||||||
use crate::{
|
use crate::{
|
||||||
AppState, ApplicationContext, Context, Error, events::interaction_create::DiscordMemberRow,
|
AppState, ApplicationContext, Context, Error,
|
||||||
|
events::interaction_create::{DiscordMemberRow, user_attempt_lock},
|
||||||
redact_digits,
|
redact_digits,
|
||||||
};
|
};
|
||||||
use poise::{CreateReply, serenity_prelude as serenity};
|
use poise::{CreateReply, serenity_prelude as serenity};
|
||||||
|
|
@ -219,6 +220,15 @@ pub async fn unlink(
|
||||||
return Ok(());
|
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 user_id = user.id.to_string();
|
||||||
let moderator = &interaction.user;
|
let moderator = &interaction.user;
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -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
|
/// 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
|
/// 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.
|
/// still references them) are pruned so the map cannot grow without bound.
|
||||||
fn user_attempt_lock(data: &Data, user_id: UserId) -> Arc<AsyncMutex<()>> {
|
///
|
||||||
|
/// 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
|
let mut locks = data
|
||||||
.state
|
.state
|
||||||
.verify_locks
|
.verify_locks
|
||||||
|
|
|
||||||
26
src/main.rs
26
src/main.rs
|
|
@ -235,15 +235,23 @@ async fn on_error(error: poise::FrameworkError<'_, Data, Error>) {
|
||||||
|
|
||||||
#[tokio::main]
|
#[tokio::main]
|
||||||
async fn 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
|
dotenv().ok(); // load env
|
||||||
|
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue