diff --git a/.env.example b/.env.example index 05f3f0a..1fe8a2a 100644 --- a/.env.example +++ b/.env.example @@ -5,6 +5,7 @@ VERIFIED_ROLE_ID="" GUILD_ID="" LOGS_CHANNEL_ID="" HONEYPOT_CHANNEL_ID="" +LEETCODE_CHANNEL_ID="" SUPABASE_URL="" SUPABASE_KEY="" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index abea2e6..68d4881 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -80,3 +80,14 @@ jobs: - name: Test run: cargo test + + # COL-BOT-05: configuration is parsed once in main.rs and stored on + # AppState. A per-event dotenv()/env::var read is a data race under the + # multi-threaded runtime (set_var is unsafe in edition 2024) and turns a + # config typo into a random future handler failure instead of a boot error. + - name: No environment reads outside main.rs + run: | + if grep -rn 'dotenv()\|env::var' src/ | grep -v '^src/main.rs:'; then + echo "::error::Environment variables must be read once in main.rs and stored on AppState." + exit 1 + fi diff --git a/.github/workflows/deploy.yml b/.github/workflows/deploy.yml index ae6e01f..e443264 100644 --- a/.github/workflows/deploy.yml +++ b/.github/workflows/deploy.yml @@ -42,7 +42,50 @@ jobs: working-directory: ${{ github.workspace }} run: | sudo docker compose up -d --build --force-recreate - echo "Deployment complete." + echo "Container created." + + # `docker compose up -d` exits 0 once the container is created, not once the + # program inside is working. And with `restart: unless-stopped` a container + # that panics at boot is restarted, so a crash loop still reads Running at any + # single instant — "Running" alone is not proof of a healthy deploy. Assert + # the bot actually reached READY (it logs "Logged in as ..." from the Ready + # handler) within a stability window AND has not restarted, and dump its logs + # and fail otherwise (OPS-04). + - name: Verify the bot came up cleanly + working-directory: ${{ github.workspace }} + run: | + id=$(sudo docker compose ps -q dsec_bot) + if [ -z "$id" ]; then + echo "::error::dsec_bot container was not created" + sudo docker compose logs --tail=200 dsec_bot || true + exit 1 + fi + + ready="" + for _ in $(seq 1 30); do + sleep 2 + running=$(sudo docker inspect -f '{{.State.Running}}' "$id" 2>/dev/null || echo "false") + restarts=$(sudo docker inspect -f '{{.RestartCount}}' "$id" 2>/dev/null || echo "0") + # A non-zero restart count means it has already crashed at least once: + # stop waiting and fail rather than let a later restart look healthy. + if [ "$restarts" != "0" ]; then + break + fi + if [ "$running" = "true" ] \ + && sudo docker compose logs dsec_bot 2>&1 | grep -q "Logged in as"; then + ready="yes" + break + fi + done + + running=$(sudo docker inspect -f '{{.State.Running}}' "$id" 2>/dev/null || echo "false") + restarts=$(sudo docker inspect -f '{{.RestartCount}}' "$id" 2>/dev/null || echo "0") + if [ "$ready" != "yes" ] || [ "$running" != "true" ] || [ "$restarts" != "0" ]; then + echo "::error::dsec_bot did not come up cleanly (running=$running restarts=$restarts ready=${ready:-no})" + sudo docker compose logs --tail=200 dsec_bot + exit 1 + fi + echo "Deployment complete: dsec_bot reached READY with no restarts." - name: Remove environment file if: always() diff --git a/Cargo.lock b/Cargo.lock index 04e9375..6a63aab 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1604,6 +1604,15 @@ dependencies = [ "winapi", ] +[[package]] +name = "matchers" +version = "0.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d1525a2a28c7f4fa0fc98bb91ae755d1e2d1505079e05539e35bc876b5d65ae9" +dependencies = [ + "regex-automata", +] + [[package]] name = "maybe-rayon" version = "0.1.1" @@ -3533,10 +3542,14 @@ version = "0.3.20" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2054a14f5307d601f88daf0553e1cbf472acc4f2c51afab632431cdcd72124d5" dependencies = [ + "matchers", "nu-ansi-term", + "once_cell", + "regex-automata", "sharded-slab", "smallvec", "thread_local", + "tracing", "tracing-core", "tracing-log", ] diff --git a/Cargo.toml b/Cargo.toml index c917e29..3542690 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -10,6 +10,6 @@ reqwest = "0.12.24" serde_json = "1.0.145" serenity = "0.12" tokio = { version = "1.21.2", features = ["macros", "rt-multi-thread"] } -tracing-subscriber = "0.3.20" +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/Dockerfile b/Dockerfile index 8a5dff4..3e32268 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,4 +1,8 @@ -FROM rust:trixie AS builder +# Pin the builder to the same Debian release as the runtime (bookworm). A binary +# built on a newer release can die at exec on the older one the day a dependency +# reaches for a newer glibc/OpenSSL symbol — with no log and, before OPS-04, no +# restart. Keep this in lockstep with the runtime FROM below. +FROM rust:1-bookworm AS builder # Install build dependencies RUN apt-get update && \ diff --git a/README.md b/README.md index 2d907e8..35c9f3f 100644 --- a/README.md +++ b/README.md @@ -59,6 +59,13 @@ docker-compose build docker-compose up ``` +## Logging + +The bot logs at `info` by default. Do **not** set `RUST_LOG` to `debug` or `trace` +on the VPS or in the `DOT_ENV` secret: at `debug` the Supabase client logs the +generated query URLs (which contain **student IDs**) and the service-account +email. Adjust the level with `RUST_LOG` locally only (e.g. `RUST_LOG=warn`). + ## Rules ### General Rules diff --git a/docker-compose.yml b/docker-compose.yml index ebb4f14..324bb9d 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -1,5 +1,12 @@ services: dsec_bot: build: . + # Bring the bot back on process exit — a panic, a bad Discord frame, or a VPS + # reboot — instead of leaving it dead until someone notices (OPS-04). No + # `healthcheck:` block: the runtime image is debian:bookworm-slim with only + # ca-certificates (no ps/curl) and the bot serves no HTTP port, so there is + # nothing to probe; `restart: unless-stopped` acts on process exit, which is + # exactly the failure mode here. Do not add one. + restart: unless-stopped env_file: - .env \ No newline at end of file diff --git a/src/commands/member_info.rs b/src/commands/member_info.rs index 6d9ccc2..a245739 100644 --- a/src/commands/member_info.rs +++ b/src/commands/member_info.rs @@ -66,46 +66,42 @@ pub async fn member_info(ctx: ApplicationContext<'_>) -> Result<(), Error> { let user_data_option = member_data(&database, &student_id).await?; - if let Some(user_data) = user_data_option { - let member_name = user_data.full_name; - let member_campus = user_data.campus; - let membership_status = user_data.membership_status; - let expiry_date = user_data.end_date; - + let Some(user_data) = user_data_option else { ctx.send( CreateReply::default() - .embed( - CreateEmbed::new() - .title("DSEC Membership Info") - .description(format!( - " + .embed(CreateEmbed::new().title("Couldn't find info").description( + "Your membership may have expired. Contact a club executive to be sure.", + )) + // Ephemeral like the other two replies: whether someone's + // membership has lapsed is their business, not the channel's. + .ephemeral(true), + ) + .await?; + return Ok(()); + }; + + let member_name = user_data.full_name; + let member_campus = user_data.campus; + let membership_status = user_data.membership_status; + let expiry_date = user_data.end_date; + + ctx.send( + CreateReply::default() + .embed( + CreateEmbed::new() + .title("DSEC Membership Info") + .description(format!( + " **Name:** {member_name} **Campus:** {member_campus} **Membership Status:** {membership_status} **Membership Expiry Date**: {expiry_date} " - )), - ) - .ephemeral(true), - ) - .await?; - } else { - ctx.send(CreateReply::default().embed( - CreateEmbed::new().title("Couldn't find info").description( - "Your membership may have expired. Contact a club executive to be sure.", - ), - )) - .await?; - } - - // let student_data = state - // .supabase - // .database() - // .from("active_members") - // .select("full_name, student_id") - // .eq("student_id", &student_id) - // .execute() - // .await?; + )), + ) + .ephemeral(true), + ) + .await?; Ok(()) } diff --git a/src/commands/mods_only.rs b/src/commands/mods_only.rs index 015bf93..799fe13 100644 --- a/src/commands/mods_only.rs +++ b/src/commands/mods_only.rs @@ -1,5 +1,4 @@ use crate::{Context, Error}; -use dotenv::dotenv; use poise::{CreateReply, serenity_prelude as serenity}; /// Send message to logs channel @@ -8,6 +7,7 @@ use poise::{CreateReply, serenity_prelude as serenity}; #[allow(clippy::too_many_arguments)] pub async fn log_embed( ctx: &serenity::Context, + logs_channel_id: serenity::ChannelId, title: Option, title_url: Option, description: Option, @@ -17,7 +17,6 @@ pub async fn log_embed( image_url: Option, timestamp: Option, ) -> Result<(), Error> { - dotenv().ok(); let mut embed = serenity::CreateEmbed::new(); // Set title and title URL @@ -60,16 +59,14 @@ pub async fn log_embed( embed = embed.timestamp(serenity::Timestamp::now()); } - // Send the embed - let logs_channel_id_env = std::env::var("LOGS_CHANNEL_ID") - .expect("missing LOGS_CHANNEL_ID") - .parse::() - .expect("Invalid LOGS_CHANNEL_ID value"); - let logs_channel_id = serenity::ChannelId::new(logs_channel_id_env); + // Send the embed (logs_channel_id is parsed once at boot; see AppState). let builder = serenity::CreateMessage::new().embed(embed); - let send_log = logs_channel_id.send_message(ctx, builder).await; - send_log.expect("LOG FAIL"); + // Do NOT .expect() here: this runs inside a message handler, and a panic on a + // transient Discord failure takes the whole handler down for that message. + if let Err(err) = logs_channel_id.send_message(ctx, builder).await { + eprintln!("[log_embed] failed to write to logs channel: {err}"); + } Ok(()) } diff --git a/src/commands/verification.rs b/src/commands/verification.rs index 3c534ee..f5e27b6 100644 --- a/src/commands/verification.rs +++ b/src/commands/verification.rs @@ -13,7 +13,7 @@ pub struct StudentRow { #[name = "Club Verification"] pub struct VerificationModal { #[name = "Full Name"] - #[placeholder = "John Doe"] + #[placeholder = "As it appears on your DUSA membership"] #[max_length = 50] pub name: String, #[name = "Student ID"] diff --git a/src/commands/weather.rs b/src/commands/weather.rs index 8b7dd02..ffb2e75 100644 --- a/src/commands/weather.rs +++ b/src/commands/weather.rs @@ -2,19 +2,25 @@ use crate::{Context, Error}; use poise::CreateReply; use serenity::{all::CreateEmbed, json::Value}; -async fn get_weather(location: String) -> Result { - let weather_api_key = std::env::var("WEATHER_TOKEN").expect("missing WEATHER_TOKEN"); - +/// Fetch the raw weather response for a location. Returns the HTTP status +/// alongside the parsed JSON body so the caller can tell a success payload from +/// an API error payload (e.g. an unknown location) without unwrapping anything +/// on external JSON. A body that is not valid JSON parses to `Value::Null`. +async fn get_weather( + location: &str, + weather_api_key: &str, +) -> Result<(reqwest::StatusCode, Value), Error> { let request_url = format!( "https://api.weatherapi.com/v1/current.json?key={key}&q={location}", key = weather_api_key, location = location ); - // retrieve weather data - let response = reqwest::get(request_url).await?.text().await?; - - Ok(response) + let response = reqwest::get(request_url).await?; + let status = response.status(); + let body = response.text().await?; + let value: Value = serde_json::from_str(&body).unwrap_or(Value::Null); + Ok((status, value)) } /// Shows weather information @@ -23,33 +29,83 @@ pub async fn weather( ctx: Context<'_>, #[description = "Location (City or Country)"] location: String, ) -> Result<(), Error> { - let weather_response = get_weather(location).await?; - let value: Value = serde_json::from_str(&weather_response)?; + // Weather is optional. Without a configured token the command is disabled + // rather than panicking on a missing key. + let Some(weather_api_key) = ctx.data().state.weather_token.as_deref() else { + ctx.send( + CreateReply::default() + .content("The weather command is not configured on this bot.") + .ephemeral(true), + ) + .await?; + return Ok(()); + }; - let location_name = value["location"]["name"].as_str().unwrap(); - let location_region = value["location"]["region"].as_str().unwrap(); - let location_country = value["location"]["country"].as_str().unwrap(); + let (status, value) = get_weather(&location, weather_api_key).await?; - let weather_condition = value["current"]["condition"]["text"].as_str().unwrap(); - let weather_temp = &value["current"]["temp_c"]; - let weather_feels_like = &value["current"]["feelslike_c"]; - let weather_wind_kph = &value["current"]["wind_kph"]; - let weather_humidity = &value["current"]["humidity"]; - let weather_cloud = &value["current"]["cloud"]; + // weatherapi.com returns a JSON error body (unknown location, bad key, quota) + // with a non-2xx status. Surface a friendly message instead of unwrapping + // fields that are not present in an error payload. + if !status.is_success() { + let message = value + .pointer("/error/message") + .and_then(Value::as_str) + .unwrap_or("Could not fetch the weather for that location."); + ctx.send( + CreateReply::default() + .content(format!("Weather lookup failed: {message}")) + .ephemeral(true), + ) + .await?; + return Ok(()); + } - let weather_icon = value["current"]["condition"]["icon"].as_str().unwrap(); + // Read every field defensively — external JSON is never unwrapped. + let text = |ptr: &str| -> String { + value + .pointer(ptr) + .and_then(Value::as_str) + .unwrap_or("Unknown") + .to_string() + }; + let number = |ptr: &str| -> String { + match value.pointer(ptr) { + Some(v) if !v.is_null() => v.to_string(), + _ => "?".to_string(), + } + }; - let embed = CreateEmbed::new() - .field("Name", location_name.to_string(), true) - .field("Region", location_region.to_string(), true) - .field("Country", location_country.to_string(), true) - .field("Condition", weather_condition.to_string(), true) - .field("Temperature", format!("{} °C", weather_temp), true) - .field("Feels like", format!("{} °C", weather_feels_like), true) - .field("Wind", format!("{} kph", weather_wind_kph), true) - .field("Humidity", format!("{}%", weather_humidity), true) - .field("Cloud", format!("{}%", weather_cloud), true) - .thumbnail(format!("https:{}", weather_icon)); + let mut embed = CreateEmbed::new() + .field("Name", text("/location/name"), true) + .field("Region", text("/location/region"), true) + .field("Country", text("/location/country"), true) + .field("Condition", text("/current/condition/text"), true) + .field( + "Temperature", + format!("{} °C", number("/current/temp_c")), + true, + ) + .field( + "Feels like", + format!("{} °C", number("/current/feelslike_c")), + true, + ) + .field("Wind", format!("{} kph", number("/current/wind_kph")), true) + .field( + "Humidity", + format!("{}%", number("/current/humidity")), + true, + ) + .field("Cloud", format!("{}%", number("/current/cloud")), true); + + // Only set the thumbnail when the API actually returned an icon URL — a + // placeholder would produce an invalid "https:Unknown" URL that Discord rejects. + if let Some(icon) = value + .pointer("/current/condition/icon") + .and_then(Value::as_str) + { + embed = embed.thumbnail(format!("https:{icon}")); + } ctx.send(CreateReply::default().embed(embed)).await?; Ok(()) diff --git a/src/events/interaction_create.rs b/src/events/interaction_create.rs index 1c04f06..5fa1ea5 100644 --- a/src/events/interaction_create.rs +++ b/src/events/interaction_create.rs @@ -12,7 +12,6 @@ use ::serenity::{ }, model::guild::Member, }; -use dotenv::dotenv; use poise::Modal as _; use poise::serenity_prelude as serenity; use serde::{Deserialize, Serialize}; @@ -23,16 +22,6 @@ pub struct DiscordMemberRow { pub discord_id: String, } -/// Read the verified role id from the environment. -fn verified_role_id() -> RoleId { - dotenv().ok(); - let role_id_string = std::env::var("VERIFIED_ROLE_ID").expect("missing VERIFIED_ROLE_ID"); - let role_id_u64: u64 = role_id_string - .parse() - .expect("Unable to parse VERIFIED_ROLE_ID into number"); - RoleId::new(role_id_u64) -} - /// Wrap an embed into an ephemeral interaction response. fn ephemeral_embed(embed: CreateEmbed) -> CreateInteractionResponse { CreateInteractionResponse::Message( @@ -97,12 +86,18 @@ async fn add_dsec_discord_table( "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 .state .supabase .database() .insert("dsec_discord_members") .values(new_member)? + .returning("student_id,discord_id") .execute() .await?; @@ -136,6 +131,68 @@ async fn grant_verified_role( Ok(()) } +/// Lower-case, trim, and collapse runs of internal whitespace to one space. +/// Used for every name comparison so a stray space or a double space in the +/// DUSA roster never rejects a real member. +fn normalise_name(raw: &str) -> String { + raw.to_lowercase() + .split_whitespace() + .collect::>() + .join(" ") +} + +/// Trim, lower-case, strip a leading "s", and drop spaces so a pasted +/// "s123 456 789 " looks up as "123456789". +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() +} + +/// Whether the submitted name matches the roster name closely enough to be the +/// same person. +/// +/// The first and last name tokens must BOTH match, and any tokens the student +/// typed in between must appear in the roster name in order — so an omitted +/// middle name is fine, but a single token, an arbitrary subset, a reordered +/// name, or a wrong surname is not. This is deliberately strict: verification is +/// already weak identity evidence (a name plus a student id), and a looser rule +/// would let a student id plus one common name token ("John", "Doe") claim the +/// verified role for someone else. +fn name_matches(roster: &str, submitted: &str) -> bool { + let roster = normalise_name(roster); + let submitted = normalise_name(submitted); + + let roster_words: Vec<&str> = roster.split_whitespace().collect(); + let submitted_words: Vec<&str> = submitted.split_whitespace().collect(); + + // A single token (or empty) is far too weak to identify a person, and a roster + // row without a distinct first and last name cannot be matched safely. + if submitted_words.len() < 2 || roster_words.len() < 2 { + return false; + } + + // The first and last name must both match. + if submitted_words.first() != roster_words.first() + || submitted_words.last() != roster_words.last() + { + return false; + } + + // Every token the student typed must appear in the roster name in order. + let mut idx = 0usize; + for &word in &submitted_words { + match roster_words[idx..].iter().position(|&w| w == word) { + Some(offset) => idx += offset + 1, + None => return false, + } + } + 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 @@ -144,15 +201,15 @@ fn cached_name_matches(data: &Data, student_id: &str, name: &str) -> bool { .lock() .expect("Failed to get cache"); match cache.get(student_id) { - Some(cached_name) => cached_name == &name.to_lowercase(), + Some(cached_name) => name_matches(cached_name, name), None => false, } } -/// Store the resolved student name in the cache (lower-cased for comparison). +/// 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(), full_name.to_lowercase()); + cache.insert(student_id.to_string(), normalise_name(full_name)); } /// Look up a student by id in the database. @@ -204,7 +261,7 @@ async fn handle_verify( return Ok(()); }; - let verified_role_id = verified_role_id(); + let verified_role_id = data.state.verified_role_id; // Fast, no-network "already verified" check using the member data that is // already attached to the button interaction. Anything slower than this @@ -231,66 +288,90 @@ 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. + // 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 discord_member = GuildId::member(guild_id, ctx, user_id).await?; - let input_student_id = modal_data.student_id.to_lowercase(); - let student_id = input_student_id - .strip_prefix("s") - .unwrap_or(&input_student_id); + let verify_result: Result<(), Error> = async { + let discord_member = GuildId::member(guild_id, ctx, user_id).await?; - 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?; - return Ok(()); + 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?; + return Ok(()); + } + + 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?; + return Ok(()); + }; + + cache_student(data, &student_id, &student.full_name); + + if name_matches(&student.full_name, &modal_data.name) { + grant_verified_role( + 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?; + } + + Ok(()) } + .await; - let Some(student) = fetch_student(data, student_id).await? else { - modal_submit + 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("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.", - ), + CreateEmbed::new() + .title("Something went wrong") + .description( + "A maintainer has been notified. Please try again in a minute.", + ), ), ) - .await?; - return Ok(()); - }; - - cache_student(data, student_id, &student.full_name); - - if student.full_name.to_lowercase() == modal_data.name.to_lowercase() { - grant_verified_role( - 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?; + .await; } Ok(()) @@ -311,3 +392,51 @@ pub async fn on_interaction_create( Ok(()) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn trims_and_collapses_names() { + assert!(name_matches("John Doe", " john doe ")); + assert!(name_matches("John Doe", "JOHN DOE")); + } + + #[test] + fn verifies_full_name_and_omitted_middle() { + assert!(name_matches("John Michael Doe", "John Michael Doe")); + assert!(name_matches("John Michael Doe", "John Doe")); + assert!(name_matches("John Michael Doe", "john michael doe")); + } + + #[test] + fn rejects_a_single_token() { + // A student id plus one common name token must never verify. + assert!(!name_matches("John Michael Doe", "John")); + assert!(!name_matches("John Michael Doe", "Doe")); + assert!(!name_matches("John Michael Doe", "Michael")); + assert!(!name_matches("John Doe", "John")); + } + + #[test] + fn rejects_wrong_surname_or_first_name() { + assert!(!name_matches("John Michael Doe", "John Smith")); + assert!(!name_matches("John Michael Doe", "Jane Doe")); + assert!(!name_matches("John Doe", "Jack Doe")); + } + + #[test] + fn rejects_a_different_person() { + assert!(!name_matches("John Michael Doe", "Jane Doe")); + assert!(!name_matches("John Doe", "Doe John")); + assert!(!name_matches("John Doe", "")); + } + + #[test] + fn normalises_student_ids() { + assert_eq!(normalise_student_id("s123456789 "), "123456789"); + assert_eq!(normalise_student_id("S123456789"), "123456789"); + assert_eq!(normalise_student_id(" 123 456 789 "), "123456789"); + } +} diff --git a/src/events/message.rs b/src/events/message.rs index 2f52edb..782d72c 100644 --- a/src/events/message.rs +++ b/src/events/message.rs @@ -1,91 +1,158 @@ -use crate::{Error, commands::mods_only::log_embed}; -use ::serenity::model::id::{ChannelId, GuildId}; -use dotenv::dotenv; +use crate::{Data, Error, commands::mods_only::log_embed}; use poise::serenity_prelude as serenity; -async fn honeypot(ctx: &serenity::Context, new_message: &serenity::Message) -> Result<(), Error> { - dotenv().ok(); - // check if it's the honeypot channel - let discord_guild_id = std::env::var("GUILD_ID") - .expect("missing GUILD_ID") - .parse::() - .expect("Invalid GUILD_ID value"); +/// Effective permissions for `member` in `channel_id`, honouring per-channel +/// permission overwrites. Returns `None` when the guild or the channel is not in +/// cache — the caller must treat that as "permissions could not be established" +/// and fail safe (do not ban), never as "no permissions". +fn channel_permissions( + ctx: &serenity::Context, + guild_id: serenity::GuildId, + channel_id: serenity::ChannelId, + member: &serenity::Member, +) -> Option { + let guild = ctx.cache.guild(guild_id)?; + let channel = guild.channels.get(&channel_id)?; + Some(guild.user_permissions_in(channel, member)) +} - let honeypot_channel_id_env = std::env::var("HONEYPOT_CHANNEL_ID") - .expect("missing HONEYPOT_CHANNEL_ID") - .parse::() - .expect("Invalid HONEYPOT_CHANNEL_ID value"); - let honeypot_channel_id = ChannelId::new(honeypot_channel_id_env); +async fn honeypot( + ctx: &serenity::Context, + new_message: &serenity::Message, + data: &Data, +) -> Result<(), Error> { + // Config is parsed once at boot and stored on AppState (COL-BOT-05). + let honeypot_channel_id = data.state.honeypot_channel_id; let current_channel_id = &new_message.channel_id; if honeypot_channel_id.eq(current_channel_id) { let user = &new_message.author; + + // Never ban a bot, a webhook, or ourselves. The honeypot exists to catch + // spam accounts; banning another club integration (or the bot itself) + // because it posted in the wrong channel is a self-inflicted outage. + if user.bot || new_message.webhook_id.is_some() || user.id == ctx.cache.current_user().id { + return Ok(()); + } + + // Never ban someone who can moderate, and fail SAFE: whenever the author's + // identity or permissions cannot be established, abstain rather than ban. + // A honeypot that occasionally misses a spammer is far cheaper than one + // that bans a moderator or a legitimate member. + let Some(message_guild_id) = new_message.guild_id else { + eprintln!("[honeypot] message has no guild_id; abstaining from ban"); + return Ok(()); + }; + let member = match message_guild_id.member(ctx, user.id).await { + Ok(member) => member, + Err(err) => { + eprintln!( + "[honeypot] could not fetch member {} to check permissions; abstaining: {err}", + user.id + ); + return Ok(()); + } + }; + // Effective permissions IN THE HONEYPOT CHANNEL (honouring overwrites). + let Some(perms) = channel_permissions(ctx, message_guild_id, *current_channel_id, &member) + else { + eprintln!( + "[honeypot] could not compute permissions for {} in {}; abstaining", + user.id, current_channel_id + ); + return Ok(()); + }; + if perms.ban_members() || perms.manage_messages() || perms.administrator() { + return Ok(()); + } + let username = &user.name; let user_id = &user.id; let avatar_url = user.avatar_url(); - let ban_user = GuildId::new(discord_guild_id) + let ban_user = data + .state + .guild_id .ban_with_reason(ctx, user_id, 2, "Message sent in honeypot channel.") .await; - if ban_user.is_ok() { - log_embed( - ctx, - Some("Honeypot activated!".to_string()), - None, - Some(format!("User got banned: {}", username)), - Some(format!("ID: {}", user_id)), - None, - avatar_url, - None, - Some(true), - ) - .await?; + match ban_user { + Ok(()) => { + log_embed( + ctx, + data.state.logs_channel_id, + Some("Honeypot activated!".to_string()), + None, + Some(format!("User got banned: {}", username)), + Some(format!("ID: {}", user_id)), + None, + avatar_url, + None, + Some(true), + ) + .await?; + } + // A honeypot that cannot ban is information the mods need. + Err(err) => { + eprintln!( + "[honeypot] failed to ban {username} ({user_id}) in the honeypot channel: {err}" + ); + } } } Ok(()) } +/// Discord rejects a thread name that is empty or over 100 characters, and the +/// resulting 400 is invisible to the person who posted (no logging is set up — +/// see OPS-04). So clamp here rather than letting the API reject it. +const MAX_THREAD_NAME_CHARS: usize = 100; +const DEFAULT_THREAD_NAME: &str = "LeetCode discussion"; + +/// Derive a thread name from a LeetCode post. +/// +/// Posts are conventionally numbered ("1. Two Sum"), so the leading two +/// characters are dropped. A first line of two characters or fewer leaves +/// nothing behind, and a very long one exceeds Discord's limit — both are +/// handled here rather than at the API. +fn create_title_from_message(message: &str) -> String { + let first = message.lines().next().unwrap_or(""); + let start = first + .char_indices() + .nth(2) + .map(|(i, _)| i) + .unwrap_or(first.len()); + let trimmed = first[start..].trim(); + + if trimmed.is_empty() { + return DEFAULT_THREAD_NAME.to_string(); + } + // Truncate on a CHARACTER boundary — byte slicing would panic mid-emoji. + trimmed.chars().take(MAX_THREAD_NAME_CHARS).collect() +} + async fn create_leetcode_thread( ctx: &serenity::Context, new_message: &serenity::Message, + data: &Data, ) -> Result<(), Error> { - dotenv().ok(); - - let leetcode_channel_id_env = std::env::var("LEETCODE_CHANNEL_ID") - .expect("missing LEETCODE_CHANNEL_ID") - .parse::() - .expect("Invalid LEETCODE_CHANNEL_ID value"); - - let leetcode_channel_id = ChannelId::new(leetcode_channel_id_env); + let leetcode_channel_id = data.state.leetcode_channel_id; let current_channel_id = &new_message.channel_id; if leetcode_channel_id.eq(current_channel_id) { - fn create_title_from_message(message: impl Into) -> String { - let message_string: String = message.into(); - - message_string - .lines() - .next() - .map(|line| { - let start = line - .char_indices() - .nth(2) - .map(|(i, _)| i) - .unwrap_or(line.len()); - &line[start..] - }) - .unwrap_or("") - .to_string() - } - let new_thread = serenity::CreateThread::new(create_title_from_message(&new_message.content)); - current_channel_id + if let Err(err) = current_channel_id .create_thread_from_message(ctx, new_message.id, new_thread) - .await?; - }; + .await + { + eprintln!( + "[leetcode] failed to create thread for message {}: {err}", + new_message.id + ); + } + } Ok(()) } @@ -93,8 +160,61 @@ async fn create_leetcode_thread( pub async fn on_message( ctx: &serenity::Context, new_message: &serenity::Message, + data: &Data, ) -> Result<(), Error> { - honeypot(ctx, new_message).await?; - create_leetcode_thread(ctx, new_message).await?; + // Independent features. A failure in one must not skip the other, and neither + // should propagate out of the event handler, where the default on_error only + // eprintln!s (OPS-04). + if let Err(err) = honeypot(ctx, new_message, data).await { + eprintln!("[on_message] honeypot failed: {err}"); + } + if let Err(err) = create_leetcode_thread(ctx, new_message, data).await { + eprintln!("[on_message] leetcode thread failed: {err}"); + } Ok(()) } + +#[cfg(test)] +mod tests { + use super::create_title_from_message; + + #[test] + fn strips_the_numbering_prefix() { + assert_eq!(create_title_from_message("1. Two Sum"), "Two Sum"); + } + + #[test] + fn empty_message_falls_back_to_the_default() { + assert_eq!(create_title_from_message(""), "LeetCode discussion"); + } + + #[test] + fn two_character_line_falls_back_to_the_default() { + // "hi" has nothing left after the two-character prefix is dropped, + // and Discord rejects an empty thread name with a 400. + assert_eq!(create_title_from_message("hi"), "LeetCode discussion"); + } + + #[test] + fn long_line_is_clamped_to_the_discord_limit() { + let long = format!("1. {}", "a".repeat(200)); + let got = create_title_from_message(&long); + assert_eq!(got.chars().count(), 100); + } + + #[test] + fn multibyte_prefix_does_not_panic_and_truncates_on_a_char_boundary() { + // A leading emoji is two chars wide in some fonts but one char here; + // byte-slicing this would panic. + let got = create_title_from_message("🔥 Daily challenge: reverse a linked list"); + assert!(!got.is_empty()); + } + + #[test] + fn only_the_first_line_is_used() { + assert_eq!( + create_title_from_message("1. Two Sum\nsome body text"), + "Two Sum" + ); + } +} diff --git a/src/main.rs b/src/main.rs index 8091baa..a9da5ac 100644 --- a/src/main.rs +++ b/src/main.rs @@ -19,6 +19,30 @@ type ApplicationContext<'a> = poise::ApplicationContext<'a, Data, Error>; pub struct AppState { pub supabase: Client, pub student_cache: 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, + pub honeypot_channel_id: serenity::ChannelId, + pub leetcode_channel_id: serenity::ChannelId, + pub verified_role_id: serenity::RoleId, + pub logs_channel_id: serenity::ChannelId, + // Optional: only /weather uses it. Read once here so no handler re-reads the + // process environment; `None` (missing or blank) simply disables /weather + // rather than stopping the bot from booting. + pub weather_token: Option, +} + +/// Read a required `u64` snowflake from the environment, recording the variable +/// name in `missing` (rather than panicking on the first one) so every offending +/// variable can be reported together. +fn required_u64(name: &str, missing: &mut Vec) -> u64 { + match std::env::var(name).ok().and_then(|v| v.parse::().ok()) { + Some(v) => v, + None => { + missing.push(name.to_string()); + 0 + } + } } impl AppState { @@ -27,6 +51,29 @@ impl AppState { #[allow(clippy::result_large_err)] pub async fn new() -> supabase::Result { dotenv().ok(); + + // Parse and validate every required id up front, collecting all failures + // into one actionable startup error instead of dying on the first one. + let mut missing: Vec = Vec::new(); + let guild_id = required_u64("GUILD_ID", &mut missing); + let honeypot_channel_id = required_u64("HONEYPOT_CHANNEL_ID", &mut missing); + let leetcode_channel_id = required_u64("LEETCODE_CHANNEL_ID", &mut missing); + let verified_role_id = required_u64("VERIFIED_ROLE_ID", &mut missing); + let logs_channel_id = required_u64("LOGS_CHANNEL_ID", &mut missing); + if !missing.is_empty() { + eprintln!( + "Missing or unparseable environment variables: {}.\nCopy .env.example to .env and fill them in.", + missing.join(", ") + ); + std::process::exit(1); + } + + // Non-fatal: the bot boots without a weather key; a missing or blank + // value becomes None and disables /weather rather than failing. + let weather_token = std::env::var("WEATHER_TOKEN") + .ok() + .filter(|t| !t.trim().is_empty()); + let supabase_url = std::env::var("SUPABASE_URL").expect("missing SUPABASE_URL"); let supabase_key = std::env::var("SUPABASE_KEY").expect("missing SUPABASE_KEY"); let supabase_user_email = @@ -52,6 +99,12 @@ impl AppState { Ok(Self { supabase: client, student_cache: 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), + verified_role_id: serenity::RoleId::new(verified_role_id), + logs_channel_id: serenity::ChannelId::new(logs_channel_id), + weather_token, }) } } @@ -70,15 +123,47 @@ async fn event_handler( events::interaction_create::on_interaction_create(ctx, interaction, data).await?; } serenity::FullEvent::Message { new_message } => { - events::message::on_message(ctx, new_message).await?; + events::message::on_message(ctx, new_message, data).await?; } _ => {} } Ok(()) } +/// Framework-level error handler. On a slash-command error it replies ephemerally +/// so the user is not left with a dead interaction, and it always runs poise's +/// default logging. `FrameworkOptions::default()` installs this default logger +/// 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}"); + } +} + #[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(); + dotenv().ok(); // load env let app_state = AppState::new() @@ -106,6 +191,7 @@ async fn main() { event_handler: |ctx, event, framework, data| { Box::pin(event_handler(ctx, event, framework, data)) }, + on_error: |error| Box::pin(on_error(error)), ..Default::default() })