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 <noreply@anthropic.com>
2026-02-09 18:04:24 -06:00 · 2026-02-09 18:04:24 -06:00 · 39b2ce73da
commit 39b2ce73da
parent b0e4e977c1
6 changed files with 160 additions and 63 deletions
--- 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",
--- 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"
--- 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);
--- 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' });
  }
--- 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' });
  }
--- 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 });