feat(bank): add a receive-IBAN check for the support form#4391
Open
TaprootFreak wants to merge 9 commits into
Open
feat(bank): add a receive-IBAN check for the support form#4391TaprootFreak wants to merge 9 commits into
TaprootFreak wants to merge 9 commits into
Conversation
The support form's "Receiver IBAN" field is a required dropdown filled from GET /bank, which lists only the shared bank accounts. Customers who deposit through a personal IBAN (virtual_iban) can never find their own receiving IBAN there, so today they are forced to pick a shared account they never transferred to - the ticket then carries a wrong IBAN that reads like a statement from the customer. The field is being replaced by free text, and this endpoint is what lets the frontend tell the customer straight away whether the IBAN they typed is one DFX receives money on. PUT /bank/receive-iban takes the IBAN in the body - never in the URL, so it stays out of access logs - and answers with one of four states: DfxIban, InvalidIban, UnknownIban, or LoginRequired. Two filters are deliberately absent. The bank lookup ignores `receive`, because a missing transfer is by nature an old one and a hit on a retired account is still money that reached DFX; filtering would tell a real customer their IBAN does not belong to DFX. The personal-IBAN lookup ignores the lifecycle state for the same reason - an expired or deactivated personal IBAN was still a real receiving IBAN. Personal IBANs are matched only against the requesting account. The guard is optional, so a global lookup would turn this into an unauthenticated oracle over customer-bound IBANs. That is why LoginRequired exists as its own state: without a login the personal IBANs stay unchecked, and answering UnknownIban there would be a false statement to a customer whose IBAN does exist. The endpoint is an input aid only - it enforces nothing, and issue creation still accepts any free text. Comparison runs on the normalised electronic format, since both the stored values and customer input carry arbitrary grouping spaces and casing. BankService takes the VirtualIbanRepository rather than the VirtualIbanService, because that service already depends on BankService and both live in the same module.
…ve-IBAN check Review turned up four real defects in the first commit. Normalization stripped only whitespace, so a customer pasting an IBAN with hyphen grouping got InvalidIban for a perfectly valid IBAN. ibantools ships electronicFormatIBAN, which strips both spaces and hyphens and uppercases, so the hand-rolled helper is gone in favour of the library. It also returns null for a non-string, which is now treated like any other unusable input - and it makes the stored side null-safe, where the old regex would have thrown. The DTO no longer carries @Transform(Util.sanitize). That transform runs before @IsString, and Util.sanitizeString calls value.trim() unguarded, so a body such as {"iban": 123} threw a TypeError that the exception filter turned into a 500 - on an endpoint that is reachable without a login. HTML sanitizing is pointless for an IBAN that is structurally validated and normalized anyway. @IsString now rejects a non-string with a clean 400. A comment records why the transform must not come back. UnknownIban was renamed to NotMatched, because the old name asserted more than the check knows. For an authenticated caller the state means "could not attribute this", not "does not belong to DFX": personal IBANs of other accounts are deliberately never checked, and mergeUserData does not move virtual_iban rows to the master, so a customer's own older personal IBAN can land there too. Every state is now documented in the enum, including that NotMatched makes no claim about DFX ownership. The endpoint is reachable unauthenticated, so it now runs behind RateLimitGuard, placed first as in the existing public endpoints. Two tests were added: hyphen-grouped input resolves to DfxIban, and unusable input yields InvalidIban instead of throwing.
Switching to electronicFormatIBAN in the previous commit traded one gap for another. The library strips only ASCII spaces and hyphens; the hand-rolled helper it replaced stripped every kind of whitespace but no hyphens. Measured against ibantools 4.5.1 with a valid IBAN in different groupings, the library alone rejects a non-breaking space, a narrow non-breaking space, a tab and a line break, while accepting ASCII spaces and hyphens. That is not an edge case for this endpoint. An IBAN pasted out of a PDF statement or off a web page very often carries a non-breaking space as its grouping character, and the whole point of the free-text field is that customers paste what they have. Such a customer would have been told their perfectly valid IBAN is invalid. The normalization helper is back, now doing both: it strips all whitespace itself and then hands the value to electronicFormatIBAN, which removes hyphens, uppercases, and returns null for anything unusable. A typeof guard keeps that null-safety for stored values as well. All three comparison sites - the input, bank.iban and virtualIban.iban - run through it. The comment that implied the library normalized completely is corrected. Four cases were added covering non-breaking space, narrow non-breaking space, tab and line break, for a collective account and for a personal IBAN. The separators are written as escape sequences so no invisible character can be lost while editing. The guard was checked by reverting the helper to the library-only form: exactly those four cases fail, and pass again once it is restored.
…nches Three safeguards of the receive-IBAN check turned out to be unpinned: inverting them left every test green. The branch order that answers a logged-out customer with DfxIban on a collective account - the single most common path through this endpoint - could be swapped for LoginRequired unnoticed. Checksum validation could be reduced to a shape check, which would answer a customer who mistyped a digit with NotMatched, telling them their IBAN is not ours instead of that it has a typo. And normalization could be dropped from the stored side of either comparison, which matters for personal IBANs because those values are persisted straight from the provider response without validation. One test each now pins these, and each mutation is caught by exactly its own test. Normalization changes approach rather than gaining another character. Stripping whitespace missed the zero-width family (U+200B, U+200D, U+2060, the direction marks) and the soft hyphen, none of which JavaScript counts as whitespace, plus dots, slashes and quotes. That was the third defect in the same function, so the deny-list of separators is replaced by an allow-list of what an IBAN may contain: letters and digits, nothing else. That is complete by construction. An IBAN: prefix stays invalid, which is correct - it is not a separator problem. Three comments overstated what they knew and are corrected. The normalization comment no longer promises to absorb every pasted form. The rate-limit comment no longer promises protection: ThrottlerModule.forRoot() is called without options, so limit is undefined and the guard's comparison is always false - the guard is inert until that is fixed separately, and the comment now only explains the ordering. The DTO comment now names the actual cause of the 500 it avoids, an unguarded value.trim() in Util.sanitizeString, rather than arguing that HTML sanitizing is pointless for an IBAN. Also moved the section header so it no longer encloses the unrelated isBankMatching, and made the constructor uniform by adding the missing readonly.
Mutation testing found three safeguards that could be removed without a single test noticing. Deleting @UseGuards entirely, or swapping OptionalJwtAuthGuard for a hard AuthGuard, left all tests green. Both are severe: without the optional guard req.user is never populated and every authenticated customer is told LoginRequired, while a hard guard turns anonymous callers away with a 401. The controller spec now asserts the route metadata the way ledger.controller.spec.ts does - path, method, and the guard list including its order - so both failure modes are caught. Removing the DTO validators, or "harmonizing" them with the @Transform(Util.trimAll) that every other IBAN DTO in the project carries, also went unnoticed. The second one is the realistic edit, and it would turn the deliberate 400 back into a 500 on a route reachable without a login. The DTO is now driven through a ValidationPipe built from the exact options in main.ts, asserting a BadRequestException for a number, an array, an object, null, undefined and an empty string, plus a positive case proving a plain string arrives unchanged - which is what catches a transform being added. The throttle is raised from 10 to 60 per minute. RateLimitGuard buckets IPv4 callers by /24, so a whole company network shares one counter, and unlike the one-shot precedents it was copied from - 2FA verification, mail login - an IBAN field gets re-checked while a customer corrects a typo. Ten would have meant two colleagues in one office locking each other out of the form they opened because something already went wrong. Two comments claimed more than the code delivers. The normalization is complete only for ASCII: non-ASCII letters and digits are stripped as separators too, so a label in a non-Latin script can be dropped where an ASCII one is not. That cannot produce a different valid IBAN, but the comment and the test name now say what actually holds. And DfxIban is phrased as belonging rather than as an invitation to pay in, because most matching rows are retired accounts.
…ents CONTRIBUTING documents camelCase for URL routes, and the closest sibling for this concept - GET /buy/personalIban - follows it, as do the two read-with-a-body precedents this endpoint was modelled on. The hyphenated route was an unjustified deviation, and renaming it is free right now: the endpoint is unreleased and has no consumer in production. Once the client library ships the call, the same rename would break everyone using it. Only the route literal and the two metadata assertions change; filenames, the enum, the DTO and the method names stay. Two comments claimed more than had been checked. The first said every other IBAN DTO in the project carries @Transform(Util.trimAll); three admin DTOs do not - update-bank-tx and create/update-fiat-output. Narrowed to customer-facing IBAN input DTOs, which is both true and the sharper form of the argument, since those are what someone would align against. The second said the bank table holds grouped IBAN values. Production stores all eighteen rows compact and uppercase; the only grouped value is a test fixture. What actually holds is the invariant behind it: no service writes bank.iban - rows arrive through migrations or by hand - and nothing normalizes the column on write, so a row can carry a grouped value at any time. The test that covers it was right and is unchanged.
…nsus Three attempts at one comment, each narrowing a claim about what every other IBAN DTO carries, and each still wrong - the last counterexample being create-support-issue, which is customer-facing, login-optional and uses Util.sanitize rather than trimAll. The lesson is not a fourth narrowing. Any claim of the form "every other DTO does X" is either already false or becomes false with the next DTO, and it was never load-bearing: the argument works from mechanism alone. The Util helpers call string methods on the raw value, @Transform runs before @IsString, and a non-string body therefore becomes a TypeError that the exception filter turns into a 500 on a route reachable without a login. That transforms exist on other IBAN fields is enough to explain why adding one here would look like tidying up; how many and which is irrelevant. I swept the remaining comments in the diff for the same shape. Everything else states either this code's own behaviour or a fact measured directly, so no claim now depends on an inventory that can drift. Also names the seed CSV as a third way rows reach the bank table, alongside migrations and manual inserts.
…tory A comment may describe this code, or name a location a reader can open. It must not quantify over an unnamed set of other files or over production data, because such a claim goes stale invisibly. Three statements still did, and the pattern across the previous rounds was to weaken the quantifier rather than remove the dependency. Removed: that transforms sit on other IBAN fields, which was the fourth variant of the same sentence and carried nothing the mechanism argument had not already established; that no service writes bank.iban, a universal negative over present and future services whose load-bearing half was only ever the column; and that the guard order matches the existing public endpoints. The 60/60 rationale stays, because it names the two endpoints it compares against. The enum lost "most bank rows are retired" for the same reason - it quantifies over the contents of the production table and cannot be checked from the repository. The warning it carried is now grounded in the method instead: the check ignores the receive flag and every lifecycle state, so a long-closed account matches just as well. The merge caveat stays, rewritten to name mergeUserData, since a named function is re-checkable in one grep and the caveat is one of the two reasons NotMatched must not be read as "not a DFX IBAN". The audit also caught the method summary still describing the endpoint in the present tense as reporting an IBAN DFX receives money on, the same framing corrected in the enum a round earlier, and one stale route spelling in a test comment.
…aths The previous commit removed one half of this sentence and kept the other, which was the same shape with the verb swapped: "nothing normalizes bank.iban on write" is a universal negative over an unnamed set of write paths, and it goes stale the moment one appears. It was also never the point of the comment - what the test demonstrates is that the comparison normalizes the stored side, which is a statement about this code and needs no inventory at all.
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
1 participant
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
The support form's "Receiver IBAN" field (reason
TRANSACTION_MISSING) is a required dropdown filled fromGET /bank, which lists only the shared bank accounts. Customers who deposit through a personal IBAN (virtual_iban) can never find their own receiving IBAN in that list, so today they are forced to pick a shared account they never transferred to. The ticket then carries a wrong IBAN that reads like a statement from the customer — in exactly the situation where someone needs help.The field is being replaced by free text. This PR adds the endpoint that lets the frontend tell the customer straight away whether the IBAN they typed is one DFX receives money on.
Changes
PUT /bank/receiveIban— the IBAN goes in the body, never in the URL, so it stays out of access logs. Verb follows the existing "read with a body" precedent (PUT /sell/quote,PUT /buy/paymentInfos). Guards:RateLimitGuard, thenOptionalJwtAuthGuard.Four response states:
DfxIbanInvalidIbanNotMatchedLoginRequiredNotMatcheddeliberately does not claim the IBAN is not a DFX IBAN. Personal IBANs of other accounts are never checked, andmergeUserDatadoes not movevirtual_ibanrows to the master account, so a customer's own older personal IBAN can land there too. Every state carries this in a comment on the enum, because the frontend wording depends on it.BankService.getReceiveIbanStatus(iban, userDataId?)— normalizes, validates the structure, then matches thebankrows and, for a logged-in caller, that account'svirtual_ibanrows.Normalization works by allow-list: an IBAN is letters and digits, so everything else is separator noise and is stripped. This covers grouping spaces of every kind, hyphens, dots, slashes, quotes and the invisible formatting characters (zero-width space, soft hyphen, direction marks) that come along when a value is pasted out of a statement PDF or an HTML mail. A deny-list of separators was tried first and proved incomplete twice —
ibantools' ownelectronicFormatIBANremoves only ASCII spaces and hyphens, and a\ssweep misses the zero-width family. The allow-list is complete for ASCII noise; non-ASCII letters and digits are stripped as separators too, so a label in a non-Latin script is dropped where an ASCII one is not. That can never produce a different valid IBAN, and the comment and test name say so rather than claiming completeness. AnIBAN:prefix stays invalid, which is correct; that is not a separator problem.Two filters are deliberately absent. The bank lookup ignores
receive: a missing transfer is by nature an old one, and a hit on a retired account is still money that reached DFX — filtering would tell a real customer their IBAN does not belong to DFX. The personal-IBAN lookup ignores the lifecycle state (active=false,Expired/Deactivated/Reserved) for the same reason.Personal IBANs are matched only against the requesting account. The guard is optional, so a global lookup would turn this into an unauthenticated oracle over customer-bound IBANs. That is why
LoginRequiredis its own state rather than falling through toNotMatched.BankServicetakes theVirtualIbanRepository, not theVirtualIbanService— that service already depends onBankServiceand both live inBankModule, so the service-level dependency would close a provider cycle and need a forward ref. No module change was needed; the repository is already a provider there.Rate limiting
The endpoint carries
@Throttle(60, 60)alongsideRateLimitGuard, so it is effectively limited today — the route-level decorator sets the value regardless of the module defaults.Sixty per minute rather than the ten used by the neighbouring one-shot endpoints (2FA verification, mail login), because
RateLimitGuard.getTrackerbuckets IPv4 callers by /24: a whole company network shares one counter, and an IBAN field is re-checked while a customer corrects a typo. Ten would have let two colleagues in one office lock each other out of the very form they opened because something already went wrong.A consumer must still treat HTTP 429 as "could not check" rather than as a failure — there is deliberately no enum state for it, and showing it as an error would tell a customer their IBAN is wrong when it was never examined.
Why a server round-trip rather than a client-side comparison
A client could in principle assemble the answer itself:
GET /bankis public, andGET /v1/buy/personalIbanreturns the caller's own personal IBANs. That was considered and rejected.The value of this endpoint is not the data, it is the normalization rule. Getting that rule right took three attempts here —
electronicFormatIBANalone misses non-ASCII whitespace, a\ssweep misses the zero-width family and the soft hyphen, and only an allow-list of ASCII alphanumerics is complete for the separators customers actually paste. Duplicating that into the frontend would mean maintaining it in two places and drifting; the project already carries three separate definitions of "is this a DFX IBAN" (is-dfx-iban.validator.ts,isBankMatching, and the rawGET /banklist). Putting the comparison behind one endpoint keeps the rule in one place.Placing
getReceiveIbanStatusonVirtualIbanServiceinstead was also considered — that service already depends onBankServiceand would need no new constructor dependency. It was left onBankServicebecause the question asked is a bank-registry question and the collective-account lookup dominates the logic; the personal-IBAN lookup is the narrower half.Scope
Deliberately not in this PR:
POST /support-issuestill accepts any free text, so any client can skip the check. Making the status available to support needs a newSupportIssueModule → BankModuleedge, so it is deliberately left out of a UX change and not addressed here.is-dfx-iban.validator.tsis untouched. It answers a related question for the opposite purpose and currently checks only the shared accounts, not personal IBANs. That is a separate behavioural change on an AML-relevant path.Util.sanitizeStringis untouched. Its unguardedvalue.trim()is why the request DTO carries no@Transform: the transform runs before@IsString, so a non-string body value throws and the exception filter turns that into a 500. The same pattern sits behind other endpoints, so fixing it belongs in its own change rather than in this one.No migration. No env or infra change. No change to any existing endpoint, DTO or behaviour —
getAllBanks,getBankand the payout selector are untouched.Frontend
Not consumed yet. Two follow-up steps are required, in order: add the call to
@dfx.swiss/react(useBank), release it, then replace the dropdown with the free-text field. Two things the frontend must get right: the wire literal isNotMatched— worth stating because it wasUnknownIbanin an earlier revision of this branch. Until the frontend lands, the dropdown behaves exactly as before.Test plan
64 tests in the two spec files, all green — 21 pre-existing and unchanged, 43 new.
receive=false(proves the missing filter is intentional); hit on a value stored in paper format (proves normalization applies to the stored side too).Expired/Deactivated/Reservedone withactive=false; a different account's personal IBAN does not match; a value stored in paper format still matches, which matters because these are persisted straight from the provider response without validation.InvalidIban, notNotMatched, so a mistyped digit is never reported as "not ours"; valid-but-unknown with a login; valid-but-unknown without a login yieldsLoginRequiredand never queries the personal IBANs; a shared-account hit without a login yieldsDfxIbanbefore personal IBANs are ever considered.IBAN:prefix is not salvaged.jwt?.accountthrough, andundefinedwhen there is no token.ValidationPipebuilt from the exact options inmain.ts, asserting aBadRequestExceptionfor a number, an array, an object,null,undefinedand an empty string, plus a positive case proving a plain string arrives unchanged. That last one is what catches a@Transformbeing re-added and turning the 400 back into a 500.Each new safeguard was checked by inverting it: every mutation is caught by exactly its own test and nothing else.
tsc --noEmit,eslintandprettier --checkare clean on all six files.Release Checklist
Pre-Release
Post-Release