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。