[jay23606]

Summary

Participant create, read, edit, move, and retry service results remove both editTokenHash and idempotencyKeyHash before routes receive them. The public row type derives from the removal helper's return type so the list of hidden fields is maintained in one place.

Response assertions cover GET, both PATCH paths, and initial/retried POST responses. The changelog describes all participant paths and both hidden check values. Updated against main. Resolves #394.

Validation

• pnpm typecheck, pnpm lint, pnpm knip, and Prettier on the changed files: passed.
• pnpm test src/lib/public-commitment.test.ts: 2 passed; verifies both properties are absent and the original row is unchanged.
• Full pnpm test: 1,142 passed, 4 failed. Three failures (missing values in .env.example, Windows spawn pnpm ENOENT, and a CRLF-sensitive golden prompt) reproduce on the unchanged pre-follow-up branch. The fourth, a bearer-verifier timeout, passed when rechecked; the current and unchanged branches both have 97 passed / 3 failed in the focused failure recheck.
• Database route tests: 2 files / 11 tests passed against a disposable local Postgres 18 instance after applying migrations. This exercised initial/retried POST responses, GET, normal PATCH, and move PATCH, including the new both-hash assertions. The test instance was stopped afterward.

Local dependency installation used pnpm's hoisted layout to avoid a Windows package-resolution failure; package manifests and the lockfile are unchanged.

[richshaw]

Hi @jay23606, thanks for picking this up! Keeping the edit-token hash out of participant responses is a good fix. At your commit it does what it says: all four responses drop editTokenHash, and lint, typecheck, knip and pnpm test all pass for me (1040 of 1040, so the 4 failures in the description look like sandbox noise).

The catch is that main has moved on since you branched. The PATCH handler in src/app/api/commitments/[id]/route.ts was rewritten (it can now move a sign-up to another slot), so this PR has a conflict. Once it's merged into current main, the hash comes back on both PATCH paths, and no test notices. Details below.

Needs fixing before merge

1. Saving an edit sends the hash again after the rebase. Your change wraps return respond(result), but that line no longer exists on main. Main's version returns early with if (!moved) return ok(commitment);, and that line sits outside the conflict, so it merges in untouched. So a normal save from the edit page (name, notes, quantity) returns the full row, hash included. I reproduced this against a real Postgres. A simple fix while rebasing:

const { moved, editTokenHash: _h, ...commitment } = result.value;

2. Moving to another slot sends the hash too. Main's move branch returns ok({ ...commitment, editToken: moved.editToken, editUrl }, { links }), which spreads the new row, hash and all. Keeping main's side of this conflict is the right call: your side no longer compiles because editTokenFor was removed, and it would break main's move test. The destructure above fixes this branch as well.

3. Nothing tests the actual responses. Issue #394 asked for a test that no response includes editTokenHash. You flagged that it's missing, and that gap is exactly how 1 and 2 slip through: on the merged code every test still passes while PATCH sends the hash. Adding

expect(payload.data).not.toHaveProperty('editTokenHash');

to the GET, PATCH and move tests in src/app/api/commitments/[id]/route.db.test.ts, and the same check on payload.data.commitment in the POST test in src/app/api/slots/[id]/commitments/route.db.test.ts (next to the existing not.toHaveProperty('slotAt')), would catch both. Without it, "Resolves #394" would close the issue with that part not done.

Worth considering

4. Remove the hash in the service, not in each route. Only verifyHash inside src/services/commitments.ts ever reads editTokenHash. But getOwnCommitment, getOwnCommitmentsForSignup, commitToSlot and updateOwnCommitment all return it, so every route has to remember to remove it, and the compiler can't catch one that forgets (which is what happened with the two new PATCH paths). If those four functions dropped it and returned Omit<CommitmentRow, 'editTokenHash'>, it typechecks on main with no route changes and fixes both PATCH leaks on its own, so the destructure in 1 isn't needed. publicPart in src/services/reminder-optout.ts already does this for the opt-out token.

5. An allow-list might be safer than removing one key. withoutEditTokenHash removes just the hash, so everything else on the row still goes to anyone holding the edit link: workspace id, participant id, position, payment id, custom field values and so on. The app itself only reads id and editUrl. Issue #394 asked us to decide what the public shape should be. With an allow-list, any column added to commitments later wouldn't go out by default. Fine as a follow-up if you'd rather keep this PR small; if you do it here, the changelog line "Clients keep the same fields otherwise" would need updating.

6. The POST response depends on key order. It spreads ...responseValue, which still holds the full commitment, and then overwrites commitment: with the cleaned one. That works today, but if someone reorders those lines the hash comes back and no test would notice. Taking it out first, with const { signupSlug, commitment, ...rest } = result.value, gives the same output with nothing left to leak.

Small things

7. The unit tests can't tell "removed" from "set to undefined". toEqual and JSON.stringify(...).not.toContain(...) both still pass if the helper returned { ...row, editTokenHash: undefined }. That wouldn't leak anything (JSON drops undefined), but not.toHaveProperty('editTokenHash') or toStrictEqual would pin the behaviour down.

8. publicCommitmentResult may not need to exist. It builds { ok: true, value } by hand, where the rest of the code does if (!result.ok) return fail(result.error); return ok(shape(result.value));. After the rebase GET is its only caller, so GET could do that inline and the helper and two of its tests could go.

9. Database tests. CLAUDE.md asks for pnpm test:db when changing POST /api/slots/{id}/commitments. Your change doesn't touch the capacity logic that rule is mostly about, so this one is minor. But the merged PATCH handler is where the leaks are, and only the database tests exercise it. CI hasn't run on this PR yet because it comes from a fork and needs a maintainer to approve the workflow, so they'll run then.

Thanks again, this is a really useful fix. With the rebase and a few response tests it should be good to go.

---

Generated by Claude Code

[richshaw]

Hi @jay23606, thanks for the quick turnaround on the last round! Moving the clean-up into the service is a real improvement. The routes can't forget it any more, and the response checks on GET, PATCH and move, plus the toStrictEqual unit test, are just what we wanted.

One thing is left before this can merge, plus a few small ones. Details are in the inline comments.

Needs fixing before merge

1. There's now a second hash on the same rows. #396 landed while this PR was open and added idempotencyKeyHash. It's the same kind of server-only value as editTokenHash, and it still goes out on sign-up, view and edit. Dropping it alongside editTokenHash fixes it.

2. A retried sign-up's response isn't checked. The repeat path (same idempotency key) uses the helper too, but no test looks at its response. It's also the best place to check idempotencyKeyHash, since that's the one response where it's always set.

Worth considering

3. An allow-list would have caught this for free. Last time I said a list of allowed fields could wait for a follow-up. A second hash showing up within a day is a good example of why it's worth doing: building the public shape from the fields we want to send means new columns stay private by default. It would also let the queries skip fetching the hashes at all (.returning({...}) / .select({...})). Still fine as a follow-up if you'd rather keep this PR small. In that case, removing both hashes is enough for now.

Small things

• The CHANGELOG.md entry needs a wording tweak (suggestion inline).
• The new POST check sits inside an unrelated cookie test, so a failure there would be hard to spot.

CI hasn't run on this PR yet because it comes from a fork. A maintainer needs to approve the run, and the database tests will run then.

Thanks again, this is really close!

---

Generated by Claude Code

[jay23606]

Hi @jay23606, thanks for picking this up! Keeping the edit-token hash out of participant responses is a good fix. At your commit it does what it says: all four responses drop editTokenHash, and lint, typecheck, knip and pnpm test all pass for me (1040 of 1040, so the 4 failures in the description look like sandbox noise).

The catch is that main has moved on since you branched. The PATCH handler in src/app/api/commitments/[id]/route.ts was rewritten (it can now move a sign-up to another slot), so this PR has a conflict. Once it's merged into current main, the hash comes back on both PATCH paths, and no test notices. Details below.

Needs fixing before merge

1. Saving an edit sends the hash again after the rebase. Your change wraps return respond(result), but that line no longer exists on main. Main's version returns early with if (!moved) return ok(commitment);, and that line sits outside the conflict, so it merges in untouched. So a normal save from the edit page (name, notes, quantity) returns the full row, hash included. I reproduced this against a real Postgres. A simple fix while rebasing:

const { moved, editTokenHash: _h, ...commitment } = result.value;

2. Moving to another slot sends the hash too. Main's move branch returns ok({ ...commitment, editToken: moved.editToken, editUrl }, { links }), which spreads the new row, hash and all. Keeping main's side of this conflict is the right call: your side no longer compiles because editTokenFor was removed, and it would break main's move test. The destructure above fixes this branch as well.

3. Nothing tests the actual responses. Issue #394 asked for a test that no response includes editTokenHash. You flagged that it's missing, and that gap is exactly how 1 and 2 slip through: on the merged code every test still passes while PATCH sends the hash. Adding

expect(payload.data).not.toHaveProperty('editTokenHash');

to the GET, PATCH and move tests in src/app/api/commitments/[id]/route.db.test.ts, and the same check on payload.data.commitment in the POST test in src/app/api/slots/[id]/commitments/route.db.test.ts (next to the existing not.toHaveProperty('slotAt')), would catch both. Without it, "Resolves #394" would close the issue with that part not done.

Worth considering

4. Remove the hash in the service, not in each route. Only verifyHash inside src/services/commitments.ts ever reads editTokenHash. But getOwnCommitment, getOwnCommitmentsForSignup, commitToSlot and updateOwnCommitment all return it, so every route has to remember to remove it, and the compiler can't catch one that forgets (which is what happened with the two new PATCH paths). If those four functions dropped it and returned Omit<CommitmentRow, 'editTokenHash'>, it typechecks on main with no route changes and fixes both PATCH leaks on its own, so the destructure in 1 isn't needed. publicPart in src/services/reminder-optout.ts already does this for the opt-out token.

5. An allow-list might be safer than removing one key. withoutEditTokenHash removes just the hash, so everything else on the row still goes to anyone holding the edit link: workspace id, participant id, position, payment id, custom field values and so on. The app itself only reads id and editUrl. Issue #394 asked us to decide what the public shape should be. With an allow-list, any column added to commitments later wouldn't go out by default. Fine as a follow-up if you'd rather keep this PR small; if you do it here, the changelog line "Clients keep the same fields otherwise" would need updating.

6. The POST response depends on key order. It spreads ...responseValue, which still holds the full commitment, and then overwrites commitment: with the cleaned one. That works today, but if someone reorders those lines the hash comes back and no test would notice. Taking it out first, with const { signupSlug, commitment, ...rest } = result.value, gives the same output with nothing left to leak.

Small things

7. The unit tests can't tell "removed" from "set to undefined". toEqual and JSON.stringify(...).not.toContain(...) both still pass if the helper returned { ...row, editTokenHash: undefined }. That wouldn't leak anything (JSON drops undefined), but not.toHaveProperty('editTokenHash') or toStrictEqual would pin the behaviour down.

8. publicCommitmentResult may not need to exist. It builds { ok: true, value } by hand, where the rest of the code does if (!result.ok) return fail(result.error); return ok(shape(result.value));. After the rebase GET is its only caller, so GET could do that inline and the helper and two of its tests could go.

9. Database tests. CLAUDE.md asks for pnpm test:db when changing POST /api/slots/{id}/commitments. Your change doesn't touch the capacity logic that rule is mostly about, so this one is minor. But the merged PATCH handler is where the leaks are, and only the database tests exercise it. CI hasn't run on this PR yet because it comes from a fork and needs a maintainer to approve the workflow, so they'll run then.

Thanks again, this is a really useful fix. With the rebase and a few response tests it should be good to go.

Generated by Claude Code

added a new commit

[richshaw]

Thank you, running CI now

[richshaw]

This one just needs to conflicts resolved and then I can approve merge