fix: OBP-Hola five-flow hardening (rolling) - #70
Merged
Conversation
…t Redirect SCA reach it Two problems in the same place, and the first was hiding behind the second. A Berlin Group consent names its accounts at creation: the TPP lists IBANs in the access object and createBerlinGroupConsentJWT resolves each to a (bank_id, account_id) view before any PSU is involved. Nothing then checked that the PSU who authorises it has anything to do with those accounts. Measured against a running instance: a consent naming another customer's IBAN, authorised by a PSU who does not hold it, bound and served that account's details and balances to the TPP. UK closed this at its own authorise step; Berlin Group never had it. Consent.assertBerlinGroupConsentAccountsHeld reads the accounts off the consent JWT -- the same views the read path will materialise, so the two cannot disagree -- and refuses with ConsentAccountNotHeldByUser. It runs at both authorisation steps: on the POST before the challenge is minted, since an OTP for a consent that can never bind would only deliver a code to someone the TPP nominated; and again on the PUT before any write, because the two are separate requests and nothing in that sequence is transactional. The second problem is why the first stayed invisible: the Consumer half of checkBerlinGroupConsentAccess refused Redirect SCA outright. The standard's same-TPP rule binds "methods submitted by a TPP" (Implementation Guidelines 4.11), but under Redirect the PSU authenticates at the ASPSP and the call arrives from the ASPSP's own front end -- not a TPP, and never the Consumer that lodged the consent. The scaRedirect ceremony could not complete at all. Nothing in the request separates that front end from a second TPP holding a PSU session, so the ASPSP declares its own: berlin_group_sca_front_end_consumer_ids, empty by default, which leaves the same-TPP rule applying to every caller. A declared front end skips that half only; a consent already bound to a PSU still re-binds to that PSU alone. The Consumer check was never what protected the consent -- the TPP that lodged it passes by definition, and is the party the access accrues to. That is what the holdings check is for, and why the two changes belong together. Verified end to end through the Portal: the scaRedirect ceremony now completes (POST authorisations 201, PUT 200, consent valid and owned by the real PSU) and account, balance and transaction reads all return. Still refused: a consent naming an IBAN the claiming PSU does not hold (403, and the consent is left untouched), and an authorisation started by a Consumer that neither lodged the consent nor is a declared front end (403).
…nnector fault grantUKConsentAccountAccess returns a Failure carrying its own reason -- an account_id the PSU does not hold, or one that does not exist at this bank. Passing that Box through connectorEmptyResponse rewrote every one of them into InvalidConnectorResponse at 400, so what reached the TPP was OBP-50200: Connector cannot return the data we requested. connectorEmptyResponse <- OBP-35037: One or more of the specified account_ids is not held by ... An authorisation decision presented as a connector fault, with the actual reason trailing behind a cause it has nothing to do with, and a status code that says the request was malformed rather than refused. A Failure here is the same kind of answer as the ConsentDoesNotMatchUser guard a few lines above, so it now gets the same treatment: its own message, at 403. Only a genuinely empty Box is still treated as a connector problem, which is what connectorEmptyResponse is for. The existing regression test asserted `code should not equal 200`, which was true of the wrapped 400 as well; it now pins 403 and that neither OBP-50200 nor connectorEmptyResponse appears in the message. Mutation-checked: restoring connectorEmptyResponse reds it on the status assertion. Regression: code.api.UKOpenBanking 394/394, code.api.v5_1_0 245/245, code.api.berlin.group 180/180.
Presenting an OBP-native consent to a UK Open Banking endpoint came back as 500 OBP-50000: Unknown Error.: Not found http request header 'Authorization', it is mandatory. A server fault for a request that was merely not entitled, and the Berlin Group side already answers the mirror case cleanly with OBP-35036. The shape of such a request explains the throw. The auth dispatcher routes the consent into its own standard's branch, that branch authenticates the request, and ukConsentId is left unset because applyUKRules never ran. checkUKConsent then finds no Authorization header to read a consent_id claim from, and threw rather than returning the Box it is declared to return. It now returns Failure(ConsentDoesNotMatchStandard), which is the same refusal the Berlin Group path gives for a UK consent, so the two standards answer their mirror cases the same way. Verified against a running instance: 500 becomes 403 OBP-35036, a UK consent at the same endpoint still returns 200, and a request carrying nothing at all is still 401. The short-circuit for consent-header authentication is untouched and now has a test of its own, since that is the path the refusal must not swallow. Mutation-checked: restoring the throw reds the new scenario with the RuntimeException itself. Regression: code.api.UKOpenBanking 396/396, code.api.berlin.group 180/180, code.api.v5_1_0 245/245.
A refused Berlin Group read answered with OBP-20060: User does not have access to the view: ReadBalancesBerlinGroup userId : 8989636c-3879-4f6d-86cc-e79e8d44e388. account : 56fb36df-... Under consent authentication that user id is the consent's own shadow user: an internal identifier minted per consent, which the TPP was never party to and cannot act on. The refusal is about a view and an account, both of which the caller named itself, so the message now says only that. The user id is still logged for anyone diagnosing the refusal. Mutation-checked: putting the user id back reds the new scenario. Regression: code.api.berlin.group 180/180 plus the new scenario, code.api.UKOpenBanking 396/396.
…tiated it
Berlin Group names a payment by its id alone -- /{paymentService}/{paymentProduct}/
{paymentId} carries no account -- so nothing in the route tied a payment to its caller.
Every payment-scoped route fetched it by id and went ahead. A paymentId was therefore a
bearer token: any authenticated TPP holding one could read the payment and its status,
list its authorisations, start an authorisation on it, and cancel it. Starting an
authorisation is the serious one: the challenge is minted for the caller's own user id,
so the second TPP could then answer it and execute someone else's payment.
Under NextGenPSD2 a payment initiation resource belongs to the TPP that created it, and
only that TPP addresses it afterwards. The initiating identity was already being recorded
on the payment (user_id, plus on_behalf_of_user_id when it was lodged under a consent);
nothing read it back. Fetching now goes through getOwnPaymentImpl, which compares those
two against the two the caller presents -- its principal and, under consent
authentication, the PSU it is acting for. Any overlap is enough, so a payment lodged on a
client-credentials token can still be authorised under the PSU's token and the other way
round. A payment carrying neither identity belongs to nobody and is refused.
All eleven payment-scoped routes go through it, including GET
/{paymentId}/cancellation-authorisations, which previously listed a payment's
cancellation authorisation ids without fetching the payment at all. That endpoint now
answers a non-existent paymentId the way its ten siblings already did, rather than with
an empty list; its scenario is updated to match.
Mutation-checked: making the guard always pass reds the new scenario on the first refusal
it asserts. Regression: code.api.berlin.group 182/182.
…g Credit
UK Open Banking splits a signed amount in two. Amount is unsigned -- the pattern for
OBActiveCurrencyAndAmount_SimpleType is ^\d{1,13}$|^\d{1,13}\.\d{1,5}$, which no negative
string matches -- and the direction sits beside it in CreditDebitIndicator
(OBCreditDebitCode: Credit | Debit). OBP holds the same fact the other way round, as one
signed BigDecimal.
Neither half was being done. Every factory passed the signed number straight into Amount
and hardcoded "Credit" next to it, so a debit of 25 was reported as a credit of -25:
wrong in both fields at once, and non-conformant in Amount whatever the direction.
A TPP reading the account could not tell money in from money out.
UKAmounts does the split once, shared by the v2.0, v3.1 and v4.0.1 factories rather than
copied into each, and every transaction and balance now goes through it. Zero is a credit,
which the standard states explicitly. A balance OBP holds as a string that will not parse
is passed through untouched rather than turned into a fabricated zero.
The "Credit" defaults are gone from the two case classes as well: a default is how the
literal reached every debit in the first place, so the direction now has to be supplied
at each construction site.
This is the CreditDebitIndicator half of UK-1. The other half -- ReadTransactionsCredits
and ReadTransactionsDebits not filtering the returned list -- is the follow-up already
noted at constant.scala:685-694, and depended on this.
Mutation-checked: restoring the literal and the signed amount reds 4 of the 7 new
scenarios. Regression: code.api.UKOpenBanking 403/403.
…erDay frequencyPerDay is "the requested maximum frequency for an access without PSU involvement per day", so everything turns on how the ASPSP decides no PSU was involved. NextGenPSD2 settles that with one header: on every AIS read and consent-management call, PSU-IP-Address "shall be contained if and only if this request was actively initiated by the PSU" (parameter PSU-IP-Address_conditionalForAis). isTppRequestsWithoutPsuInvolvement read it the other way round. Only a request carrying PSU-IP-Address: 0.0.0.0 or a no-psu-involved device header counted; a request that simply omitted PSU-IP-Address -- the exact shape the standard reserves for unattended access -- was never counted at all. Since getHeaderValue answers a random long for a header that is absent, absence could not match anything by construction. Each TPP therefore decided whether its own daily limit applied to it, by opting in or not. Absence of PSU-IP-Address is now the declaration it is defined to be. The two sentinels are still honoured, for a TPP that sends the header unconditionally and marks the unattended case in its value instead. Header lookup is case-insensitive, as HTTP requires, and a blank value counts as absent. Mutation-checked: restoring the sentinel-only reading reds the two scenarios about an absent header. Regression: code.api.berlin.group 182/182, code.api.UKOpenBanking 403/403.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| } | ||
|
|
||
| scenario("a request carrying the PSU's address was initiated by the PSU", PsuInvolvement) { | ||
| BerlinGroupCheck.isTppRequestsWithoutPsuInvolvement(headers("PSU-IP-Address" -> "192.168.8.78")) should be(false) |
| } | ||
|
|
||
| scenario("the header name is matched case-insensitively, as HTTP requires", PsuInvolvement) { | ||
| BerlinGroupCheck.isTppRequestsWithoutPsuInvolvement(headers("psu-ip-address" -> "192.168.8.78")) should be(false) |
| scenario("the sentinel values still mark an unattended request", PsuInvolvement) { | ||
| BerlinGroupCheck.isTppRequestsWithoutPsuInvolvement(headers("PSU-IP-Address" -> "0.0.0.0")) should be(true) | ||
| BerlinGroupCheck.isTppRequestsWithoutPsuInvolvement( | ||
| headers("PSU-IP-Address" -> "192.168.8.78", "PSU-Device-ID" -> "no-psu-involved")) should be(true) |
| BerlinGroupCheck.isTppRequestsWithoutPsuInvolvement( | ||
| headers("PSU-IP-Address" -> "192.168.8.78", "PSU-Device-ID" -> "no-psu-involved")) should be(true) | ||
| BerlinGroupCheck.isTppRequestsWithoutPsuInvolvement( | ||
| headers("PSU-IP-Address" -> "192.168.8.78", "PSU-Device-Name" -> "no-psu-involved")) should be(true) |
|
|
||
| scenario("a real device id alongside a PSU address does not make the request unattended", PsuInvolvement) { | ||
| BerlinGroupCheck.isTppRequestsWithoutPsuInvolvement( | ||
| headers("PSU-IP-Address" -> "192.168.8.78", "PSU-Device-ID" -> "99435c7e-ad88-49ec-a2ad-99ddcb1f7721")) should be(false) |
…ware A Berlin Group consent asking for four accesses a day got none: the first unattended call answered 429, with usesSoFarTodayCounter already stamped to 4. Measured across several limits, one HTTP request always spent the whole allowance -- frequencyPerDay=2: one request -> 429, counter 0 -> 2 frequencyPerDay=3: one request -> 429, counter 0 -> 3 frequencyPerDay=6: one request -> 429, counter 0 -> 6 -- which is not what checkFrequencyPerDay does. It grants exactly frequencyPerDay accesses. It was simply being asked many times per request. The authentication pipeline runs once per API version in the route chain: each version wraps its own routes in its own ResourceDocMiddleware, and a middleware whose index holds no matching doc still runs best-effort authentication before falling through to the next one (ResourceDocMiddleware's `case None` branch). Roughly ten passes per request, each carrying the Consent-ID header into applyBerlinGroupRules and spending an access. The comment above the call -- "This function MUST be called only once per call" -- states a precondition its caller has never met. Only the middleware that matched a ResourceDoc attaches it to the CallContext, so its presence identifies the pass that will actually serve the request. Both the check and the increment are now gated on that, which is also what "an access" means. The wider consequence of those extra passes -- the whole pipeline, its consent validation, signature verification and database reads, running ~10x per request for every endpoint -- is left alone here: the fallthrough is deliberate, so that is a decision to take on its own rather than a side effect of this fix. Measured against a running server, frequencyPerDay=4: four 200s, then 429, counter following 1,2,3,4. A request carrying the PSU's address is still not counted at all. Regression: code.api.berlin.group 182/182.
…to the PSU
The ownership guard added in the previous commit compared the people a payment records
against the people the caller presents. That leaves the case Berlin Group actually cares
about: a payment initiation belongs to the TPP that created it, and two TPPs can serve
the same PSU. A second TPP calling with the same PSU's credentials matched on the person
and was let straight through -- GET /{paymentId}/status answered 200 under a different
consumer key.
The scripted probe caught it; nothing in the repository could have, because the payment
recorded no consumer to compare against. It records one now, alongside the user ids that
were already there, and addressing a payment requires the TPP to match as well as the
person. Payments lodged before the column existed carry no consumer and fall back to the
person check rather than becoming unaddressable.
The new scenario needed a caller shape DefaultUsers does not have -- user2 and user3
change the person as well as the consumer -- so it issues resourceUser1 a token under
testConsumer2: same person, different TPP.
Mutation-checked: dropping the TPP comparison reds that scenario and only that scenario.
Regression: code.api.berlin.group 183/183.
Converting a VRP consent-request builds a private custom view named _vrp-<uuid>, grants it to the PSU, hangs a counterparty off it and gives that counterparty a limit. Together they are the mandate: the view carries CAN_ADD_TRANSACTION_REQUEST_TO_BENEFICIARY, and the limit is how much may be paid under it. Revoking the consent dropped only the shadow user's access. The PSU kept a live standing payment authority for a mandate they had just cancelled, and a set of these accumulated on the account for every mandate ever requested -- one test account had collected three _vrp- views, two of them from consents that were never even used, and 52 limit rows. Nothing in the API removed any of it. Each artefact is named after the view and the view belongs to exactly one consent, so this can be undone without guessing. Revocation now gives back the PSU's grant, deletes the counterparty's limit, and removes the view -- but only once no access row still points at it, which removeCustomView already refuses to do otherwise. So a view something else still holds is left in place rather than orphaned. The counterparty row stays. It is a payee record that settled transactions refer to, and deleting it would take history with it; with the view and the limit gone it grants nothing. The release runs outside the shadow-user lookup and after it. Outside, because a VRP consent that never reached SCA has no shadow user and its mandate still has to be released -- an abandoned mandate is exactly the case that accumulated. After, because the view can only go once every access row is gone, the shadow user's included. Getting this wrong is what the new scenario caught: placed inside the comprehension, it never ran at all for an unauthorised consent. The "_vrp-" prefix now lives in Constant, read by both the conversion that writes it and the revocation that looks for it. Regression: code.api.v5_1_0.VRPConsentRequestTest 7/7 including the new scenario. Both harness probes that measured this now pass against a running server.
…ifies
UK Open Banking writes an amount as {"Amount": "...", "Currency": "..."} --
OBActiveOrHistoricCurrencyAndAmount, both members capitalised. The v2.0 and v3.1
factories emitted {"currency": ..., "amount": ...}, because they reused OBP's shared
AmountOfMoneyJsonV121, which spells the same two members in lower case. Every amount in
both versions was affected: transaction amounts, charges, instructed amounts, balances
and credit lines. v4.0.1 already had its own AmountV401 and was correct.
The shared class is used by OBP's own endpoints and cannot be renamed, so the UK
responses take their own shape, as v4.0.1 already does.
This one was hiding a second defect. A probe that read only the lower-case spelling saw
no debit in a v4.0.1 response, concluded there was none to check, and passed -- so the
CreditDebitIndicator bug fixed in 944b592 stayed green in the harness until the casing
was noticed.
v2.0's balances also reported the account owner's *name* as the CreditDebitIndicator, in
a field the standard restricts to Credit or Debit. It is derived from the balance now,
like every other amount here. The Type field on those same balances says "Credit", which
is not a member of OBBalanceType1Code either; that one needs a decision about which
balance type is meant, so it is left alone and recorded rather than guessed at.
Regression: code.api.UKOpenBanking 403/403, code.api.ResourceDocs1_4_0 90/90,
code.api.v5_1_0 246/246. No test asserted the lower-case spelling.
…implicit one
The OBP account-routing scheme means two things at once. It is an implicit self-identifier
-- an address under it is normally the account id, with no row in bankaccountrouting --
but a bank may also register an OBP routing whose address is something else entirely, and
that row is stored like any other scheme's.
getBankAccountByRoutingLegacy honoured only the implicit reading, so an account with a
registered OBP routing was unreachable through every endpoint that resolves by routing.
The row was right there in the table and the answer was "Bank Account not found", which
is what stopped a consent-request naming an account that way from ever converting:
{"scheme":"OBP","address":"hola-testuser01-uk-current"} -> 404 OBP-30073
{"scheme":"OBP","address":"726b08a5-..."} (the account id) -> 201
{"scheme":"IBAN","address":"DE89..."} -> 201
The implicit reading is tried first and still wins wherever both would match, so no
address that resolves today resolves differently. The fallback runs only when the implicit
reading finds *nothing*.
Not when it finds an ambiguity. The first version of this used `or`, which also replaced
"this address matches several accounts" with whatever the routing table said -- nothing --
turning a precise complaint into a bare "not found". The new scenario caught that; reading
the diff would not have.
Mutation-checked: removing the fallback reds the registered-routing scenario alone.
…P uses Found by filling in the VRP form in a browser rather than by the API probes. Naming the debtor account by its registered OBP routing address failed at consent-request creation, one step earlier than the conversion fixed in the previous commit and in a different function: 404 OBP-30018: Bank Account not found. Please specify valid values for BANK_ID and ACCOUNT_ID. Current BankId is gh.29.uk.x1 and Current AccountId is hola-testuser01-uk-current getBankAccountByRoutings -- the plural one, which createVRPConsentRequest calls -- carries its own copy of the implicit-OBP shortcut and had the same blind spot as the singular resolver. Two copies of one rule, so fixing the first did not fix the second. It now asks the resolver that knows both readings, and still falls back to checkBankAccountExists when neither answers, so a genuinely unknown account reports itself exactly as it did before. Verified end to end afterwards: the VRP consent-request converts, the mandate binds, and revoking it releases the view, the PSU's grant and the limit while leaving the PSU's own ten baseline access rows untouched. Regression: the routing suite 5/5, including a new scenario for the plural resolver.
RestConnector_vMar2019_FrozenTest went red, and it was right to. Adding consumer_id to TransactionRequest changed the frozen structure of a type the REST connector sends and receives, so every connector implementor would have seen a new field appear -- for a fact only one server-side guard needs. The field is reverted from obp-commons and from toTransactionRequest. The mConsumerId column stays, because that is where "which TPP lodged this payment" belongs, and the Berlin Group ownership guard reads it straight off the stored row instead. Same behaviour, no change to the connector contract: the frozen test passes again and the Berlin Group suite still holds, including the scenario where a second TPP acting for the same PSU is refused. Regression: code.connector.RestConnector_vMar2019_FrozenTest 5/5, code.api.berlin.group 183/183.
… claimed yet The UK approval screen showed the PSU a bank and a consent id and nothing else -- no permissions, no status, no expiry -- so they were asked to approve a consent without being told what it granted. The markup was there all along; the data never arrived. The screen fetches the consent to fill those fields, and it arrives under its own Consumer rather than the TPP's. The lodging-Consumer comparison therefore refuses precisely the caller whose job is to inform the PSU: 403 OBP-35015. The loader treats that as non-fatal and renders the page bare, which is why it looked like a display bug. This is the same difficulty the Berlin Group Redirect flow already hit, and it takes the same answer: nothing in a request distinguishes the ASPSP's own screen from a second TPP holding a PSU session, so the ASPSP declares which Consumer is its own. The props key is generalised to sca_front_end_consumer_ids, since it was never Berlin-Group-specific; the old berlin_group_sca_front_end_consumer_ids is still read, so a configured instance needs no edit. Narrow on purpose. It applies only while the consent is unclaimed -- the window the approval screen exists for -- so once a PSU is bound, the PSU comparison governs and a declared front end gets no further than anyone else. It is inert unless an ASPSP declares a front end at all, which is the default. Measured against a running server: the screen now shows AWAITINGAUTHORISATION, the expiry and all four requested permissions, while an undeclared Consumer reading the same unclaimed consent is still refused 403. Mutation-covered by three new scenarios: a declared front end may read an unclaimed consent, may not read a claimed one, and an undeclared caller is still refused. Regression: code.api.UKOpenBanking 406/406, code.api.berlin.group 183/183.
… restrict the rows These are independently-selectable Permissions in the UK profile, and the ASPSP must refuse a consent that names a transactions depth without at least one of them -- which OBP already does. Then it returned every transaction regardless. A consent granting Credits only still returned the debits, so the PSU's choice of direction was decorative and the TPP saw money going out of an account it had only been permitted to watch coming in. This is the follow-up recorded at constant.scala:685-699. It was blocked on CreditDebitIndicator being a hardcoded literal, since a filter and a label that disagree are worse than neither; that was fixed in 944b592, so the direction is now derivable and both read it from the same place. Applied in the endpoint rather than as a can_* permission, as that note decided: direction restricts which rows come back, not which fields are visible, so the view's permission set is the wrong instrument. The two direction views are resolved exactly as Detail-or-Basic already is, and holding both -- or neither -- restricts nothing: neither is the plain Basic case, both is a TPP asking for everything. Shared by v3.1 and v4.0.1 rather than written twice, which is also what keeps the filter and the label from drifting apart. Mutation-checked against a running server: disabling the filter reds exactly the four direction-restricted cases across both versions and leaves the two unrestricted ones green. Repro: .local-testing/OBP-Hola/uk_direction.py, 6/6. Regression: code.api.UKOpenBanking 410/410, code.api.berlin.group 183/183.
Five fixes to my own earlier commits, found by reviewing the branch as a whole.
**Direction restriction was applied after the page limit.** The filter trimmed a page the
database had already limited, so a direction-restricted consent got a short page it could
not tell from the end of the data -- and with Constant.Pagination.limit defaulting to 50,
with no pagination parameter from the TPP at all. On an account whose first page is
debits, a Credits-only consent saw one row where eleven existed. The restriction is now
pushed into the query as OBPTransactionDirection so the database applies it and the limit
together; the endpoint filter stays, because a connector other than the mapped one may
ignore the param and that filter is what actually enforces the consent's scope.
The two-transaction fixture is why the earlier tests could not catch this. The new probe
seeds sixty debits ahead of ten credits, and mutation-checking it against the previous
behaviour reds all four cases.
**A declared SCA front end could reach a consent that already had a PSU.** The Berlin
Group guard fell through to the front-end exception whenever the caller presented no PSU,
which is exactly what a client-credentials caller presents -- so the exception applied to
consents already bound to somebody else, the opposite of what its own paragraph promises.
Now conditioned on the consent being unclaimed, as the UK twin already was.
**The counterparty-limit deletion was fire-and-forget.** Its Future was discarded and
getCounterparties' Box was flattened with getOrElse(Nil), so a failed lookup or a failed
delete left the standing limit alive -- the very leak the commit fixes -- while the code
logged "released". Both are observed now, and say so when they fail.
**unsignedAmount used toString.** BigDecimal renders a negative scale in scientific
notation, so BigDecimal("1E+3") came out "1E+3", which the Amount pattern this exists to
satisfy rejects. toPlainString.
**A superseded scaladoc block** was left stacked above scaFrontEndConsumerIds, still
asserting the key is Berlin-Group-specific.
New scenarios cover the plain rendering, the query restriction, and that the restriction
and the post-filter agree on every amount -- two enforcements of one rule that must not
diverge.
Regression: code.api.UKOpenBanking 413/413, code.api.berlin.group 183/183,
code.api.v5_1_0 246/246, RestConnector_vMar2019_FrozenTest 5/5.
A cleanup failure must not fail a revoke that already happened. The Await added to observe the counterparty-limit deletion runs after the status flip has committed and after the shadow user's access is gone, and a second attempt is refused with ConsentAlreadyRevoked -- so letting a timeout or a connector failure escape turned a completed revoke into a 500 and abandoned the views still queued behind it. Caught and logged instead. A transaction whose amount the view withheld is now admitted by neither direction. The filter reads already-moderated rows, so a missing amount means the view did not grant CAN_SEE_TRANSACTION_AMOUNT rather than that the amount is zero; creditDebitIndicator maps that to Credit for labelling, which as a permission test handed every debit to a Credits-only consent. The direction boundary now lives once, on OBPTransactionDirection, and both enforcements build from it: the connector's SQL predicate and the endpoint filter the test checks against. The scenario claiming the two agree used to model the query in the test itself, so it agreed with its own copy and could not detect the drift it existed to catch. The v3.1 and v4.0.1 transaction reads were the same thirty lines with a different factory at the end, which is why the direction rule had to be written into both. They now share UKTransactionsQuery and keep only their route and their yield. LocalMappedConnector's two identical query builders are likewise one method. That also clears the duplication gate the previous commit pushed over its threshold. Remote connectors take a frozen outbound message that cannot carry the direction, so they still return both and the filter trims an already-limited page. Nothing here can repair that without the connector, but returning the short page silently is how the defect stayed invisible: a full page that lost rows to the filter is now logged with the connector that needs the param.
GET /consents/CONSENTID/authorisations had passesPsd2Aisp and nothing else, so any AISP caller could list the challenge ids of a consent lodged by somebody else. Its immediate neighbour, GET /consents/CONSENTID, already compares the consent's consumer against the caller's and answers 403 -- two reads of the same consent, twelve lines apart, disagreeing about who may perform them. The PUT that answers a challenge is guarded, so this leaked identifiers rather than access. Guarded the same way as the sibling.
… example
Berlin Group hangs four request bodies off one authorisation path, and the
handlers dispatch on the body's shape. Only transactionAuthorisation was ever
recognised, and the fall-through was a hardcoded example rather than an
error -- so anything else got a fabricated success.
Two consequences, both reachable by a conforming TPP:
An empty body is how the standard starts an authorisation, and it fell to
that fall-through. The caller got 201 with the literal authorisationId
"123auth456.", then discovered at the PUT that the id matched no challenge.
Neither start handler reads scaAuthenticationData -- the POST mints the
challenge and the PUT answers it -- so an empty body and a
transactionAuthorisation body are the same request here, and both now start a
real authorisation.
On the PUT, the final else was labelled "authorisationConfirmation variant"
but tested nothing, so an unreadable body was answered "scaStatus":
"finalised" -- the terminal success state of strong customer authentication --
for an authorisation nothing had happened to. It is guarded by the checker
that already existed for it.
What remains mocked is what is declared as mocked: the updatePsuAuthentication
and selectPsuAuthenticationMethod Embedded steps. Everything else is now a
400 that says which shapes are accepted.
None of this was reachable through OBP's own clients, which always send
{"scaAuthenticationData": ""} -- the single branch that worked was the only
one ever exercised.
|
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
2 participants
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.
Rolling PR for the OBP-Hola five-flow hardening campaign — the follow-up to
#68 and #69,
working through the defect list in
OBP-Hola/FIVE-FLOW-VERIFICATION-2026-08-05.md.Each commit is one self-contained fix with its own tests and mutation check, and each is
independently green — review commit by commit. Base is
75a4ac6f5(#69).Every fix is gated on three things before it is committed: targeted tests green and
mutation-verified (reverting the fix reds them precisely), full regression of the affected
standards green, and no new failures in the live probe suite.
Commits so far
1.
e51836c54— Berlin Group: check account holdings on bind, and let Redirect SCA reach itTwo problems in the same place, the first hidden behind the second.
The security one. A BG consent names its accounts at creation — the TPP lists IBANs and
createBerlinGroupConsentJWTresolves each to a(bank_id, account_id)view before any PSU isinvolved. Nothing then checked that the PSU who authorises it has anything to do with those
accounts. Measured on a running instance: a consent naming another customer's IBAN, authorised by
a PSU who does not hold it, bound and then served that account's details and balances to the
TPP. UK closed this at its own authorise step; Berlin Group never had it.
Consent.assertBerlinGroupConsentAccountsHeldreads the accounts off the consent JWT — the sameviews the read path materialises, so the two cannot disagree — and refuses with
ConsentAccountNotHeldByUser. It runs at both authorisation steps: on the POST before thechallenge is minted (an OTP for a consent that can never bind would only deliver a code to
someone the TPP nominated), and again on the PUT before any write (separate requests, nothing
transactional).
Why it stayed invisible. The Consumer half of
checkBerlinGroupConsentAccessrefused RedirectSCA outright. The same-TPP rule binds "methods submitted by a TPP" (Implementation Guidelines
4.11), but under Redirect the PSU authenticates at the ASPSP and the call arrives from the ASPSP's
own front end — not a TPP, and never the Consumer that lodged the consent. The scaRedirect
ceremony could not complete at all.
Nothing in the request separates that front end from a second TPP holding a PSU session, so the
ASPSP declares its own:
berlin_group_sca_front_end_consumer_ids, empty by default, which leavesthe same-TPP rule applying to every caller. A declared front end skips that half only — a consent
already bound to a PSU still re-binds to that PSU alone.
The Consumer check was never what protected the consent: the TPP that lodged it passes by
definition and is the party the access accrues to. That is what the holdings check is for, and why
the two changes belong in one commit.
Verified end to end through the Portal — the ceremony now completes (
POST .../authorisations201,
PUT200, consentvalidand owned by the real PSU) and account/balance/transaction readsall return 200. Still refused: a consent naming an IBAN the claiming PSU does not hold (403, and
the consent is left untouched —
museridstill null, status stillreceived), and anauthorisation started by a Consumer that neither lodged the consent nor is a declared front end
(403
OBP-35015).Regression:
code.api.berlin.group180/180,code.api.UKOpenBanking394/394,code.api.v5_1_0245/245.