Skip to content

fix: prevent self and last-admin deletion#2

Open
joelev wants to merge 1 commit intosdamico:mainfrom
joelev:fix/admin-delete-guards
Open

fix: prevent self and last-admin deletion#2
joelev wants to merge 1 commit intosdamico:mainfrom
joelev:fix/admin-delete-guards

Conversation

@joelev
Copy link
Contributor

@joelev joelev commented Mar 2, 2026

Problem

DELETE /api/admin/admins?id=... allowed two dangerous operations:

  • deleting the currently authenticated admin user
  • deleting the final remaining admin account

Either path can lock teams out of admin management and create avoidable operational risk.

Root Cause

The delete handler performed a direct DELETE FROM admins WHERE id = ... without:

  • strict ID validation
  • requester identity checks
  • minimum-admin-count guardrails

What Changed

api/admin/admins.js

  • Added strict id validation (positive integer only).
  • Added not-found handling (404 when target admin does not exist).
  • Added self-deletion protection (409 with clear error).
  • Added last-admin protection (409 with clear error).
  • Kept successful delete behavior unchanged (204) when guards pass.

api/_lib/admin-auth.js

  • Added getAuthedAdminEmail(req) helper to resolve authenticated admin email (from admin-auth token or site-auth session joined to admins).
  • Exported helper for use in admin management handlers.

Risk Assessment

  • Low: behavior only tightens invalid/dangerous delete operations.
  • Low: no schema changes, no new external dependencies.
  • Expected: clients must handle 409 and 404 responses for blocked/invalid delete attempts.

Rollback Plan

  1. Revert commit a9ebc5b if unexpected regressions appear.
  2. No migration rollback required (code-only change).

Validation

  • Targeted smoke checks (local harness with mocked DB/auth) passed:
    1. self-delete attempt -> 409
    2. last-admin delete attempt -> 409
    3. deleting another admin when >1 remains -> 204
  • npm run build passes:
    • Built content/page.html (63624 bytes, 9 slides)

@joelev
Copy link
Contributor Author

joelev commented Mar 2, 2026

Reviewer checklist:\n\n1. Confirm self-deletion is blocked with 409 in /api/admin/admins.\n2. Confirm last-admin deletion is blocked with 409.\n3. Confirm deleting a different admin still returns 204 when >=2 admins exist.\n4. Confirm invalid/non-positive id now returns 400 and unknown id returns 404.\n5. Confirm no behavior changes for GET/POST admin listing/creation.

Copy link
Owner

@sdamico sdamico left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvements overall — the self-deletion guard, last-admin protection, parseInt + Number.isInteger validation, and the 404 for unknown IDs are all solid additions. Two issues worth addressing before merging:


Critical: TOCTOU race in the last-admin guard

The SELECT COUNT(*) and DELETE are separate statements with no transaction wrapping them. Two concurrent DELETE requests for different admins — say admin A deletes admin B while admin B deletes admin A — can both read cnt = 2, both pass the guard, and both execute their DELETE, leaving the table empty.

Current code:
```js
const { rows: counts } = await sqlSELECT COUNT(*)::int AS cnt FROM admins;
if (counts[0].cnt <= 1) {
res.writeHead(409, { 'Content-Type': 'application/json' });
res.end(JSON.stringify({ error: 'cannot delete the last admin account' }));
return;
}

await sqlDELETE FROM admins WHERE id = ${id};
```

Suggested fix — single atomic conditional DELETE:
```js
const { rows: deleted } = await sqlDELETE FROM admins WHERE id = ${id} AND (SELECT COUNT(*) FROM admins) > 1 RETURNING id;

if (deleted.length === 0) {
// Either the row was already gone or it was the last admin
// Re-check to return the right status code
const { rows: exists } = await sqlSELECT id FROM admins WHERE id = ${id};
if (exists.length === 0) {
res.writeHead(404, { 'Content-Type': 'application/json' });
res.end(JSON.stringify({ error: 'admin not found' }));
} else {
res.writeHead(409, { 'Content-Type': 'application/json' });
res.end(JSON.stringify({ error: 'cannot delete the last admin account' }));
}
return;
}

res.writeHead(204);
res.end();
```

The subquery in the WHERE clause makes the count-check and the delete a single atomic operation. No two concurrent requests can both sneak through.

An alternative is wrapping the existing code in a transaction with sql.begin() and a SELECT ... FOR UPDATE lock, but the single-statement approach above is simpler and avoids a round-trip.


Minor: getAuthedAdminEmail can return null, silently skipping the self-deletion check

```js
const requesterEmail = await getAuthedAdminEmail(req);
if (requesterEmail && requesterEmail.toLowerCase() === target[0].email.toLowerCase()) {
```

When getAuthedAdminEmail returns null (e.g. a password-only session that somehow passes isAdminAuthed), the self-deletion guard is silently bypassed. The outer isAdminAuthed check mitigates this in practice, but the silent skip is a latent footgun.

Consider a short-circuit:
```js
const requesterEmail = await getAuthedAdminEmail(req);
if (!requesterEmail) {
res.writeHead(401, { 'Content-Type': 'application/json' });
res.end(JSON.stringify({ error: 'could not resolve requester identity' }));
return;
}
if (requesterEmail.toLowerCase() === target[0].email.toLowerCase()) {
res.writeHead(409, { 'Content-Type': 'application/json' });
res.end(JSON.stringify({ error: 'cannot delete your own admin account' }));
return;
}
```

This makes the behavior explicit and surfaces any auth-layer inconsistency rather than hiding it.


Happy to discuss alternative approaches — the conditional DELETE is the most straightforward path to atomicity here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants