Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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`
Original file line number Diff line number Diff line change
@@ -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.
2 changes: 1 addition & 1 deletion agent-hub/haven/diagrams/dev-loop.prime-mermaid.md
Original file line number Diff line number Diff line change
Expand Up @@ -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`. |
Expand Down
65 changes: 65 additions & 0 deletions src/__tests__/candidate/candidate.service.test.ts
Original file line number Diff line number Diff line change
@@ -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');
});
});
});
Loading