rotateSessionId uses raw BEGIN/COMMIT/ROLLBACK instead of db.transaction() #51

New Issue

Closed

opened 2026-05-16 21:43:01 -05:00 by null · 0 comments

null commented

2026-05-16 21:43:01 -05:00

Owner

Severity: HIGH 🟠

Affected Files

services/authService.js — lines 131–136

Problem

rotateSessionId() uses manual transaction control:

db.prepare('BEGIN').run();
try {
  // ... session rotation logic ...
  db.prepare('COMMIT').run();
} catch (e) {
  db.prepare('ROLLBACK').run();
  return null;
}

better-sqlite3 provides .transaction() for exactly this — it's atomic and handles rollback automatically. Every other transactional operation in the codebase uses the idiomatic db.transaction() pattern.

The manual pattern works in the happy path but has two problems:

If ROLLBACK itself throws (e.g., no active transaction due to a prior error), the exception propagates uncaught.
Inconsistency — this is the only place in the entire codebase using raw SQL transaction commands. Any developer reading the code has to double-check whether this is intentional or a bug.

Impact

Potential unhandled exception if ROLLBACK fails
Code inconsistency — the only manual transaction in the codebase
Harder to maintain as migrations add more transactional patterns

Fix

Replace with idiomatic db.transaction():

const rotateSession = db.transaction((oldSessionId, userId) => {
  // ... session rotation logic ...
});
const result = rotateSession(oldSessionId, userId);

Acceptance Criteria

rotateSessionId uses db.transaction() instead of raw BEGIN/COMMIT/ROLLBACK
Session rotation still works correctly (login, password change, etc.)
Error handling preserves existing behavior (returns null on failure)
No manual transaction SQL remains in authService.js

## Severity: HIGH 🟠 ## Affected Files - `services/authService.js` — lines 131–136 ## Problem `rotateSessionId()` uses manual transaction control: ```javascript db.prepare('BEGIN').run(); try { // ... session rotation logic ... db.prepare('COMMIT').run(); } catch (e) { db.prepare('ROLLBACK').run(); return null; } ``` better-sqlite3 provides `.transaction()` for exactly this — it's atomic and handles rollback automatically. Every other transactional operation in the codebase uses the idiomatic `db.transaction()` pattern. The manual pattern works in the happy path but has two problems: 1. **If ROLLBACK itself throws** (e.g., no active transaction due to a prior error), the exception propagates uncaught. 2. **Inconsistency** — this is the only place in the entire codebase using raw SQL transaction commands. Any developer reading the code has to double-check whether this is intentional or a bug. ## Impact - Potential unhandled exception if ROLLBACK fails - Code inconsistency — the only manual transaction in the codebase - Harder to maintain as migrations add more transactional patterns ## Fix Replace with idiomatic `db.transaction()`: ```javascript const rotateSession = db.transaction((oldSessionId, userId) => { // ... session rotation logic ... }); const result = rotateSession(oldSessionId, userId); ``` ## Acceptance Criteria - [ ] `rotateSessionId` uses `db.transaction()` instead of raw BEGIN/COMMIT/ROLLBACK - [ ] Session rotation still works correctly (login, password change, etc.) - [ ] Error handling preserves existing behavior (returns null on failure) - [ ] No manual transaction SQL remains in authService.js