Add security hardening: helmet, CORS allowlist, body limit, ID validation #8

Open
iswa wants to merge 1 commits from security/fix-footguns into master
8 changed files with 119 additions and 9 deletions
Showing only changes of commit 481b5a536b - Show all commits

View File

@@ -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.

View File

@@ -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",

View File

@@ -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": {

View File

@@ -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' });
});
});

View File

@@ -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' });
});
});

View File

@@ -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);

View File

@@ -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' });

View File

@@ -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' });