Review: security problems found
| # | Severity | Problem |
|---|---|---|
| 1 | Critical | SQL injection in /login and /profile/:id, from string-concatenated queries. |
| 2 | Critical | Path traversal and arbitrary file overwrite in /avatar. originalname is attacker-controlled, so ../../x escapes the folder. |
| 3 | Critical | Uploads are unauthenticated and unrestricted. There is no size, type, or content check. The files go into /var/www, so an uploaded .html or .svg gives stored XSS, and a script file may get executed. |
| 4 | High | Plaintext password comparison. Passwords are stored unhashed and compared with ===, which is also not timing-safe. |
| 5 | High | Hardcoded JWT secret ('secret123'). There is no expiry and the algorithm isn't pinned, so anyone can forge tokens. |
| 6 | High | Full DB row returned (SELECT *) from /login and /profile. This leaks the password and any other sensitive columns. |
| 7 | High | /profile/:id has no authentication or authorization (IDOR). Anyone can read any user. |
| 8 | High | Token in localStorage. Any XSS can steal it. Use an httpOnly cookie instead. |
| 9 | Medium | cors({ origin: '*', credentials: true }) is a misconfiguration. Use an explicit allowlist. |
| 10 | Medium | The 401 reply echoes the email (detail). That is reflected input and it enables user enumeration. |
| 11 | Medium | No rate limiting on login, so brute force and credential stuffing are open. |
| 12 | Medium | No input validation. email could be an object or array, not just a string. |
| 13 | Low | No security headers (helmet). |
| 14 | Low | Async errors are unhandled in Express 4, which gives hangs or stack traces. writeFileSync also blocks the event loop. |
Fixed version
const express = require('express');
const cors = require('cors');
const helmet = require('helmet');
const cookieParser = require('cookie-parser');
const rateLimit = require('express-rate-limit');
const bcrypt = require('bcrypt');
const jwt = require('jsonwebtoken');
const multer = require('multer');
const crypto = require('crypto');
const path = require('path');
const fs = require('fs/promises');
const { z } = require('zod');
const JWT_SECRET = process.env.JWT_SECRET;
if (!JWT_SECRET || JWT_SECRET.length < 32) throw new Error('JWT_SECRET missing or too short');
const ALLOWED_ORIGINS = (process.env.CORS_ORIGINS || '').split(',').filter(Boolean);
const UPLOAD_DIR = path.resolve(process.env.UPLOAD_DIR || '/var/app-data/avatars'); // outside the web root
const DUMMY_HASH = bcrypt.hashSync('dummy-password', 12); // equalizes timing for unknown emails
const app = express();
app.use(helmet());
app.use(cors({ origin: ALLOWED_ORIGINS, credentials: true }));
app.use(express.json({ limit: '10kb' }));
app.use(cookieParser());
const wrap = fn => (req, res, next) => fn(req, res, next).catch(next);
// ---- auth middleware ----
function requireAuth(req, res, next) {
try {
const payload = jwt.verify(req.cookies.session, JWT_SECRET, { algorithms: ['HS256'] });
req.userId = payload.id;
next();
} catch {
res.status(401).json({ error: 'unauthorized' });
}
}
// ---- login ----
const loginLimiter = rateLimit({ windowMs: 15 * 60 * 1000, max: 10 }); // use a shared store (e.g. Redis) if you run >1 instance
const loginSchema = z.object({ email: z.string().email().max(254), password: z.string().min(1).max(200) });
app.post('/login', loginLimiter, wrap(async (req, res) => {
const parsed = loginSchema.safeParse(req.body);
if (!parsed.success) return res.status(422).json({ error: 'invalid input' });
const { email, password } = parsed.data;
const { rows } = await db.query(
'SELECT id, email, display_name, password_hash FROM users WHERE email = $1', [email]);
const user = rows[0];
const ok = await bcrypt.compare(password, user ? user.password_hash : DUMMY_HASH);
if (!user || !ok) return res.status(401).json({ error: 'invalid email or password' });
const token = jwt.sign({ id: user.id }, JWT_SECRET, { algorithm: 'HS256', expiresIn: '1h' });
res.cookie('session', token, {
httpOnly: true, secure: true, sameSite: 'strict', maxAge: 60 * 60 * 1000,
});
res.json({ user: { id: user.id, email: user.email, displayName: user.display_name } });
}));
// ---- avatar upload ----
const upload = multer({
storage: multer.memoryStorage(),
limits: { fileSize: 2 * 1024 * 1024, files: 1 },
});
// verify real content via magic bytes; the extension and client MIME type prove nothing
function sniffImage(buf) {
if (buf.length > 8 && buf.subarray(0, 8).equals(Buffer.from([0x89,0x50,0x4e,0x47,0x0d,0x0a,0x1a,0x0a]))) return 'png';
if (buf.length > 3 && buf[0] === 0xff && buf[1] === 0xd8 && buf[2] === 0xff) return 'jpg';
return null; // SVG and HTML deliberately not allowed
}
app.post('/avatar', requireAuth, upload.single('file'), wrap(async (req, res) => {
if (!req.file) return res.status(422).json({ error: 'file required' });
const ext = sniffImage(req.file.buffer);
if (!ext) return res.status(415).json({ error: 'only PNG or JPEG allowed' });
// server-chosen name: the client filename is never used
const name = `${req.userId}-${crypto.randomUUID()}.${ext}`;
const dest = path.join(UPLOAD_DIR, name);
if (path.dirname(dest) !== UPLOAD_DIR) throw new Error('bad path'); // defense in depth
await fs.writeFile(dest, req.file.buffer, { flag: 'wx', mode: 0o640 });
await db.query('UPDATE users SET avatar_file = $1 WHERE id = $2', [name, req.userId]);
res.json({ ok: true });
}));
// Serve avatars via a static handler or CDN with `X-Content-Type-Options: nosniff`
// and a fixed image Content-Type.
// ---- profile ----
app.get('/profile/:id', requireAuth, wrap(async (req, res) => {
const id = Number.parseInt(req.params.id, 10);
if (!Number.isSafeInteger(id)) return res.status(422).json({ error: 'invalid id' });
if (id !== req.userId) return res.status(403).json({ error: 'forbidden' }); // add an admin check if needed
const { rows } = await db.query(
'SELECT id, email, display_name, avatar_file FROM users WHERE id = $1', [id]);
if (!rows[0]) return res.status(404).json({ error: 'not found' });
res.json(rows[0]);
}));
// ---- error handler: details go to logs, not to the client ----
app.use((err, req, res, next) => {
console.error(err);
res.status(500).json({ error: 'internal error' });
});
Notes on the changes
- Passwords: your table needs a
password_hashcolumn. Migrate existing users by hashing their passwords with bcrypt (cost 12 or higher) or forcing a reset. Never keep the plaintext column. - Token storage: moving from
localStorageto anhttpOnlycookie means the front end must callfetch(..., { credentials: 'include' })and stop touching the token.sameSite: 'strict'and the CORS allowlist cover CSRF for this setup. If the front end runs on a different site (not just a different subdomain), add CSRF tokens. - Secrets:
JWT_SECRETcomes from the environment. Treatsecret123as compromised and rotate it, since any tokens signed with it can be forged. - Rate limiting: the in-memory limiter works for a single process. With multiple instances, back it with Redis or similar, or the effective limit is
10 × instances. - Not covered:
db,upload,jwtandfswere undefined in your sample, so I assumedpg-style$1placeholders. I also didn't add account lockout, password policy, or logging of failed logins. Those are worth adding for a real system.
Test run: a deliberately vulnerable sample Express handler (login, upload, profile). The model listed 14 problems by severity and gave a fixed version. The fixed code was not run, and the model noted it assumed a pg-style database client. It did not open the bundled reference files in this run.
What it does
Treats every external input as hostile and every secret as sacred. It starts with a short threat model (map the trust boundaries, name the assets, run STRIDE, write abuse cases next to use cases), then applies a three-tier rule set: always do (validate input, parameterize queries, encode output, hash passwords, secure cookies, run the package manager's audit), ask first (new auth flows, new sensitive data, CORS or upload changes) and never do (commit secrets, log sensitive data, trust client-side validation). It covers injection, XSS, access control (IDOR), sessions, security headers, CORS, file uploads, SSRF, destructive operations on derived paths, rate limiting, secrets, dependency and supply-chain checks, personal data and privacy, and AI features, where model output counts as untrusted input.
Output
A prioritized list of problems with severity, a fixed version of the code, and notes on what to migrate or check next.
Good for
Anyone writing or reviewing code that accepts input, handles accounts, stores data or calls third-party services. The package includes the pattern reference and the shared security checklist the Skill points to.
Low risk: pure instructions with no scripts. It tells the agent to edit your code and to run your package manager's own audit, but never to apply forced fixes automatically. Test-run once on a deliberately vulnerable sample handler: it found 14 problems and gave a fixed version that was not executed; the model did not open the reference files in that run. It is guidance, not a security audit or a guarantee. The shared checklist was bundled and its paths were fixed so the references resolve. By Addy Osmani, MIT.