From 03ba19042d8b061bcff368b0eb55cb1cc3266049 Mon Sep 17 00:00:00 2001 From: Jan Willem Mannaerts Date: Sat, 28 Feb 2026 16:05:48 +0100 Subject: [PATCH] Harden security across frontend and backend 1. AdfRenderer: validate href starts with https?:// before rendering links 2. Logout route: add requireAuth middleware 3. Jira API params: validate sprintId, boardId, issueIdOrKey are alphanumeric 4. CSP header: add Content-Security-Policy with restrictive defaults 5. OAuth callback: align frontendUrl fallback with index.js 6. Rate limiting: express-rate-limit on API routes + Socket.IO event throttling 7. Session KV keys: prefix with cloudId for tenant isolation defense-in-depth 8. saveScopedEstimate: use withSessionCas for atomic read-update-delete Co-Authored-By: Claude Opus 4.6 --- backend/package-lock.json | 28 +++++++++++ backend/package.json | 1 + backend/src/index.js | 57 +++++++++++++++++----- backend/src/routes/jira.js | 7 ++- backend/src/routes/poker.js | 2 +- backend/src/services/jiraService.js | 10 ++++ backend/src/services/pokerService.js | 63 +++++++++++++------------ frontend/src/components/AdfRenderer.jsx | 6 ++- 8 files changed, 127 insertions(+), 47 deletions(-) diff --git a/backend/package-lock.json b/backend/package-lock.json index 6fd0aa9..75ec30a 100644 --- a/backend/package-lock.json +++ b/backend/package-lock.json @@ -13,6 +13,7 @@ "cors": "^2.8.5", "dotenv": "^16.4.5", "express": "^4.21.1", + "express-rate-limit": "^8.2.1", "jsonwebtoken": "^9.0.3", "nats": "^2.28.2", "socket.io": "^4.8.1" @@ -555,6 +556,24 @@ "url": "https://opencollective.com/express" } }, + "node_modules/express-rate-limit": { + "version": "8.2.1", + "resolved": "https://registry.npmjs.org/express-rate-limit/-/express-rate-limit-8.2.1.tgz", + "integrity": "sha512-PCZEIEIxqwhzw4KF0n7QF4QqruVTcF73O5kFKUnGOyjbCCgizBBiFaYpd/fnBLUMPw/BWw9OsiN7GgrNYr7j6g==", + "license": "MIT", + "dependencies": { + "ip-address": "10.0.1" + }, + "engines": { + "node": ">= 16" + }, + "funding": { + "url": "https://github.com/sponsors/express-rate-limit" + }, + "peerDependencies": { + "express": ">= 4.11" + } + }, "node_modules/express/node_modules/debug": { "version": "2.6.9", "resolved": "https://registry.npmjs.org/debug/-/debug-2.6.9.tgz", @@ -799,6 +818,15 @@ "integrity": "sha512-k/vGaX4/Yla3WzyMCvTQOXYeIHvqOKtnqBduzTHpzpQZzAskKMhZ2K+EnBiSM9zGSoIFeMpXKxa4dYeZIQqewQ==", "license": "ISC" }, + "node_modules/ip-address": { + "version": "10.0.1", + "resolved": "https://registry.npmjs.org/ip-address/-/ip-address-10.0.1.tgz", + "integrity": "sha512-NWv9YLW4PoW2B7xtzaS3NCot75m6nK7Icdv0o3lfMceJVRfSoQwqD4wEH5rLwoKJwUiZ/rfpiVBhnaF0FK4HoA==", + "license": "MIT", + "engines": { + "node": ">= 12" + } + }, "node_modules/ipaddr.js": { "version": "1.9.1", "resolved": "https://registry.npmjs.org/ipaddr.js/-/ipaddr.js-1.9.1.tgz", diff --git a/backend/package.json b/backend/package.json index 13b71f7..a808032 100644 --- a/backend/package.json +++ b/backend/package.json @@ -14,6 +14,7 @@ "cors": "^2.8.5", "dotenv": "^16.4.5", "express": "^4.21.1", + "express-rate-limit": "^8.2.1", "jsonwebtoken": "^9.0.3", "nats": "^2.28.2", "socket.io": "^4.8.1" diff --git a/backend/src/index.js b/backend/src/index.js index 360222c..57bb0ac 100644 --- a/backend/src/index.js +++ b/backend/src/index.js @@ -4,6 +4,7 @@ import { createServer } from 'http'; import express from 'express'; import cors from 'cors'; import cookieParser from 'cookie-parser'; +import rateLimit from 'express-rate-limit'; import dotenv from 'dotenv'; import { Server } from 'socket.io'; import natsAdapter from '@mickl/socket.io-nats-adapter'; @@ -53,6 +54,18 @@ app.use((_req, res, next) => { res.setHeader('X-Frame-Options', 'DENY'); res.setHeader('Referrer-Policy', 'no-referrer'); res.setHeader('Permissions-Policy', 'geolocation=(), microphone=(), camera=()'); + res.setHeader('Content-Security-Policy', [ + "default-src 'self'", + "script-src 'self'", + "style-src 'self' 'unsafe-inline'", + `connect-src 'self' wss://${isProd ? new URL(frontendUrl).host : '*'}`, + "img-src 'self' https://avatar-management--avatars.us-west-2.prod.public.atl-paas.net https://secure.gravatar.com data:", + "font-src 'self'", + "object-src 'none'", + "base-uri 'self'", + "form-action 'self'", + "frame-ancestors 'none'" + ].join('; ')); if (isProd) { res.setHeader('Strict-Transport-Security', 'max-age=31536000; includeSubDomains'); } @@ -62,6 +75,7 @@ app.use((_req, res, next) => { app.use(cors(corsOptions)); app.use(cookieParser()); app.use(express.json({ limit: '1mb' })); +app.use('/api/', rateLimit({ windowMs: 60_000, max: 100, standardHeaders: true, legacyHeaders: false })); app.get('/api/health', (_req, res) => { res.json({ status: 'ok' }); @@ -92,8 +106,8 @@ const io = new Server(httpServer, { } }); -async function emitSessionState(sessionId) { - const snapshot = await getSessionSnapshot(sessionId); +async function emitSessionState(sessionId, tenantCloudId) { + const snapshot = await getSessionSnapshot(sessionId, tenantCloudId); if (!snapshot) return; io.to(`poker:${sessionId}`).emit('poker:participants', { @@ -123,6 +137,24 @@ async function emitSessionState(sessionId) { } } +function socketThrottle(socket, limitPerMinute = 60) { + const counts = new Map(); + return (event, handler) => { + socket.on(event, async (...args) => { + const now = Date.now(); + const entry = counts.get(event) || { count: 0, resetAt: now + 60_000 }; + if (now > entry.resetAt) { entry.count = 0; entry.resetAt = now + 60_000; } + entry.count++; + counts.set(event, entry); + if (entry.count > limitPerMinute) { + socket.emit('poker:error', { error: 'Too many requests, slow down.' }); + return; + } + await handler(...args); + }); + }; +} + io.on('connection', (socket) => { const user = getSocketUser(socket); if (!user || !user.jiraCloudId) { @@ -131,8 +163,9 @@ io.on('connection', (socket) => { return; } socket.user = user; + const throttled = socketThrottle(socket); - socket.on('poker:join', async ({ sessionId }) => { + throttled('poker:join', async ({ sessionId }) => { try { if (!sessionId) { socket.emit('poker:error', { error: 'sessionId is required.' }); @@ -156,14 +189,14 @@ io.on('connection', (socket) => { socket.emit('poker:error', { error: 'Session not found.' }); return; } - await emitSessionState(sessionId); + await emitSessionState(sessionId, socket.user.jiraCloudId); } catch (error) { console.error('[socket] poker:join failed:', error); socket.emit('poker:error', { error: safeError(error) }); } }); - socket.on('poker:vote', async ({ sessionId, vote }) => { + throttled('poker:vote', async ({ sessionId, vote }) => { try { if (!sessionId) { socket.emit('poker:error', { error: 'sessionId is required.' }); @@ -174,7 +207,7 @@ io.on('connection', (socket) => { socket.emit('poker:error', { error: 'Session not found.' }); return; } - if (!await isSessionParticipant(sessionId, socket.user.jiraAccountId)) { + if (!await isSessionParticipant(sessionId, socket.user.jiraCloudId, socket.user.jiraAccountId)) { socket.emit('poker:error', { error: 'Join the session before voting.' }); return; } @@ -189,9 +222,9 @@ io.on('connection', (socket) => { socket.emit('poker:error', { error: 'Unable to submit vote for this session.' }); return; } - const reveal = await revealIfComplete(sessionId); + const reveal = await revealIfComplete(sessionId, socket.user.jiraCloudId); - await emitSessionState(sessionId); + await emitSessionState(sessionId, socket.user.jiraCloudId); if (reveal?.allVoted) { io.to(`poker:${sessionId}`).emit('poker:revealed', { @@ -207,7 +240,7 @@ io.on('connection', (socket) => { } }); - socket.on('poker:save', async ({ sessionId, estimate }) => { + throttled('poker:save', async ({ sessionId, estimate }) => { try { if (!sessionId) { socket.emit('poker:error', { error: 'sessionId is required.' }); @@ -218,7 +251,7 @@ io.on('connection', (socket) => { socket.emit('poker:error', { error: 'Session not found.' }); return; } - if (!await isSessionParticipant(sessionId, socket.user.jiraAccountId)) { + if (!await isSessionParticipant(sessionId, socket.user.jiraCloudId, socket.user.jiraAccountId)) { socket.emit('poker:error', { error: 'Join the session before saving.' }); return; } @@ -260,7 +293,7 @@ io.on('connection', (socket) => { } }); - socket.on('poker:leave', async ({ sessionId }) => { + throttled('poker:leave', async ({ sessionId }) => { try { if (!sessionId) return; if (!await canAccessSession(sessionId, socket.user.jiraCloudId)) return; @@ -270,7 +303,7 @@ io.on('connection', (socket) => { userKey: socket.user.jiraAccountId }); socket.leave(`poker:${sessionId}`); - await emitSessionState(sessionId); + await emitSessionState(sessionId, socket.user.jiraCloudId); } catch (error) { console.error('[socket] poker:leave failed:', error); socket.emit('poker:error', { error: safeError(error) }); diff --git a/backend/src/routes/jira.js b/backend/src/routes/jira.js index 3865981..a6c591e 100644 --- a/backend/src/routes/jira.js +++ b/backend/src/routes/jira.js @@ -36,8 +36,7 @@ router.get('/oauth/start', async (_req, res) => { router.get('/oauth/callback', async (req, res) => { const { state, code } = req.query; - const isProd = process.env.NODE_ENV === 'production'; - const frontendUrl = process.env.FRONTEND_URL || (isProd ? '' : 'http://localhost:5174'); + const frontendUrl = process.env.FRONTEND_URL || 'http://localhost:5174'; try { const entry = await kvOAuthState.get(String(state)); @@ -68,7 +67,7 @@ router.get('/oauth/callback', async (req, res) => { res.redirect(`${frontendUrl}?auth=success`); } catch (error) { console.error('[oauth] Callback failed:', error.message); - const safeMessage = isProd ? 'Jira authentication failed.' : error.message; + const safeMessage = process.env.NODE_ENV === 'production' ? 'Jira authentication failed.' : error.message; res.redirect(`${frontendUrl}?auth=error&message=${encodeURIComponent(safeMessage)}`); } }); @@ -82,7 +81,7 @@ router.get('/me', requireAuth, (req, res) => { }); }); -router.post('/logout', (_req, res) => { +router.post('/logout', requireAuth, (_req, res) => { clearSessionCookie(res); res.json({ ok: true }); }); diff --git a/backend/src/routes/poker.js b/backend/src/routes/poker.js index 8f76d7f..e4f3959 100644 --- a/backend/src/routes/poker.js +++ b/backend/src/routes/poker.js @@ -51,7 +51,7 @@ router.get('/sessions/:sessionId', requireAuth, async (req, res) => { return res.status(404).json({ error: 'Session not found.' }); } - const snapshot = await getSessionSnapshot(req.params.sessionId); + const snapshot = await getSessionSnapshot(req.params.sessionId, req.user.jiraCloudId); if (!snapshot) { return res.status(404).json({ error: 'Session not found.' }); } diff --git a/backend/src/services/jiraService.js b/backend/src/services/jiraService.js index c6f4639..4fafeae 100644 --- a/backend/src/services/jiraService.js +++ b/backend/src/services/jiraService.js @@ -193,9 +193,16 @@ async function jiraFetch(jiraAccountId, path, options = {}) { return response.json(); } +const SAFE_ID = /^[a-zA-Z0-9_:-]+$/; +function assertSafeParam(value, name) { + if (!value || !SAFE_ID.test(value)) throw new Error(`Invalid ${name}`); +} + export async function getSprintIssues(jiraAccountId, sprintId, boardId) { + assertSafeParam(sprintId, 'sprintId'); let spField = 'customfield_10016'; if (boardId) { + assertSafeParam(boardId, 'boardId'); try { const config = await jiraFetch(jiraAccountId, `/rest/agile/1.0/board/${boardId}/configuration`); const estField = config?.estimation?.field?.fieldId; @@ -222,6 +229,8 @@ export async function getSprintIssues(jiraAccountId, sprintId, boardId) { } export async function updateIssueEstimate(jiraAccountId, issueIdOrKey, estimate, boardId) { + assertSafeParam(issueIdOrKey, 'issueIdOrKey'); + assertSafeParam(boardId, 'boardId'); await jiraFetch(jiraAccountId, `/rest/agile/1.0/issue/${issueIdOrKey}/estimation?boardId=${boardId}`, { method: 'PUT', headers: { 'Content-Type': 'application/json' }, @@ -259,6 +268,7 @@ export async function getBoards(jiraAccountId, projectKeyOrId) { } export async function getBoardSprints(jiraAccountId, boardId) { + assertSafeParam(boardId, 'boardId'); const response = await jiraFetch( jiraAccountId, `/rest/agile/1.0/board/${boardId}/sprint?state=active,future` diff --git a/backend/src/services/pokerService.js b/backend/src/services/pokerService.js index ab62d00..4990820 100644 --- a/backend/src/services/pokerService.js +++ b/backend/src/services/pokerService.js @@ -38,18 +38,22 @@ function deserializeSession(data) { return data; } -async function getSession(sessionId) { - const entry = await kvSessions.get(sessionId); +function sessionKey(cloudId, sessionId) { + return `${cloudId}.${sessionId}`; +} + +async function getSession(cloudId, sessionId) { + const entry = await kvSessions.get(sessionKey(cloudId, sessionId)); if (!entry) return null; return deserializeSession(entry.json()); } async function putSession(session) { - await kvSessions.put(session.id, JSON.stringify(serializeSession(session))); + await kvSessions.put(sessionKey(session.tenantCloudId, session.id), JSON.stringify(serializeSession(session))); } -async function withSessionCas(sessionId, transformFn) { - return withCasRetry(kvSessions, sessionId, (raw) => { +async function withSessionCas(cloudId, sessionId, transformFn) { + return withCasRetry(kvSessions, sessionKey(cloudId, sessionId), (raw) => { const session = deserializeSession(raw); if (!session) return undefined; const result = transformFn(session); @@ -100,13 +104,13 @@ export async function createScopedSession({ issueKey, issueId, issueTitle, roomI const indexEntry = await kvSessionIndex.get(issueIndexKey(tenantCloudId, issueKey)); if (indexEntry) { const existingId = indexEntry.string(); - const existing = await getSession(existingId); + const existing = await getSession(tenantCloudId, existingId); if (existing && existing.tenantCloudId === tenantCloudId) { if (existing.state === 'VOTING') { return getSnapshot(existing); } // Clean up stale revealed/saved sessions - await kvSessions.delete(existingId); + await kvSessions.delete(sessionKey(tenantCloudId, existingId)); await kvSessionIndex.delete(issueIndexKey(tenantCloudId, issueKey)); } } @@ -133,26 +137,26 @@ export async function createScopedSession({ issueKey, issueId, issueTitle, roomI return getSnapshot(session); } -export async function getSessionSnapshot(sessionId) { - const session = await getSession(sessionId); +export async function getSessionSnapshot(sessionId, tenantCloudId) { + const session = await getSession(tenantCloudId, sessionId); if (!session) return null; return getSnapshot(session); } export async function canAccessSession(sessionId, tenantCloudId) { - const session = await getSession(sessionId); + const session = await getSession(tenantCloudId, sessionId); if (!session) return false; return session.tenantCloudId === tenantCloudId; } -export async function isSessionParticipant(sessionId, userKey) { - const session = await getSession(sessionId); +export async function isSessionParticipant(sessionId, tenantCloudId, userKey) { + const session = await getSession(tenantCloudId, sessionId); if (!session) return false; return session.participants.has(userKey); } export async function joinSession({ sessionId, tenantCloudId, userKey, userName, avatarUrl }) { - const result = await withSessionCas(sessionId, (session) => { + const result = await withSessionCas(tenantCloudId, sessionId, (session) => { if (session.tenantCloudId !== tenantCloudId) return undefined; session.participants.set(userKey, { userKey, userName, avatarUrl }); session.votes.delete(userKey); @@ -163,7 +167,7 @@ export async function joinSession({ sessionId, tenantCloudId, userKey, userName, } export async function leaveSession({ sessionId, tenantCloudId, userKey }) { - const result = await withSessionCas(sessionId, (session) => { + const result = await withSessionCas(tenantCloudId, sessionId, (session) => { if (session.tenantCloudId !== tenantCloudId) return undefined; session.participants.delete(userKey); session.votes.delete(userKey); @@ -174,7 +178,7 @@ export async function leaveSession({ sessionId, tenantCloudId, userKey }) { } export async function submitVote({ sessionId, tenantCloudId, userKey, vote }) { - const result = await withSessionCas(sessionId, (session) => { + const result = await withSessionCas(tenantCloudId, sessionId, (session) => { if (session.state === 'REVEALED' || session.state === 'SAVED') return undefined; if (session.state !== 'VOTING') return undefined; if (session.tenantCloudId !== tenantCloudId) return undefined; @@ -185,7 +189,7 @@ export async function submitVote({ sessionId, tenantCloudId, userKey, vote }) { }); if (!result) { // Return current snapshot for REVEALED/SAVED states - const current = await getSession(sessionId); + const current = await getSession(tenantCloudId, sessionId); if (current && (current.state === 'REVEALED' || current.state === 'SAVED')) { return getSnapshot(current); } @@ -194,8 +198,8 @@ export async function submitVote({ sessionId, tenantCloudId, userKey, vote }) { return getSnapshot(result); } -export async function revealIfComplete(sessionId) { - const result = await withSessionCas(sessionId, (session) => { +export async function revealIfComplete(sessionId, tenantCloudId) { + const result = await withSessionCas(tenantCloudId, sessionId, (session) => { const allVoted = session.participants.size > 0 && session.votes.size === session.participants.size; @@ -219,7 +223,7 @@ export async function revealIfComplete(sessionId) { if (!result) { // Not all voted — return current snapshot - const current = await getSession(sessionId); + const current = await getSession(tenantCloudId, sessionId); if (!current) return null; return { ...getSnapshot(current), allVoted: false }; } @@ -234,17 +238,18 @@ export async function revealIfComplete(sessionId) { } export async function saveScopedEstimate({ sessionId, estimate, tenantCloudId, userKey }) { - const session = await getSession(sessionId); - if (!session) return null; - if (session.tenantCloudId !== tenantCloudId) return null; - if (!session.participants.has(userKey)) return null; + const result = await withSessionCas(tenantCloudId, sessionId, (session) => { + if (session.tenantCloudId !== tenantCloudId) return undefined; + if (!session.participants.has(userKey)) return undefined; + session.savedEstimate = estimate; + session.state = 'SAVED'; + return session; + }); + if (!result) return null; - session.savedEstimate = estimate; - session.state = 'SAVED'; - - const snapshot = getSnapshot(session); + const snapshot = getSnapshot(result); // Clean up — session is done - await kvSessions.delete(sessionId); - await kvSessionIndex.delete(issueIndexKey(tenantCloudId, session.issueKey)); + await kvSessions.delete(sessionKey(tenantCloudId, sessionId)); + await kvSessionIndex.delete(issueIndexKey(tenantCloudId, snapshot.session.issueKey)); return snapshot; } diff --git a/frontend/src/components/AdfRenderer.jsx b/frontend/src/components/AdfRenderer.jsx index aecb3a9..5080ff0 100644 --- a/frontend/src/components/AdfRenderer.jsx +++ b/frontend/src/components/AdfRenderer.jsx @@ -79,7 +79,11 @@ function parseWikiInline(text) { } else if (match[5] != null) { parts.push({match[5]}); } else if (match[6] != null && match[7] != null) { - parts.push({match[6]}); + if (/^https?:\/\//i.test(match[7])) { + parts.push({match[6]}); + } else { + parts.push({match[6]}); + } } else if (match[8] != null) { parts.push({match[8]}); }