fix: public payment PII leak, internal-wallet auth, broken groups & admin guards - #268
Merged
martinzhames merged 1 commit intoSep 1, 2026
Conversation
…dmin guard - dupdab#200: add PublicPaymentViewDto and return it from GET /pay/:reference so the unauthenticated public payment page no longer exposes customerEmail, customerWalletAddress, merchant metadata, merchantId or settlementId. - dupdab#201: protect InternalWalletController with a new InternalServiceGuard that requires a constant-time-checked x-internal-service-secret shared secret and fails closed when INTERNAL_SERVICE_SECRET is unset. - dupdab#202: GroupsController now reads req.user.merchantId (the shape JwtStrategy actually produces) instead of the always-undefined req.user.id, plus a controller spec covering every mutating handler. - dupdab#203: AdminGuard now checks user.role against MerchantRole.ADMIN/SUPERADMIN instead of the never-populated user.isAdmin flag, plus a guard spec. closes dupdab#200 closes dupdab#201 closes dupdab#202 closes dupdab#203
|
@dev-fani Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
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.
Summary
Fixes four issues — two security, two correctness — across payments, blockchain-wallet, groups and auth.
GET /pay/:referenceno longer returns the rawPaymententityInternalWalletControlleris now actually internal-only, not internal-by-conventionundefinedas the user idAdminGuardnow recognises real admins instead of 403-ing everyone#200 — Public
GET /pay/:referenceexposed full payment records incl. PIIThe public (deliberately unauthenticated) payment page called
findByReference()and serialized the rawPaymententity, leakingcustomerEmail,customerWalletAddress, merchant-suppliedmetadata, and internal fields (merchantId,settlementId, fee/settlement figures, tx hashes) to anyone holding a shareable payment reference.PublicPaymentViewDto(src/payments/dto/public-payment-view.dto.ts) exposing only what a payment page needs to render: reference, amounts, currency, network, status, deposit address/memo, description, QR, expiry.PublicPaymentController.getByReference()now maps throughPublicPaymentViewDto.from(payment)instead of returning the entity.#201 —
InternalWalletControllerhad zero auth guardsPOST /internal/wallet/provisionhad no@UseGuards(...); its "internal only" status was a comment, not enforcement. Anyone reaching the ingress could provision/probe Stellar wallets for arbitraryuserIds.InternalServiceGuard(src/auth/guards/internal-service.guard.ts): requires anx-internal-service-secretheader matchingINTERNAL_SERVICE_SECRET, compared in constant time (SHA-256 +timingSafeEqual). Fails closed — if the secret is not configured the endpoint returns 401 rather than being open.InternalWalletControlleris annotated with@UseGuards(InternalServiceGuard)and documents the required header in Swagger.INTERNAL_SERVICE_SECRETadded to.env.exampleand to the Joi config schema (min(16), required).#202 —
JwtStrategynever sets.id, butGroupsControllerreadreq.user.idJwtStrategy.validate()and the API-key branch ofJwtAuthGuardproduce{ merchantId, email, role }— neverid. Every mutating handler inGroupsControllerreadreq.user.id, soundefinedflowed intoGroupMember.userId(a non-nullable uuid column) and every create/join/update/gate request failed at the DB layer.req.user.idreferences inGroupsControllerchanged toreq.user.merchantId.src/groups/groups.controller.spec.tsasserts each mutating handler (create,update,remove,regenerateInviteCode,join,setGate,removeGate) forwards the realmerchantIdand neverundefined.#203 —
AdminGuardcheckeduser.isAdmin, a field that is never setThe
Merchantentity only hasrole: MerchantRole(ADMIN/MERCHANT/SUPERADMIN); nothing ever setsisAdminon the request user. EveryAdminGuard-protected route (incl.AdminMerchantsControllerbulk suspend/activate) was a permanent 403.AdminGuardnow allowsuser.role === MerchantRole.ADMIN || user.role === MerchantRole.SUPERADMINagainst the fieldsJwtStrategyactually populates.src/auth/guards/admin.guard.spec.tsasserts admin & superadmin pass, plain merchant / missing user / legacyisAdminflag are rejected.Testing
New unit/controller specs added for #201, #202 and #203; existing
test/blockchain-wallet.e2e-spec.tsupdated to stub the newInternalServiceGuard. Note:node_modulesis not installed in this environment, so the suite was not executed locally — CI should run it.closes #200
closes #201
closes #202
closes #203