Skip to content

Commit c7bcee8

Browse files
committed
fix: improve code quality, security & reliability across extension and server
Extension: - Add AbortController with 10s timeout to fetchStats/fetchLeaderboard - Clear provider loading state on fetch errors to avoid stuck spinners - Fix XSS vulnerability in leaderboard tooltip (isTrusted=false, appendText) - Fix setLoading() to always fire tree refresh in both providers - Replace local StatsData interface with UserStatsDTO from shared package Server: - Strengthen GITHUB_WEBHOOK_SECRET validation (trim + min 32 chars) - Add max-length validation (255) to session language/project fields - Extract parseStreakData helper to deduplicate streak parsing logic - Fix Lua streak script timezone issue with UTC yesterday comparison - Fix comment numbering consistency in stats route - Add cursor-based pagination to /achievements endpoint - Fix streak achievements to award all applicable milestones (not just highest) - Fail fast on missing rawBody in webhooks (no re-serialization fallback) - Minimize GitHub push payload schema (remove PII: email, author, pusher) - Harden signature verification (sha256= prefix, getHeader helper, utf8) - Add delivery-id deduplication to prevent duplicate webhook processing - Fix achievement race condition with P2002 error handling Shared: - Add webhookDelivery Redis key for deduplication
1 parent 2b1beb8 commit c7bcee8

8 files changed

Lines changed: 260 additions & 158 deletions

File tree

apps/extension/src/extension.ts

Lines changed: 30 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,11 @@ class DevRadarExtension implements vscode.Disposable {
311311

312312
/** Fetch user stats from the server. */
313313
private async fetchStats(): Promise<void> {
314+
const controller = new AbortController();
315+
const timeout = setTimeout(() => {
316+
controller.abort();
317+
}, 10_000);
318+
314319
try {
315320
const token = this.authService.getToken();
316321
if (!token) return;
@@ -321,21 +326,35 @@ class DevRadarExtension implements vscode.Disposable {
321326
Authorization: `Bearer ${token}`,
322327
'Content-Type': 'application/json',
323328
},
329+
signal: controller.signal,
324330
});
325331

326332
if (response.ok) {
327333
const json = (await response.json()) as { data: UserStatsDTO };
328334
this.statsProvider.updateStats(json.data);
329335
} else {
330336
this.logger.warn('Failed to fetch stats', { status: response.status });
337+
this.statsProvider.setLoading(false);
331338
}
332339
} catch (error) {
333-
this.logger.warn('Failed to fetch stats', error);
340+
if (error instanceof Error && error.name === 'AbortError') {
341+
this.logger.warn('Stats fetch timed out after 10s');
342+
} else {
343+
this.logger.warn('Failed to fetch stats', error);
344+
}
345+
this.statsProvider.setLoading(false);
346+
} finally {
347+
clearTimeout(timeout);
334348
}
335349
}
336350

337351
/** Fetch friends leaderboard from the server. */
338352
private async fetchLeaderboard(): Promise<void> {
353+
const controller = new AbortController();
354+
const timeout = setTimeout(() => {
355+
controller.abort();
356+
}, 10_000);
357+
339358
try {
340359
const token = this.authService.getToken();
341360
if (!token) return;
@@ -346,6 +365,7 @@ class DevRadarExtension implements vscode.Disposable {
346365
Authorization: `Bearer ${token}`,
347366
'Content-Type': 'application/json',
348367
},
368+
signal: controller.signal,
349369
});
350370

351371
if (response.ok) {
@@ -355,9 +375,17 @@ class DevRadarExtension implements vscode.Disposable {
355375
this.leaderboardProvider.updateLeaderboard(json.data.leaderboard, json.data.myRank);
356376
} else {
357377
this.logger.warn('Failed to fetch leaderboard', { status: response.status });
378+
this.leaderboardProvider.setLoading(false);
358379
}
359380
} catch (error) {
360-
this.logger.warn('Failed to fetch leaderboard', error);
381+
if (error instanceof Error && error.name === 'AbortError') {
382+
this.logger.warn('Leaderboard fetch timed out after 10s');
383+
} else {
384+
this.logger.warn('Failed to fetch leaderboard', error);
385+
}
386+
this.leaderboardProvider.setLoading(false);
387+
} finally {
388+
clearTimeout(timeout);
361389
}
362390
}
363391

apps/extension/src/views/index.ts

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,5 @@ export { FriendsProvider, type FriendInfo } from './friendsProvider';
22
export { FriendRequestsProvider } from './friendRequestsProvider';
33
export { ActivityProvider, type ActivityEvent } from './activityProvider';
44
export { StatusBarManager } from './statusBarItem';
5-
export { StatsProvider, type StatsData } from './statsProvider';
5+
export { StatsProvider } from './statsProvider';
66
export { LeaderboardProvider } from './leaderboardProvider';
7-

apps/extension/src/views/leaderboardProvider.ts

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -58,10 +58,15 @@ class LeaderboardTreeItem extends vscode.TreeItem {
5858
/** Build rich tooltip with details. */
5959
private buildTooltip(entry: LeaderboardEntry): vscode.MarkdownString {
6060
const md = new vscode.MarkdownString();
61-
md.isTrusted = true;
61+
md.isTrusted = false;
6262

63-
md.appendMarkdown(`### ${entry.displayName ?? entry.username}\n\n`);
64-
md.appendMarkdown(`**@${entry.username}**\n\n`);
63+
md.appendMarkdown(`### `);
64+
md.appendText(entry.displayName ?? entry.username);
65+
md.appendMarkdown(`\n\n`);
66+
67+
md.appendMarkdown(`**@`);
68+
md.appendText(entry.username);
69+
md.appendMarkdown(`**\n\n`);
6570
md.appendMarkdown(`📊 **Rank:** #${String(entry.rank)}\n\n`);
6671
md.appendMarkdown(`⏱️ **This Week:** ${this.formatScore(entry.score)}\n\n`);
6772

@@ -168,9 +173,7 @@ export class LeaderboardProvider
168173
/** Set loading state. */
169174
setLoading(loading: boolean): void {
170175
this.isLoading = loading;
171-
if (loading) {
172-
this.onDidChangeTreeDataEmitter.fire(undefined);
173-
}
176+
this.onDidChangeTreeDataEmitter.fire(undefined);
174177
}
175178

176179
/** Clear leaderboard data. */

apps/extension/src/views/statsProvider.ts

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,15 +8,7 @@
88
import * as vscode from 'vscode';
99

1010
import type { Logger } from '../utils/logger';
11-
import type { StreakInfo, WeeklyStatsDTO, AchievementDTO } from '@devradar/shared';
12-
13-
/** Stats data structure matching API response. */
14-
export interface StatsData {
15-
streak: StreakInfo;
16-
todaySession: number;
17-
weeklyStats: WeeklyStatsDTO | null;
18-
recentAchievements: AchievementDTO[];
19-
}
11+
import type { UserStatsDTO } from '@devradar/shared';
2012

2113
/** Tree item for stats display. */
2214
class StatsTreeItem extends vscode.TreeItem {
@@ -38,7 +30,7 @@ export class StatsProvider implements vscode.TreeDataProvider<StatsTreeItem>, vs
3830
private readonly onDidChangeTreeDataEmitter = new vscode.EventEmitter<
3931
StatsTreeItem | undefined
4032
>();
41-
private stats: StatsData | null = null;
33+
private stats: UserStatsDTO | null = null;
4234
private isLoading = true;
4335

4436
readonly onDidChangeTreeData = this.onDidChangeTreeDataEmitter.event;
@@ -133,7 +125,7 @@ export class StatsProvider implements vscode.TreeDataProvider<StatsTreeItem>, vs
133125
}
134126

135127
/** Updates the stats data and triggers a tree refresh. */
136-
updateStats(stats: StatsData): void {
128+
updateStats(stats: UserStatsDTO): void {
137129
this.stats = stats;
138130
this.isLoading = false;
139131
this.onDidChangeTreeDataEmitter.fire(undefined);
@@ -143,9 +135,7 @@ export class StatsProvider implements vscode.TreeDataProvider<StatsTreeItem>, vs
143135
/** Set loading state. */
144136
setLoading(loading: boolean): void {
145137
this.isLoading = loading;
146-
if (loading) {
147-
this.onDidChangeTreeDataEmitter.fire(undefined);
148-
}
138+
this.onDidChangeTreeDataEmitter.fire(undefined);
149139
}
150140

151141
/** Clear stats (e.g., on logout). */

apps/server/src/config.ts

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,11 @@ const envSchema = z.object({
3434
GITHUB_CLIENT_SECRET: z.string().min(1, 'GITHUB_CLIENT_SECRET is required'),
3535
GITHUB_CALLBACK_URL: z.string().url(),
3636
/* GitHub Webhooks (optional - for Gamification feature) */
37-
GITHUB_WEBHOOK_SECRET: z.string().min(8).optional(),
37+
GITHUB_WEBHOOK_SECRET: z
38+
.string()
39+
.trim()
40+
.min(32, 'GITHUB_WEBHOOK_SECRET must be at least 32 characters')
41+
.optional(),
3842
/* Logging */
3943
LOG_LEVEL: z.enum(['fatal', 'error', 'warn', 'info', 'debug', 'trace']).default('info'),
4044
});

apps/server/src/routes/stats.ts

Lines changed: 73 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,14 @@ import { broadcastToUsers } from '@/ws/handler';
2828
/** Schema for session recording payload. */
2929
const SessionPayloadSchema = z.object({
3030
sessionDuration: z.number().int().min(0).max(86400), // Max 24 hours
31-
language: z.string().optional(),
32-
project: z.string().optional(),
31+
language: z.string().max(255).optional(),
32+
project: z.string().max(255).optional(),
33+
});
34+
35+
/** Schema for achievements query with pagination. */
36+
const AchievementsQuerySchema = z.object({
37+
limit: z.coerce.number().int().min(1).max(100).default(50),
38+
cursor: z.string().optional(),
3339
});
3440

3541
/** Allowlist of known programming languages to prevent unbounded Redis hash growth. */
@@ -103,6 +109,30 @@ function calculateStreakStatus(lastActiveDate: string | null): StreakInfo['strea
103109
return 'broken';
104110
}
105111

112+
/** Get yesterday's date in YYYY-MM-DD format (UTC) for Lua script timezone safety. */
113+
function getYesterdayDate(): string {
114+
const yesterday = new Date();
115+
yesterday.setUTCDate(yesterday.getUTCDate() - 1);
116+
const parts = yesterday.toISOString().split('T');
117+
return parts[0] ?? '';
118+
}
119+
120+
/** Parse streak data from Redis hgetall result into StreakInfo. */
121+
function parseStreakData(data: Record<string, string> | null): StreakInfo {
122+
const currentStreak = parseInt(data?.count ?? '0', 10);
123+
const longestStreak = parseInt(data?.longest ?? '0', 10);
124+
const lastActiveDate = data?.lastDate ?? null;
125+
const streakStatus = calculateStreakStatus(lastActiveDate);
126+
127+
return {
128+
currentStreak,
129+
longestStreak,
130+
lastActiveDate,
131+
isActiveToday: streakStatus === 'active',
132+
streakStatus,
133+
};
134+
}
135+
106136
/** Registers stats routes on the Fastify instance. */
107137
export function statsRoutes(app: FastifyInstance): void {
108138
const db = getDb();
@@ -137,19 +167,8 @@ export function statsRoutes(app: FastifyInstance): void {
137167
const todaySessionStr = results?.[1]?.[1] as string | null;
138168
const todaySession = todaySessionStr ? parseInt(todaySessionStr, 10) : 0;
139169

140-
// Build streak info
141-
const currentStreak = parseInt(streakData?.count ?? '0', 10);
142-
const longestStreak = parseInt(streakData?.longest ?? '0', 10);
143-
const lastActiveDate = streakData?.lastDate ?? null;
144-
const streakStatus = calculateStreakStatus(lastActiveDate);
145-
146-
const streak: StreakInfo = {
147-
currentStreak,
148-
longestStreak,
149-
lastActiveDate,
150-
isActiveToday: streakStatus === 'active',
151-
streakStatus,
152-
};
170+
// Build streak info using shared helper
171+
const streak = parseStreakData(streakData);
153172

154173
// Get weekly stats from PostgreSQL
155174
const weekStart = getWeekStart();
@@ -210,19 +229,7 @@ export function statsRoutes(app: FastifyInstance): void {
210229
const redis = getRedis();
211230

212231
const streakData = await redis.hgetall(REDIS_KEYS.streakData(userId));
213-
214-
const currentStreak = parseInt(streakData.count ?? '0', 10);
215-
const longestStreak = parseInt(streakData.longest ?? '0', 10);
216-
const lastActiveDate = streakData.lastDate ?? null;
217-
const streakStatus = calculateStreakStatus(lastActiveDate);
218-
219-
const streak: StreakInfo = {
220-
currentStreak,
221-
longestStreak,
222-
lastActiveDate,
223-
isActiveToday: streakStatus === 'active',
224-
streakStatus,
225-
};
232+
const streak = parseStreakData(streakData);
226233

227234
return reply.send({ data: streak });
228235
}
@@ -247,14 +254,16 @@ export function statsRoutes(app: FastifyInstance): void {
247254

248255
const { sessionDuration, language, project } = result.data;
249256
const today = getTodayDate();
257+
const yesterday = getYesterdayDate();
250258
const redis = getRedis();
251259

252-
// Lua script for atomic streak update
260+
// Lua script for atomic streak update (UTC-safe: uses string comparison)
253261
// Returns: [newStreak, shouldCheckAchievements] - 1 if streak was updated, 0 otherwise
254262
const STREAK_UPDATE_SCRIPT = `
255263
local streakKey = KEYS[1]
256264
local today = ARGV[1]
257265
local streakTtl = tonumber(ARGV[2])
266+
local yesterday = ARGV[3]
258267
259268
-- Read current streak data
260269
local lastDate = redis.call('HGET', streakKey, 'lastDate')
@@ -266,21 +275,12 @@ export function statsRoutes(app: FastifyInstance): void {
266275
return {count, 0}
267276
end
268277
269-
-- Calculate new streak
278+
-- Calculate new streak using UTC-safe string comparison
270279
local newStreak = 1
271-
if lastDate then
272-
-- Parse dates and calculate difference
273-
local ly, lm, ld = lastDate:match('(%d+)-(%d+)-(%d+)')
274-
local ty, tm, td = today:match('(%d+)-(%d+)-(%d+)')
275-
local lastTime = os.time({year=ly, month=lm, day=ld})
276-
local todayTime = os.time({year=ty, month=tm, day=td})
277-
local daysDiff = math.floor((todayTime - lastTime) / 86400)
278-
279-
if daysDiff == 1 then
280-
newStreak = count + 1
281-
end
282-
-- daysDiff > 1 means streak broken, reset to 1
280+
if lastDate == yesterday then
281+
newStreak = count + 1
283282
end
283+
-- Any other date means streak broken, reset to 1
284284
285285
local newLongest = math.max(newStreak, longest)
286286
@@ -298,7 +298,8 @@ export function statsRoutes(app: FastifyInstance): void {
298298
1,
299299
streakKey,
300300
today,
301-
(STREAK_TTL_SECONDS * 2).toString()
301+
(STREAK_TTL_SECONDS * 2).toString(), // 50h TTL: 25h grace + 25h safety buffer
302+
yesterday
302303
)) as [number, number];
303304

304305
const newStreak = streakResult[0];
@@ -320,7 +321,7 @@ export function statsRoutes(app: FastifyInstance): void {
320321
pipeline.hincrby(REDIS_KEYS.networkIntensity(minute), 'count', 1);
321322
pipeline.expire(REDIS_KEYS.networkIntensity(minute), NETWORK_ACTIVITY_TTL_SECONDS);
322323

323-
// 5. Track language if provided (validate to prevent unbounded hash growth)
324+
// 4. Track language if provided (validate to prevent unbounded hash growth)
324325
if (language) {
325326
const normalizedLang = language.toLowerCase().trim();
326327
const safeLang = LANGUAGE_ALLOWLIST.has(normalizedLang) ? normalizedLang : 'other';
@@ -395,28 +396,52 @@ export function statsRoutes(app: FastifyInstance): void {
395396
);
396397

397398
/**
398-
* GET /stats/achievements - Get all user achievements
399+
* GET /stats/achievements - Get all user achievements (paginated)
399400
*/
400401
app.get(
401402
'/achievements',
402403
{ onRequest: [app.authenticate] },
403404
async (request: FastifyRequest, reply: FastifyReply) => {
404405
const { userId } = request.user as { userId: string };
406+
const queryResult = AchievementsQuerySchema.safeParse(request.query);
407+
408+
if (!queryResult.success) {
409+
return reply.status(400).send({
410+
error: { code: 'INVALID_QUERY', message: 'Invalid query parameters' },
411+
});
412+
}
413+
414+
const { limit, cursor } = queryResult.data;
405415

406416
const achievementRecords = await db.achievement.findMany({
407417
where: { userId },
408418
orderBy: { earnedAt: 'desc' },
419+
take: limit + 1, // Fetch one extra to check if there's more
420+
...(cursor && {
421+
cursor: { id: cursor },
422+
skip: 1, // Skip the cursor item itself
423+
}),
409424
});
410425

411-
const achievements: AchievementDTO[] = achievementRecords.map((a) => ({
426+
const hasMore = achievementRecords.length > limit;
427+
const items = hasMore ? achievementRecords.slice(0, limit) : achievementRecords;
428+
const nextCursor = hasMore ? items[items.length - 1]?.id : undefined;
429+
430+
const achievements: AchievementDTO[] = items.map((a) => ({
412431
id: a.id,
413432
type: a.type as AchievementDTO['type'],
414433
title: a.title,
415434
description: a.description,
416435
earnedAt: a.earnedAt.toISOString(),
417436
}));
418437

419-
return reply.send({ data: achievements });
438+
return reply.send({
439+
data: achievements,
440+
pagination: {
441+
hasMore,
442+
nextCursor,
443+
},
444+
});
420445
}
421446
);
422447

@@ -477,7 +502,7 @@ export function statsRoutes(app: FastifyInstance): void {
477502
logger.info({ userId, streak, type: milestone.type }, 'Streak achievement earned');
478503
}
479504
}
480-
break; // Only one achievement per streak update
505+
// Continue checking for lower milestones that may not have been awarded
481506
}
482507
}
483508
}

0 commit comments

Comments
 (0)