feat(auth): deletion barrier at mint + fail-closed account fencing (slice 2/5 of #374)

[lourou]

What this is

Slice 2 of 5 of #374, stacked on #397 (slice 1: data model). Content extracted verbatim from the #374 tip (45cd889); only surgical trims where a file mixes tiers.

This slice: the identity barrier and the fail-closed auth tier

• Barrier at mint (src/accounts/deletion/barrier.ts, identity-hash.ts, src/accounts/repository.ts, generate-token.ts): POST /v2/auth/token consults the DeletedIdentity keyed-HMAC barrier only after full SIWE validation succeeds → terminal 410 identity_deleted; mint and (future) teardown co-serialize on pg_advisory_xact_lock over the identity hash, with an in-transaction barrier re-check (IdentityBarredError), so a delete racing a mint can never recreate the account.
• Fail-closed auth (src/middleware/auth.ts, require-live-account.ts): the fence lives inside JWT authentication itself — an accountId claim is honored only while the Account row exists; deleted-account tokens get a generic 401 on every route (never a deletion-specific signal), infrastructure failure returns 500 (never a 401 that could make a client wipe its session); requireAccount is now async and fail-closed. Sole carve-out: DELETE /v2/accounts/me (method + path), for idempotent deletion-record replay once slice 3 lands.
• Router-fencing audit (tests/deletion/router-fencing.test.ts): source audit (only the fenced middlewares may call verifyJwtToken) + behavioral audit of every JWT surface on the real /v2 router. Two cases deferred to their slices, marked in-file: the subscription/claim surface (slice 5) and the DELETE carve-out replay test (slice 3).
• Account-scoped agent-asset upload keys (agent-assets.router.ts, get-presigned-url.ts, one-line mount swap in src/api/v2/index.ts): presigned uploads now require a live account and produce a/<accountId>/<uuid> object keys — the shape slice 3's S3 purge executor derives. JWT callers always use their own account; the trusted agent-key caller may assert an (existing) owner.
• Existing tests adapted (verbatim from feat: account deletion (barrier, teardown, tombstones) + subscription reclaim #374): agent-templates reader helpers, connections, prompt-hints admin — all now create the Account row their JWTs reference.

No new migrations (slice 1 shipped the schema). No teardown endpoint, no tombstones, no claim, no outbox. No client-facing request-schema change (the 410 is a new response for a state no shipped client can be in until deletion ships; nothing for assertLegacyShapeValidates). No credit movement.

Deploy note (required env)

config.ts now refuses to start without DELETION_HASH_SECRET (>= 64 chars, openssl rand -hex 32). Set it in every environment (dev + prod task definitions) before deploying this slice. Treat it as permanent: rotating it would orphan every barrier row. This is deliberately distinct from the freely-rotatable nonce secret.

Note also: every authenticated request now performs one indexed PK lookup on Account (the fail-closed fence, no positive caching — by design, see #374's review rounds).

The series

1. feat(db): deletion + subscription-lineage data model (slice 1/5 of #374) #397 — data model: all migrations + schema + schema tests
2. (this PR) barrier at mint + fail-closed auth + router fencing
3. Teardown endpoint + purge outbox/executors, behind ACCOUNT_DELETION_ENABLED=false
4. Tombstone semantics in verify/SSN/RTDN (+ assertLegacyShapeValidates contract pin)
5. Claim endpoint + reconciliation sweep + adversarial suites

Validation

• Targeted suites (barrier-mint, router-fencing, auth-require-account, account-auth-check, agent-assets-presigned): 35/35
• Full suite: 1661 passed / 0 failed (183 files, CI-style env)
• tsc --noEmit, eslint, prettier --check all clean

---

^{Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.}

Note

Add deletion barrier at account mint and fail-closed account fencing to auth middleware

• Introduces an HMAC-based DeletedIdentity barrier: upsertAuthMethodAndAccount in repository.ts acquires a per-identity advisory lock and checks the barrier before creating accounts, throwing IdentityBarredError if the identity was previously deleted.
• The generateToken handler in generate-token.ts pre-checks the deletion barrier and maps IdentityBarredError to a 410 identity_deleted response, preventing account recreation after deletion.
• Auth middleware (authMiddleware, authMiddlewareAllowNSE, requireAccount) now performs a live database lookup on the account row for every JWT-authenticated request, returning 401 if the account no longer exists (fail-closed fencing).
• DELETION_HASH_SECRET (≥64 hex chars) is now required at startup; identity and account hashes are computed via HMAC-SHA256 in identity-hash.ts.
• The /api/v2/agents/assets presigned URL route now scopes S3 object keys per-account (a/{accountId}/{uuid}) and accepts either a JWT or agent API key.
• Risk: every JWT-authenticated request now incurs an additional DB lookup to verify the account row exists; deleted accounts' tokens receive 401 across all routes (except DELETE /v2/accounts/me for idempotent retries).

^{Macroscope summarized c5c852d.}

[coderabbitai]

Warning

Review limit reached

@lourou, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 10 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: eda3caa7-e403-4737-bf17-ce30d1bf45b8

📥 Commits

Reviewing files that changed from the base of the PR and between 11e09e5 and c5c852d.

📒 Files selected for processing (23)

• .env.example
• src/accounts/deletion/barrier.ts
• src/accounts/deletion/identity-hash.ts
• src/accounts/repository.ts
• src/accounts/require-live-account.ts
• src/api/v2/agents/assets/agent-assets.router.ts
• src/api/v2/agents/assets/handlers/get-presigned-url.ts
• src/api/v2/auth/handlers/generate-token.ts
• src/api/v2/index.ts
• src/config.ts
• src/middleware/auth.ts
• tests/account-auth-check.test.ts
• tests/agent-assets-presigned.test.ts
• tests/agent-prompt-hints.admin.test.ts
• tests/agent-templates.conventions.test.ts
• tests/agent-templates.cross.helpers.ts
• tests/agent-templates.detail.test.ts
• tests/agent-templates.list.test.ts
• tests/auth-require-account.test.ts
• tests/connections.test.ts
• tests/deletion/barrier-mint.test.ts
• tests/deletion/router-fencing.test.ts
• tests/setup.ts

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

Claude finished @lourou's task —— View job

---

Code Review Complete

This is a well-architected implementation of account deletion defense mechanisms. The code demonstrates strong security engineering with fail-closed patterns, careful concurrency handling, and comprehensive testing. Below is detailed feedback organized by category.

---

✅ Security & Correctness

Excellent implementation of fail-closed authentication (src/middleware/auth.ts:38-65)

The deletion fence in enforceLiveAccountClaim correctly implements fail-closed security:

• Database lookup failures → 500 (not 401) ✅
• Missing account → generic 401 (never leaks deletion state) ✅
• No positive caching (by design) ✅
• Single carve-out for idempotent deletion replay is well-documented ✅

Strong concurrency control (src/accounts/repository.ts:31-38)

The advisory lock pattern correctly serializes mint vs. deletion:

await tx.$executeRaw`SELECT pg_advisory_xact_lock(hashtextextended(${identityHash}, 0))`

• Transaction-scoped lock prevents race conditions ✅
• Re-check barrier inside transaction after lock acquisition ✅
• Handles concurrent first-login P2002 retry correctly (line 85-92) ✅

HMAC barrier implementation is cryptographically sound (src/accounts/deletion/identity-hash.ts)

• Uses HMAC-SHA256 with dedicated secret ✅
• Domain separation between identity hashes and account refs ✅
• Case-insensitive on address (EIP-55 compatibility) ✅
• Secret rotation warning is clear and correct ✅

Router fencing audit is comprehensive (tests/deletion/router-fencing.test.ts)

Two-layer audit approach is excellent:

1. Source audit: grep-based enforcement that only allowlisted files call verifyJwtToken ✅
2. Behavioral audit: every JWT surface on real v2 router tested against deleted account ✅

---

🔍 Potential Issues

1. Minor: Device backfill failure is swallowed silently (src/api/v2/auth/handlers/generate-token.ts:228-233)

The AccountNotLiveError from requireLiveAccount during device backfill is caught and logged as warn, but the token mint still succeeds. This is marked "best-effort" and "fail-soft" in comments, which is reasonable, but consider:

} catch (err) {
  req.log.warn({ err, deviceId: body.deviceId, accountId }, "auth.device.account_backfill_failed");
}

Impact: Low - the backfill is truly optional and self-heals on next mint
Recommendation: Current behavior is correct, but consider adding a specific check for AccountNotLiveError to log at info instead of warn since it's an expected race condition, not a failure.

2. Advisory: requireAccount performs duplicate lookup (src/middleware/auth.ts:245-284)

Both enforceLiveAccountClaim (line 50) and requireAccount (line 263) perform the same Account.findUnique lookup. For routes using both authMiddleware + requireAccount, this is two sequential database hits per request.

Impact: Low - acceptable trade-off for fail-closed guarantees and middleware composability
Recommendation: Document this is intentional (defense in depth) or consider caching the lookup result in res.locals if the duplicated query becomes a measurable concern.

3. Code Style: Inconsistent error response format

Most errors return { error: "message" } but the 410 barrier includes a code field:

// generate-token.ts:121-124
res.status(410).json({
  error: "This identity has been deleted",
  code: "identity_deleted",
});

Impact: None - this is actually good API design (structured error codes for client handling)
Recommendation: Consider standardizing this pattern across other critical errors.

---

⚡ Performance Considerations

1. No positive caching by design (documented in PR)

Every authenticated request now performs an indexed PK lookup on Account. The PR description explicitly notes this.

Analysis:

• Lookup is O(1) indexed PK read ✅
• Fail-closed design requires it ✅
• No N+1 concerns (single query per request) ✅
• Trade-off is appropriate for security guarantees ✅

Recommendation: Monitor query latency metrics post-deploy. If this becomes a bottleneck at scale, consider:

• Read replicas for auth lookups
• Connection pooling tuning
• Caching with extremely short TTL (1-5s) if fail-closed can tolerate brief windows

2. Advisory lock on every SIWE mint (src/accounts/repository.ts:37)

Mints are serialized per-identity via PostgreSQL advisory lock.

Impact: Low - advisory locks are fast, and different identities don't block each other
Recommendation: Current implementation is correct. Only concurrent mints from the same wallet serialize (expected case: duplicate requests, not normal load).

---

🧪 Test Coverage

Excellent coverage overall (1661/1661 passing)

Specific strengths:

• ✅ Barrier mint tests cover all edge cases (barrier-mint.test.ts)
• ✅ Router fencing has both source + behavioral audits
• ✅ Auth middleware deletion scenarios tested (account-auth-check.test.ts)
• ✅ requireAccount fail-closed behavior tested (auth-require-account.test.ts)
• ✅ Case-insensitive hash collision test (barrier-mint.test.ts:94-104)
• ✅ Idempotent barrier write test (barrier-mint.test.ts:116-127)

Test improvements to consider:

1. Concurrency test missing: No test for the mint-vs-deletion race condition that the advisory lock prevents. Consider adding:

test("concurrent mint and deletion: barrier prevents resurrection", async () => {
  // Two transactions racing: one minting, one deleting
  // The advisory lock should prevent account resurrection
});

2. Infrastructure failure scenarios: The fail-closed pattern in enforceLiveAccountClaim handles DB errors → 500, but I don't see a test for Prisma throwing an error during barrier check (generate-token.ts:127-130). Current test suite has one in account-auth-check.test.ts:64-81 which is good.

---

📋 Configuration & Deployment

Environment variable handling is excellent (src/config.ts:140-148)

if (
  !process.env.DELETION_HASH_SECRET ||
  process.env.DELETION_HASH_SECRET.length < 64
) {
  throw new Error(
    "DELETION_HASH_SECRET is not configured or too short (need >= 64 chars / 32 bytes hex)",
  );
}

✅ Fail-fast at startup (not at first deletion)
✅ Length validation enforced
✅ Clear error message with required format
✅ Documented in .env.example with warnings about permanence

Deploy note is clear: PR description correctly emphasizes setting DELETION_HASH_SECRET before deploying.

One suggestion: Consider adding a startup log line that confirms DELETION_HASH_SECRET is set (without logging the value itself) for operational visibility.

---

🏗️ Code Quality & Best Practices

1. Excellent documentation

Every complex function has clear docstrings explaining:

• Purpose and security properties
• Concurrency semantics
• Failure modes
• Why specific approaches were chosen

2. Proper error handling hierarchy

Custom error types (IdentityBarredError, AccountNotLiveError) with proper prototype chain setup ✅

3. Strong separation of concerns

• Barrier logic isolated in src/accounts/deletion/ ✅
• Hash functions separate from barrier operations ✅
• Middleware properly layered (auth → fence → require) ✅

4. Type safety

Good use of TypeScript, Zod schemas, and Prisma types throughout ✅

Minor suggestion: The enforceLiveAccountClaim return type could be more explicit:

// Current
const enforceLiveAccountClaim = async (...): Promise<boolean> => {
  // Returns false after writing the response when the request must not proceed.
}

// Consider
const enforceLiveAccountClaim = async (...): Promise<{ shouldProceed: boolean }> => {
  // More self-documenting
}

---

🔐 Security-Specific Feedback

1. No unauthenticated deletion oracle ✅

Barrier check only runs after full SIWE validation (generate-token.ts:115-116). Cannot probe for deleted accounts without valid signature.

2. Generic 401 prevents information leakage ✅

Deleted accounts get { error: "Unauthorized" } on all routes except the mint-path 410, which is the only intentional signal.

3. Agent asset keys now account-scoped ✅

The S3 key pattern a/${accountId}/${uuid} (get-presigned-url.ts:36) enables slice 3's S3 purge. Good forward planning.

4. Router fencing is auditable ✅

The source audit (router-fencing.test.ts:50-73) will fail CI if someone adds a new JWT verification call outside the fenced middlewares. This is excellent preventive security.

---

📝 Minor Code Style Notes

1. Comment formatting is inconsistent

Some files use // for doc comments, others use proper /** */ JSDoc. The codebase convention appears to prefer JSDoc based on existing patterns.

2. Magic numbers

enforceLiveAccountClaim (middleware/auth.ts:43) uses hard-coded UUID validation via accountIdSchema. Consider extracting the validation into a named helper if reused elsewhere (though this may already exist in @/utils/account-id).

---

🎯 Adherence to CLAUDE.md

✅ No client-facing request schema changes: PR correctly notes the 410 is a new response for a state no shipped client can be in
✅ No credit/balance changes: This slice doesn't touch the ledger
✅ Backwards compatibility: Device-only JWTs still work (they don't hit the fence since !payload.accountId short-circuits)

---

Summary

This is high-quality, production-ready code. The implementation demonstrates:

• Strong security engineering (fail-closed patterns, no information leakage)
• Careful concurrency control (advisory locks, transaction semantics)
• Comprehensive test coverage (1661 passing tests, source + behavioral audits)
• Excellent documentation and error handling
• Proper separation of concerns

No blocking issues. The minor observations above are optimizations and suggestions for future consideration, not bugs.

Recommended next steps:

1. ✅ Deploy slice 2 with DELETION_HASH_SECRET configured
2. Monitor Account PK lookup latency in production metrics
3. Consider adding the concurrency race test mentioned above for completeness

Overall assessment: LGTM 🚢

---

[macroscopeapp]

Approvability

Verdict: Needs human review

This PR implements deletion barriers and fail-closed account fencing in core authentication middleware. Changes to auth/security infrastructure—including new HMAC-based identity barriers, transaction-level advisory locking, and database lookups on every authenticated request—require human review regardless of code ownership.

^{You can customize Macroscope's approvability policy. Learn more.}