Skip to content

Commit cd2386d

Browse files
bokelleyclaude
andcommitted
fix: harden input validation on admin resolve endpoint
Address code review and security review findings: - Validate attemptId as UUID before hitting DB - Reject whitespace-only reason, enforce 1000-char limit - Validate score values are finite numbers in 0-100 range - Reject array scores - Map TOCTOU race condition to 409 instead of 500 - Surface module/credential side-effect failures as warnings Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent bad1c6c commit cd2386d

1 file changed

Lines changed: 44 additions & 10 deletions

File tree

server/src/routes/certification.ts

Lines changed: 44 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -931,15 +931,23 @@ export function createCertificationRouters() {
931931
adminRouter.post('/attempts/:attemptId/resolve', async (req, res) => {
932932
try {
933933
const { attemptId } = req.params;
934+
const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i;
935+
if (!UUID_RE.test(attemptId)) {
936+
return res.status(400).json({ error: 'Invalid attempt ID format' });
937+
}
938+
934939
const { action, scores, reason } = req.body as {
935940
action: 'cancel' | 'complete';
936941
scores?: Record<string, number>;
937942
reason: string;
938943
};
939944

940-
if (!reason || typeof reason !== 'string') {
945+
if (!reason || typeof reason !== 'string' || reason.trim().length === 0) {
941946
return res.status(400).json({ error: 'reason is required' });
942947
}
948+
if (reason.length > 1000) {
949+
return res.status(400).json({ error: 'reason must be under 1000 characters' });
950+
}
943951
if (action !== 'cancel' && action !== 'complete') {
944952
return res.status(400).json({ error: 'action must be "cancel" or "complete"' });
945953
}
@@ -954,36 +962,62 @@ export function createCertificationRouters() {
954962
}
955963

956964
if (action === 'cancel') {
957-
const updated = await certDb.cancelAttempt(attemptId, reason);
958-
return res.json({ attempt: updated });
965+
try {
966+
const updated = await certDb.cancelAttempt(attemptId, reason.trim());
967+
return res.json({ attempt: updated });
968+
} catch (err) {
969+
if (err instanceof Error && err.message.includes('not in_progress')) {
970+
return res.status(409).json({ error: 'Attempt is no longer in_progress' });
971+
}
972+
throw err;
973+
}
959974
}
960975

961976
// action === 'complete'
962-
if (!scores || typeof scores !== 'object' || Object.keys(scores).length === 0) {
963-
return res.status(400).json({ error: 'scores are required for complete action' });
977+
if (!scores || Array.isArray(scores) || typeof scores !== 'object' || Object.keys(scores).length === 0) {
978+
return res.status(400).json({ error: 'scores must be a non-empty object' });
964979
}
980+
const scoreValues = Object.values(scores);
981+
if (!scoreValues.every(s => typeof s === 'number' && Number.isFinite(s))) {
982+
return res.status(400).json({ error: 'All score values must be finite numbers' });
983+
}
984+
if (!scoreValues.every(s => s >= 0 && s <= 100)) {
985+
return res.status(400).json({ error: 'Score values must be between 0 and 100' });
986+
}
987+
965988
const overallScore = Math.round(
966-
Object.values(scores).reduce((sum, s) => sum + s, 0) / Object.values(scores).length
989+
scoreValues.reduce((sum, s) => sum + s, 0) / scoreValues.length
967990
);
968-
const passing = Object.values(scores).every(s => s >= 70) && overallScore >= 70;
969-
970-
const updated = await certDb.adminCompleteAttempt(attemptId, scores, overallScore, passing, reason);
991+
const passing = scoreValues.every(s => s >= 70) && overallScore >= 70;
992+
993+
let updated;
994+
try {
995+
updated = await certDb.adminCompleteAttempt(attemptId, scores, overallScore, passing, reason.trim());
996+
} catch (err) {
997+
if (err instanceof Error && err.message.includes('not in_progress')) {
998+
return res.status(409).json({ error: 'Attempt is no longer in_progress' });
999+
}
1000+
throw err;
1001+
}
9711002

9721003
// If passing, also mark the module as completed and check credentials
1004+
const warnings: string[] = [];
9731005
if (passing && updated.module_id) {
9741006
try {
9751007
await certDb.completeModule(updated.workos_user_id, updated.module_id, scores);
9761008
} catch (modError) {
1009+
warnings.push('Module completion failed — run backfill');
9771010
logger.error({ error: modError, attemptId, moduleId: updated.module_id }, 'Failed to mark module complete after admin resolve');
9781011
}
9791012
try {
9801013
await certDb.checkAndAwardCredentials(updated.workos_user_id);
9811014
} catch (credError) {
1015+
warnings.push('Credential check failed — run backfill');
9821016
logger.error({ error: credError, attemptId }, 'Failed to check credentials after admin resolve');
9831017
}
9841018
}
9851019

986-
return res.json({ attempt: updated });
1020+
return res.json({ attempt: updated, ...(warnings.length > 0 && { warnings }) });
9871021
} catch (error) {
9881022
logger.error({ error, attemptId: req.params.attemptId }, 'Failed to resolve stuck attempt');
9891023
res.status(500).json({ error: 'Internal server error' });

0 commit comments

Comments
 (0)