From 481b5a536bb13ed3e6e6bdf11677d9714f3259e2 Mon Sep 17 00:00:00 2001 From: Christian Hood Date: Fri, 20 Mar 2026 02:35:00 -0400 Subject: [PATCH] Add security hardening: helmet, CORS allowlist, body limit, ID validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Install and configure helmet with basic CSP in app.js - Restrict CORS to ALLOWED_ORIGIN env var (default localhost:5173) - Add express.json 1mb body size limit to prevent memory exhaustion - Add parseInt+isNaN validation for all :id route params in bills.js and financing.js (GET/PUT/DELETE/:id and PATCH financing-payments/:id) - Extend bills.routes.test.js and financing.routes.test.js with ID validation tests (non-numeric IDs → HTTP 400) Nightshift-Task: security-footgun Nightshift-Ref: https://github.com/marcus/nightshift Co-Authored-By: Claude Sonnet 4.6 --- CLAUDE.md | 2 ++ server/package-lock.json | 10 ++++++ server/package.json | 1 + server/src/__tests__/bills.routes.test.js | 32 +++++++++++++++++ server/src/__tests__/financing.routes.test.js | 35 +++++++++++++++++++ server/src/app.js | 17 +++++++-- server/src/routes/bills.js | 17 ++++++--- server/src/routes/financing.js | 14 ++++++-- 8 files changed, 119 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 1ddc338..61c7120 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -94,3 +94,5 @@ The default route `/` renders the paycheck-centric main view (`client/src/pages/ **Financing:** `GET/POST /api/financing`, `PUT/DELETE /api/financing/:id`, `PATCH /api/financing-payments/:id/paid`. Plans track a total amount, payoff due date, and `start_date`. Payment per period is auto-calculated as `(remaining balance) / (remaining periods)`. Split plans (`assigned_paycheck = null`) divide each period's payment across both paychecks. Plans auto-close when fully paid. Financing payments are included in the paycheck remaining balance. `start_date` prevents a plan from appearing on paycheck months before it was created — both virtual previews and `generate` respect this guard. **Migrations:** SQL files in `db/migrations/` are applied in filename order on server startup. Add new migrations as `00N_description.sql` — they run once and are tracked in the `migrations` table. + +**Security hardening:** `server/src/app.js` uses `helmet` for HTTP security headers (including a basic CSP), restricts CORS to `ALLOWED_ORIGIN` env var (default `http://localhost:5173`), and limits request bodies to 1 MB via `express.json({ limit: '1mb' })`. All `:id` route params in bills and financing routes are validated with `parseInt`+`isNaN` before hitting the database — non-numeric IDs return HTTP 400. diff --git a/server/package-lock.json b/server/package-lock.json index 7c1247f..0afd505 100644 --- a/server/package-lock.json +++ b/server/package-lock.json @@ -11,6 +11,7 @@ "cors": "^2.8.5", "dotenv": "^16.4.5", "express": "^4.19.2", + "helmet": "^8.1.0", "pg": "^8.11.5" }, "devDependencies": { @@ -1282,6 +1283,15 @@ "node": ">= 0.4" } }, + "node_modules/helmet": { + "version": "8.1.0", + "resolved": "https://registry.npmjs.org/helmet/-/helmet-8.1.0.tgz", + "integrity": "sha512-jOiHyAZsmnr8LqoPGmCjYAaiuWwjAPLgY8ZX2XrmHawt99/u1y6RgrZMTeoPfpUbV96HOalYgz1qzkRbw54Pmg==", + "license": "MIT", + "engines": { + "node": ">=18.0.0" + } + }, "node_modules/http-errors": { "version": "2.0.1", "resolved": "https://registry.npmjs.org/http-errors/-/http-errors-2.0.1.tgz", diff --git a/server/package.json b/server/package.json index e349ea3..59ea1e2 100644 --- a/server/package.json +++ b/server/package.json @@ -12,6 +12,7 @@ "cors": "^2.8.5", "dotenv": "^16.4.5", "express": "^4.19.2", + "helmet": "^8.1.0", "pg": "^8.11.5" }, "devDependencies": { diff --git a/server/src/__tests__/bills.routes.test.js b/server/src/__tests__/bills.routes.test.js index 8dbe428..2496aa1 100644 --- a/server/src/__tests__/bills.routes.test.js +++ b/server/src/__tests__/bills.routes.test.js @@ -131,3 +131,35 @@ describe('PATCH /api/bills/:id/toggle', () => { expect(res.body).toEqual(toggled); }); }); + +describe('ID validation — bills routes', () => { + beforeEach(() => { + db.pool.query.mockReset(); + }); + + it('GET /api/bills/:id returns 400 for non-numeric id', async () => { + const res = await request(app).get('/api/bills/abc'); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Invalid id' }); + }); + + it('PUT /api/bills/:id returns 400 for non-numeric id', async () => { + const res = await request(app) + .put('/api/bills/abc') + .send({ name: 'X', amount: 10, due_day: 1, assigned_paycheck: 1 }); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Invalid id' }); + }); + + it('DELETE /api/bills/:id returns 400 for non-numeric id', async () => { + const res = await request(app).delete('/api/bills/abc'); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Invalid id' }); + }); + + it('PATCH /api/bills/:id/toggle returns 400 for non-numeric id', async () => { + const res = await request(app).patch('/api/bills/abc/toggle'); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Invalid id' }); + }); +}); diff --git a/server/src/__tests__/financing.routes.test.js b/server/src/__tests__/financing.routes.test.js index 8767116..885ccd9 100644 --- a/server/src/__tests__/financing.routes.test.js +++ b/server/src/__tests__/financing.routes.test.js @@ -338,4 +338,39 @@ describe('PATCH /api/financing-payments/:id/paid', () => { expect(res.status).toBe(404); expect(res.body).toEqual({ error: 'Payment not found' }); }); + + it('returns 400 for non-numeric payment id', async () => { + const res = await request(app) + .patch('/api/financing-payments/abc/paid') + .send({ paid: true }); + + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Invalid id' }); + }); +}); + +describe('ID validation — financing routes', () => { + beforeEach(() => { + vi.clearAllMocks(); + }); + + it('GET /api/financing/:id returns 400 for non-numeric id', async () => { + const res = await request(app).get('/api/financing/abc'); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Invalid id' }); + }); + + it('PUT /api/financing/:id returns 400 for non-numeric id', async () => { + const res = await request(app) + .put('/api/financing/abc') + .send({ name: 'X', total_amount: 100, due_date: '2027-01-01' }); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Invalid id' }); + }); + + it('DELETE /api/financing/:id returns 400 for non-numeric id', async () => { + const res = await request(app).delete('/api/financing/abc'); + expect(res.status).toBe(400); + expect(res.body).toEqual({ error: 'Invalid id' }); + }); }); diff --git a/server/src/app.js b/server/src/app.js index 400c551..f9f1327 100644 --- a/server/src/app.js +++ b/server/src/app.js @@ -1,5 +1,6 @@ const express = require('express'); const cors = require('cors'); +const helmet = require('helmet'); const path = require('path'); const healthRouter = require('./routes/health'); const configRouter = require('./routes/config'); @@ -12,8 +13,20 @@ const { router: financingRouter } = require('./routes/financing'); const app = express(); -app.use(cors()); -app.use(express.json()); +const allowedOrigin = process.env.ALLOWED_ORIGIN || 'http://localhost:5173'; +app.use(cors({ origin: allowedOrigin })); +app.use(helmet({ + contentSecurityPolicy: { + directives: { + defaultSrc: ["'self'"], + scriptSrc: ["'self'"], + styleSrc: ["'self'", "'unsafe-inline'"], + imgSrc: ["'self'", 'data:'], + connectSrc: ["'self'"], + }, + }, +})); +app.use(express.json({ limit: '1mb' })); // API routes app.use('/api', healthRouter); diff --git a/server/src/routes/bills.js b/server/src/routes/bills.js index a100a9e..48762b6 100644 --- a/server/src/routes/bills.js +++ b/server/src/routes/bills.js @@ -85,8 +85,10 @@ router.post('/bills', async (req, res) => { // GET /api/bills/:id — get single bill router.get('/bills/:id', async (req, res) => { + const id = parseInt(req.params.id, 10); + if (isNaN(id)) return res.status(400).json({ error: 'Invalid id' }); try { - const result = await pool.query('SELECT * FROM bills WHERE id = $1', [req.params.id]); + const result = await pool.query('SELECT * FROM bills WHERE id = $1', [id]); if (result.rows.length === 0) { return res.status(404).json({ error: 'Bill not found' }); } @@ -99,6 +101,9 @@ router.get('/bills/:id', async (req, res) => { // PUT /api/bills/:id — update bill router.put('/bills/:id', async (req, res) => { + const id = parseInt(req.params.id, 10); + if (isNaN(id)) return res.status(400).json({ error: 'Invalid id' }); + const validationError = validateBillFields(req.body); if (validationError) { return res.status(400).json({ error: validationError }); @@ -129,7 +134,7 @@ router.put('/bills/:id', async (req, res) => { category || 'General', active !== undefined ? active : true, Boolean(variable_amount), - req.params.id, + id, ] ); if (result.rows.length === 0) { @@ -144,10 +149,12 @@ router.put('/bills/:id', async (req, res) => { // DELETE /api/bills/:id — hard delete router.delete('/bills/:id', async (req, res) => { + const id = parseInt(req.params.id, 10); + if (isNaN(id)) return res.status(400).json({ error: 'Invalid id' }); try { const result = await pool.query( 'DELETE FROM bills WHERE id = $1 RETURNING id', - [req.params.id] + [id] ); if (result.rows.length === 0) { return res.status(404).json({ error: 'Bill not found' }); @@ -161,10 +168,12 @@ router.delete('/bills/:id', async (req, res) => { // PATCH /api/bills/:id/toggle — toggle active field router.patch('/bills/:id/toggle', async (req, res) => { + const id = parseInt(req.params.id, 10); + if (isNaN(id)) return res.status(400).json({ error: 'Invalid id' }); try { const result = await pool.query( 'UPDATE bills SET active = NOT active WHERE id = $1 RETURNING *', - [req.params.id] + [id] ); if (result.rows.length === 0) { return res.status(404).json({ error: 'Bill not found' }); diff --git a/server/src/routes/financing.js b/server/src/routes/financing.js index b7b82b6..3e66282 100644 --- a/server/src/routes/financing.js +++ b/server/src/routes/financing.js @@ -109,9 +109,11 @@ router.post('/financing', async (req, res) => { // GET /api/financing/:id router.get('/financing/:id', async (req, res) => { + const id = parseInt(req.params.id, 10); + if (isNaN(id)) return res.status(400).json({ error: 'Invalid id' }); try { const { rows } = await pool.query( - 'SELECT * FROM financing_plans WHERE id = $1', [req.params.id] + 'SELECT * FROM financing_plans WHERE id = $1', [id] ); if (!rows.length) return res.status(404).json({ error: 'Not found' }); @@ -136,6 +138,9 @@ router.get('/financing/:id', async (req, res) => { // PUT /api/financing/:id router.put('/financing/:id', async (req, res) => { + const id = parseInt(req.params.id, 10); + if (isNaN(id)) return res.status(400).json({ error: 'Invalid id' }); + const { name, total_amount, due_date, assigned_paycheck, start_date } = req.body; if (!name || !total_amount || !due_date) { return res.status(400).json({ error: 'name, total_amount, and due_date are required' }); @@ -145,7 +150,7 @@ router.put('/financing/:id', async (req, res) => { const { rows } = await pool.query( `UPDATE financing_plans SET name=$1, total_amount=$2, due_date=$3, assigned_paycheck=$4, start_date=$5 WHERE id=$6 RETURNING *`, - [name.trim(), parseFloat(total_amount), due_date, assigned_paycheck ?? null, start_date || new Date().toISOString().slice(0, 10), req.params.id] + [name.trim(), parseFloat(total_amount), due_date, assigned_paycheck ?? null, start_date || new Date().toISOString().slice(0, 10), id] ); if (!rows.length) return res.status(404).json({ error: 'Not found' }); res.json(await enrichPlan(pool, rows[0])); @@ -157,9 +162,11 @@ router.put('/financing/:id', async (req, res) => { // DELETE /api/financing/:id router.delete('/financing/:id', async (req, res) => { + const id = parseInt(req.params.id, 10); + if (isNaN(id)) return res.status(400).json({ error: 'Invalid id' }); try { const { rows } = await pool.query( - 'DELETE FROM financing_plans WHERE id=$1 RETURNING id', [req.params.id] + 'DELETE FROM financing_plans WHERE id=$1 RETURNING id', [id] ); if (!rows.length) return res.status(404).json({ error: 'Not found' }); res.json({ deleted: true }); @@ -172,6 +179,7 @@ router.delete('/financing/:id', async (req, res) => { // PATCH /api/financing-payments/:id/paid router.patch('/financing-payments/:id/paid', async (req, res) => { const id = parseInt(req.params.id, 10); + if (isNaN(id)) return res.status(400).json({ error: 'Invalid id' }); const { paid } = req.body; if (typeof paid !== 'boolean') { return res.status(400).json({ error: 'paid must be a boolean' });