fix(integrity): transaction-wrap three multi-statement writes + review cleanups

CODE_STANDARDS 'every multi-statement write runs inside db.transaction()'
hits from the review: snowball plan create (abandon-existing + insert),
webauthn disable (credential delete + flag clear — a failure between them
recreates the stale-flag state), admin password reset (hash update +
session invalidation). Cleanups: dead ApiError import (payments), dead
action: params on the four snowball transition callers, createSession
consolidated into auth.cts's top-level import.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
null 2026-07-10 22:57:10 -05:00
parent 2a07e762a2
commit a17ce13f07
4 changed files with 30 additions and 29 deletions

View File

@ -234,10 +234,14 @@ router.put('/users/:id/password', async (req: Req, res: Res) => {
if (!user) throw NotFoundError('User not found'); if (!user) throw NotFoundError('User not found');
const hash = await hashPassword(password); const hash = await hashPassword(password);
// Password update + session invalidation land together: a failure between
// them would leave a reset password with the old sessions still live.
db.transaction(() => {
db.prepare( db.prepare(
"UPDATE users SET password_hash=?, must_change_password=1, updated_at=datetime('now') WHERE id=?", "UPDATE users SET password_hash=?, must_change_password=1, updated_at=datetime('now') WHERE id=?",
).run(hash, targetId); ).run(hash, targetId);
db.prepare('DELETE FROM sessions WHERE user_id = ?').run(targetId); db.prepare('DELETE FROM sessions WHERE user_id = ?').run(targetId);
})();
logAudit({ logAudit({
user_id: req.user.id, user_id: req.user.id,

View File

@ -27,6 +27,7 @@ const {
invalidateOtherSessions, invalidateOtherSessions,
recordLogin, recordLogin,
recordFailedLogin, recordFailedLogin,
createSession,
} = require('../services/authService.cts'); } = require('../services/authService.cts');
const { decryptSecret } = require('../services/encryptionService.cts'); const { decryptSecret } = require('../services/encryptionService.cts');
const { getCsrfToken } = require('../middleware/csrf.cts'); const { getCsrfToken } = require('../middleware/csrf.cts');
@ -283,7 +284,6 @@ router.post('/totp/challenge', async (req: Req, res: Res) => {
throw AuthError('Invalid authenticator code.'); throw AuthError('Invalid authenticator code.');
} }
const { createSession } = require('../services/authService.cts');
const session = await createSession(userId); const session = await createSession(userId);
if (!session) throw new ApiError('SERVER_ERROR', 'Failed to create session', 500); if (!session) throw new ApiError('SERVER_ERROR', 'Failed to create session', 500);
@ -586,10 +586,14 @@ router.post('/webauthn/disable', requireAuth, async (req: Req, res: Res) => {
if (!(await bcrypt.compare(password, user.password_hash))) if (!(await bcrypt.compare(password, user.password_hash)))
throw AuthError('Password is incorrect'); throw AuthError('Password is incorrect');
// Credential delete + flag clear land together: a failure between them would
// leave webauthn_enabled=1 with zero credentials (the stale-flag state).
db.transaction(() => {
db.prepare('DELETE FROM webauthn_credentials WHERE user_id = ?').run(req.user.id); db.prepare('DELETE FROM webauthn_credentials WHERE user_id = ?').run(req.user.id);
db.prepare( db.prepare(
"UPDATE users SET webauthn_enabled = 0, updated_at = datetime('now') WHERE id = ?", "UPDATE users SET webauthn_enabled = 0, updated_at = datetime('now') WHERE id = ?",
).run(req.user.id); ).run(req.user.id);
})();
logAudit({ logAudit({
user_id: req.user.id, user_id: req.user.id,
@ -632,7 +636,6 @@ router.post('/webauthn/challenge', async (req: Req, res: Res) => {
throw AuthError('Security key verification failed.'); throw AuthError('Security key verification failed.');
} }
const { createSession } = require('../services/authService.cts');
const s = await createSession(session.userId); const s = await createSession(session.userId);
if (!s) throw new ApiError('SERVER_ERROR', 'Failed to create session', 500); if (!s) throw new ApiError('SERVER_ERROR', 'Failed to create session', 500);

View File

@ -1,12 +1,7 @@
// @ts-nocheck — route controller converted to .cts; handler req/res typed, full type-check deferred (thin glue over typed services). // @ts-nocheck — route controller converted to .cts; handler req/res typed, full type-check deferred (thin glue over typed services).
import type { Req, Res, Next } from '../types/http'; import type { Req, Res, Next } from '../types/http';
const express = require('express'); const express = require('express');
const { const { ValidationError, NotFoundError, ConflictError } = require('../utils/apiError.cts');
ApiError,
ValidationError,
NotFoundError,
ConflictError,
} = require('../utils/apiError.cts');
const router = require('express').Router(); const router = require('express').Router();
const { getDb } = require('../db/database.cts'); const { getDb } = require('../db/database.cts');
const { computeBalanceDelta, applyBalanceDelta } = require('../services/billsService.cts'); const { computeBalanceDelta, applyBalanceDelta } = require('../services/billsService.cts');

View File

@ -417,7 +417,9 @@ router.post('/plans', (req: Req, res: Res) => {
debts: debtSnaps, debts: debtSnaps,
}); });
// Abandon any existing active/paused plan first // Abandon-existing + insert-new land together or neither: a failure between
// them must not leave the user with no active plan.
const result = db.transaction(() => {
db.prepare( db.prepare(
` `
UPDATE snowball_plans SET status = 'abandoned', updated_at = datetime('now') UPDATE snowball_plans SET status = 'abandoned', updated_at = datetime('now')
@ -425,7 +427,7 @@ router.post('/plans', (req: Req, res: Res) => {
`, `,
).run(userId); ).run(userId);
const result = db return db
.prepare( .prepare(
` `
INSERT INTO snowball_plans (user_id, name, method, status, extra_payment, plan_snapshot, notes, started_at, created_at, updated_at) INSERT INTO snowball_plans (user_id, name, method, status, extra_payment, plan_snapshot, notes, started_at, created_at, updated_at)
@ -433,6 +435,7 @@ router.post('/plans', (req: Req, res: Res) => {
`, `,
) )
.run(userId, planName, planMethod, extraCents, planSnapshot, notes || null); .run(userId, planName, planMethod, extraCents, planSnapshot, notes || null);
})();
const plan = db.prepare('SELECT * FROM snowball_plans WHERE id = ?').get(result.lastInsertRowid); const plan = db.prepare('SELECT * FROM snowball_plans WHERE id = ?').get(result.lastInsertRowid);
res.status(201).json(enrichPlanWithProgress(db, plan)); res.status(201).json(enrichPlanWithProgress(db, plan));
@ -521,7 +524,6 @@ router.post('/plans/:id/pause', (req: Req, res: Res) =>
transitionPlan(req, res, { transitionPlan(req, res, {
allowedFrom: ['active'], allowedFrom: ['active'],
setSql: "status = 'paused', paused_at = datetime('now')", setSql: "status = 'paused', paused_at = datetime('now')",
action: 'pause',
past: 'paused', past: 'paused',
}), }),
); );
@ -529,7 +531,6 @@ router.post('/plans/:id/resume', (req: Req, res: Res) =>
transitionPlan(req, res, { transitionPlan(req, res, {
allowedFrom: ['paused'], allowedFrom: ['paused'],
setSql: "status = 'active', paused_at = NULL", setSql: "status = 'active', paused_at = NULL",
action: 'resume',
past: 'resumed', past: 'resumed',
}), }),
); );
@ -537,7 +538,6 @@ router.post('/plans/:id/complete', (req: Req, res: Res) =>
transitionPlan(req, res, { transitionPlan(req, res, {
allowedFrom: ['active', 'paused'], allowedFrom: ['active', 'paused'],
setSql: "status = 'completed', completed_at = datetime('now')", setSql: "status = 'completed', completed_at = datetime('now')",
action: 'complete',
past: 'completed', past: 'completed',
}), }),
); );
@ -545,7 +545,6 @@ router.post('/plans/:id/abandon', (req: Req, res: Res) =>
transitionPlan(req, res, { transitionPlan(req, res, {
allowedFrom: ['active', 'paused'], allowedFrom: ['active', 'paused'],
setSql: "status = 'abandoned'", setSql: "status = 'abandoned'",
action: 'abandon',
past: 'abandoned', past: 'abandoned',
}), }),
); );