From 1d43875e5a17726e91669c916829bcb348fd7b7f Mon Sep 17 00:00:00 2001 From: nmemmert Date: Thu, 16 Jul 2026 07:48:54 -0400 Subject: [PATCH] Fix 11 bugs: restore crash, draft leak, email failures, memory leaks Critical fixes: - sanitizeLoadedHitStats/VisitorStats: restore full state shape so a snapshot restore no longer crashes hit-counting middleware (missing byPathReal, byPathBot, byDayReal, byDayBot, botReasons, ipHashIndex) - /questions/share/:id: read state.questions only, not draft questions - inbound-email: validate date with Number.isFinite before toISOString - study-reminders: wrap each send in try/catch so one failure doesn't block remaining users; persist sent-markers after each success Security: - getClientIp: use req.ip (trust-proxy-resolved) instead of raw x-forwarded-for header to prevent IP spoofing - env-snapshot.env: delete immediately after backup tar stream ends so secrets don't linger on disk between exports Correctness / UX: - contact form: email failures no longer 500 the user after the submission is already saved; log and fall through instead - study-account profile: cap data URI avatar at 6 MB - admin enrollment PATCH: validate slug against study catalog - signup: return 503 at MAX_STUDY_USERS instead of silently dropping oldest accounts Memory leaks: - contactHits, downloadHits Maps: prune stale entries at 5000 entries - resendEmailSubmissionIndex: trim to 2000 entries (oldest first) Co-Authored-By: Claude Sonnet 4.6 --- server/data.js | 21 ++++++++++++++++++++- server/helpers.js | 6 ++---- server/routes/admin-assets.js | 7 ++++++- server/routes/admin-backup.js | 11 ++++++++++- server/routes/contact.js | 16 ++++++++++++++-- server/routes/downloads.js | 5 +++++ server/routes/inbound-email.js | 2 +- server/routes/public.js | 2 +- server/routes/study-account.js | 4 ++++ server/routes/study-auth.js | 8 +++++--- server/study-helpers.js | 9 ++++++++- 11 files changed, 76 insertions(+), 15 deletions(-) diff --git a/server/data.js b/server/data.js index c1ca487..3ea39a6 100644 --- a/server/data.js +++ b/server/data.js @@ -1058,21 +1058,40 @@ function sanitizeLoadedContactSubmissions(value) { function sanitizeLoadedHitStats(value) { return { totalHits: Number(value?.totalHits) || 0, + realHits: Number(value?.realHits) || 0, + botHits: Number(value?.botHits) || 0, firstHitAt: typeof value?.firstHitAt === 'string' ? value.firstHitAt : null, lastHitAt: typeof value?.lastHitAt === 'string' ? value.lastHitAt : null, byPath: value?.byPath && typeof value.byPath === 'object' ? value.byPath : {}, + byPathReal: value?.byPathReal && typeof value.byPathReal === 'object' ? value.byPathReal : {}, + byPathBot: value?.byPathBot && typeof value.byPathBot === 'object' ? value.byPathBot : {}, byDay: value?.byDay && typeof value.byDay === 'object' ? value.byDay : {}, + byDayReal: value?.byDayReal && typeof value.byDayReal === 'object' ? value.byDayReal : {}, + byDayBot: value?.byDayBot && typeof value.byDayBot === 'object' ? value.byDayBot : {}, + botReasons: value?.botReasons && typeof value.botReasons === 'object' ? value.botReasons : {}, } } function sanitizeLoadedVisitorStats(value) { + const loadedVisitors = value?.visitors && typeof value.visitors === 'object' ? value.visitors : {} + + let ipHashIndex = value?.ipHashIndex && typeof value.ipHashIndex === 'object' ? value.ipHashIndex : {} + if (Object.keys(ipHashIndex).length === 0 && Object.keys(loadedVisitors).length > 0) { + for (const [vid, visitor] of Object.entries(loadedVisitors)) { + if (visitor?.ipHash && typeof visitor.ipHash === 'string') { + ipHashIndex[visitor.ipHash] = vid + } + } + } + return { totalVisits: Number(value?.totalVisits) || 0, uniqueVisitors: Number(value?.uniqueVisitors) || 0, returningVisits: Number(value?.returningVisits) || 0, firstVisitAt: typeof value?.firstVisitAt === 'string' ? value.firstVisitAt : null, lastVisitAt: typeof value?.lastVisitAt === 'string' ? value.lastVisitAt : null, - visitors: value?.visitors && typeof value.visitors === 'object' ? value.visitors : {}, + visitors: loadedVisitors, + ipHashIndex, recentVisits: Array.isArray(value?.recentVisits) ? value.recentVisits.slice(0, MAX_RECENT_VISITS) : [], geoCacheByIp: value?.geoCacheByIp && typeof value.geoCacheByIp === 'object' ? value.geoCacheByIp : {}, } diff --git a/server/helpers.js b/server/helpers.js index 50af0e2..434ea2a 100644 --- a/server/helpers.js +++ b/server/helpers.js @@ -472,10 +472,8 @@ export function normalizeIp(rawIp) { } export function getClientIp(req) { - const forwarded = req.headers['x-forwarded-for'] - if (forwarded) { - return normalizeIp(forwarded) - } + // Use req.ip: Express derives this from x-forwarded-for according to the + // configured trust proxy hop count, preventing header spoofing by clients. return normalizeIp(req.ip) } diff --git a/server/routes/admin-assets.js b/server/routes/admin-assets.js index 93ac678..0a48bca 100644 --- a/server/routes/admin-assets.js +++ b/server/routes/admin-assets.js @@ -14,6 +14,8 @@ import { import { hashStudyPassword, getStudyCatalog, + normalizeStudySlug, + isEnrollableStudySlug, } from '../study-helpers.js' export function register(app) { @@ -157,7 +159,10 @@ export function register(app) { } if (typeof addEnrollment === 'string' && addEnrollment.trim()) { - const slug = addEnrollment.trim() + const slug = normalizeStudySlug(addEnrollment) + if (!slug || !isEnrollableStudySlug(slug)) { + res.status(400).json({ message: `Unknown study slug: ${addEnrollment.trim()}` }); return + } if (!Array.isArray(user.enrolledStudySlugs)) user.enrolledStudySlugs = [] if (!user.enrolledStudySlugs.includes(slug)) user.enrolledStudySlugs.push(slug) } diff --git a/server/routes/admin-backup.js b/server/routes/admin-backup.js index cf72d15..6fded54 100644 --- a/server/routes/admin-backup.js +++ b/server/routes/admin-backup.js @@ -1,4 +1,4 @@ -import { mkdir, mkdtemp, readdir, rm, cp, writeFile } from 'node:fs/promises' +import { mkdir, mkdtemp, readdir, rm, cp, writeFile, unlink } from 'node:fs/promises' import os from 'node:os' import path from 'node:path' import express from 'express' @@ -62,6 +62,8 @@ const ENV_SNAPSHOT_KEYS = [ 'TITUS_STUDY_DOWNLOAD_NAME', ] +// Write env snapshot into DATA_DIR for inclusion in the archive, then delete +// it immediately after the stream finishes so secrets don't linger on disk. async function writeEnvSnapshot() { const lines = [ '# Siteforge environment snapshot — regenerated on every full backup export.', @@ -78,6 +80,10 @@ async function writeEnvSnapshot() { await writeFile(path.join(DATA_DIR, 'env-snapshot.env'), `${lines.join('\n')}\n`, 'utf8') } +async function deleteEnvSnapshot() { + await unlink(path.join(DATA_DIR, 'env-snapshot.env')).catch(() => {}) +} + // Files that identify an archive as a Siteforge data backup. At least one // must be present at the top level of an uploaded archive before we restore. const KNOWN_DATA_FILES = [ @@ -170,9 +176,12 @@ export function register(app) { console.error('[admin-backup] export stream failed:', err) res.destroy(err) }) + archive.on('end', () => { deleteEnvSnapshot() }) + res.on('close', () => { deleteEnvSnapshot() }) archive.pipe(res) } catch (err) { console.error('[admin-backup] export failed:', err) + deleteEnvSnapshot() if (!res.headersSent) res.status(500).json({ message: 'Full backup export failed.' }) } }) diff --git a/server/routes/contact.js b/server/routes/contact.js index 5392d0d..d7e0e9f 100644 --- a/server/routes/contact.js +++ b/server/routes/contact.js @@ -65,6 +65,11 @@ function registerResendMessageForSubmission(submissionId, stream, sendResult) { const resendMessageId = extractResendMessageId(sendResult) if (!resendMessageId || !submissionId || !stream) return state.resendEmailSubmissionIndex.set(resendMessageId, { submissionId, stream }) + // Trim the index when it grows large; oldest entries are least likely to receive webhooks + if (state.resendEmailSubmissionIndex.size > 2000) { + const firstKey = state.resendEmailSubmissionIndex.keys().next().value + state.resendEmailSubmissionIndex.delete(firstKey) + } upsertContactEmailStatus(submissionId, stream, { resendEmailId: resendMessageId }) } @@ -82,6 +87,11 @@ function contactRateLimit(req, res, next) { if (now - entry.start > windowMs) { entry.count = 0; entry.start = now } entry.count += 1 contactHits.set(ip, entry) + // Prune stale entries to prevent unbounded growth + if (contactHits.size > 5000) { + const cutoff = now - windowMs + for (const [k, v] of contactHits) { if (v.start < cutoff) contactHits.delete(k) } + } if (entry.count > 5) { res.status(429).json({ message: 'Too many messages. Please wait a few minutes.' }) return @@ -251,7 +261,8 @@ export function register(app) { lastEventType: 'email.failed', error: String(welcomeErr?.message ?? welcomeErr ?? 'unknown error').slice(0, 600), }) - throw welcomeErr + console.error('[contact] welcome email failed:', welcomeErr) + // Submission is already saved — don't 500 the user; fall through to admin notification. } } else if (shouldSendWelcome && USE_RESEND_AUTOMATION_WELCOME) { upsertContactEmailStatus(submission.id, 'welcome', { status: 'automation-enabled', lastEventType: 'email.automation.enabled', error: null }) @@ -284,7 +295,8 @@ export function register(app) { lastEventType: 'email.failed', error: String(adminSendErr?.message ?? adminSendErr ?? 'unknown error').slice(0, 600), }) - throw adminSendErr + console.error('[contact] admin notification email failed:', adminSendErr) + // Submission is already saved — don't 500 the user. } res.json({ ok: true, welcomeSent, welcomeHandledByAutomation: shouldSendWelcome && USE_RESEND_AUTOMATION_WELCOME }) diff --git a/server/routes/downloads.js b/server/routes/downloads.js index 72ce74f..e04fc8a 100644 --- a/server/routes/downloads.js +++ b/server/routes/downloads.js @@ -28,6 +28,11 @@ function studyDownloadRateLimit(req, res, next) { if (now - entry.start > windowMs) { entry.count = 0; entry.start = now } entry.count += 1 downloadHits.set(ip, entry) + // Prune stale entries to prevent unbounded growth + if (downloadHits.size > 5000) { + const cutoff = now - windowMs + for (const [k, v] of downloadHits) { if (v.start < cutoff) downloadHits.delete(k) } + } if (entry.count > 10) { res.status(429).json({ message: 'Too many download requests. Please wait a few minutes.' }) return diff --git a/server/routes/inbound-email.js b/server/routes/inbound-email.js index 04803f4..f8efe93 100644 --- a/server/routes/inbound-email.js +++ b/server/routes/inbound-email.js @@ -43,7 +43,7 @@ export function register(app) { const submission = { id: randomUUID(), - submittedAt: date ? new Date(date).toISOString() : new Date().toISOString(), + submittedAt: (typeof date === 'string' || typeof date === 'number') && Number.isFinite(Date.parse(date)) ? new Date(date).toISOString() : new Date().toISOString(), name: fromName || fromEmail, email: fromEmail, message: [subject ? `Subject: ${subject}` : '', body ?? ''].filter(Boolean).join('\n\n'), diff --git a/server/routes/public.js b/server/routes/public.js index 4745acb..54bef73 100644 --- a/server/routes/public.js +++ b/server/routes/public.js @@ -93,7 +93,7 @@ export function register(app) { if (!id || !/^[\w-]{1,120}$/.test(id)) { res.redirect(302, '/questions'); return } - const sourceQuestions = state.draftQuestions ?? state.questions + const sourceQuestions = state.questions const question = sourceQuestions.find(q => q.id === id && q.isApproved === true && q.answer) if (!question) { res.redirect(302, '/questions'); return diff --git a/server/routes/study-account.js b/server/routes/study-account.js index b664426..e2ed4d5 100644 --- a/server/routes/study-account.js +++ b/server/routes/study-account.js @@ -237,6 +237,10 @@ export function register(app) { res.status(400).json({ message: 'Avatar must be a valid uploaded image, data URI, or https URL.' }) return } + if (avatarUrl.startsWith('data:image/') && avatarUrl.length > 6 * 1024 * 1024) { + res.status(400).json({ message: 'Avatar data URI is too large. Please use the upload endpoint instead.' }) + return + } user.displayName = displayName user.avatarUrl = avatarUrl diff --git a/server/routes/study-auth.js b/server/routes/study-auth.js index 12104c1..5032e97 100644 --- a/server/routes/study-auth.js +++ b/server/routes/study-auth.js @@ -81,6 +81,11 @@ export function register(app) { return } + if (state.studyUsers.length >= MAX_STUDY_USERS) { + res.status(503).json({ message: 'Account registration is temporarily unavailable. Please try again later.' }) + return + } + const now = new Date().toISOString() const user = { id: randomUUID(), @@ -97,9 +102,6 @@ export function register(app) { } state.studyUsers.push(user) - if (state.studyUsers.length > MAX_STUDY_USERS) { - state.studyUsers = state.studyUsers.slice(state.studyUsers.length - MAX_STUDY_USERS) - } queueStudyUsersWrite() if (subscribe) { diff --git a/server/study-helpers.js b/server/study-helpers.js index 6232bc1..827fabb 100644 --- a/server/study-helpers.js +++ b/server/study-helpers.js @@ -650,9 +650,16 @@ export async function scheduleStudyReminders(sendStudyReminderEmail) { const canonical = state.cachedSiteContent?.seo?.canonicalUrl || 'https://versebyversewithnate.us/' const base = canonical.endsWith('/') ? canonical.slice(0, -1) : canonical const sectionUrl = `${base}/study/${study.slug}/${section.id}` - await sendStudyReminderEmail(email, displayName, study.title, section.title, section.reference, sectionUrl) + try { + await sendStudyReminderEmail(email, displayName, study.title, section.title, section.reference, sectionUrl) + } catch (err) { + console.error(`[study-reminders] failed to send reminder to ${email} for ${studySlug}/${sectionId}:`, err) + continue + } userSent[studySlug] = [...sentForStudy, sectionId] state.studyReminders.users[user.id] = userSent + // Persist after each successful send so a later crash doesn't re-send + queueStudyRemindersWrite() } } }