diff --git a/broccolini-discord.js b/broccolini-discord.js index e2b2c13..e000e93 100644 --- a/broccolini-discord.js +++ b/broccolini-discord.js @@ -165,6 +165,14 @@ client.on('messageCreate', async msg => { await handleDiscordReply(msg); }); +// HTTP server handles + readiness flag. Assigned inside the ready callback +// (httpServer, appReady) and the INTERNAL_API_SECRET branch below +// (internalServer); declared here so they're visible to the ready callback, +// the express middleware below, and the shutdown handler at the bottom. +let httpServer = null; +let internalServer = null; +let appReady = false; + client.once('ready', async () => { if (!process.env.MONGODB_URI) { console.error('MONGODB_URI is not set in .env. Broccolini Bot requires MongoDB.'); @@ -228,7 +236,7 @@ client.login(CONFIG.DISCORD_TOKEN); const app = express(); app.use(express.json()); // Reject API traffic with 503 until ready event has fired and routes are mounted. -let appReady = false; +// (appReady is declared at module top so the ready callback can flip it.) app.use((req, res, next) => { if (!appReady && req.path.startsWith('/api')) { return res.status(503).json({ error: 'Bot is starting; API not ready yet.' }); @@ -243,8 +251,6 @@ const internalApi = require('./routes/internalApi'); const internalApp = express(); internalApp.use('/internal', internalApi); -let httpServer = null; -let internalServer = null; if (CONFIG.INTERNAL_API_SECRET) { // Must bind all-interfaces inside the bot container: the settings-site is a // separate container on broccoli-net and reaches this API over the docker diff --git a/handlers/buttons.js b/handlers/buttons.js index c950066..8979e80 100644 --- a/handlers/buttons.js +++ b/handlers/buttons.js @@ -442,9 +442,11 @@ async function runFinalClose(interaction, ticket, sendEmail = true) { await sendTicketClosedEmail(ticket, closerDisplayName, interaction.user.id); } + // $unset welcomeMessageId so a future reopen on this thread doesn't carry + // a stale message ID pointing into the now-deleted channel. await Ticket.updateOne( { gmailThreadId: ticket.gmailThreadId }, - { $set: { discordThreadId: null, status: 'closed' } } + { $set: { discordThreadId: null, status: 'closed' }, $unset: { welcomeMessageId: '' } } ); if (transcriptMsg?.id) { diff --git a/handlers/commands/close.js b/handlers/commands/close.js index 05525d7..73582f0 100644 --- a/handlers/commands/close.js +++ b/handlers/commands/close.js @@ -66,9 +66,11 @@ async function finalizeForceClose(channelRef, clientRef) { if (!freshTicket || freshTicket.status === 'closed') return; try { + // $unset welcomeMessageId so a future reopen on this thread doesn't carry + // a stale message ID pointing into the now-deleted channel. await Ticket.updateOne( { gmailThreadId: freshTicket.gmailThreadId }, - { $set: { status: 'closed' } } + { $set: { status: 'closed' }, $unset: { welcomeMessageId: '' } } ); await enqueueSend(channelRef, 'Ticket force-closed. Archiving...'); diff --git a/handlers/messages.js b/handlers/messages.js index 50f8589..ae299ad 100644 --- a/handlers/messages.js +++ b/handlers/messages.js @@ -31,7 +31,11 @@ async function handleDiscordReply(m) { if (ticket.claimerId && !isStaffMember && m.author.id !== ticket.claimerId) { const dmEnabled = await getNotifyDm(ticket.claimerId); if (dmEnabled) { - const staffMember = await m.guild.members.fetch(ticket.claimerId).catch(() => null); + // Cache-first: GuildMembers intent keeps the cache populated; only fetch + // on miss (e.g. cold cache after restart). Avoids a REST round-trip on + // every customer reply in a busy ticket. + const staffMember = m.guild.members.cache.get(ticket.claimerId) + || await m.guild.members.fetch(ticket.claimerId).catch(() => null); if (staffMember) { const jumpLink = `https://discord.com/channels/${m.guild.id}/${m.channel.id}/${m.id}`; await staffMember diff --git a/routes/internalApi.js b/routes/internalApi.js index 019d19e..ebe8b62 100644 --- a/routes/internalApi.js +++ b/routes/internalApi.js @@ -19,6 +19,17 @@ const internalLimiter = rateLimit({ message: { error: 'Too many requests, please try again later.' } }); +// /restart calls process.exit; defense-in-depth tighter floor in case the +// shared INTERNAL_API_SECRET ever leaks. 2/min is enough for an operator- +// driven retry but not enough to crash-loop the container. +const restartLimiter = rateLimit({ + windowMs: 60 * 1000, + max: 2, + standardHeaders: true, + legacyHeaders: false, + message: { error: 'Too many restart attempts.' } +}); + router.use(internalLimiter); // Middleware: verify internal secret @@ -111,7 +122,7 @@ router.get('/discord/guild', async (req, res) => { // POST /restart — restart the bot process let scheduledRestart = null; -router.post('/restart', express.json(), (req, res) => { +router.post('/restart', restartLimiter, express.json(), (req, res) => { const { mode, scheduledFor } = req.body; if (mode === 'immediate') { diff --git a/services/configPersistence.js b/services/configPersistence.js index f3b6fb5..5911b42 100644 --- a/services/configPersistence.js +++ b/services/configPersistence.js @@ -150,7 +150,16 @@ function writeEnvFile(updates) { const roundtrip = readEnvFile(); if (roundtrip.size !== expected) { - throw new Error(`writeEnvFile: key count mismatch after write (expected ${expected}, got ${roundtrip.size})`); + const expectedKeys = new Set(updates.keys()); + const actualKeys = new Set(roundtrip.keys()); + const missing = [...expectedKeys].filter(k => !actualKeys.has(k)); + const extra = [...actualKeys].filter(k => !expectedKeys.has(k)); + throw new Error( + `writeEnvFile: key count mismatch after write ` + + `(expected ${expected}, got ${roundtrip.size})` + + (missing.length ? `. Missing: [${missing.join(', ')}]` : '') + + (extra.length ? `. Extra: [${extra.join(', ')}]` : '') + ); } } diff --git a/tests/utils.test.js b/tests/utils.test.js index 7cc67a6..c203370 100644 --- a/tests/utils.test.js +++ b/tests/utils.test.js @@ -44,6 +44,13 @@ describe('stripEmailQuotes', () => { const input = 'New reply.\r\nOn Monday Bob wrote:\r\n> quoted'; expect(stripEmailQuotes(input)).toBe('New reply.'); }); + + it('picks earliest cutoff when multiple markers match', () => { + // Earlier in the body: "On X wrote:". Later: "_____" underline. + // The earliest cutoff is the reply marker, not the underline. + const input = 'My new reply.\nOn Mon Bob wrote:\n> quoted text\n_____\nsignature'; + expect(stripEmailQuotes(input)).toBe('My new reply.'); + }); }); describe('stripMobileFooter', () => { diff --git a/utils.js b/utils.js index 8cce297..d5af413 100644 --- a/utils.js +++ b/utils.js @@ -112,22 +112,29 @@ function getCleanBody(payload) { function stripEmailQuotes(text) { let cleaned = text.replace(/\r\n/g, '\n'); + // Pick the earliest match across all markers, not just the first marker that + // matches anywhere. The previous order-dependent loop could truncate at a + // late "_____" signature underline even when an earlier "On X wrote:" reply + // header was the real cutoff. const markers = [ - /\n_{5,}\s*$/m, + /\nOn .* wrote:/i, /\nFrom:\s.*<.*@.*>/i, /\nSent:\s.*$/i, /\nTo:\s.*$/i, /\nSubject:\s.*$/i, - /\nOn .* wrote:/i + /\n_{5,}\s*$/m ]; + let earliest = -1; for (const m of markers) { const match = cleaned.match(m); - if (match) { - cleaned = cleaned.substring(0, match.index); - break; + if (match && (earliest === -1 || match.index < earliest)) { + earliest = match.index; } } + if (earliest !== -1) { + cleaned = cleaned.substring(0, earliest); + } return cleaned.trim(); }