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.
它做什麼
把每個外部輸入都當作有敵意的,把每個金鑰都當作機密。它先做一次簡短的威脅建模(畫出信任邊界、列出資產、過一遍 STRIDE、在使用案例旁寫濫用案例),然後套用三層規則:一律做(驗證輸入、參數化查詢、輸出編碼、雜湊密碼、安全的 Cookie、執行套件管理員內建的稽核),先問再做(新增認證流程、新增敏感資料、改 CORS 或上傳),絕不做(提交金鑰、記錄敏感資料、把用戶端驗證當安全邊界)。涵蓋注入、XSS、存取控制(IDOR)、工作階段、安全回應標頭、CORS、檔案上傳、SSRF、由推導路徑觸發的破壞性操作、限流、金鑰、相依套件與供應鏈檢查、個人資料與隱私,以及 AI 功能(模型輸出同樣視為不可信輸入)。
輸出
附嚴重程度的問題清單、修正後的程式碼,以及需要遷移或繼續檢查的事項。
適合什麼場景
撰寫或審查會接收輸入、處理帳號、儲存資料或呼叫第三方服務的程式碼的人。套件內附有 Skill 引用的模式參考與共用的安全檢查清單。
低風險:純指令檔,沒有腳本。它會讓智能體修改你的程式碼,並執行你的套件管理員內建的稽核,但不會自動執行強制修復。已用一個故意寫得有漏洞的範例介面試用一次:找出 14 個問題並給出修正版本,修正後的程式碼沒有執行;那次模型沒有開啟套件內的參考檔。它是指引,不是安全稽核,也不是保證。共用的檢查清單已一起打包並修正路徑,確保引用找得到。作者 Addy Osmani,MIT 授權。