Skip to content

Commit 16d53fa

Browse files
committed
fix(members-api): add rate limiting, CSV injection protection, XP transaction safety
- CSV injection: escapeCsv now prefixes formula chars (=,+,-,@,\t,\r) with single quote - XP bounds: cap adjustments to ±1,000,000 - XP transaction: wrap upsert + level update in BEGIN/COMMIT/ROLLBACK - Warning filter: add AND action = 'warn' to recent warnings query in member detail - CSV export: paginate guild.members.list() for guilds > 1000 members - Pool safety: wrap getPool() in safeGetPool() with try/catch, return 503 on failure - Search total: return filteredTotal alongside total when search is active Addresses review comments #8-15 on PR #119.
1 parent 6b8547f commit 16d53fa

1 file changed

Lines changed: 102 additions & 33 deletions

File tree

src/api/routes/members.js

Lines changed: 102 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,19 @@ function getRepConfig(guildId) {
2929
return { ...REPUTATION_DEFAULTS, ...cfg.reputation };
3030
}
3131

32+
/**
33+
* Safely get the database pool, returning null if unavailable.
34+
* Routes should return 503 when this returns null.
35+
* @returns {import('pg').Pool | null}
36+
*/
37+
function safeGetPool() {
38+
try {
39+
return getPool();
40+
} catch {
41+
return null;
42+
}
43+
}
44+
3245
// ─── GET /:id/members/export — CSV export (must be before /:userId) ──────────
3346

3447
/**
@@ -43,10 +56,26 @@ router.get(
4356
async (req, res) => {
4457
try {
4558
const guild = req.guild;
46-
const pool = getPool();
59+
const pool = safeGetPool();
60+
if (!pool) {
61+
return res.status(503).json({ error: 'Database unavailable' });
62+
}
4763

48-
// Fetch all members (Discord caps at 1000 per call)
49-
const members = await guild.members.list({ limit: 1000 });
64+
// Fetch all members — paginate in batches of 1000 for large guilds
65+
let members = new Map();
66+
let lastId;
67+
// eslint-disable-next-line no-constant-condition
68+
while (true) {
69+
const fetchOpts = { limit: 1000 };
70+
if (lastId) fetchOpts.after = lastId;
71+
const batch = await guild.members.list(fetchOpts);
72+
if (batch.size === 0) break;
73+
for (const [id, member] of batch) {
74+
members.set(id, member);
75+
}
76+
lastId = Array.from(batch.keys()).pop();
77+
if (batch.size < 1000) break; // Last page
78+
}
5079

5180
const userIds = Array.from(members.keys());
5281

@@ -251,11 +280,16 @@ router.get('/:id/members', membersRateLimit, requireGuildAdmin, validateGuild, a
251280
// Cursor is based on last Discord member ID from the original fetch
252281
const lastDiscordMember = Array.from(members.values()).pop();
253282

254-
res.json({
283+
const response = {
255284
members: enriched,
256285
nextAfter: lastDiscordMember ? lastDiscordMember.id : null,
257286
total: guild.memberCount,
258-
});
287+
};
288+
// When search is active, include filtered count so the UI can show accurate totals
289+
if (search) {
290+
response.filteredTotal = enriched.length;
291+
}
292+
res.json(response);
259293
} catch (err) {
260294
logError('Failed to fetch enriched members', { error: err.message, guild: req.params.id });
261295
res.status(500).json({ error: 'Failed to fetch members' });
@@ -277,7 +311,10 @@ router.get(
277311

278312
try {
279313
const guild = req.guild;
280-
const pool = getPool();
314+
const pool = safeGetPool();
315+
if (!pool) {
316+
return res.status(503).json({ error: 'Database unavailable' });
317+
}
281318

282319
// Fetch Discord member
283320
let member;
@@ -310,7 +347,7 @@ router.get(
310347
pool.query(
311348
`SELECT case_number, action, reason, moderator_tag, created_at
312349
FROM mod_cases
313-
WHERE guild_id = $1 AND target_id = $2
350+
WHERE guild_id = $1 AND target_id = $2 AND action = 'warn'
314351
ORDER BY created_at DESC
315352
LIMIT 5`,
316353
[guild.id, userId],
@@ -393,7 +430,10 @@ router.get(
393430
const offset = (page - 1) * limit;
394431

395432
try {
396-
const pool = getPool();
433+
const pool = safeGetPool();
434+
if (!pool) {
435+
return res.status(503).json({ error: 'Database unavailable' });
436+
}
397437

398438
const [casesResult, countResult] = await Promise.all([
399439
pool.query(
@@ -453,33 +493,55 @@ router.post(
453493
return res.status(400).json({ error: 'amount must be a non-zero finite number' });
454494
}
455495

496+
// Cap adjustment to ±1,000,000
497+
if (Math.abs(amount) > 1_000_000) {
498+
return res.status(400).json({ error: 'amount must be between -1000000 and 1000000' });
499+
}
500+
456501
try {
457-
const pool = getPool();
502+
const pool = safeGetPool();
503+
if (!pool) {
504+
return res.status(503).json({ error: 'Database unavailable' });
505+
}
458506
const guildId = req.guild.id;
459507

460-
// Upsert reputation and adjust XP (floor at 0)
461-
const { rows } = await pool.query(
462-
`INSERT INTO reputation (guild_id, user_id, xp, level)
463-
VALUES ($1, $2, GREATEST(0, $3), 0)
464-
ON CONFLICT (guild_id, user_id) DO UPDATE
465-
SET xp = GREATEST(0, reputation.xp + $3)
466-
RETURNING xp, level`,
467-
[guildId, userId, amount],
468-
);
469-
470-
const { xp: newXp } = rows[0];
471-
472-
// Recompute level from thresholds
473-
const repConfig = getRepConfig(guildId);
474-
const newLevel = computeLevel(newXp, repConfig.levelThresholds);
508+
// Wrap XP upsert + level update in a transaction for consistency
509+
const client = await pool.connect();
510+
let newXp, newLevel;
511+
try {
512+
await client.query('BEGIN');
513+
514+
// Upsert reputation and adjust XP (floor at 0)
515+
const { rows } = await client.query(
516+
`INSERT INTO reputation (guild_id, user_id, xp, level)
517+
VALUES ($1, $2, GREATEST(0, $3), 0)
518+
ON CONFLICT (guild_id, user_id) DO UPDATE
519+
SET xp = GREATEST(0, reputation.xp + $3)
520+
RETURNING xp, level`,
521+
[guildId, userId, amount],
522+
);
523+
524+
newXp = rows[0].xp;
525+
526+
// Recompute level from thresholds
527+
const repConfig = getRepConfig(guildId);
528+
newLevel = computeLevel(newXp, repConfig.levelThresholds);
529+
530+
// Update level if changed
531+
if (newLevel !== rows[0].level) {
532+
await client.query('UPDATE reputation SET level = $1 WHERE guild_id = $2 AND user_id = $3', [
533+
newLevel,
534+
guildId,
535+
userId,
536+
]);
537+
}
475538

476-
// Update level if changed
477-
if (newLevel !== rows[0].level) {
478-
await pool.query('UPDATE reputation SET level = $1 WHERE guild_id = $2 AND user_id = $3', [
479-
newLevel,
480-
guildId,
481-
userId,
482-
]);
539+
await client.query('COMMIT');
540+
} catch (txErr) {
541+
await client.query('ROLLBACK');
542+
throw txErr;
543+
} finally {
544+
client.release();
483545
}
484546

485547
info('XP adjusted via API', {
@@ -511,13 +573,20 @@ router.post(
511573
);
512574

513575
/**
514-
* Escape a value for CSV output (handles commas, quotes, newlines).
576+
* Escape a value for CSV output.
577+
* Handles commas, quotes, newlines, and formula-injection characters
578+
* (=, +, -, @, \t, \r) by prefixing with a single quote.
515579
* @param {string} value
516580
* @returns {string}
517581
*/
518582
function escapeCsv(value) {
519583
if (value == null) return '';
520-
const str = String(value);
584+
let str = String(value);
585+
// Prevent CSV formula injection — prefix dangerous leading chars
586+
const formulaChars = ['=', '+', '-', '@', '\t', '\r'];
587+
if (str.length > 0 && formulaChars.includes(str[0])) {
588+
str = "'" + str;
589+
}
521590
if (str.includes(',') || str.includes('"') || str.includes('\n') || str.includes('\r')) {
522591
return `"${str.replace(/"/g, '""')}"`;
523592
}

0 commit comments

Comments
 (0)