Skip to content

test(user-repository): add failure-handling regression coverage #1027 - #1179

Open
ejiromowarin wants to merge 2 commits into
RevoraOrg:masterfrom
ejiromowarin:fix/1027-user-failure-handling-regression
Open

ejiromowarin wants to merge 2 commits into
RevoraOrg:masterfrom
ejiromowarin:fix/1027-user-failure-handling-regression

Conversation

@ejiromowarin

Copy link
Copy Markdown

Overview

This PR adds a focused, test-only regression suite that pins the three explicit
failure / empty-result exits of UserRepository — the exact branches named in the
issue (src/db/repositories/userRepository.ts:129, :182, :201). No production
code is touched, so the existing public contract is preserved exactly and the suite
documents current behaviour instead of redefining it.

Related Issue

Closes #1027

Changes

🧪 User Repository Failure Handling

  • [ADD] src/db/repositories/userRepository.regression.test.ts — 43 focused cases
    • line 129 — createUser with an empty RETURNING set: exact
      Error('Failed to create user') message and Error class (never
      UniqueConstraintError), never resolves with a partial or fabricated user,
      still throws when the driver reports rowCount: 1 for zero rows (decision is
      pinned to rows.length), still rejects on an unusable row (rows: [null]),
      deterministic across repeated calls, and the message leaks no SQL text, email
      or password hash.
    • line 182 — updateUser({ id }) with no updatable field: exact
      Error('User not found'), exactly one SELECT and never an UPDATE,
      explicitly-undefined optionals count as "not provided", an empty-string id
      is an ordinary lookup miss, deterministic, and never resolves with a user.
    • line 201 — updateUser whose UPDATE … RETURNING * matched no row: exact
      Error('Failed to update user'), the statement is issued exactly once, keyed
      on rows.length (a stale rowCount still throws), never resolves with a
      partial user, 23505 stays UniqueConstraintError, non-23505 errors are
      re-thrown by identity, and nothing sensitive leaks into the message.
    • Neighbouring success paths and boundaries for all three: fully mapped rows;
      five bound parameters in positional order for createUser; every supported
      field bound in declaration order with the id last for updateUser;
      startup/standard defaults and unknown stored tier → standard; stored
      NULL name → undefined; last_oidc_groups: null treated as a provided
      value (real UPDATE clearing groups) while [] serialises to "[]";
      empty-string and quote-laden emails bound as parameters; hostile email
      payloads never interpolated into SQL; updateUser({ id }) no-op read returns
      the existing user without an UPDATE.
    • updateKycRiskTier delegation: success plus propagation of the exact
      'Failed to update user' / 'User not found' contract to its callers.

📄 Documentation

  • [ADD] docs/user-failure-handling-regression.md — pinned contract rules,
    security assumptions and abuse/failure-path table, deliberately pinned follow-up
    items, run instructions, evidence and mutation results.

⚙️ Tooling

  • [MODIFY] package.json — adds test:coverage:user-failure, a focused run
    that applies the 95% threshold to the touched file.

Verification Results

npx jest src/db/repositories/userRepository.regression.test.ts --coverage=false
✅ 43/43 passed (1 suite, ~2 s, exits cleanly — no open handles)

npm run test:coverage:user-failure
✅ 72/72 passed
✅ userRepository.ts: 100% statements / 100% branches / 100% functions / 100% lines
   (gate: 95%, so every failure exit is provably executed)

surrounding suite + direct consumers (userRepository.test.ts, userRepository
.regression.test.ts, kycRiskTierService.test.ts, registerService.test.ts,
startupAuthService.test.ts, scim.test.ts)
✅ 157/157 passed (6 suites)

npx eslint src/db/repositories/userRepository.regression.test.ts
✅ exit 0 — no errors or warnings

npx tsc --noEmit
✅ 250 diagnostics — the same count as the pre-change baseline, none in either
   changed file

npm run validate:alert-mappings
✅ OK: All 22 known alerts have mapping entries.

Mutation check (each guard weakened in a scratch checkout, then restored —
nothing mutated is committed)
✅ 4/4 mutants killed:
   M1  line 129 throw → return null          → 6 focused tests fail
   M2  line 182 throw → return undefined     → 7 focused tests fail
   M3  line 201 rowCount instead of rows.len → 1 focused test fails
   M4  line 129 message drift                → 2 focused tests fail

⚠️ Pre-existing failures on the base commit (not introduced here)

  • npx jest --ci --coverage=false is not green on the base commit: 44 suites report
    FAIL and the run then stalls on a suite whose HTTP server never tears down, so no
    summary is printed. The new spec runs in ~2 s and exits cleanly.
  • Four of those failing suites were run twice — with this diff and with it stashed:
    18 failed / 263 passed both times.
  • npm run audit:ci fails identically with and without the diff
    (@stellar/stellar-sdk, express, qs, stellar-sdk, toml, plus a zod
    outdated-gate false positive); the gate reads package-lock.json, which this diff
    does not touch.
  • .github/workflows/ci.yml only triggers for PRs targeting main, and upstream has
    no main branch (every merged PR here targets master), so the audit /
    alert-mappings jobs never run on this repo's PRs. The local runs above are the
    substitute evidence; the workflow fix is deliberately out of scope for this PR.

Security Notes

  • A lost write (routing, replica, aborted transaction) yields an empty RETURNING
    set — the suite proves the call fails loudly instead of reporting success.
  • Failure messages are fixed literals, asserted to leak no SQL text, email or
    password hash: safe for logs/alerts, and not a probe surface for statement or
    credential detail.
  • Hostile email content (including '; DROP TABLE …) stays a bound parameter on
    both createUser and updateUser; the generated SQL never contains the payload.
  • Duplicate-email races stay distinct from empty-result failures: 23505 →
    UniqueConstraintError(field: 'email'), empty results → plain Error, so a
    softened guard cannot be mistaken for (or masked by) a conflict.
  • Deliberately pinned open items (documented, not changed): the repository performs
    no validation/normalisation (Callers: RegisterService, scim.ts, oidcRoute.ts),
    rows: [null] throws a TypeError (only "never returns a user" is asserted), and
    updatePasswordHash reports success for an unknown id.
Acceptance Criteria Status
Cover the named behavior with focused automated tests, including the relevant success and failure paths ✅ 43 cases — three failure exits, their neighbouring success paths, and boundary inputs
Preserve the existing public contract unless the change includes an explicit compatibility plan ✅ test-only diff; userRepository.ts is unchanged, messages/classes pinned as-is
Make error and boundary behavior observable and deterministic ✅ exact message + class assertions, rows.length vs rowCount, empty-string id/email, NULL name, last_oidc_groups: null, hostile SQL payloads
Run the focused test file and the surrounding suite ✅ 43/43 focused, 157/157 across six suites
Run the repository's configured lint, type, build, or contract checks ✅ eslint clean on the new file, tsc at baseline, alert-mapping contract OK, audit:ci unchanged (pre-existing)
Include the exercised cases and results in the pull request description ✅ this description + docs/user-failure-handling-regression.md

Locks the three explicit failure / empty-result exits of UserRepository so a
softened guard fails CI instead of silently succeeding in production:

- createUser, empty RETURNING set  -> plain Error('Failed to create user')
- updateUser({ id }) with no field -> plain Error('User not found')
- updateUser, empty UPDATE result  -> plain Error('Failed to update user')

New suite: src/db/repositories/userRepository.regression.test.ts (43 cases).
It pins exact messages, the Error class (never UniqueConstraintError for an
empty result), rows.length as the decision key (not rowCount), that no failure
path ever resolves with a partial/fabricated user, that failure messages leak
no SQL/email/password hash, that hostile email content stays a bound
parameter, and that the updateKycRiskTier delegation keeps the same contract.

Evidence (see docs/user-failure-handling-regression.md):
- focused suite 43/43, coverage run 72/72 with userRepository.ts at
  100% statements/branches/functions/lines against the 95% gate
- repository suite + direct consumers 157/157
- 4/4 mutants killed (throw->return null, throw->return undefined,
  rows.length->rowCount, message drift)
- eslint clean on the new file; tsc 250 error lines, same as baseline
- pre-existing repo failures (whole-suite stall, npm audit gate, 18 failures
  in four unrelated suites) reproduce identically with this diff stashed
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.

Add regression coverage for User failure handling

1 participant