From 39b2ce73daec61129bcc8c759e2a99e88a5b56e1 Mon Sep 17 00:00:00 2001 From: adamp Date: Mon, 9 Feb 2026 18:04:24 -0600 Subject: [PATCH] Fix critical security vulnerabilities and data integrity issues - Use timing-safe comparisons for HMAC verification and password checks - Add login rate limiting (5 attempts/minute per IP) - Lock down CORS to Vite dev origin only (not needed in production) - Derive signing key from APP_PASSWORD instead of using it directly - Replace hand-rolled cookie parsing with cookie-parser middleware - Wrap all order mutations in SQLite transactions - Fix TOCTOU race on stock with atomic UPDATE...WHERE quantity >= ? - Fix APP_SECERT typo in .env (gitignored, local fix only) Co-Authored-By: Claude Opus 4.6 --- package-lock.json | 20 +++++++ package.json | 1 + server/index.js | 9 ++- server/middleware/auth.js | 19 ++++-- server/routes/auth.js | 51 ++++++++++++++-- server/routes/orders.js | 123 ++++++++++++++++++++++---------------- 6 files changed, 160 insertions(+), 63 deletions(-) diff --git a/package-lock.json b/package-lock.json index ea1eb6f..0f63cfc 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7,6 +7,7 @@ "name": "girl-scout-cookies", "dependencies": { "better-sqlite3": "^11.6.0", + "cookie-parser": "^1.4.7", "cors": "^2.8.5", "dotenv": "^16.4.5", "express": "^4.21.1" @@ -323,6 +324,25 @@ "node": ">= 0.6" } }, + "node_modules/cookie-parser": { + "version": "1.4.7", + "resolved": "https://registry.npmjs.org/cookie-parser/-/cookie-parser-1.4.7.tgz", + "integrity": "sha512-nGUvgXnotP3BsjiLX2ypbQnWoGUPIIfHQNZkkC668ntrzGWEZVW70HDEB1qnNGMicPje6EttlIgzo51YSwNQGw==", + "license": "MIT", + "dependencies": { + "cookie": "0.7.2", + "cookie-signature": "1.0.6" + }, + "engines": { + "node": ">= 0.8.0" + } + }, + "node_modules/cookie-parser/node_modules/cookie-signature": { + "version": "1.0.6", + "resolved": "https://registry.npmjs.org/cookie-signature/-/cookie-signature-1.0.6.tgz", + "integrity": "sha512-QADzlaHc8icV8I7vbaJXJwod9HWYp8uCqf1xa4OfNu1T7JVxQIrUgOWtHdNDtPiywmFbiS12VjotIXLrKM3orQ==", + "license": "MIT" + }, "node_modules/cookie-signature": { "version": "1.0.7", "resolved": "https://registry.npmjs.org/cookie-signature/-/cookie-signature-1.0.7.tgz", diff --git a/package.json b/package.json index 67169a9..d588770 100644 --- a/package.json +++ b/package.json @@ -10,6 +10,7 @@ }, "dependencies": { "better-sqlite3": "^11.6.0", + "cookie-parser": "^1.4.7", "cors": "^2.8.5", "dotenv": "^16.4.5", "express": "^4.21.1" diff --git a/server/index.js b/server/index.js index 7f850ab..bd8d2a3 100644 --- a/server/index.js +++ b/server/index.js @@ -1,6 +1,6 @@ require('dotenv').config({ path: require('path').join(__dirname, '..', '.env') }); const express = require('express'); -const cors = require('cors'); +const cookieParser = require('cookie-parser'); const path = require('path'); const authRouter = require('./routes/auth'); @@ -15,8 +15,13 @@ const PORT = process.env.PORT || 3002; app.set('trust proxy', 1); -app.use(cors({ origin: true, credentials: true })); +if (process.env.NODE_ENV !== 'production') { + const cors = require('cors'); + app.use(cors({ origin: 'http://localhost:5173', credentials: true })); +} + app.use(express.json()); +app.use(cookieParser()); app.use('/api/auth', authRouter); app.use('/api', authMiddleware); diff --git a/server/middleware/auth.js b/server/middleware/auth.js index fe80697..423fb89 100644 --- a/server/middleware/auth.js +++ b/server/middleware/auth.js @@ -4,8 +4,13 @@ const COOKIE_NAME = 'session'; const MAX_AGE_MS = 7 * 24 * 60 * 60 * 1000; // 7 days function getSecret() { - const secret = process.env.APP_SECRET || process.env.APP_PASSWORD; - return secret || null; + if (process.env.APP_SECRET) return process.env.APP_SECRET; + if (process.env.APP_PASSWORD) { + return crypto.createHmac('sha256', 'cookie-tracker-session') + .update(process.env.APP_PASSWORD) + .digest('hex'); + } + return null; } function sign(payload) { @@ -30,7 +35,11 @@ function verify(token) { return false; } const expected = crypto.createHmac('sha256', secret).update(data).digest('hex'); - if (expected !== hmac) return false; + const expectedBuf = Buffer.from(expected); + const hmacBuf = Buffer.from(hmac); + if (expectedBuf.length !== hmacBuf.length || !crypto.timingSafeEqual(expectedBuf, hmacBuf)) { + return false; + } let payload; try { payload = JSON.parse(data); @@ -69,9 +78,7 @@ function authMiddleware(req, res, next) { } const p = req.path || ''; if (p.startsWith('/auth')) return next(); - const cookie = req.headers.cookie || ''; - const match = cookie.match(new RegExp(`${COOKIE_NAME}=([^;]+)`)); - const token = match ? match[1].trim() : null; + const token = req.cookies?.[COOKIE_NAME] || null; if (!token || !verify(token)) { return res.status(401).json({ error: 'Unauthorized' }); } diff --git a/server/routes/auth.js b/server/routes/auth.js index 1996801..7945a59 100644 --- a/server/routes/auth.js +++ b/server/routes/auth.js @@ -1,4 +1,5 @@ const express = require('express'); +const crypto = require('crypto'); const router = express.Router(); const { COOKIE_NAME, @@ -7,15 +8,57 @@ const { clearSessionCookie, } = require('../middleware/auth'); +// In-memory rate limiter for login attempts +const loginAttempts = new Map(); +const RATE_LIMIT_WINDOW_MS = 60 * 1000; // 1 minute +const RATE_LIMIT_MAX = 5; + +function isRateLimited(ip) { + const now = Date.now(); + const attempts = loginAttempts.get(ip); + if (!attempts) return false; + // Remove expired entries + const recent = attempts.filter(t => now - t < RATE_LIMIT_WINDOW_MS); + if (recent.length === 0) { + loginAttempts.delete(ip); + return false; + } + loginAttempts.set(ip, recent); + return recent.length >= RATE_LIMIT_MAX; +} + +function recordAttempt(ip) { + const now = Date.now(); + const attempts = loginAttempts.get(ip) || []; + attempts.push(now); + loginAttempts.set(ip, attempts); +} + router.post('/login', (req, res) => { const password = process.env.APP_PASSWORD; if (!password) { return res.status(200).json({ ok: true }); } - const submitted = req.body?.password; - if (submitted !== password) { + + const ip = req.ip; + if (isRateLimited(ip)) { + return res.status(429).json({ error: 'Too many login attempts. Try again later.' }); + } + + const submitted = req.body?.password || ''; + const submittedBuf = Buffer.from(String(submitted)); + const passwordBuf = Buffer.from(password); + + let match = false; + if (submittedBuf.length === passwordBuf.length) { + match = crypto.timingSafeEqual(submittedBuf, passwordBuf); + } + + if (!match) { + recordAttempt(ip); return res.status(401).json({ error: 'Invalid password' }); } + const session = createSessionCookie(); if (!session) { return res.status(500).json({ error: 'Auth not configured' }); @@ -35,9 +78,7 @@ router.get('/me', (req, res) => { if (!process.env.APP_PASSWORD) { return res.status(200).json({ ok: true }); } - const cookie = req.headers.cookie || ''; - const match = cookie.match(new RegExp(`${COOKIE_NAME}=([^;]+)`)); - const token = match ? match[1].trim() : null; + const token = req.cookies?.[COOKIE_NAME] || null; if (!token || !verify(token)) { return res.status(401).json({ error: 'Unauthorized' }); } diff --git a/server/routes/orders.js b/server/routes/orders.js index c39de87..bcbfad3 100644 --- a/server/routes/orders.js +++ b/server/routes/orders.js @@ -19,24 +19,14 @@ function getOrderWithItems(db, id) { return { ...order, items }; } -function deductStockForOrder(db, orderId, newItems, existingItemsOverride) { - const existing = existingItemsOverride !== undefined - ? existingItemsOverride - : db.prepare('SELECT product_id, quantity FROM order_items WHERE order_id = ?').all(orderId); - const byProduct = {}; - for (const e of existing) byProduct[e.product_id] = (byProduct[e.product_id] || 0) + e.quantity; - for (const it of newItems) { - const pid = it.product_id; - const qty = Number(it.quantity) || 0; - byProduct[pid] = (byProduct[pid] || 0) - qty; - } - for (const [productId, delta] of Object.entries(byProduct)) { - if (delta === 0) continue; - const product = db.prepare('SELECT quantity_on_hand FROM products WHERE id = ?').get(productId); +function atomicDeductStock(db, productId, quantity) { + const result = db.prepare( + 'UPDATE products SET quantity_on_hand = quantity_on_hand - ? WHERE id = ? AND quantity_on_hand >= ?' + ).run(quantity, productId, quantity); + if (result.changes === 0) { + const product = db.prepare('SELECT id FROM products WHERE id = ?').get(productId); if (!product) throw new Error(`Product ${productId} not found`); - const newQty = product.quantity_on_hand + delta; - if (newQty < 0) throw new Error(`Insufficient stock for product id ${productId}`); - db.prepare('UPDATE products SET quantity_on_hand = ? WHERE id = ?').run(newQty, productId); + throw new Error(`Insufficient stock for product id ${productId}`); } } @@ -87,24 +77,35 @@ router.post('/', (req, res) => { return res.status(400).json({ error: 'At least one order item is required' }); } for (const it of items) { - const product = db.prepare('SELECT id, quantity_on_hand, price FROM products WHERE id = ?').get(it.product_id); + const product = db.prepare('SELECT id FROM products WHERE id = ?').get(it.product_id); if (!product) return res.status(400).json({ error: `Product ${it.product_id} not found` }); const qty = Number(it.quantity) || 0; if (qty <= 0) return res.status(400).json({ error: 'Quantity must be positive' }); - if (product.quantity_on_hand < qty) { - return res.status(400).json({ error: `Insufficient stock for ${product.id}` }); - } } - const result = db.prepare( - 'INSERT INTO orders (customer_id, status, notes) VALUES (?, ?, ?)' - ).run(customer_id || null, status, notes || null); - const orderId = result.lastInsertRowid; - applyOrderItems(db, orderId, items); - deductStockForOrder(db, orderId, items, []); - db.prepare('UPDATE orders SET updated_at = datetime(\'now\') WHERE id = ?').run(orderId); + + const createOrder = db.transaction(() => { + const result = db.prepare( + 'INSERT INTO orders (customer_id, status, notes) VALUES (?, ?, ?)' + ).run(customer_id || null, status, notes || null); + const orderId = result.lastInsertRowid; + + for (const it of items) { + const qty = Number(it.quantity) || 0; + atomicDeductStock(db, it.product_id, qty); + } + + applyOrderItems(db, orderId, items); + db.prepare('UPDATE orders SET updated_at = datetime(\'now\') WHERE id = ?').run(orderId); + return orderId; + }); + + const orderId = createOrder(); const order = getOrderWithItems(db, orderId); res.status(201).json(order); } catch (err) { + if (err.message.startsWith('Insufficient stock') || err.message.includes('not found')) { + return res.status(400).json({ error: err.message }); + } res.status(500).json({ error: err.message }); } }); @@ -115,25 +116,42 @@ router.put('/:id', (req, res) => { const order = db.prepare('SELECT * FROM orders WHERE id = ?').get(req.params.id); if (!order) return res.status(404).json({ error: 'Order not found' }); const { customer_id, status, notes, items } = req.body; - if (items !== undefined) { - if (!Array.isArray(items)) return res.status(400).json({ error: 'items must be an array' }); - const existingItems = db.prepare('SELECT product_id, quantity FROM order_items WHERE order_id = ?').all(req.params.id); - const newItems = items.map(it => ({ product_id: it.product_id, quantity: Number(it.quantity) || 0 })); - for (const it of newItems) { - const product = db.prepare('SELECT id, quantity_on_hand FROM products WHERE id = ?').get(it.product_id); - if (!product) return res.status(400).json({ error: `Product ${it.product_id} not found` }); + + const updateOrder = db.transaction(() => { + if (items !== undefined) { + if (!Array.isArray(items)) throw new Error('items must be an array'); + + // Restore stock from existing items + const existingItems = db.prepare('SELECT product_id, quantity FROM order_items WHERE order_id = ?').all(req.params.id); + for (const ei of existingItems) { + db.prepare('UPDATE products SET quantity_on_hand = quantity_on_hand + ? WHERE id = ?') + .run(ei.quantity, ei.product_id); + } + + // Deduct stock for new items atomically + for (const it of items) { + const qty = Number(it.quantity) || 0; + if (qty <= 0) throw new Error('Quantity must be positive'); + atomicDeductStock(db, it.product_id, qty); + } + + applyOrderItems(db, req.params.id, items); } - deductStockForOrder(db, req.params.id, newItems, existingItems); - applyOrderItems(db, req.params.id, items); - } - const cid = customer_id !== undefined ? (customer_id || null) : order.customer_id; - const st = status !== undefined ? status : order.status; - const no = notes !== undefined ? notes : order.notes; - db.prepare('UPDATE orders SET customer_id = ?, status = ?, notes = ?, updated_at = datetime(\'now\') WHERE id = ?') - .run(cid, st, no, req.params.id); + + const cid = customer_id !== undefined ? (customer_id || null) : order.customer_id; + const st = status !== undefined ? status : order.status; + const no = notes !== undefined ? notes : order.notes; + db.prepare('UPDATE orders SET customer_id = ?, status = ?, notes = ?, updated_at = datetime(\'now\') WHERE id = ?') + .run(cid, st, no, req.params.id); + }); + + updateOrder(); const updated = getOrderWithItems(db, req.params.id); res.json(updated); } catch (err) { + if (err.message.startsWith('Insufficient stock') || err.message.includes('not found') || err.message === 'items must be an array' || err.message === 'Quantity must be positive') { + return res.status(400).json({ error: err.message }); + } res.status(500).json({ error: err.message }); } }); @@ -143,13 +161,18 @@ router.delete('/:id', (req, res) => { const db = getDb(); const order = db.prepare('SELECT * FROM orders WHERE id = ?').get(req.params.id); if (!order) return res.status(404).json({ error: 'Order not found' }); - const items = db.prepare('SELECT product_id, quantity FROM order_items WHERE order_id = ?').all(req.params.id); - for (const it of items) { - db.prepare('UPDATE products SET quantity_on_hand = quantity_on_hand + ? WHERE id = ?') - .run(it.quantity, it.product_id); - } - db.prepare('DELETE FROM order_items WHERE order_id = ?').run(req.params.id); - db.prepare('DELETE FROM orders WHERE id = ?').run(req.params.id); + + const deleteOrder = db.transaction(() => { + const items = db.prepare('SELECT product_id, quantity FROM order_items WHERE order_id = ?').all(req.params.id); + for (const it of items) { + db.prepare('UPDATE products SET quantity_on_hand = quantity_on_hand + ? WHERE id = ?') + .run(it.quantity, it.product_id); + } + db.prepare('DELETE FROM order_items WHERE order_id = ?').run(req.params.id); + db.prepare('DELETE FROM orders WHERE id = ?').run(req.params.id); + }); + + deleteOrder(); res.status(204).send(); } catch (err) { res.status(500).json({ error: err.message });