diff --git a/agent-hub/evidence/implementer/2026-09-28/fix-candidate-password-leak-plan.md b/agent-hub/evidence/implementer/2026-09-28/fix-candidate-password-leak-plan.md new file mode 100644 index 0000000..0014e8c --- /dev/null +++ b/agent-hub/evidence/implementer/2026-09-28/fix-candidate-password-leak-plan.md @@ -0,0 +1,116 @@ +# 2026-09-28 — fix-candidate-password-leak (plan + diff) + +- Worker: implementer +- Version: 0.1.0 +- Node: `fix-candidate-password-leak` (`haven/diagrams/dev-loop.prime-mermaid.md`) +- Issue: [#152](https://github.com/datvt243/resume-nodejs-api/issues/152) — Password hash leaked in candidate profile response +- Branch: `152-password-hash-leaked` (base `staging`) +- Task (verbatim): "fix bug #152 tới #156" — this note covers #152 only, part of a 5-issue batch requested in one `/todo` invocation, each issue processed as its own implementer→verifier round per the skill's normal flow. + +## Hub bytes before: 88066 + +## Node lookup + +Matched the existing PENDING node `fix-candidate-password-leak` directly +(task resolves to GitHub issue #152, filed against this exact node). + +## Bookkeeping-gap finding (read before writing anything) + +Same pattern as `fix-create-response-null-id` (sealed earlier this +session) and `fix-idor-broken-access-control`: the fix is already live on +`staging`. There IS an evidence note for this exact node from +`evidence/implementer/2026-08-21/fix-candidate-password-leak-diff.md` +(part of the same bundled commit `f355e2f`), but that note's own +`## Status` line still reads `sealed_pending_verifier` — it was never +picked up by a verifier pass, so the diagram row stayed PENDING for over +a month even though the code fix has been live the whole time. Confirmed +today by reading the live code, `src/candidate/candidate.service.ts`: + +```ts +export const handlerGetInformationById = async (id: string, props: { select: string } = { select: '' }) => { + const { select = '' } = props; + const find = MODEL.findById(id).select(select || '-password'); + return await find.exec(); +}; + +export const handlerGetInformationByEmail = async (email: string) => { + const safeEmailQuery = candidateQuerySafe.safeQuery({}, { email }); + const find = await MODEL.findOne(safeEmailQuery).select('-password').exec(); + return find; +}; +``` +(lines 37-53 today) — exactly matches the diff already recorded in the +2026-08-21 note. This note supersedes that stale note's pending status by +closing the real remaining gap: **no regression test existed anywhere** +for either function (confirmed: no `candidate.service.test.ts` file +existed in `src/__tests__/candidate/` before this diff, and no assertion +about an absent `password` field anywhere in +`candidate.controller.test.ts`). + +## Diff (smallest diff — no `src/` production code, 1 new test file) + +- `src/__tests__/candidate/candidate.service.test.ts` (new) — 3 tests: + 1. `handlerGetInformationByEmail` always calls `.select('-password')`. + 2. `handlerGetInformationById` defaults to `.select('-password')` when + no explicit `select` is given. + 3. `handlerGetInformationById` passes a given whitelisted select string + straight through unchanged (proves the old double-wrap no-op is + gone — a direct regression test for the exact root-cause bug). + +## Command + +``` +npm test +``` +Output (verbatim tail): +``` +Test Suites: 27 passed, 27 total +Tests: 143 passed, 143 total +Snapshots: 0 total +Time: 7.739 s +Ran all test suites. +``` +(143 = 140 from the last SEALED node (`fix-create-response-null-id`) + 3 +new tests here. Same pre-existing, unrelated "worker process has failed +to exit gracefully" harness warning as before — not a failure.) + +``` +npm run build +``` +Output: `tsc` clean, `copy` step ran with no errors. + +## Acceptance + +| Criterion | Evidence | +|---|---| +| Trace to exactly one diagram node | `fix-candidate-password-leak` | +| Smallest diff | 1 new test file only, 0 production `src/` changes (fix already live since `f355e2f`, 2026-08-21) | +| `GET /api/v1/candidate/:email` response has no `password` field | `handlerGetInformationByEmail` test — asserts `.select('-password')` is always called | +| `PUT`/`PATCH /candidate/update` response has no `password` field | `handlerGetInformationById` tests — asserts default `-password` select, and that an explicit select string is no longer silently dropped | +| Regression test asserting password absence | `src/__tests__/candidate/candidate.service.test.ts`, all 3 tests | +| Exact test command run + output read back | `npm test` → `Tests: 143 passed, 143 total`; `npm run build` clean | +| Evidence note written | This file | + +## Noticed, not done + +- The stale 2026-08-21 evidence note for this same node + (`evidence/implementer/2026-08-21/fix-candidate-password-leak-diff.md`) + is left as-is per the evidence directory's own rule (`NEVER DELETE` — + "fix a wrong note by adding a correction, don't delete it"). This note + is that correction. +- Live HTTP end-to-end (curl against a real running server) not + performed — same sandbox constraint as `fix-create-response-null-id` + (no local Mongo/Redis, Docker daemon unreachable). The 2026-08-21 note + already contains a real `npm run dev` manual verification transcript + for this exact behavior (pasted GET/PUT responses with no `password` + field) from when the fix was first written — cited here as prior + evidence of live behavior, not re-run today. + +## Seal gate + +No outward-facing action taken (no commit/push). Only a local file write: +1 new test file under `src/__tests__/`. Pending verifier. + +## Status + +`sealed_pending_verifier` diff --git a/agent-hub/evidence/verifier/2026-09-28/fix-candidate-password-leak-seal.md b/agent-hub/evidence/verifier/2026-09-28/fix-candidate-password-leak-seal.md new file mode 100644 index 0000000..a08fbc1 --- /dev/null +++ b/agent-hub/evidence/verifier/2026-09-28/fix-candidate-password-leak-seal.md @@ -0,0 +1,89 @@ +# 2026-09-28 — fix-candidate-password-leak (verifier verdict) + +- Worker: verifier (subagent, dispatched via Agent tool) +- Node: `fix-candidate-password-leak` (`haven/diagrams/dev-loop.prime-mermaid.md`) +- Evidence reviewed: `evidence/implementer/2026-09-28/fix-candidate-password-leak-plan.md` + (the 2026-09-28 note supersedes the stale `2026-08-21/fix-candidate-password-leak-diff.md` + note per its own text — that older note was NOT used as verdict input, + only its existence was cross-checked as context) +- New PM status: SEALED (was PENDING) + +## Isolation proof + +Dispatched as an independent Agent-tool spawn with task description +"Independent verifier pass for fix-candidate-password-leak" — fresh +context, no memory of the implementer session that wrote the 2026-09-28 +note. Self-grading is moot here by construction. + +## Reasoning + +Walking GitHub issue #152's acceptance criteria one at a time +(`gh issue view 152`): + +1. **`GET /api/v1/candidate/:email` response has no `password` field.** + Confirmed by independently reading current + `src/candidate/candidate.service.ts` (lines 37-53): + `handlerGetInformationByEmail` calls + `MODEL.findOne(safeEmailQuery).select('-password').exec()` — matches + the note's cited snippet verbatim. + +2. **`PUT`/`PATCH /candidate/update` response has no `password` field.** + Confirmed in the same file: `handlerGetInformationById` runs + `MODEL.findById(id).select(select || '-password')` — defaults to + excluding password when no explicit select is given, and no longer + double-wraps an explicit select string in `whitelistSelect([select])` + (the prior no-op bug). Matches the note's cited snippet verbatim. + +3. **Existing tests still pass; a regression test asserts password is + absent from the response.** Confirmed by reading + `src/__tests__/candidate/candidate.service.test.ts` directly — the + file exists and contains exactly the 3 tests the note claims: + `handlerGetInformationByEmail` asserts `select` was called with + `'-password'`; `handlerGetInformationById` asserts the same default; + a third test asserts an explicit whitelisted select string + (`'firstName lastName phone'`) passes through unchanged, which is a + direct regression test for the root-cause double-wrap bug. `npm test` + output in the note reads `Test Suites: 27 passed, 27 total / Tests: + 143 passed, 143 total` — a standard, non-truncated Jest summary; 143 = + 140 (the last SEALED node, `fix-create-response-null-id`, per + `worker-runs.log`) + 3 new tests here, internally consistent. + +**Test command**: `npm test`, matches `doctrine/MEMORY.md` exactly. + +**Bookkeeping-gap claim independently confirmed**: the production fix +(commit `f355e2f`, 2026-08-21) is genuinely already live and unchanged — +the code read today matches both the note's citation and the stale +2026-08-21 note's original diff. The real gap this note closes is the +missing regression test, which now exists. + +**Proportion (SmallestDiff)**: 1 new test file, 0 production `src/` +changes — proportionate; the production fix already exists and is +stable, so a regression-test-only diff is the smallest diff that closes +the actual remaining gap. + +**Seal gate**: `git status --short` on branch `152-password-hash-leaked` +shows only 2 untracked paths — the implementer's evidence note and the +1 new test file. Nothing staged, committed, or pushed. Matches the +note's "no outward-facing action taken" claim. + +## Forbidden states scan + +- `ADHOC_WORK` — no: node `fix-candidate-password-leak` pre-existed on + the diagram (row read before edit, was PENDING). +- `NO_EVIDENCE` — no: implementer note exists at the cited path, dated + and complete. +- `EDIT_UNVERIFIED` — no: `npm test` output is real, verbatim, not + truncated, and consistent with the described diff (143 = 140 + 3). +- `CODE_IN_HAVEN` — no: the only new file is + `src/__tests__/candidate/candidate.service.test.ts`, outside `haven/`; + no runnable code touched `agent-hub/`. +- `DIAGRAM_DRIFT` — resolved by this verdict: node row updated PENDING → + SEALED in place, no reorder. + +## Re-run + +`none` — audit-only, per recipe default for a non-outward-facing, non- +release-gate bug/test-coverage fix. The note's command matched doctrine, +output was verbatim and covered every acceptance criterion, and +independent reads of the cited source file and new test file confirmed +the claims. No partial or full re-run was warranted. diff --git a/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md b/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md index 3d04a76..4315b7b 100644 --- a/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md +++ b/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md @@ -57,7 +57,7 @@ flowchart TD | `agent-hub-token-cleanup-20260830` | SEALED | 2026-08-30 — archived, see `haven/diagrams/dev-loop-archive.md`. Evidence: `evidence/implementer/2026-08-30/agent-hub-token-cleanup-diff.md`. | | `fix-chrome-executable-path` | PENDING | `src/services/createPDF.ts:14-25` — Chrome executable path hardcoded, breaks PDF export in CI/Docker. See Traps in `doctrine/domains/PROJECT.md`. First candidate node. | | `fix-idor-broken-access-control` | SEALED | **Critical.** All CRUD APIs for candidate_profile (education/experience/award/certificate/project/reference/generalInformation) + `candidate.service.ts` + `fnExportPDF` never cross-check `candidateId`/`_id` against `req.user._id` (JWT) — they trust client-supplied `req.body.candidateId`/`_id`. Live-tested confirmed: User B could read/delete/edit User A's data, overwrite A's profile. Root cause: `verifyToken.middleware.ts` sets `req.user` but nothing cross-checks it. Found while testing the full API (task: "test the whole API again"). Code already fixed on `staging` (commit `f355e2f`, folded in via `1ec67de`'s ancestry) — bookkeeping-gap backfill, same pattern as #73/#74: `verifyToken.middleware.ts` forces `req.body.candidateId` to the authenticated `_id`; `baseUpdateDocument`/`baseDeleteDocument` check the existing document's real owner, not the payload; `candidate.controller.ts` forces `value._id`; `candidate_me/index.ts` `fnExportPDF` uses `req.user._id` directly. SEALED 2026-09-08 after independent re-read of every cited file (router, middleware, services/index.ts, BaseController/BaseService, candidate.controller.ts, candidate_me/index.ts, generalInformation.controller.ts) — evidence: `evidence/verifier/2026-09-08/fix-idor-broken-access-control-seal.md`. | -| `fix-candidate-password-leak` | PENDING | `src/candidate/candidate.service.ts` — `handlerGetInformationByEmail` has no `.select()` at all; `handlerGetInformationById` double-wraps `whitelistSelect([select])`, making the select a permanent no-op. Result: `GET /api/v1/candidate/:email` and `PUT/PATCH /candidate/update` return the raw bcrypt password hash in the response. | +| `fix-candidate-password-leak` | SEALED | GitHub issue #152. `src/candidate/candidate.service.ts` — `handlerGetInformationByEmail` has no `.select()` at all; `handlerGetInformationById` double-wraps `whitelistSelect([select])`, making the select a permanent no-op. Result: `GET /api/v1/candidate/:email` and `PUT/PATCH /candidate/update` return the raw bcrypt password hash in the response. Backfill, same pattern as `fix-idor-broken-access-control`/`fix-create-response-null-id`: the fix itself was already live since commit `f355e2f` (2026-08-21) — `handlerGetInformationByEmail` uses `.select('-password')`, `handlerGetInformationById` defaults to `.select(select \|\| '-password')` — independently confirmed by reading current `src/candidate/candidate.service.ts` (lines 37-53), matches the cited snippet verbatim. A stale evidence note from 2026-08-21 recorded this same diff but was never picked up by a verifier (`sealed_pending_verifier` left unresolved for a month); this pass covers the real remaining gap: no regression test existed anywhere. 1 new test file, 0 production changes, proportionate: `src/__tests__/candidate/candidate.service.test.ts` (confirmed present, 3 tests — `handlerGetInformationByEmail` always calls `.select('-password')`; `handlerGetInformationById` defaults to `-password` with no explicit select; `handlerGetInformationById` passes a given whitelisted select string through unchanged, proving the double-wrap no-op is gone). `npm test`: 143/143 passed (140 prior + 3 new), verbatim and not truncated, command matches `doctrine/MEMORY.md`. All 3 issue-#152 acceptance criteria met: GET `/candidate/:email` no `password` field, PUT/PATCH `/candidate/update` no `password` field, regression test added. SEALED 2026-09-28 after independent verifier pass (audit-only, no re-run — confirmed via `git status --short` on `152-password-hash-leaked` that nothing was committed/pushed). Evidence: `evidence/verifier/2026-09-28/fix-candidate-password-leak-seal.md`. | | `fix-refresh-token-expiry-unused` | PENDING | `TOKEN_EXP_IN` (`.env`, already exported in config) is never used at any `jwtSign()` call site (`auth.service.ts`, `auth.controller.ts`, `api/v1/auth/services/login.ts`) — access and refresh tokens always share the same default 1h expiry, so the refresh token is pointless. | | `fix-v2-register-missing-await` | PENDING | `src/api/v1/auth/services/register.ts:44` — `bcryptGenerateSalt(password)` missing `await`, the Promise gets assigned straight into the Mongoose model's password field → every `POST /api/v2/auth/register` fails with a Promise→string cast error. | | `fix-create-response-null-id` | SEALED | Minor. `BaseService.ts` `handlerCreate`'s `hookAfterSave` reassigns the local destructured `data` variable, never actually updating what `baseCreateDocument` returns → every `POST .../create` response has `data._id: null` instead of the real new ID. Backfill, same pattern as `fix-idor-broken-access-control`: the fix itself was already live since commit `f355e2f` (2026-08-21, `src/services/index.ts` lines ~300-302, `const replacement = await props.hookAfterSave(...); if (replacement !== undefined) _data = replacement;`) — independently confirmed via `git show f355e2f -- src/services/index.ts` and the current file, both match. This node closed the real gap: no regression test existed. 2 new test files added (`src/__tests__/services/baseCreateDocument.test.ts` — root-cause unit coverage; `src/__tests__/candidate_profile/BaseService.test.ts` — unmocked `createCrudService`/`BaseService.ts`/`services/index.ts` spot-check, only the Mongoose model faked), 0 production changes — proportionate. `npm test`: 140/140 passed. Live HTTP end-to-end was not performed (no local Mongo/Redis/Docker in the implementer's sandbox, disclosed honestly, not hidden) — accepted the unmocked code-path spot-check as a reasonable substitute given the fix has been live and stable for 5+ weeks. SEALED 2026-09-28 after independent verifier pass (audit-only, no re-run — note's `npm test` output was verbatim, not truncated, command matched `doctrine/MEMORY.md`; confirmed via `git status --short` on `157-post-create-responses` that nothing was committed/pushed). Evidence: `evidence/verifier/2026-09-28/fix-create-response-null-id-seal.md`. | diff --git a/src/__tests__/candidate/candidate.service.test.ts b/src/__tests__/candidate/candidate.service.test.ts new file mode 100644 index 0000000..d59db9b --- /dev/null +++ b/src/__tests__/candidate/candidate.service.test.ts @@ -0,0 +1,65 @@ +/** + * Regression coverage for issue #152 — GET /api/v1/candidate/:email and + * PUT/PATCH /candidate/update used to leak the raw bcrypt password hash. + * Root cause was 2 independent bugs in src/candidate/candidate.service.ts: + * handlerGetInformationByEmail had no .select() at all, and + * handlerGetInformationById double-wrapped an already-whitelisted, + * space-joined select string in candidateQuerySafe.whitelistSelect([select]) + * (treating the whole string as one field name, which never matched the + * allow-list, silently making the select a no-op). Both were already fixed + * on `staging` (commit f355e2f, 2026-08-21) — this just closes the missing + * regression-test gap. + */ +import * as MODELS from '@/models'; +import { handlerGetInformationByEmail, handlerGetInformationById } from '@/candidate/candidate.service'; + +jest.mock('@/models', () => ({ + Candidate: { findOne: jest.fn(), findById: jest.fn() }, + generalInformation: {}, + Experience: {}, + Education: {}, + Reference: {}, + Project: {}, + Certificate: {}, + Award: {}, + Application: {}, + Profile: {}, + Visit: {}, +})); + +describe('candidate.service.ts password exclusion (issue #152)', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + describe('handlerGetInformationByEmail', () => { + it('always excludes the password field via .select(\'-password\')', async () => { + const select = jest.fn().mockReturnValue({ exec: jest.fn().mockResolvedValue({ _id: '1', email: 'a@b.com' }) }); + (MODELS.Candidate.findOne as jest.Mock).mockReturnValue({ select }); + + await handlerGetInformationByEmail('a@b.com'); + + expect(select).toHaveBeenCalledWith('-password'); + }); + }); + + describe('handlerGetInformationById', () => { + it('defaults to excluding password when no explicit select is given', async () => { + const select = jest.fn().mockReturnValue({ exec: jest.fn().mockResolvedValue({ _id: '1' }) }); + (MODELS.Candidate.findById as jest.Mock).mockReturnValue({ select }); + + await handlerGetInformationById('1'); + + expect(select).toHaveBeenCalledWith('-password'); + }); + + it('uses the given whitelisted select string as-is, without re-wrapping it (no more double-wrap no-op)', async () => { + const select = jest.fn().mockReturnValue({ exec: jest.fn().mockResolvedValue({ _id: '1' }) }); + (MODELS.Candidate.findById as jest.Mock).mockReturnValue({ select }); + + await handlerGetInformationById('1', { select: 'firstName lastName phone' }); + + expect(select).toHaveBeenCalledWith('firstName lastName phone'); + }); + }); +});