fix(users): bind stellar-address writes to the authenticated caller - #78
Open
rudra496 wants to merge 1 commit into
Open
fix(users): bind stellar-address writes to the authenticated caller#78rudra496 wants to merge 1 commit into
rudra496 wants to merge 1 commit into
Conversation
PATCH /users/:id/stellar-address was authenticated but not authorized: any valid JWT could overwrite any user's payout address, and the address is re-read at escrow release time, so the next merged bounty pays the attacker instead of the contributor. The controller now passes req.user.userId through to the service, which rejects cross-user writes with 403 before touching the repository — unless the caller carries the maintainer role, which covers the explicit support override. Unit tests prove the no-write guarantee and the maintainer path; e2e tests cover the route matrix (own id, cross-user, maintainer, no token).
|
@rudra496 is attempting to deploy a commit to the chonilius' projects Team on Vercel. A member of the Team first needs to authorize it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #39
What changed
PATCH /users/:id/stellar-addresswas authenticated but not authorized — the handler never injected@Req(), so nothing compared the caller's identity to the:idin the URL. SinceBountiesService.markMergedAndReleasere-readscontributor.stellarAddressfresh at payout time, any logged-in user could point a victim's next merged bounty at their own wallet.UsersController.setStellarAddressnow passesreq.user.userId(the identityJwtStrategy.validateattaches) through to the service.UsersService.setStellarAddresstakes the caller id and rejects cross-user writes withForbiddenExceptionbefore any repository write — the acceptance criterion's no-write-before-rejection guarantee. A caller carryingUserRole.MAINTAINERis the one explicit override, using the roles already persisted on the entity; no new role infrastructure.findOne, not two).Tests
users.service.spec.ts(new): owner sets own ✓; contributor targeting another user →ForbiddenException,savenever called, victim's stored address untouched ✓; maintainer override allowed ✓; same-id skips caller lookup ✓.users.e2e-spec.ts: route matrix — unauthenticated rejected, own-id call passes caller through, cross-user forwards the mismatch and returns 403, maintainer path returns 200.npx jest122 passing with the same 7 escrow DB-integration failures present onmain(verified by baseline run — they need a live database, unrelated to this change). Users suites: 4/4 unit, 6/6 e2e.tsc --noEmit+nest build+ eslint clean on changed files.Sibling-route audit (per the issue's third requirement)
The same "URL
:id+ mutation + no@Req()" shape exists on every mutating route in the codebase —bounties(fund/claim/refund),escrow(release/split-release/refund),maintenance-pool(deposit/assign-reward),milestones(fund/add-issue/resolve),teams(assign),reputation(recompute),github(sync). Most of those routes are also unauthenticated today (#38) and several trust client-supplied ids in the body (#40), so I've deliberately not touched them here — the fix for those needs the auth-guard rollout plus per-route ownership decisions, and doing it as one blanket change would re-create exactly this bug class. Once guards land, each of those routes should get the same caller-binding treatment this PR applies to the payout-address route.