From 3e9ad658d05ed3ca914600cc5225cfe44243600d Mon Sep 17 00:00:00 2001 From: indifferentketchup Date: Fri, 8 May 2026 20:42:48 +0000 Subject: [PATCH] audit week 3 [SEC-004 + SEC-005]: scope members.fetch + redact PII in debug logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [SEC-004] services/staffThread.js — addRoleMembersToThread previously called the unscoped guild.members.fetch() on every ticket creation, chunking every member of the guild. With STAFF_THREAD_AUTO_ADD_ROLE on and a 50-member staff role, the 300ms-per-add loop also blocked ticket creation for ~15s. - Read role.members directly (computed from guild.members.cache, kept in sync via the GuildMembers gateway intent set on the client). Skip the explicit unscoped fetch in the hot path. - Cache-cold fallback: one scoped guild.members.fetch({ withPresences: false }) — irrelevant presence sync stays off the wire. - createStaffThread no longer awaits the add-loop. Wraps the call in setImmediate(...) so ticket creation returns immediately while the rate-limited add-loop runs in the background. [SEC-005] services/debugLog.js — stacks/messages posted to the debug channel could leak customer email addresses (interpolated through ticket errors) and Discord member/channel IDs. Add a redactPII helper applied to both logError's message + stack and logWarn's body: - Email regex /[\w.+-]+@[\w.-]+\.\w+/g → [EMAIL_REDACTED] - Discord snowflake /\b\d{18,20}\b/g → [ID_REDACTED] interaction.user.tag in the User: line is intentionally not redacted — it's needed for triage and is not PII (Discord usernames are public). --- services/debugLog.js | 25 ++++++++++++++++++++++--- services/staffThread.js | 31 +++++++++++++++++++++++-------- 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/services/debugLog.js b/services/debugLog.js index fa75f38..0150fe6 100644 --- a/services/debugLog.js +++ b/services/debugLog.js @@ -11,6 +11,24 @@ function setClient(c) { client = c; } +// --- PII redaction --- + +// Email addresses (loose regex — covers most RFC 5321 local parts that show up +// in support traffic) and Discord snowflakes (18–20 digit numeric IDs) get +// redacted before stack/message text reaches the debug channel. Both can land +// in error stacks via senderEmail interpolation, channel IDs in error +// messages, etc. — redacting at the boundary keeps the debug channel useful +// for triage without leaking customer addresses or staff member IDs. +const EMAIL_REDACT_RE = /[\w.+-]+@[\w.-]+\.\w+/g; +const SNOWFLAKE_REDACT_RE = /\b\d{18,20}\b/g; + +function redactPII(s) { + if (s == null) return ''; + return String(s) + .replace(EMAIL_REDACT_RE, '[EMAIL_REDACTED]') + .replace(SNOWFLAKE_REDACT_RE, '[ID_REDACTED]'); +} + // --- Helpers --- async function sendToChannel(channelId, embed, overrideClient) { @@ -38,9 +56,10 @@ async function logError(context, error, interaction = null, overrideClient = nul const commandLine = (interaction?.commandName || interaction?.customId) ? `Command/Button: ${interaction.commandName || interaction.customId}\n` : ''; - const stack = (error.stack || error.message || String(error)).slice(0, 1500); + const message = redactPII(error.message || String(error)); + const stack = redactPII(error.stack || error.message || String(error)).slice(0, 1500); await channel.send({ - content: `\`[${context}]\` ${error.message || String(error)}\n${userLine}${commandLine}\n\`\`\`${stack}\`\`\`` + content: `\`[${context}]\` ${message}\n${userLine}${commandLine}\n\`\`\`${stack}\`\`\`` }); } catch (_) { // ignore send failures @@ -52,7 +71,7 @@ async function logError(context, error, interaction = null, overrideClient = nul async function logWarn(context, message, overrideClient = null) { const embed = new EmbedBuilder() .setTitle(`Warning: ${context}`) - .setDescription(String(message).slice(0, 4000)) + .setDescription(redactPII(String(message)).slice(0, 4000)) .setColor(0xFFFF00) .setTimestamp(); await sendToChannel(CONFIG.DEBUGGING_CHANNEL_ID, embed, overrideClient); diff --git a/services/staffThread.js b/services/staffThread.js index 52ed213..cd436da 100644 --- a/services/staffThread.js +++ b/services/staffThread.js @@ -11,9 +11,10 @@ * is logged via logWarn. * - invitable: false means only staff with MANAGE_THREADS can add additional * members — this is intentional for privacy. - * - guild.members.fetch() in addRoleMembersToThread can be slow on large - * servers. The 300ms delay between adds avoids the thread member add rate - * limit (approximately 5/second). + * - addRoleMembersToThread reads from role.members (cache-derived) and only + * falls back to a scoped guild.members.fetch on cache miss. The 300ms + * delay between adds avoids the thread member add rate limit (~5/sec). + * It runs via setImmediate so it doesn't block ticket creation. */ const { ChannelType } = require('discord.js'); const { CONFIG } = require('../config'); @@ -39,7 +40,11 @@ async function createStaffThread(channel, client) { }); if (CONFIG.STAFF_THREAD_AUTO_ADD_ROLE && CONFIG.STAFF_THREAD_ROLE_ID) { - await addRoleMembersToThread(thread, channel.guild, client); + // Run off the critical path — the add loop is rate-limited at 300ms per + // member and would block ticket creation for ~15s on a 50-member role. + setImmediate(() => { + addRoleMembersToThread(thread, channel.guild, client).catch(() => {}); + }); } return thread; @@ -55,16 +60,26 @@ async function createStaffThread(channel, client) { /** * Add all members of the staff role to the thread. + * + * Prefers role.members (computed from guild.members.cache, kept in sync via + * the GuildMembers gateway intent — see broccolini-discord.js intents). Only + * falls back to a scoped guild.members.fetch on cache miss (e.g. cold cache + * just after restart). Previously called the unscoped guild.members.fetch() + * on every ticket creation, which chunked all members of the guild — wasted + * gateway/REST budget and added ~15s to ticket creation on busy guilds. */ async function addRoleMembersToThread(thread, guild, client) { try { const role = await guild.roles.fetch(CONFIG.STAFF_THREAD_ROLE_ID).catch(() => null); if (!role) return; - await guild.members.fetch(); - const members = guild.members.cache.filter(m => - m.roles.cache.has(CONFIG.STAFF_THREAD_ROLE_ID) && !m.user.bot - ); + let members = role.members.filter(m => !m.user.bot); + if (members.size === 0) { + // Cache cold (first ticket after restart). withPresences: false skips + // the presence sync, which is irrelevant for thread-add and expensive. + await guild.members.fetch({ withPresences: false }).catch(() => {}); + members = role.members.filter(m => !m.user.bot); + } for (const [, member] of members) { await thread.members.add(member.id).catch(() => {});