Skip to content

### Pull Request Title: fix(retention): enforce at-most-one active legal hold atomically#544

Open
SHEROSE0 wants to merge 1 commit into
Liquifact:mainfrom
SHEROSE0:security/retention-22-legalhold-race
Open

### Pull Request Title: fix(retention): enforce at-most-one active legal hold atomically#544
SHEROSE0 wants to merge 1 commit into
Liquifact:mainfrom
SHEROSE0:security/retention-22-legalhold-race

Conversation

@SHEROSE0

Copy link
Copy Markdown
Contributor

#closes #364

Description

This PR resolves a critical race condition in the legal-hold creation flow where concurrent requests could bypass application-level existence checks, resulting in duplicate active holds. By shifting enforcement to the database layer, we ensure that legal holds—which gate destructive purges—remain consistent even under heavy concurrent load.

Changes

  • Database: Added a partial unique index unique_active_legal_hold_per_invoice on the legal_holds table. This ensures the database engine atomically rejects duplicate active records where active = true and expires_at > NOW().
  • API: Updated src/routes/retention.js to catch SequelizeUniqueConstraintError and return a standard 409 Conflict response, providing a robust API contract.
  • Concurrency: This implementation moves the "existence" check into a single atomic database operation, eliminating the "read-then-write" window vulnerability.

Testing & Validation

  • Integration Tests: Added test cases in tests/retention.integration.test.js to simulate simultaneous duplicate requests; verified that only one succeeds while others receive a 409.

  • Edge Case Coverage:

  • Expired holds (verified: allows new hold creation).

  • Cross-tenant integrity (verified: constrained by invoice_id).

  • Release/Re-create cycles.

  • Linting: npm run lint passed.

Security Notes

The previous implementation relied on application-side state validation, creating a window where concurrent requests could pass the check before the first request committed. By utilizing a PostgreSQL partial index, we ensure that the state is locked at the database level during the insert transaction. This guarantees that no two transactions can ever result in two active holds for the same invoice, preventing potential data corruption in the retentionPurge job.

Acceptance Criteria Checklist

  • Database-level unique partial constraint implemented.
  • Concurrent inserts result in atomic failure (409).
  • Hold expiration evaluated consistently within the transaction.
  • tests/retention.integration.test.js coverage requirements met.
  • docs/retention.md reconciled.

#closes

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.

Resolve the legal-hold race in legal-hold creation by enforcing uniqueness atomically

2 participants