From 2a64f29a0056fb077377aa981d58bda78135f7ad Mon Sep 17 00:00:00 2001 From: nmemmert Date: Fri, 9 Oct 2026 09:00:28 -0400 Subject: [PATCH] Second hardening pass: atomic data writes, bible-passage limits, webhook SSRF guard, note sanitizing, CI gates; v2.10.1 - Atomic temp-file + rename for all JSON data writes (no truncated files on crash) - /api/bible-passage: rate limit, range caps (60 verses), bounded cache - Admin webhooks: https-only, block private/internal/Tailscale addresses (checked at registration and delivery), no redirects - Resend webhook token compare is timing-safe - DOMPurify on rendered study notes and broadcast preview - CI runs lint + tests before build - Permissions-Policy and HSTS (production) headers; JSON error handler for /api - Tests for URL safety and atomic writes Co-Authored-By: Claude Sonnet 5.5 --- .gitea/workflows/docker-publish.yml | 6 ++++ .github/workflows/docker-publish.yml | 6 ++++ package-lock.json | 16 +++++++-- package.json | 3 +- server.js | 16 +++++++++ server/atomic-write.js | 17 ++++++++++ server/auth.js | 3 +- server/data.js | 10 ++++-- server/nuggets-seafile-watcher.js | 3 +- server/routes/admin-content.js | 12 +++++-- server/routes/bible-passage.js | 26 ++++++++++++++- server/routes/contact.js | 6 ++-- server/routes/nuggets-host-upload.js | 3 +- server/url-safety.js | 50 ++++++++++++++++++++++++++++ src/AdminPage.tsx | 3 +- src/studyPages.tsx | 3 +- test/atomic-write.test.js | 26 +++++++++++++++ test/url-safety.test.js | 17 ++++++++++ 18 files changed, 209 insertions(+), 17 deletions(-) create mode 100644 server/atomic-write.js create mode 100644 server/url-safety.js create mode 100644 test/atomic-write.test.js create mode 100644 test/url-safety.test.js diff --git a/.gitea/workflows/docker-publish.yml b/.gitea/workflows/docker-publish.yml index 81479ba..c881235 100644 --- a/.gitea/workflows/docker-publish.yml +++ b/.gitea/workflows/docker-publish.yml @@ -31,6 +31,12 @@ jobs: - name: Install dependencies run: npm ci --legacy-peer-deps + - name: Lint + run: npm run lint + + - name: Run tests + run: npm test + - name: Run production build run: npm run build diff --git a/.github/workflows/docker-publish.yml b/.github/workflows/docker-publish.yml index 0aa34fb..47a7cd9 100644 --- a/.github/workflows/docker-publish.yml +++ b/.github/workflows/docker-publish.yml @@ -33,6 +33,12 @@ jobs: - name: Install dependencies run: npm ci --legacy-peer-deps + - name: Lint + run: npm run lint + + - name: Run tests + run: npm test + - name: Run production build run: npm run build diff --git a/package-lock.json b/package-lock.json index a0db04a..5648748 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "siteforge", - "version": "2.9.8", + "version": "2.10.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "siteforge", - "version": "2.9.8", + "version": "2.10.0", "dependencies": { "@aws-sdk/client-s3": "^3.1135.0", "@dnd-kit/core": "^6.3.1", @@ -18,6 +18,7 @@ "@tiptap/starter-kit": "^3.31.4", "chart.js": "^4.4.0", "docx": "^9.6.1", + "dompurify": "^3.4.16", "express": "^5.2.1", "express-rate-limit": "^8.4.1", "fuse.js": "^7.4.2", @@ -3829,7 +3830,7 @@ "version": "2.0.7", "resolved": "https://registry.npmjs.org/@types/trusted-types/-/trusted-types-2.0.7.tgz", "integrity": "sha512-ScaPdn1dQczgbl0QFTeTOmVHFULt394XJgOQNoyVhZ6r2vLnMLJfBPd53SB52T/3G36VI1/g2MZaX0cwDuXsfw==", - "dev": true, + "devOptional": true, "license": "MIT" }, "node_modules/@types/unist": { @@ -5272,6 +5273,15 @@ "integrity": "sha512-WRNW+sJgj5OBN4/0JpHFqtqzhpbnV0GuB+OozA9gCL7a993SmU+1JBZCzLNxYsbMfIeDL+lTsphD5jN5N+n0zg==", "license": "MIT" }, + "node_modules/dompurify": { + "version": "3.4.16", + "resolved": "https://registry.npmjs.org/dompurify/-/dompurify-3.4.16.tgz", + "integrity": "sha512-sqo+pNp3qRhCIpbgRi1y8Tgk27Bo2Ry7w0dC1NBeNTdZChWjz9Xb/KOoZbRP/R6pQZ80Qw8YhXw13hWWBbMRnQ==", + "license": "(MPL-2.0 OR Apache-2.0)", + "optionalDependencies": { + "@types/trusted-types": "^2.0.7" + } + }, "node_modules/duck": { "version": "0.1.12", "resolved": "https://registry.npmjs.org/duck/-/duck-0.1.12.tgz", diff --git a/package.json b/package.json index 3602d44..d709dec 100644 --- a/package.json +++ b/package.json @@ -1,7 +1,7 @@ { "name": "siteforge", "private": true, - "version": "2.10.0", + "version": "2.10.1", "type": "module", "scripts": { "dev": "vite", @@ -24,6 +24,7 @@ "@tiptap/starter-kit": "^3.31.4", "chart.js": "^4.4.0", "docx": "^9.6.1", + "dompurify": "^3.4.16", "express": "^5.2.1", "express-rate-limit": "^8.4.1", "fuse.js": "^7.4.2", diff --git a/server.js b/server.js index fd7667f..fc27dff 100644 --- a/server.js +++ b/server.js @@ -84,6 +84,10 @@ app.use((_req, res, next) => { res.setHeader('X-Content-Type-Options', 'nosniff') res.setHeader('X-Frame-Options', 'SAMEORIGIN') res.setHeader('Referrer-Policy', 'strict-origin-when-cross-origin') + res.setHeader('Permissions-Policy', 'camera=(), microphone=(), geolocation=(), payment=()') + if (process.env.NODE_ENV === 'production') { + res.setHeader('Strict-Transport-Security', 'max-age=31536000') + } res.setHeader( 'Content-Security-Policy', [ @@ -147,6 +151,18 @@ app.use((req, res, next) => { // Public routes (robots, sitemap, static, SPA fallback) registerPublic(app) +// JSON error responses for API routes instead of Express's default HTML page. +app.use((err, req, res, _next) => { + if (res.headersSent) return + const status = Number.isInteger(err?.status) && err.status >= 400 && err.status < 600 ? err.status : 500 + if (status >= 500) console.error('[error]', req.method, req.path, err) + if (req.path.startsWith('/api/')) { + res.status(status).json({ message: status >= 500 ? 'Internal server error.' : String(err?.message ?? 'Request failed.') }) + return + } + res.status(status).type('text/plain').send(status >= 500 ? 'Internal server error.' : 'Bad request.') +}) + const PORT = Number(process.env.PORT ?? 4173) Promise.all([ loadHitStatsFromDisk(), diff --git a/server/atomic-write.js b/server/atomic-write.js new file mode 100644 index 0000000..66c1620 --- /dev/null +++ b/server/atomic-write.js @@ -0,0 +1,17 @@ +import { writeFile as fsWriteFile, rename, unlink } from 'node:fs/promises' +import { randomBytes } from 'node:crypto' + +// Drop-in replacement for fs.promises.writeFile that never leaves a truncated +// file behind: the data goes to a sibling temp file which is then renamed over +// the target (atomic on the same filesystem). A crash mid-write leaves the +// previous version intact instead of a half-written JSON file. +export async function writeFile(file, data, options) { + const tmp = `${file}.${process.pid}.${randomBytes(4).toString('hex')}.tmp` + try { + await fsWriteFile(tmp, data, options) + await rename(tmp, file) + } catch (err) { + await unlink(tmp).catch(() => {}) + throw err + } +} diff --git a/server/auth.js b/server/auth.js index e34913c..dfa2875 100644 --- a/server/auth.js +++ b/server/auth.js @@ -1,5 +1,6 @@ import { createHash, randomUUID, timingSafeEqual, createHmac, randomFillSync } from 'node:crypto' -import { readFile, writeFile } from 'node:fs/promises' +import { readFile } from 'node:fs/promises' +import { writeFile } from './atomic-write.js' import { readFileSync } from 'node:fs' import path from 'node:path' import { parseCookies, cookieSecureFlag } from './helpers.js' diff --git a/server/data.js b/server/data.js index 85764c3..1acf86a 100644 --- a/server/data.js +++ b/server/data.js @@ -1,5 +1,7 @@ import { migrateScriptKeys } from './episode-script-keys.js' -import { mkdir, readFile, readdir, stat, unlink, writeFile } from 'node:fs/promises' +import { mkdir, readFile, readdir, stat, unlink } from 'node:fs/promises' +import { writeFile } from './atomic-write.js' +import { assertPublicHttpsUrl } from './url-safety.js' import path from 'node:path' import { randomUUID } from 'node:crypto' import { sanitizeSiteContent } from './helpers.js' @@ -533,12 +535,14 @@ export async function fireWebhooks(eventType, payload) { if (targets.length === 0) return const body = JSON.stringify({ event: eventType, data: payload, firedAt: new Date().toISOString() }) for (const wh of targets) { - fetch(wh.url, { + // Re-check at delivery time: DNS may have changed since the webhook was registered. + assertPublicHttpsUrl(wh.url).then(() => fetch(wh.url, { method: 'POST', headers: { 'Content-Type': 'application/json', 'User-Agent': 'Siteforge-Webhook/1.0' }, body, + redirect: 'manual', signal: AbortSignal.timeout(8000), - }) + })) .then(r => { appendAuditEntry('webhook-fired', `${eventType} → ${wh.url} (${r.status})`) }) diff --git a/server/nuggets-seafile-watcher.js b/server/nuggets-seafile-watcher.js index 7bc2b1b..fa89fa5 100644 --- a/server/nuggets-seafile-watcher.js +++ b/server/nuggets-seafile-watcher.js @@ -1,4 +1,5 @@ -import { readFile, writeFile } from 'node:fs/promises' +import { readFile } from 'node:fs/promises' +import { writeFile } from './atomic-write.js' import { sendNuggetsUploadNotification } from './email.js' import { SEAFILE_URL, diff --git a/server/routes/admin-content.js b/server/routes/admin-content.js index 659ffe8..bfd0f9a 100644 --- a/server/routes/admin-content.js +++ b/server/routes/admin-content.js @@ -1,4 +1,6 @@ -import { mkdir, readFile, writeFile } from 'node:fs/promises' +import { mkdir, readFile } from 'node:fs/promises' +import { writeFile } from '../atomic-write.js' +import { assertPublicHttpsUrl } from '../url-safety.js' import { sanitizeSiteContent } from '../helpers.js' import { requireAdminAuth, isValidAdminSession } from '../auth.js' import { @@ -524,12 +526,18 @@ export function register(app) { res.json({ webhooks: state.webhooks }) }) - app.post('/api/admin-webhooks', requireAdminAuth, (req, res) => { + app.post('/api/admin-webhooks', requireAdminAuth, async (req, res) => { const { url, events } = req.body ?? {} if (!url || typeof url !== 'string' || !/^https:\/\//i.test(url.trim())) { res.status(400).json({ message: 'A valid HTTPS URL is required.' }) return } + try { + await assertPublicHttpsUrl(url.trim()) + } catch (err) { + res.status(400).json({ message: `Webhook URL rejected: ${err.message}` }) + return + } if (state.webhooks.length >= 50) { res.status(400).json({ message: 'Maximum of 50 webhooks reached.' }) return diff --git a/server/routes/bible-passage.js b/server/routes/bible-passage.js index 5a4d592..aa675d1 100644 --- a/server/routes/bible-passage.js +++ b/server/routes/bible-passage.js @@ -1,6 +1,19 @@ +import rateLimit from 'express-rate-limit' import { BIBLE_BOOKS } from '../../shared/bible-books.js' const passageCache = new Map() +const PASSAGE_CACHE_MAX = 2000 +const MAX_CHAPTER = 150 +const MAX_VERSE = 176 +const MAX_VERSES_PER_REQUEST = 60 + +const passageRateLimiter = rateLimit({ + windowMs: 60 * 1000, + max: 60, + standardHeaders: true, + legacyHeaders: false, + message: { message: 'Too many passage requests. Please slow down.' }, +}) // Keyed by lowercase, whitespace-stripped book name/alias (matching the format // the admin's "Bible Book" field and the /api/bible-passage `book` query param @@ -27,7 +40,7 @@ function parseReference(reference) { } export function register(app) { - app.get('/api/bible-passage', async (req, res) => { + app.get('/api/bible-passage', passageRateLimiter, async (req, res) => { const book = String(req.query.book ?? '').trim().toLowerCase().replace(/[\s-]/g, '') const reference = String(req.query.reference ?? '').trim() @@ -43,6 +56,16 @@ export function register(app) { return } + if ( + parsed.chapter < 1 || parsed.chapter > MAX_CHAPTER + || parsed.verseStart < 1 || parsed.verseEnd > MAX_VERSE + || parsed.verseEnd < parsed.verseStart + || parsed.verseEnd - parsed.verseStart + 1 > MAX_VERSES_PER_REQUEST + ) { + res.status(400).json({ message: `Passage out of range (max ${MAX_VERSES_PER_REQUEST} verses).` }) + return + } + const cacheKey = `BSB:${bookNum}:${parsed.chapter}:${parsed.verseStart}:${parsed.verseEnd}` if (passageCache.has(cacheKey)) { res.json({ text: passageCache.get(cacheKey) }) @@ -66,6 +89,7 @@ export function register(app) { } const text = results.map(v => `v. ${v.verse} ${String(v.text).trim()}`).join('\n\n') + if (passageCache.size >= PASSAGE_CACHE_MAX) passageCache.delete(passageCache.keys().next().value) passageCache.set(cacheKey, text) res.json({ text }) } catch { diff --git a/server/routes/contact.js b/server/routes/contact.js index 75077f3..dc2016d 100644 --- a/server/routes/contact.js +++ b/server/routes/contact.js @@ -1,4 +1,4 @@ -import { randomUUID } from 'node:crypto' +import { randomUUID, timingSafeEqual } from 'node:crypto' import { Resend } from 'resend' import { requireAdminAuth } from '../auth.js' import { escapeHtml, splitName, buildAbsoluteUrl } from '../helpers.js' @@ -311,7 +311,9 @@ export function register(app) { || String(req.query?.token || '').trim() || (req.get('authorization') || '').replace(/^Bearer\s+/i, '').trim() - if (!providedToken || providedToken !== expectedToken) { + const tokenA = Buffer.from(providedToken, 'utf8') + const tokenB = Buffer.from(expectedToken, 'utf8') + if (!providedToken || tokenA.length !== tokenB.length || !timingSafeEqual(tokenA, tokenB)) { res.status(401).json({ message: 'Unauthorized webhook.' }); return } diff --git a/server/routes/nuggets-host-upload.js b/server/routes/nuggets-host-upload.js index c81ffea..6ce4bab 100644 --- a/server/routes/nuggets-host-upload.js +++ b/server/routes/nuggets-host-upload.js @@ -1,5 +1,6 @@ import { randomUUID } from 'node:crypto' -import { readFile, writeFile, unlink, mkdir } from 'node:fs/promises' +import { readFile, unlink, mkdir } from 'node:fs/promises' +import { writeFile } from '../atomic-write.js' import path from 'node:path' import multer from 'multer' import rateLimit from 'express-rate-limit' diff --git a/server/url-safety.js b/server/url-safety.js new file mode 100644 index 0000000..34caee7 --- /dev/null +++ b/server/url-safety.js @@ -0,0 +1,50 @@ +import { lookup } from 'node:dns/promises' +import net from 'node:net' + +function isPrivateIPv4(ip) { + const [a, b] = ip.split('.').map(Number) + return ( + a === 0 || a === 10 || a === 127 + || (a === 100 && b >= 64 && b <= 127) // CGNAT / Tailscale + || (a === 169 && b === 254) // link-local, cloud metadata + || (a === 172 && b >= 16 && b <= 31) + || (a === 192 && b === 168) + || a >= 224 // multicast / reserved + ) +} + +export function isPrivateAddress(ip) { + if (net.isIPv4(ip)) return isPrivateIPv4(ip) + if (net.isIPv6(ip)) { + const lower = ip.toLowerCase() + if (lower === '::' || lower === '::1') return true + if (lower.startsWith('fc') || lower.startsWith('fd')) return true // unique local + if (/^fe[89ab]/.test(lower)) return true // link-local + const mapped = lower.match(/^::ffff:(\d+\.\d+\.\d+\.\d+)$/) + if (mapped) return isPrivateIPv4(mapped[1]) + return false + } + return true // not an IP at all: treat as unsafe +} + +// Throws unless `rawUrl` is an https URL whose host resolves only to public +// addresses. Called when a webhook is registered and again before each delivery +// (DNS can change between the two). +export async function assertPublicHttpsUrl(rawUrl) { + let parsed + try { + parsed = new URL(rawUrl) + } catch { + throw new Error('Malformed URL.') + } + if (parsed.protocol !== 'https:') throw new Error('Only https:// URLs are allowed.') + if (parsed.username || parsed.password) throw new Error('URLs with embedded credentials are not allowed.') + const host = parsed.hostname.replace(/^\[|\]$/g, '') + if (host === 'localhost' || host.endsWith('.localhost') || host.endsWith('.local') || host.endsWith('.internal')) { + throw new Error('Internal hostnames are not allowed.') + } + const addresses = net.isIP(host) ? [{ address: host }] : await lookup(host, { all: true }) + if (addresses.length === 0 || addresses.some(a => isPrivateAddress(a.address))) { + throw new Error('URL resolves to a private or internal address.') + } +} diff --git a/src/AdminPage.tsx b/src/AdminPage.tsx index d137a53..52fd6de 100644 --- a/src/AdminPage.tsx +++ b/src/AdminPage.tsx @@ -1,3 +1,4 @@ +import DOMPurify from 'dompurify' import React, { useEffect, useRef, useState } from 'react' import { DndContext, closestCenter, PointerSensor, useSensor, useSensors } from '@dnd-kit/core' import { arrayMove, SortableContext, verticalListSortingStrategy, useSortable } from '@dnd-kit/sortable' @@ -6868,7 +6869,7 @@ const [dashboardNow, setDashboardNow] = useState(() => new Date())
Preview rendered HTML
+ dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(broadcastBody.replace(/\{\{name\}\}/g, 'Nate (preview)')) }} />
)} {bcAudience?.segmentConfigured === false &&

RESEND_SEGMENT_ID isn’t set on the server, so there’s no Resend segment to send to.

} diff --git a/src/studyPages.tsx b/src/studyPages.tsx index 0a33ca2..5d67d78 100644 --- a/src/studyPages.tsx +++ b/src/studyPages.tsx @@ -1,4 +1,5 @@ import { useEffect, useMemo, useRef, useState, useCallback } from 'react' +import DOMPurify from 'dompurify' import { Link, useLocation, useNavigate, useParams } from 'react-router-dom' import type { SiteContent, StudyProgram, StudySection } from './content' import { DEFAULT_COLOSSIANS_STUDY_SECTIONS } from './legacyColossiansData' @@ -2791,7 +2792,7 @@ export function ColossiansStudyNotesPage({ content }: Props) { html?.trim() ? (

{ANCHOR_LABELS[key] ?? key}

-
+
) : null )) diff --git a/test/atomic-write.test.js b/test/atomic-write.test.js new file mode 100644 index 0000000..47889ce --- /dev/null +++ b/test/atomic-write.test.js @@ -0,0 +1,26 @@ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { mkdtemp, readFile, readdir, writeFile as plainWrite, rm } from 'node:fs/promises' +import os from 'node:os' +import path from 'node:path' +import { writeFile } from '../server/atomic-write.js' + +test('replaces file contents and leaves no temp files', async () => { + const dir = await mkdtemp(path.join(os.tmpdir(), 'atomic-')) + const file = path.join(dir, 'a.json') + await plainWrite(file, 'old') + await writeFile(file, '{"ok":true}', 'utf8') + assert.equal(await readFile(file, 'utf8'), '{"ok":true}') + assert.deepEqual(await readdir(dir), ['a.json']) + await rm(dir, { recursive: true }) +}) + +test('a failed write keeps the old file and cleans up', async () => { + const dir = await mkdtemp(path.join(os.tmpdir(), 'atomic-')) + const file = path.join(dir, 'a.json') + await plainWrite(file, 'old') + await assert.rejects(() => writeFile(file, { not: 'a string' })) + assert.equal(await readFile(file, 'utf8'), 'old') + assert.deepEqual(await readdir(dir), ['a.json']) + await rm(dir, { recursive: true }) +}) diff --git a/test/url-safety.test.js b/test/url-safety.test.js new file mode 100644 index 0000000..1bd2a8a --- /dev/null +++ b/test/url-safety.test.js @@ -0,0 +1,17 @@ +import { test } from 'node:test' +import assert from 'node:assert/strict' +import { isPrivateAddress, assertPublicHttpsUrl } from '../server/url-safety.js' + +test('private, loopback, link-local and tailscale addresses are flagged', () => { + for (const ip of ['127.0.0.1', '10.0.0.5', '192.168.1.1', '172.16.0.1', '169.254.169.254', '100.64.1.1', '::1', 'fd00::1', 'fe80::1', '::ffff:10.0.0.1']) { + assert.ok(isPrivateAddress(ip), ip) + } + for (const ip of ['8.8.8.8', '1.1.1.1', '2606:4700:4700::1111']) assert.ok(!isPrivateAddress(ip), ip) +}) + +test('assertPublicHttpsUrl rejects unsafe URLs', async () => { + for (const url of ['http://example.com', 'https://localhost/x', 'https://127.0.0.1/', 'https://169.254.169.254/latest', 'https://user:pw@example.com', 'https://nas.local/hook', 'not a url']) { + await assert.rejects(() => assertPublicHttpsUrl(url), url) + } + await assert.doesNotReject(() => assertPublicHttpsUrl('https://8.8.8.8/hook')) +})