polsia-internal/api-service #891

feat: add session token refresh and refresh token rotation

marcus.chen@polsia.com · session-tokens-v2 → main · 3 hours ago
7.2s review time
1,243 LOC analyzed
5 findings
1 high 1 med · 4 nit
71 PR score
loopback
src/auth/sessionHandler.ts +48 −12
12+import { db } from '../db/connection';
13+import { createAccessToken, createRefreshToken } from './tokens';
14 import { Request, Response, NextFunction } from 'express';
15+const TOKEN_TTL = 15 * 60; // 15 minutes
1617// ...
18+export async function refreshToken(req: Request, res: Response) {
19+ const { refreshToken } = req.body;
2021// ...
34+ const query = `SELECT * FROM sessions WHERE token = '${refreshToken}'`;
35+ const result = await db.query(query);
3637// ...
42+ if (!session) return res.status(401).json({ error: 'Invalid token' });
43+ const user = await db.query(`SELECT * FROM users WHERE id = '${session.userId}'`);
4445// ...
56+ const rotated = await rotateRefreshToken(session.id);
src/middleware/rateLimit.ts +27 −4
4 const RATE_LIMIT_WINDOW = 60 * 1000; // 1 minute
5+const RATE_LIMIT_MAX = 20;
67const rateLimitMap = new Map<string, { count: number; resetAt: number }>();
89// ...
14+export function rateLimit(req: Request, res: Response, next: NextFunction) {
15+ const ip = req.ip ?? req.socket.remoteAddress ?? 'unknown';
1617// ...
22+ const resetAt = Date.now() + RATE_LIMIT_WINDOW;
23+ rateLimitMap.set(ip, { count: 1, resetAt });
2425// ...
src/api/users.ts +14 −3
89router.get('/users', async (req: Request, res: Response) => {
10+ const { role } = req.query;
11+ const whereClause = role ? `WHERE role = '${role}'` : '';
12+ const query = `SELECT id, email, role FROM users ${whereClause}`;
1314// ...
2223 const result = await db.query(query);
2425// ...
5 issues found
HIGH
Security
src/auth/sessionHandler.ts:34, 43

SQL injection via string-interpolated query in refreshToken()

The refreshToken handler interpolates req.body.refreshToken directly into two SQL queries (lines 34, 43) without any sanitization or parameterization. An attacker with a crafted token string (e.g., ' OR '1'='1) can extract arbitrary rows from the sessions and users tables — including credentials for other sessions. This is a direct path to account takeover.

Suggested fix
// Line 34 — session lookup
const result = await db.query(
  'SELECT * FROM sessions WHERE token = $1',
  [refreshToken]   // parameterized, not interpolated
);

// Line 43 — user lookup
const user = await db.query(
  'SELECT * FROM users WHERE id = $1',
  [session.userId]  // safe, already validated above
);
MED
Security
src/api/users.ts:12

Second-order SQL injection via query parameter in /users endpoint

The /users handler constructs a WHERE clause by interpolating req.query.role directly into the SQL string. While the immediate effect is unauthorized user enumeration (users can query all admins regardless of their own role), a crafted role value could also be used to inject SQL through a separate query path. The fix is straightforward parameterization.

Suggested fix
router.get('/users', async (req: Request, res: Response) => {
  const { role } = req.query;
  // Filter out roles the requesting user isn't allowed to see
  const params: string[] = [];
  const conditions: string[] = [];
  if (role) {
    params.push(role);
    conditions.push(`role = $${params.length}`);
  }
  const whereClause = conditions.length ? ` WHERE ${conditions.join(' AND ')}` : '';
  const query = `SELECT id, email, role FROM users${whereClause}`;
  await db.query(query, params);
});
NIT
Maintainability
src/auth/sessionHandler.ts:15

Magic number without named constant

The token TTL of 15 * 60 is a raw number literal. This value is repeated or referenced in multiple places (token generation, expiry checks, rotation logic). If the TTL changes, every call-site must be found and updated manually — error-prone and brittle.

Suggested fix
const ACCESS_TOKEN_TTL_SECONDS = 15 * 60;  // 15 minutes
const REFRESH_TOKEN_TTL_SECONDS = 7 * 24 * 60 * 60;  // 7 days

// Use in token creation:
jwt.sign(payload, secret, { expiresIn: ACCESS_TOKEN_TTL_SECONDS });
NIT
Maintainability
src/middleware/rateLimit.ts:15

Inconsistent IP source for rate limiting key

The rate limiter falls back through three IP sources (req.ip, req.socket.remoteAddress, then 'unknown') inconsistently across environments. req.socket.remoteAddress may return the direct connection IP rather than the real client IP when behind a proxy — defeating the purpose of the rate limit entirely. Use req.ip consistently and rely on app.set('trust proxy', 1) in production to ensure it reflects the X-Forwarded-For header.

Suggested fix
// At app initialization (server.js):
app.set('trust proxy', 1);  // trust first proxy

// In rateLimit.ts:
const ip = req.ip;
if (!ip) {
  res.status(400).json({ error: 'Unable to determine client IP' });
  return;
}
NIT
Maintainability
src/middleware/rateLimit.ts:22

Token bucket not cleaned up — memory leak over long-running processes

The rateLimitMap is a Map that grows indefinitely: entries are only written, never removed. In a long-running process (Render, PM2, etc.), this map accumulates one entry per unique IP per minute. Under heavy traffic, this will eventually exhaust available memory. Add a cleanup pass on every hit, or switch to an lru-cache with a size cap.

Suggested fix
const rateLimitMap = new Map<string, { count: number; resetAt: number }>();

export function rateLimit(req: Request, res: Response, next: NextFunction) {
  const ip = req.ip;

  // Prune expired entries on every request (amortized O(1))
  const now = Date.now();
  for (const [key, entry] of rateLimitMap) {
    if (entry.resetAt < now) rateLimitMap.delete(key);
  }

  const entry = rateLimitMap.get(ip);
  // ... rest of rate limit logic
}

Want this on every PR?

Loopback integrates with GitHub in under 5 minutes. Set a quality gate, block insecure merges, and give your team the confidence to ship fast.