diff --git a/agent-hub/evidence/implementer/2026-09-29/add-candidate-self-delete-plan.md b/agent-hub/evidence/implementer/2026-09-29/add-candidate-self-delete-plan.md new file mode 100644 index 0000000..166114d --- /dev/null +++ b/agent-hub/evidence/implementer/2026-09-29/add-candidate-self-delete-plan.md @@ -0,0 +1,136 @@ +# 2026-09-29 — add-candidate-self-delete (implementer plan) + +- Worker: implementer +- Version: 0.1.0 +- Node: `add-candidate-self-delete` (`haven/diagrams/dev-loop.prime-mermaid.md`) +- Task (verbatim): Candidate self-delete (issue #158): DELETE + /api/v1/candidate, authenticated, using only req.user._id (never a + client-supplied id, same IDOR-safe pattern as fix-idor-broken-access-control). + Must cascade-delete the candidate's data across all CV section models + (education/experience/award/certificate/project/reference/generalInformation, + plus application/profile since those now exist) by candidateId, and clean + up any uploaded files (CV PDF, section images) tied to that candidate. + Acceptance criteria: DELETE /api/v1/candidate removes the candidate + document and every CV-section document owned by that candidate; uploaded + files (resume PDF, section images) are removed from disk, not just + orphaned; a candidate cannot delete another candidate's account under any + input (id always comes from req.user._id); regression test covers the + cascade across all section models. Diagram node: add-candidate-self-delete + (currently PENDING). + +## Hub bytes before: 100436 + +## Finding: already fully implemented, bookkeeping-gap backfill +Same pattern as `fix-idor-broken-access-control`/`fix-candidate-password-leak`/ +`fix-create-response-null-id`: reading the real code shows the feature is +already 100% live and correct, just never backfilled with a diagram node +transition or evidence. + +- `DELETE /api/v1/candidate` -> `fnDelete` (`src/candidate/candidate.controller.ts:157-168`) + -> `handlerDelete` (`src/candidate/candidate.service.ts:118-148`). Introduced + by commit `32953ed` ("feat: add DELETE /api/v1/candidate for self-service + account deletion"), predating this hub's own history (before commit + `f355e2f`'s bug-fix batch, 2026-08-21) — this node has simply sat PENDING + on the diagram since the hub started tracking it. +- `fnDelete` reads NO client-supplied id anywhere — it calls + `handlerDelete((req as any).user?._id, (req as any).lang)` directly, no + body/params/query id read at all. Structurally IDOR-safe by construction, + not just by convention (nothing to override even if a client tried). +- `routers/api/v1/index.ts:27` — `router.use('/candidate', verifyToken, + routeCandidate)` — confirms the whole `/candidate` router (including this + route) sits behind `verifyToken`, so `req.user._id` is genuinely populated + by the time `fnDelete` runs. +- `handlerDelete` (`candidate.service.ts:20-30`) already has a + `CV_SECTION_MODELS` array covering ALL 9 models: `generalInformation`, + `Experience`, `Education`, `Reference`, `Project`, `Certificate`, `Award`, + `Application`, `Profile` — the last 2 were added later, 1-line-each, by + `add-application-tracker`/#132 and `add-cv-profile-selection`/#133 + specifically so this cascade wouldn't orphan those newer sections (both + those nodes' own evidence/diagram rows call this out explicitly). Confirms + the "plus application/profile since those now exist" part of the task is + already satisfied. +- File cleanup on disk already implemented: `CV_UPLOAD_DIR`/`{_id}-cv.pdf` + resume PDF (`candidate.service.ts:136-139`), plus every + project/certificate/award image (`IMAGE_SECTION_MODELS`, images collected + via `.find({candidateId}, {images:1})` BEFORE those documents are deleted, + then `fs.unlinkSync` per file, `candidate.service.ts:123-131,141-145`). + +The real remaining gap this node closes: **zero regression tests existed** +for `handlerDelete` (confirmed via `grep -rn "handlerDelete" src/__tests__` +before this diff — no hits) — exactly the issue's own 4th acceptance +criterion ("Regression test covers the cascade across all section models"). + +## Diff +| File | Why | +|---|---| +| `src/__tests__/candidate/candidate.service.test.ts` | New `handlerDelete` describe block (5 tests), appended to the existing file (already covers `candidate.service.ts`'s other handlers for issue #152). Extended the shared `jest.mock('@/models', ...)` factory to give `deleteMany`/`deleteOne`/`find` real `jest.fn()`s instead of `{}` placeholders — needed by `handlerDelete`, doesn't affect the pre-existing #152 tests (they only ever touch `Candidate.findOne`/`findById`). 0 production changes — the feature was already correct. | + +## Bug found and fixed during implementation (self-caught, in the test file only) +First attempt used `jest.mock('fs')` at the top of `candidate.service.test.ts` +to control `existsSync`/`unlinkSync`. `jest.mock()` calls are hoisted by +ts-jest/babel to run BEFORE the file's own `import` statements — so the +auto-mocked `fs` was already in place when `candidate.service.ts`'s import +chain (`utils/index.ts` -> `utils/bcrypt.ts` -> `bcrypt`) transitively +loaded the real `bcrypt` package, whose `@mapbox/node-pre-gyp` native-binding +resolver calls a REAL `fs.existsSync()` to find its own `package.json` at +import time. With `fs` auto-mocked, that call returned `undefined`, and the +whole suite failed to load: `node_modules/bcrypt/package.jsondoes not exist`. +Confirmed real (not a fluke): reproduced consistently, confirmed `bcrypt` +loads fine standalone via plain `node -e "require('bcrypt')"`, and traced +the stack trace to `pre-binding.js` inside `candidate.service.ts`'s own +import chain. Fixed by switching to `jest.spyOn(fs, 'existsSync')`/ +`jest.spyOn(fs, 'unlinkSync')` in `beforeEach` instead — `jest.spyOn` is a +normal runtime statement, not hoisted, so it only takes effect after every +import (including bcrypt's) has already resolved for real. `afterEach(() => +jest.restoreAllMocks())` added to fully restore `fs` after this describe +block. Not added to `doctrine/domains/PROJECT.md`'s Traps table — this is a +test-authoring footgun specific to `jest.mock('fs')` + any module in the +same file's import graph that needs real `fs` at load time (bcrypt here), +not a `src/` runtime bug; noted here for whoever writes the next fs-mocking +test in this codebase. + +## Command +`npm test` (from `/Users/_david/Workspace/Project/resume/resume-nodejs-api`) + +## Output (verbatim, tail) +``` +A worker process has failed to exit gracefully and has been force exited. This is likely caused by tests leaking due to improper teardown. Try running with --detectOpenHandles to find leaks. Active timers can also cause this, ensure that .unref() was called on them. +Test Suites: 29 passed, 29 total +Tests: 160 passed, 160 total +Snapshots: 0 total +Time: 7.634 s, estimated 12 s +Ran all test suites. +``` +(The "worker process failed to exit gracefully" warning is the same +pre-existing Jest/open-handle notice seen on prior nodes this session, +unrelated to this diff — every suite still passed, 0 failures. 160 = 155 +baseline, from the immediately preceding sealed node `add-bulk-import-cv-sections` +this session, + 5 new `handlerDelete` tests.) + +Also ran `npm run build` (tsc && copy) — clean, no typecheck errors, no +output beyond the copy step. + +## Acceptance +| Criterion | Evidence | +|---|---| +| `DELETE /api/v1/candidate` removes the candidate document and every CV-section document owned by that candidate | `handlerDelete` (`candidate.service.ts:133-134`) — `Promise.all(CV_SECTION_MODELS.map(...deleteMany({candidateId:_id})))` then `MODEL.deleteOne({_id})`. Regression test "cascades deleteMany({candidateId}) across every CV section model, then deletes the candidate document itself" asserts all 9 models' `deleteMany` called with `{candidateId: 'cand1'}` AND `Candidate.deleteOne` called with `{_id: 'cand1'}` — part of `Tests: 160 passed, 160 total` | +| Uploaded files (résumé PDF, section images) are removed from disk, not just orphaned | `candidate.service.ts:136-145` — `fs.unlinkSync` on the CV file path and every collected image path, gated by `fs.existsSync`. 3 regression tests: "removes the candidate's uploaded CV file from disk when one exists", "never calls unlinkSync for a CV file that does not exist on disk", "removes every project/certificate/award image file from disk, collected before those documents are deleted" — all pass | +| A candidate cannot delete another candidate's account under any input (id always comes from req.user._id) | `fnDelete` (`candidate.controller.ts:157-168`) never reads any id from `req.body`/`req.params`/`req.query` — only `(req as any).user?._id`, populated exclusively by `verifyToken` (confirmed via `routers/api/v1/index.ts:27` mounting `/candidate` behind `verifyToken`). Structural guarantee, not test-covered at the controller layer per this codebase's own established convention (see `candidate.controller.test.ts`'s file header: "the rest of candidate.controller.ts is thin wiring already covered indirectly elsewhere, same precedent as every other controller in this codebase") — `fnDelete` has zero branching/logic to unit-test beyond what's already visible by inspection | +| Regression test covers the cascade across all section models | 5 new tests in `src/__tests__/candidate/candidate.service.test.ts`'s new `handlerDelete (issue #158)` describe block, part of `Tests: 160 passed, 160 total` | + +## Noticed, not done +- No controller-level test added for `fnDelete` itself — see the 3rd + acceptance-criterion row above for why (zero logic to test beyond what + the service-level tests + code inspection already prove; matches this + codebase's own stated convention for thin controller wiring). +- The `jest.mock('fs')` -> `jest.spyOn(fs, ...)` footgun (see "Bug found and + fixed" above) is worth a one-line addition to a future test-authoring + note in `haven/workers/implementer/MEMORY.md`'s "Patterns that work here" + (currently `<>`) — not added in this diff, flagged for whoever + next fills that section. + +## Seal gate +Not applicable to this recipe step — no outward-facing action (commit, +push, delete, external API call) has happened yet. The single test-file +diff was shown in full in the session transcript. Commit/push waits for +`/ship` (not invoked this round; `/todo` was run without `--ship`). diff --git a/agent-hub/evidence/verifier/2026-09-29/add-candidate-self-delete-seal.md b/agent-hub/evidence/verifier/2026-09-29/add-candidate-self-delete-seal.md new file mode 100644 index 0000000..2e658c4 --- /dev/null +++ b/agent-hub/evidence/verifier/2026-09-29/add-candidate-self-delete-seal.md @@ -0,0 +1,150 @@ +# 2026-09-29 — add-candidate-self-delete (verifier verdict) + +- Worker: verifier (subagent, dispatched via Agent tool) +- Node: `add-candidate-self-delete` +- New PM status: SEALED + +## Isolation proof +Dispatched fresh via the Agent tool with the task string "Run verify_seal +for node `add-candidate-self-delete`, judging the evidence note at +`agent-hub/evidence/implementer/2026-09-29/add-candidate-self-delete-plan.md`" +— no memory of any implementer session, everything below was re-derived +independently from the repo (reading real files, running `git log`/`git +diff`/`git status`, re-running `npm test`/`npm run build` myself) rather +than trusted from the note's prose. + +## Reasoning + +1. **Diff matches the note's claim, genuinely minimal.** `git status + --short` on `158-candidate-self-delete-allow`: only `M + src/__tests__/candidate/candidate.service.test.ts` and the new + untracked evidence-note directory. `git diff staging --stat -- src/`: + 1 file, `+122/-11`. No change to `candidate.service.ts`, + `candidate.controller.ts`, or any route file — confirmed by their + absence from the diff stat, not inferred. + +2. **`candidate.service.ts` read in full.** `CV_SECTION_MODELS` + (lines 20-30) lists all 9: generalInformation, Experience, Education, + Reference, Project, Certificate, Award, Application, Profile. + `handlerDelete` (118-148): `if (!(await MODEL.findById(_id))) return + { success: false, ... }` before any delete call — confirmed no + `deleteMany`/`deleteOne` is reachable on that branch. Then + `Promise.all(CV_SECTION_MODELS.map(model => model.deleteMany({ + candidateId: _id })))` followed by `MODEL.deleteOne({ _id })`. Image + filenames are collected via `IMAGE_SECTION_MODELS` (`[Project, + Certificate, Award]`) `.find({candidateId}, {images:1})` BEFORE the + cascade delete runs (lines 127-131, ordered strictly before line 133), + then each image and the CV PDF (`CV_UPLOAD_DIR/{_id}-cv.pdf`) is + `fs.unlinkSync`'d gated by `fs.existsSync` (136-145). + +3. **`candidate.controller.ts`'s `fnDelete` read in full** (157-168): the + only identifier used is `(req as any).user?._id`; the function body + contains no reference to `req.body`, `req.params`, or `req.query` + anywhere. Structural, not conventional. + +4. **Routing read in full.** `routers/api/v1/index.ts:27` — `router.use( + '/candidate', verifyToken, routeCandidate)` mounts the entire + candidate router (including `DELETE /`) behind `verifyToken`. + `candidate.route.ts:261` — `router.delete('/', fnDelete)`, no + alternate/unauthenticated route to the same handler found anywhere in + the file. + +5. **Git history independently confirmed.** `git log --oneline -- src/ + candidate/candidate.service.ts | tail -20` shows `32953ed feat: add + DELETE /api/v1/candidate for self-service account deletion`, + chronologically before `f355e2f fix: close broken access control, + password leak, and 3 other API bugs` in the same log — matches the + note's claim exactly. + +6. **Test diff read in full via `git diff staging`.** New `describe` + block `candidate.service.ts handlerDelete (issue #158)` has exactly 5 + `it(...)` tests: not-found short-circuit (asserts `deleteOne` and + every section's `deleteMany` NOT called), full 9-model cascade + (asserts `deleteMany({candidateId:'cand1'})` on all 9 keys + + `Candidate.deleteOne({_id:'cand1'})`), CV-file-exists-so-unlink, + CV-file-missing-so-no-unlink, and project/certificate/award image + cleanup collected before deletion. `grep -n "jest.mock('fs')"` in the + file matches ONLY the explanatory code comment (line 92 context), + never an actual `jest.mock('fs')` call; the file does use + `jest.spyOn(fs, 'existsSync'/'unlinkSync')` in `beforeEach` and + `jest.restoreAllMocks()` in `afterEach` — the self-reported bug/fix + narrative is real, not just narrated. The shared `jest.mock('@/models', + ...)` factory was extended in place (placeholders `{}` → real + `{deleteMany: jest.fn()}` etc., `Project`/`Certificate`/`Award` also + gained `find: jest.fn()`) — confirmed the pre-existing `describe( + 'candidate.service.ts password exclusion (issue #152)')` block (lines + 1-64) is byte-for-byte unmodified in the diff context and its 3 tests + (`handlerGetInformationByEmail`, `handlerGetInformationById` x2) still + read from the file untouched. + +7. **`npm test` independently re-run** (not audit-only — self-selected + given the unusual "0 production diff" claim, an explicit re-run + scenario worth the cost here). Verbatim tail of my own run: + ``` + Test Suites: 29 passed, 29 total + Tests: 160 passed, 160 total + Snapshots: 0 total + Time: 8.24 s + Ran all test suites. + ``` + Matches the note's claimed `29 passed, 29 total` / `160 passed, 160 + total` exactly. Same pre-existing "worker process failed to exit + gracefully" notice, unrelated to this diff (present on prior SEALED + nodes too). + +8. **`npm run build` independently re-run.** Output: `tsc && npm run + copy` → `cp -R ./src/views ./src/public ./dist/`, no typecheck errors, + clean. `git status --short` after build shows no new/stray changes + (dist/ is gitignored, as expected). + +9. **`doctrine/domains/PROJECT.md` Traps table checked** — none of the 7 + listed traps (Chrome path, CORS wildcard, body-size limit, auth + validation skip, lint script, public/ static exposure, prod port) + relate to this diff; nothing here reintroduces any of them. + +10. **Controller-not-unit-tested reasoning checked directly.** + `candidate.controller.test.ts`'s file header reads: "The rest of + candidate.controller.ts is thin wiring already covered indirectly + elsewhere, same precedent as every other controller in this + codebase (not unit-tested per-function)." `fnDelete` itself is 12 + lines (157-168), a single try/catch, zero conditionals — a + reasonable call given the codebase's own stated precedent, and the + IDOR-safety claim (criterion 3) is structural (verified directly + above), not something a controller-level mock test would add + confidence to. + +**Proportion**: diff is exactly 1 test file, 0 production code — smaller +than the node's own scope could have required (a naive implementer could +have "fixed" already-correct code); appropriately minimal, matches +`SmallestDiff`. + +**Forbidden states scan**: `ADHOC_WORK` — no, real diagram node exists and +this ran through the verify_seal recipe. `NO_EVIDENCE` — no, implementer +note + this verdict note both exist. `EDIT_UNVERIFIED` — no, `npm test`/ +`npm run build` were independently re-run and read back verbatim above, +not inferred. `CODE_IN_HAVEN` — no `.ts`/`.js` files were added to +`haven/`. `DIAGRAM_DRIFT` — being corrected by this SEAL (PM status now +matches the confirmed-live code state). + +## Re-run +`full` — re-ran `npm test` (29/29 suites, 160/160 tests, matched exactly) +and `npm run build` (clean) myself from the repo root, plus independently +read every file the note cited (`candidate.service.ts`, +`candidate.controller.ts`, `routers/api/v1/index.ts`, +`candidate.route.ts`, the full test diff, `candidate.controller.test.ts`'s +header) rather than auditing the note's prose alone, and ran `git log`/ +`git diff`/`git status` myself to confirm the minimal-diff and git-history +claims. Justification: this node's central claim ("0 production code +changed, feature already fully live") is unusual enough, and the +coordinating task explicitly requested this depth of independent +confirmation, to warrant paying the re-run cost rather than defaulting to +audit-only. + +## Hub bytes before +100436 (per implementer's note) + +## Hub bytes after +103617 (measured after updating diagram PM status, same 5-category +`/hub-tokens` per-session-total formula: root .md files + doctrine/ + the +active non-archive haven/diagrams/ file + implementer worker bundle + +verifier worker bundle, raw byte counts summed) diff --git a/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md b/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md index 17ac2f0..5544d14 100644 --- a/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md +++ b/agent-hub/haven/diagrams/dev-loop.prime-mermaid.md @@ -63,7 +63,7 @@ flowchart TD | `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` | SEALED | `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. One level past the other backfills in this batch: the named buggy file doesn't exist anywhere in the repo any more (`find src/api -iname "*.ts"` empty) — deleted outright by `consolidate-v1-v2-auth` (#77, SEALED 2026-08-29), which routes v2's `/register` through the exact same `authRegister` → `handlerRegister` (`src/auth/auth.service.ts:48`) that v1 always used, and that path already correctly `await`s `bcryptGenerateSalt`. Independently confirmed: `src/routers/api/v2/auth.route.ts` imports `authRegister` from `@/auth/auth.controller`; `handlerRegister` has `const bcryptPwd = await bcryptGenerateSalt(password);`; the existing `auth.service.test.ts` "should register successfully with new email" test mocks the hash via `mockResolvedValue` and asserts `CandidateModel.create` receives the plain string `mockHash` — a missing `await` would leave `bcryptPwd` a Promise and fail that `toHaveBeenCalledWith`, so this is a real load-bearing assertion, not an accidental-pass happy path. The genuine gap this diff closes: nothing proved v2's route actually reaches that handler. New `src/__tests__/auth/v2AuthRoute.test.ts` inspects the real `router.stack` and asserts the `/register` layer's handle is the literal `authRegister` function reference (`.toBe`, identity not deep-equal) — re-run independently by the verifier (`npx jest v2AuthRoute.test.ts auth.service.test.ts`): 2 suites, 14 tests, all pass. 1 new test file, 0 production changes — proportionate given the vulnerable code path no longer exists. Live end-to-end not performed (same sandbox constraint as the rest of this batch: no Mongo/Redis/Docker), disclosed honestly. SEALED 2026-09-28 after independent verifier pass (audit-only + targeted test re-run; confirmed via `git status --short` on `156-post-apiv2-authregister` that nothing was committed/pushed). Evidence: `evidence/verifier/2026-09-28/fix-v2-register-missing-await-seal.md`. | | `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`. | -| `add-candidate-self-delete` | PENDING | Feature (not a bug). No endpoint lets a candidate delete their own account — needed to clean up 2 test accounts created during live-verification of the 5 bug fixes above on production (`livecheck+...@example.com`, `livecheckB+...@example.com`). Requirement: `DELETE /api/v1/candidate`, using only `req.user._id` (never an id from the client — follows the IDOR-safe pattern from `fix-idor-broken-access-control`), cascade-deletes data across all 7 CV section models by `candidateId`. | +| `add-candidate-self-delete` | SEALED | GitHub issue #158. Backfill, same pattern as `fix-idor-broken-access-control`/`fix-candidate-password-leak`/`fix-create-response-null-id`: `DELETE /api/v1/candidate` was already fully implemented and live, introduced by commit `32953ed` ("feat: add DELETE /api/v1/candidate for self-service account deletion"), predating this hub's own history (before `f355e2f`'s bug-fix batch, 2026-08-21) — sat PENDING on the diagram purely for lack of a backfilled node/evidence. `fnDelete` (`candidate.controller.ts:157-168`) reads no id from `req.body`/`req.params`/`req.query`, only `(req as any).user?._id`; `router.use('/candidate', verifyToken, routeCandidate)` (`routers/api/v1/index.ts:27`) confirms the whole router, including `router.delete('/', fnDelete)` (`candidate.route.ts:261`), sits behind `verifyToken` — structurally IDOR-safe, independently re-confirmed by reading both files, not just the note's citation. `handlerDelete` (`candidate.service.ts:118-148`) checks existence first (`MODEL.findById`, returns `success:false` with zero deletes if not found), cascades `deleteMany({candidateId})` across all 9 `CV_SECTION_MODELS` (generalInformation/Experience/Education/Reference/Project/Certificate/Award/Application/Profile — independently read and counted) then `Candidate.deleteOne`, and cleans up on-disk files: the uploaded CV PDF and every project/certificate/award image (`IMAGE_SECTION_MODELS`), images collected via `.find({candidateId},{images:1})` BEFORE their documents are deleted, each removal gated by `fs.existsSync`. The real gap this node closed: zero regression tests existed for `handlerDelete`. Diff: 1 file, `src/__tests__/candidate/candidate.service.test.ts` — 5 new tests (not-found short-circuit, full 9-model cascade, CV-file-exists-unlink, CV-file-missing-no-unlink, project/certificate/award image cleanup), independently read in full via `git diff staging`; shared `jest.mock('@/models', ...)` factory extended (not replaced) with `deleteMany`/`deleteOne`/`find`, pre-existing #152 password-exclusion tests confirmed still present and unmodified. Self-reported footgun independently confirmed real: a first attempt with `jest.mock('fs')` (hoisted above imports) broke bcrypt's native-binding resolution at import time; the file as committed uses `jest.spyOn(fs, 'existsSync'/'unlinkSync')` in `beforeEach` + `jest.restoreAllMocks()` in `afterEach` — confirmed via `grep -n "jest.mock('fs')"` returning only the explanatory code comment, never an actual call. 0 production code changed — confirmed via `git diff staging --stat -- src/` showing only the one test file. No controller-level test added for `fnDelete` — reasonable given its 12 lines/single try-catch/zero branching, and matches `candidate.controller.test.ts`'s own file-header precedent ("thin wiring already covered indirectly elsewhere, same precedent as every other controller in this codebase"), independently read and confirmed verbatim. SEALED 2026-09-29 after independent verifier full re-run (self-selected given the unusual "0 production diff" claim): `git status --short`/`git diff staging` on `158-candidate-self-delete-allow` confirmed only the test file + new evidence note changed; `git log --oneline -- src/candidate/candidate.service.ts` confirmed `32953ed` predates `f355e2f`; `npm test` independently reproduced 29/29 suites, 160/160 tests exactly as claimed; `npm run build` independently reproduced clean, no typecheck errors. Evidence: `evidence/implementer/2026-09-29/add-candidate-self-delete-plan.md`, `evidence/verifier/2026-09-29/add-candidate-self-delete-seal.md`. No commit/push has happened yet (`/todo` invoked without `--ship`). | | `fix-candidate-me-candidateid-not-string` | SEALED | **Critical, found by accident while testing #79.** GitHub issue #153. `candidate_me/index.ts` `handlerGetAboutMe` — `_id` from the raw Mongoose document is an ObjectId instance, passed straight into `idQuerySafe.safeQuery({}, { candidateId: _id })` — `QuerySafe.safeQuery` only accepts `typeof value === 'string'`, so an ObjectId silently fails that check and `candidateId` gets dropped from the filter → `model.find({})` returns CV data (education/experience/award/certificate/project/generalInformation) for **every candidate mixed together**, on every `GET /api/me/:email` request (public, no auth) and `/download-pdf`. Live-tested confirmed: a brand-new candidate profile returned real data belonging to `votan.it@gmail.com`. Backfill, same pattern as `fix-create-response-null-id`/`fix-candidate-password-leak`: the fix itself (`.toString()` on `_id`) was already live since the #135 pass (2026-09-19) — independently confirmed via `src/candidate_me/index.ts` lines 122-129, matching the note's citation exactly; also confirmed `fnExportPDF` (`/download-pdf`) calls `handlerGetAboutMe` internally rather than re-implementing its own filter, so no separate call site needed a fix, and that the only other `candidateId`-bearing query in the file (`Profile.findOne` at line 100) bypasses `QuerySafe` entirely and was never exposed to this bug class. This node's real gap: no regression test exercised an ObjectId-typed `_id` (existing tests used a plain string, which already passes the string-only `typeof` check even with the old buggy code). 1 new describe block added to `src/__tests__/candidate_me/index.test.ts` (ObjectId-shaped `_id` via `{ toString() }`, asserts stringified `candidateId` reaches every section's `.find()`, never the raw object, never an unfiltered `{}`), 0 production changes — proportionate. `npm test`: 141/141 passed, independently re-run by the verifier (both the targeted file and the full suite) given the Critical severity, numbers matched exactly. Live HTTP end-to-end against 2+ real accounts (issue's 2nd acceptance criterion) was not performed (no local Mongo/Redis/Docker in the implementer's sandbox, disclosed honestly) — accepted the unmocked code-path regression test as a reasonable substitute, same pattern already accepted for `fix-create-response-null-id`/`fix-candidate-password-leak`, given the fix has been live and stable for 9+ days and the issue's own diagnostic section already recorded the original live-test finding against production data. SEALED 2026-09-28 after independent verifier pass (partial re-run; confirmed via `git status --short` on `153-critical-candidate-me` that nothing was committed/pushed). Evidence: `evidence/verifier/2026-09-28/fix-candidate-me-candidateid-not-string-seal.md`. | | `feat-i18n-api-messages-auth` | PENDING | Feature, GitHub issue #78 (phase 1 of several). i18n infrastructure (hand-rolled `t(key, lang)`, reads `locales/vi.json`/`en.json`, middleware detects `Accept-Language`, defaults `vi`) + fully migrates the auth flow (register/login/logout/refresh). Does NOT migrate Joi validation messages (different architecture — Joi schemas are built once at module load with no request context; needs error TYPE → i18n key mapping, left as a follow-up). Does NOT touch candidate/CV section messages (separate follow-up). | | `feat-i18n-full-coverage` | PENDING | Feature, GitHub issue #78 (phase 2/2 — complete). Joi validation messages: a generic system translating by `detail.type` + `fieldLabels` (`utils/valid.ts`), no longer relying on hardcoded `.messages()` per schema. Mongoose `required` messages: same approach in `handleError` (`utils/helper.ts`). Every candidate/CV section success/error message (`services/index.ts`, `BaseController.ts`, `BaseService.ts`, `candidate.service.ts`, `generalInformation.*`) cascades across all 7 CV sections. Bug found during implementation: `t()`'s dot-path walker misparsed Joi type strings containing a dot (`any.required` was read as 3 nested levels) — caught via a real live test (curl in 2 languages), not code review. Fix: a dedicated `tErrorType()` function, flat lookup with no dot-path walking. | diff --git a/src/__tests__/candidate/candidate.service.test.ts b/src/__tests__/candidate/candidate.service.test.ts index d59db9b..96a8978 100644 --- a/src/__tests__/candidate/candidate.service.test.ts +++ b/src/__tests__/candidate/candidate.service.test.ts @@ -10,20 +10,21 @@ * on `staging` (commit f355e2f, 2026-08-21) — this just closes the missing * regression-test gap. */ +import fs from 'fs'; import * as MODELS from '@/models'; -import { handlerGetInformationByEmail, handlerGetInformationById } from '@/candidate/candidate.service'; +import { handlerGetInformationByEmail, handlerGetInformationById, handlerDelete } from '@/candidate/candidate.service'; jest.mock('@/models', () => ({ - Candidate: { findOne: jest.fn(), findById: jest.fn() }, - generalInformation: {}, - Experience: {}, - Education: {}, - Reference: {}, - Project: {}, - Certificate: {}, - Award: {}, - Application: {}, - Profile: {}, + Candidate: { findOne: jest.fn(), findById: jest.fn(), deleteOne: jest.fn() }, + generalInformation: { deleteMany: jest.fn() }, + Experience: { deleteMany: jest.fn() }, + Education: { deleteMany: jest.fn() }, + Reference: { deleteMany: jest.fn() }, + Project: { deleteMany: jest.fn(), find: jest.fn() }, + Certificate: { deleteMany: jest.fn(), find: jest.fn() }, + Award: { deleteMany: jest.fn(), find: jest.fn() }, + Application: { deleteMany: jest.fn() }, + Profile: { deleteMany: jest.fn() }, Visit: {}, })); @@ -63,3 +64,113 @@ describe('candidate.service.ts password exclusion (issue #152)', () => { }); }); }); + +/** + * Regression coverage for issue #158 — candidate self-delete cascade. The + * feature itself (DELETE /api/v1/candidate -> fnDelete -> handlerDelete) + * was already live since commit 32953ed ("add DELETE /api/v1/candidate for + * self-service account deletion") with no matching diagram node/evidence + * note at the time (bookkeeping-gap backfill, same pattern as + * fix-idor-broken-access-control/fix-candidate-password-leak) — this closes + * the missing regression-test gap called out in the issue's own 4th + * acceptance criterion ("Regression test covers the cascade across all + * section models"). + */ +const CV_SECTION_MODEL_KEYS = [ + 'generalInformation', + 'Experience', + 'Education', + 'Reference', + 'Project', + 'Certificate', + 'Award', + 'Application', + 'Profile', +] as const; + +describe('candidate.service.ts handlerDelete (issue #158)', () => { + // jest.mock('fs') (module-level, hoisted above imports) would auto-mock + // fs BEFORE bcrypt's own node-pre-gyp native-binding resolution runs at + // import time (candidate.service.ts -> utils/index.ts -> utils/bcrypt.ts + // -> bcrypt) -- broke the whole suite with "package.json does not + // exist" the first time this was tried. jest.spyOn (a real statement, + // not hoisted) runs after imports already resolved, so it's safe. + beforeEach(() => { + jest.clearAllMocks(); + jest.spyOn(fs, 'existsSync').mockReturnValue(false); + jest.spyOn(fs, 'unlinkSync').mockImplementation(() => undefined); + for (const key of CV_SECTION_MODEL_KEYS) { + (MODELS as any)[key].deleteMany.mockResolvedValue({ deletedCount: 1 }); + } + (MODELS.Project.find as jest.Mock).mockResolvedValue([]); + (MODELS.Certificate.find as jest.Mock).mockResolvedValue([]); + (MODELS.Award.find as jest.Mock).mockResolvedValue([]); + (MODELS.Candidate.deleteOne as jest.Mock).mockReturnValue({ exec: jest.fn().mockResolvedValue({}) }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + it('returns idNotFound and deletes nothing when the candidate does not exist', async () => { + (MODELS.Candidate.findById as jest.Mock).mockResolvedValue(null); + + const result = await handlerDelete('missing-id'); + + expect(result.success).toBe(false); + expect(MODELS.Candidate.deleteOne).not.toHaveBeenCalled(); + for (const key of CV_SECTION_MODEL_KEYS) { + expect((MODELS as any)[key].deleteMany).not.toHaveBeenCalled(); + } + }); + + it('cascades deleteMany({candidateId}) across every CV section model, then deletes the candidate document itself', async () => { + (MODELS.Candidate.findById as jest.Mock).mockResolvedValue({ _id: 'cand1' }); + (fs.existsSync as jest.Mock).mockReturnValue(false); + + const result = await handlerDelete('cand1'); + + for (const key of CV_SECTION_MODEL_KEYS) { + expect((MODELS as any)[key].deleteMany).toHaveBeenCalledWith({ candidateId: 'cand1' }); + } + expect(MODELS.Candidate.deleteOne).toHaveBeenCalledWith({ _id: 'cand1' }); + expect(result.success).toBe(true); + }); + + it("removes the candidate's uploaded CV file from disk when one exists", async () => { + (MODELS.Candidate.findById as jest.Mock).mockResolvedValue({ _id: 'cand2' }); + (fs.existsSync as jest.Mock).mockImplementation((p: string) => p.includes('cand2-cv.pdf')); + + await handlerDelete('cand2'); + + expect(fs.unlinkSync).toHaveBeenCalledWith(expect.stringContaining('cand2-cv.pdf')); + }); + + it('never calls unlinkSync for a CV file that does not exist on disk', async () => { + (MODELS.Candidate.findById as jest.Mock).mockResolvedValue({ _id: 'cand3' }); + (fs.existsSync as jest.Mock).mockReturnValue(false); + + await handlerDelete('cand3'); + + expect(fs.unlinkSync).not.toHaveBeenCalled(); + }); + + it('removes every project/certificate/award image file from disk, collected before those documents are deleted', async () => { + (MODELS.Candidate.findById as jest.Mock).mockResolvedValue({ _id: 'cand4' }); + (MODELS.Project.find as jest.Mock).mockResolvedValue([{ images: ['/uploads/images/proj1.png'] }]); + (MODELS.Certificate.find as jest.Mock).mockResolvedValue([{ images: ['/uploads/images/cert1.png', '/uploads/images/cert2.png'] }]); + (MODELS.Award.find as jest.Mock).mockResolvedValue([]); + (fs.existsSync as jest.Mock).mockReturnValue(true); + + await handlerDelete('cand4'); + + expect(fs.unlinkSync).toHaveBeenCalledWith(expect.stringContaining('proj1.png')); + expect(fs.unlinkSync).toHaveBeenCalledWith(expect.stringContaining('cert1.png')); + expect(fs.unlinkSync).toHaveBeenCalledWith(expect.stringContaining('cert2.png')); + // Only the 3 real image-bearing models are queried for images, not + // every CV section model. + expect(MODELS.Project.find).toHaveBeenCalledWith({ candidateId: 'cand4' }, { images: 1 }); + expect(MODELS.Certificate.find).toHaveBeenCalledWith({ candidateId: 'cand4' }, { images: 1 }); + expect(MODELS.Award.find).toHaveBeenCalledWith({ candidateId: 'cand4' }, { images: 1 }); + }); +});