fix(security): bind money-moving DTOs to authenticated caller identity

[davieslennox0]

Closes #40

Description

Fixes the class of vulnerabilities where money-moving DTOs trust client-supplied identity fields instead of binding to the authenticated caller (req.user).

Two rules now hold across the API:

1. Who benefits is never read from the body. The claimant and the funder come from the verified token.
2. A claimed pairing of identity and address must be proven. Where a request supplies both recipientId and recipientAddress, the service rejects the pair unless the user record agrees.

A note on scope: this needed guards to mean anything

None of the bounty/escrow/milestone/maintenance-pool routes were behind JwtAuthGuard when I started — the codebase says so explicitly in IdempotencyInterceptor's doc comment, and #40 acknowledges it ("once auth lands"). That makes AC 1 unreachable on its own: you cannot derive the contributor from the authenticated caller when there is no authenticated caller, and a funderAddress check against an undefined req.user is worse than no check because it looks like protection.

So JwtAuthGuard is added to exactly the five routes whose identity now comes from the caller, and no others:

• POST /bounties/:id/claim
• POST /bounties/:id/fund
• POST /escrow/fund
• POST /milestones/:id/fund
• POST /maintenance-pools/:id/deposit

Release, split-release, refund, milestone-resolve and pool-assign-reward are deliberately not guarded here — their fix is the service-layer cross-check below, which needs no caller identity, and blanket-guarding the mutating surface belongs to the companion auth issue.

One side effect worth knowing: IdempotencyInterceptor.resolveCallerId now returns a real userId on those five routes, so their idempotency keys are scoped per user instead of sharing the 'anonymous' bucket. Doc comment updated.

Changes

ClaimBountyDto — removed outright

The DTO file is deleted and POST /bounties/:id/claim takes no request body at all; the contributor is req.user.userId. An empty class would have been the smaller diff, but it leaves a slot that invites the field back — with no body parameter there is nothing to spoof, and reintroducing one becomes a visible change rather than a one-line addition.

The global ValidationPipe runs whitelist: true, forbidNonWhitelisted: false, so a client still sending {"contributorId": "<B's id>"} has it silently stripped rather than getting a 400. Per the issue's "pick one behavior and test it explicitly": the field is ignored and the claimant is forced to the caller. That's asserted directly.

No "claim on behalf of" path was added. If maintainer-side assignment is wanted it needs its own route and its own authorization check, as the issue suggests.

EscrowService — recipientId/recipientAddress cross-check

assertRecipientAddressMatchesUser() rejects a pair the user record disagrees with, and is called from release(), splitRelease() (per recipient) and releasePartial() before any Soroban invocation.

releasePartial() is not in the issue's list but carries the identical bug: it is what /milestones/:id/issues/:issueId/resolve and /maintenance-pools/:id/assign-reward reach, and both accept the same client-supplied pair.

Deliberate details:

• It lives in the service, not the controllers. This is the last common point every release path passes through, so no future route, job or webhook handler can reach the chain around it. The internal caller (BountiesService.markMergedAndRelease) already derives the address from the user record, so for it the check simply restates an invariant it already holds.
• A recipientId naming no user, or one with no linked address, is rejected too. Treating an unlinked (null) address as "matches anything" would reopen the hole.
• Missing user and genuine mismatch return the same message, so the endpoint doesn't become an oracle for which user ids exist.
• recipientId remains optional. With no attribution claimed there is no pairing to disprove, and the address stands on its own as before — no existing caller breaks.

funderAddress — validated, not documented away

All four are on endpoints where the caller is the party whose wallet is debited, so naming someone else's address is never a legitimate request. None was left as intentionally permissionless.

The brief expected req.user.stellarAddress; req.user is only { userId, username } (JwtStrategy.validate), and a token claim would be a stale snapshot anyway. The check is therefore a user-record lookup — UsersService.assertOwnsStellarAddress() → ForbiddenException — run in the controller before the service is called. A caller with no linked address cannot fund.

BountiesService — a latent empty-recipient release

markMergedAndRelease fell back to recipientAddress: '' when a payee had no linked wallet, releasing to nobody while still marching the bounty on to PAID. The new escrow check catches that, but would report it as a recipient mismatch; it now fails with user <id> has no linked Stellar address, naming the actual problem. This is a behaviour change on the merge-webhook path: such a merge now raises instead of attempting an empty release.

DTO Audit Table

Every DTO under src/ — note that FundBountyDto, FundMilestoneDto, DepositDto, AssignRewardDto, ResolveIssueDto and SetStellarAddressDto are declared inline in their controllers, not in *.dto.ts files, so a search restricted to dto/ misses six of them.

DTO	Field	Pattern	Resolution
ClaimBountyDto	contributorId	who benefits	Fixed — DTO deleted; claimant is req.user.userId
ReleaseEscrowDto	recipientId + recipientAddress	who benefits	Fixed — cross-checked in EscrowService.release()
SplitRecipientDto	recipientId + recipientAddress	who benefits	Fixed — cross-checked per entry in splitRelease()
ResolveIssueDto (inline, milestones)	recipientId + recipientAddress	who benefits	Fixed — cross-checked via releasePartial(). Not listed in the issue
AssignRewardDto (inline, pool)	recipientId + recipientAddress	who benefits	Fixed — cross-checked via releasePartial(). Not listed in the issue
FundEscrowDto	funderAddress	who funds	Fixed — guard + must equal caller's linked address
FundBountyDto (inline)	funderAddress	who funds	Fixed — guard + must equal caller's linked address
FundMilestoneDto (inline)	funderAddress	who funds	Fixed — guard + must equal caller's linked address
DepositDto (inline)	funderAddress	who funds	Fixed — guard + must equal caller's linked address
TeamMemberSplitDto	userId	who benefits	No change needed — the payout address is derived server-side from the user record (bounties.service.ts), never paired with a client address. Who may create a team naming arbitrary users is an authorization gap, not an identity-binding one → companion guards issue
SetStellarAddressDto	stellarAddress (+ :id path param)	who benefits	Out of scope — companion IDOR issue. See the dependency note below
CreateBountyDto	sponsorId	attribution (who pays)	Flagged, not changed — client-supplied, drives sponsor dashboard spend. Misattribution, not misdirected funds
CreateMilestoneDto	sponsorId	attribution (who pays)	Flagged, not changed — same
CreateTeamDto	createdById	attribution	Flagged, not changed — should come from req.user once these routes are guarded
CreatePoolDto	createdById	attribution	Flagged, not changed — same
CreateBountyDto	issueId	resource	N/A — names what is funded, not who benefits
CreateMilestoneDto	repositoryId	resource	N/A
CreatePoolDto	repositoryId	resource	N/A
FundEscrowDto	bountyId, milestoneId, maintenancePoolId	resource	N/A
PublicUserDto	—	response only	N/A — never client input

Dependency worth flagging

This fix is only as strong as the endpoint that links addresses to users. PATCH /users/:id/stellar-address still takes the target from the path param rather than req.user, so a caller who can rewrite another user's stellarAddress can make the recipient cross-check pass with an address they control. That is the companion setStellarAddress IDOR issue, and I've left it there rather than fixing another issue's acceptance criteria — but the two should land together to get the intended guarantee. The issue itself anticipates exactly this combination.

Tests Added

• A: user A cannot cause bounty.claimedById to be set to user B's ID — the claimant reaching the service is always the caller, and the handler's arity is asserted so a reintroduced body parameter fails the suite
• B: a mismatched recipientId/recipientAddress throws BadRequestException before any Soroban call, on release(), splitRelease() (one bad entry poisons the whole split) and releasePartial(); asserted via the public methods rather than the private helper, so deleting a call site fails the test
• C: a matching pair passes and reaches the chain, recording the Payment with both fields
• Plus: unattributed release (no recipientId) still works; unknown recipient rejected; recipient with a null address rejected; rejection message leaks neither address; funder binding enforced on the funding routes; assertOwnsStellarAddress accepts own / rejects other, unlinked, and vanished users

146/146 unit tests pass (was 118 passing + 7 failing before — the 7 are escrow-fk-integrity.integration.spec.ts, which needs a live Postgres and passes once one is provided). E2E is unchanged from main: app.e2e-spec.ts fails on both, as it boots the full AppModule and requires real GitHub OAuth credentials. tsc --noEmit, nest build and eslint are all clean — lint output is byte-identical to main's (2 pre-existing prettier errors in test/users.e2e-spec.ts, untouched to keep this diff focused).

Type of Change

• fix: bug fix
• security

Checklist

• My code follows the coding conventions of this project
• I have added/updated tests if needed
• My changes generate no new warnings or errors

[vercel]

@Kasbit is attempting to deploy a commit to the chonilius' projects Team on Vercel.

A member of the Team first needs to authorize it.