fix(history): require auth for transaction list and redact public single DTO#4166
fix(history): require auth for transaction list and redact public single DTO#4166TaprootFreak wants to merge 3 commits into
Conversation
…gle DTO List and export endpoints no longer accept wallet address as sole credential. Callers must use JWT or CoinTracking API-key headers; userAddress is an optional ownership-scoped filter. Unauthenticated single-tx lookups return a reduced DTO without bank/chargeback targets, fee breakdowns, or external ids so status links keep working without leaking private financial data.
|
Companion packages PR: DFXswiss/packages#189 |
Ready-for-review process2 review passes until 0 open findings. Pass 1 fixed: unused CI was already green on the first push; re-running after the follow-up commit. |
Status update (ready, not merged)
Requested reviewers: CODEOWNERS. |
Frontend dependency follow-upServices consumer bump is open and ready: DFXswiss/services#1175 ( Together with packages#189/#190 this completes the in-repo frontend dependency chain for the history-auth change (still pending API merge for the server-side gate). |
Gate the input and output tx identifiers on their payment method: a fiat leg reuses the same DTO fields for a private bank reference (outputTxId carries bankTx.remittanceInfo on a sell), so an unauthenticated status link handed out the remittance reference of the payout. Strip the chargeback identifiers unconditionally. The hash resolves to the chargeback target on a block explorer, and a chargeback may go to an address other than the input sender, so it can disclose an address no other public field does. Add PublicTransactionReasonMap, an exhaustively typed fail-closed allow list, and only disclose an AML reason it classifies as operational. Hiding the reason alone is not enough: KYC_REQUIRED is produced by nothing but private reasons, so the state discloses the reason on its own - mask it to the neutral sibling the same AML branch returns for any other pending check. Restore instanceof UserData in isAccountSubject. Reading the address getter dereferences the organization relation and throws for organization accounts that do not have it loaded. Drop the silent `?? []` fallback so a missing users relation fails loud instead of quietly dropping the staking history from an export.
|
Follow-up commit What was still leaking1. 2. Hiding the AML reason was ineffective, because the state gave it away. 3. Chargeback identifiers are stripped unconditionally. Exposing them was tempting (for sells the hash is on-chain), but it would re-expose the Also fixed
Notes for reviewers
|
1 participant
Summary
GET /transaction, CoinTracking/ChainReport export, andPUT /transaction/csvno longer accept walletuserAddressas the only credential. Callers must authenticate with Bearer JWT or DFX-ACCESS-KEY / SIGN / TIMESTAMP.userAddressis an ownership-scoped filter on the authenticated account (403 if the address is not owned).GET /transaction/singlestays reachable without login for status links, but returns a public DTO (no chargeback IBAN/target, fee breakdown, deposit address, external ids). Owners and staff (SUPPORT hierarchy / REALUNIT) still receive the full compact DTO.HistoryAccessServicecentralizes list auth; deprecated unauthenticated address loaders always fail closed.Consumer sync
DFXswiss/packages@dfx.swiss/react@1.5.1omitsuserAddressfor account sessionsDFXswiss/services@dfx.swiss/react@1.5.1); no app code change (list already uses/transaction/detail+ JWT)DFX-ACCESS-*headers (already supported on/history/:exportType) or JWTTest plan
HistoryAccessService(JWT, API key, ownership, staff)toPublicDtoredactionisAccountSubjectwith realUserDatainstances (postal-address getter ≠ wallet string)/tx/T…without login shows status without IBAN/feesGET /transaction?userAddress=…→ 401Security notes
UserData.addresspostal getter as a wallet string (regression covered by tests).