|
| 1 | +# PR Title |
| 2 | + |
| 3 | +fix(security): validate bcrypt passwordHistory and ownership-check distlock release (#797, #807) |
| 4 | + |
| 5 | +## Summary |
| 6 | + |
| 7 | +This PR resolves both security issues assigned to `judithJn`, hardening a high-impact code path on every password change (`users.passwordHistory`) and eliminating a Redis lock-takeover vulnerability in the distributed orchestration layer. |
| 8 | + |
| 9 | +## Issue #797 — `DistributedLockService.releaseLock` did not validate lock ownership |
| 10 | + |
| 11 | +**Risk:** the old `releaseLock` issued a plain Redis `DEL`, so any caller that knew the key could release a lock it did not own — a critical race condition for payment processing and payout disbursements. |
| 12 | + |
| 13 | +**Fix in `src/orchestration/locks/distributed-lock.service.ts`:** |
| 14 | + |
| 15 | +- `acquireLock` now generates a UUID v4 token and stores it as the Redis value via `SET key token PX <ttl> NX`. Return type is `Promise<string | null>` — the token on success, `null` when contended. |
| 16 | +- `releaseLock(key, token)` runs a Redis Lua `EVAL` script that **atomically** does a `GET`+`DEL`: the key is deleted only if the stored token matches the caller-supplied one; otherwise Redis returns `0`. Return type is `Promise<boolean>` so callers can tell a successful release from a rejected one. |
| 17 | +- The Lua script is the single source of truth for atomicity — no race window between check and delete, and no raw `DEL` is ever issued (regression-guarded by a unit test). |
| 18 | + |
| 19 | +## Issue #807 — `User.passwordHistory` could be persisted as plaintext |
| 20 | + |
| 21 | +**Risk:** `User.passwordHistory` is a `string[]`/`text[]` column with no application-level validation. A programming error could push raw passwords into that array and they would then live in the database indefinitely. |
| 22 | + |
| 23 | +**Fix delivered in two layers:** |
| 24 | + |
| 25 | +1. **Persistence gate** — new `src/users/entities/password-history.subscriber.ts`: |
| 26 | + - TypeORM `@EventSubscriber` on the `User` entity. |
| 27 | + - `@BeforeInsert` and `@BeforeUpdate` validate every entry of `passwordHistory` against `BCRYPT_HASH_REGEX = /^\$2[aby]\$\d{2}\$/`, covering all real bcrypt variants (`$2a$`, `$2b$`, `$2y$`) and rejecting phpass (`$2x$`), MD5-crypt (`$1$`), and plaintext. |
| 28 | + - Rejects with `InternalServerErrorException` and a logged safe preview, because plaintext reaching the persistence layer indicates a backend bug, not a user error. |
| 29 | + - Discovery follows the existing `TenantRlsSubscriber` pattern (`@EventSubscriber()` decorator + `users.module.ts` provider entry). |
| 30 | +2. **Service gate** — defence-in-depth in `src/users/services/password-history.service.ts`: |
| 31 | + - `addToHistory(...)` now refuses any non-bcrypt payload with `BadRequestException` before the change ever leaves the application. |
| 32 | + |
| 33 | +JSDoc on `User.passwordHistory` now documents the invariant and points at the subscriber. |
| 34 | + |
| 35 | +## Test coverage |
| 36 | + |
| 37 | +- `src/users/entities/password-history.subscriber.spec.ts` — 26 tests: |
| 38 | + - `listenTo` returns the User entity. |
| 39 | + - `beforeInsert`: empty/undefined history passes; valid `$2a$`/`$2b$`/`$2y$` hashes pass; plaintext, malformed cost factor, wrong algorithm tag, and `$2x$` phpass are rejected with `InternalServerErrorException`. |
| 40 | + - `beforeUpdate`: skips when `passwordHistory` is not in `updatedColumns`; validates the proposed entity value; warns-and-skips when the ORM event has no in-place entity (partial-update path); explicitly does NOT validate against `databaseEntity` (which holds the OLD row state — that would have been a silent bypass). |
| 41 | + - Direct `BCRYPT_HASH_REGEX` checks including `$2x$` rejection. |
| 42 | +- `src/orchestration/locks/distributed-lock.service.spec.ts` — 11 tests: |
| 43 | + - `acquireLock` returns a UUID v4 token on success, `null` on contention, uses 5 s default TTL, generates a fresh token per call, and stores it with `PX <ttl> NX`. |
| 44 | + - `releaseLock` invokes Redis `EVAL` with the correct script/keys/token, returns `true`/`false`/`false` for integer-1/integer-0/string-'0' respectively, and **never** calls `DEL` — the regression guard for issue #797. |
| 45 | + - Ownership security: a release attempt with a wrong token returns `false`; the legitimate owner still succeeds with their real token. |
| 46 | + |
| 47 | +All 37 new tests pass. `pnpm typecheck` passes cleanly. |
| 48 | + |
| 49 | +## Files changed |
| 50 | + |
| 51 | +- `src/orchestration/locks/distributed-lock.service.ts` *(modified)* |
| 52 | +- `src/orchestration/locks/distributed-lock.service.spec.ts` *(new)* |
| 53 | +- `src/users/entities/user.entity.ts` *(JSDoc only)* |
| 54 | +- `src/users/services/password-history.service.ts` *(modified — service gate)* |
| 55 | +- `src/users/entities/password-history.subscriber.ts` *(new)* |
| 56 | +- `src/users/entities/password-history.subscriber.spec.ts` *(new)* |
| 57 | +- `src/users/users.module.ts` *(subscriber provider registered)* |
| 58 | + |
| 59 | +## Breaking API changes |
| 60 | + |
| 61 | +- `DistributedLockService.acquireLock(key, ttl)` returns `Promise<string | null>` (was `Promise<boolean>`). Callers must now capture the token. |
| 62 | +- `DistributedLockService.releaseLock(key)` now requires a `(key, token)` signature and returns `Promise<boolean>` (was `Promise<void>`). The plain `DEL` fallback is deliberately not provided. |
0 commit comments