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 协议。