Hotfix 4.4.4 - FinTS bank import review fixes - #332
Open
lukas-staab wants to merge 35 commits into
Open
Conversation
Every FinTS page posts back to its own URL: the new-credentials form targets
itself, the TAN prompt posts to request()->url() (FintsController::renderTanInput)
and the TAN mode/medium pickers post to their own route. The legacy router allows
that already ('method' => ['GET', 'POST'] on the credentials node, inherited by
its children), but the Laravel routes in front of it were GET-only. Every submit
therefore fell through to the catch-all route at the bottom of the group and lost
its route name, which broke breadcrumb resolution and surfaced as an error page.
The breadcrumb for legacy.konto.credentials.import-transactions was missing
entirely, so the statement-update page had no trail of its own.
Refs: OP#603
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The online-banking PIN was read with Symfony's getAlnum(), which is
preg_replace('/[^[:alnum:]]/', '', ...) and therefore dropped every special
character and umlaut before the PIN ever reached the bank. phpFinTS documents
explicitly that a PIN may contain "alphabetical or even arbitrary characters"
(Fhp\Options\Credentials::create), so a PIN like "geheim!23" was silently turned
into something else. From the outside a mangled PIN is indistinguishable from a
wrong one - and three wrong ones lock the online-banking access at the bank, so
this could cost the user their access without them ever mistyping. It is now
taken verbatim.
The TAN keeps having whitespace removed, because banks print TANs in groups
("123 456"), but nothing else is stripped: some schemes are alphanumeric and
dropping a character would burn one of the three attempts.
getAlpha() had the same effect on two labels, where it dropped digits and spaces:
the name of a bank access and the name of an imported account both lost anything
numeric ("Konto 2024" -> "Konto"). Both are now stored as entered, cut to the
column width instead of relying on the stripping to keep them short.
Refs: OP#604
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
$cents is computed from the signed float, so multiplying it by sign($amount) a
second time turned every negative amount positive: convertToCent("-123.45")
returned +12345.
This is latent today, and the consequence first suspected - "incremental import
aborts for an overdrawn account" - does not actually occur. The only caller that
omits the credit/debit mark reads the stored saldo and sets $tryRewind = true in
the same breath, while the guard that would compare that value is bound to
$tryRewind === false; by the time the flag flips, $oldSaldoCent has been
overwritten with the computed running saldo. The wrong value is write-only. Bank
data cannot reach the branch at all: MT940 carries unsigned magnitudes plus a
separate credit/debit mark, which is the other, non-null code path.
It is fixed first because repairing that dead guard (next commit) makes the defect
live immediately - a negative stored saldo would then abort every sync. No
changelog entry: nothing observable changes until the guard actually runs.
abs() in the mark branch is defensive only. It is a no-op for the unsigned
magnitudes banks send, and merely stops a signed amount from cancelling out
against its own mark.
Pinned by tests/Pest/Legacy/FintsAmountConversionTest.php, which reaches the
private method by reflection and also covers the float-drift case (8.20 * 100 is
819.9999... in binary floating point, so the rounding is load-bearing).
Refs: OP#619
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The bank returns a range that overlaps what is already stored, so saveStatements()
has to recognise the already-known transactions and skip them. That anchor was
looked up including customer_ref, which holds the SEPA end-to-end id - and MT940
usually leaves it empty or reports NOTPROVIDED. The lookup therefore failed
routinely, and a failed lookup had no branch of its own: $tryRewind simply stayed
true, every transaction of the range looked new, and months of bookings were
inserted a second time while the UI reported "N Einträge importiert". Silent
duplication of accounting data was the worst outcome found in the review.
customer_ref is gone from the criteria - the running saldo is a much stronger key -
and a resume point that is never reached is now a hard error that rolls back.
The saldo check against the stored value was dead code. $oldSaldoCent was assigned
together with $tryRewind = true, while the comparison was guarded by
$tryRewind === false; by the time the flag flipped, $oldSaldoCent had been
overwritten with the running statement-to-statement saldo. So the intended promise
- "the incoming start balance must match what we stored" - never held: a stored
saldo of 100.00 against an opening balance of 999.99 passed silently. The stored
value is now kept separately and compared at the one point where it is meaningful,
namely when the last already-stored transaction has been consumed.
Stopping early on sync_until is tracked, so ending a replay at the configured
cut-off is not mistaken for a missing resume point.
Both saldo messages were raw internals ("12345 !== 67890 at statement from ...")
even though they reach the treasurer as a flash message; they now name the amounts
and dates in plain German.
Control flow verified by simulating the loop over six scenarios: empty account,
normal incremental run, restated bank value (now refused instead of duplicating),
tampered stored saldo, gap between statements, and a sync_until stop mid-replay.
An end-to-end test needs legacy-DB fixtures that do not exist yet (tests/ has no
DBConnector precedent) and is filed separately.
Refs: OP#605
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ring $fintsHandler is declared ?FintsConnectionHandler without a default and is only assigned when the bank password is present in the session. render(), actionViewSepa(), actionPickTanMode(), actionPickTanMedium(), actionNewSepaKonto() and actionImportNewSepaStatements() dereferenced it regardless, so once the session had expired - SESSION_LIFETIME is 120 minutes, and reading a statement is not something one does every hour - any click produced "Typed property FintsController::$fintsHandler must not be accessed before initialization", i.e. a bare error page with no hint that re-authenticating was all that was needed. The property now defaults to null and every use goes through requireFintsHandler(), which flashes an explanation and redirects to the bank login for that credential (or to the credential list when the route carries no credential id). actionLogout() keeps its own check on purpose: logging out of a connection that is already gone is not worth a redirect, it just says so. Refs: OP#606 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fhp\Protocol\UnexpectedResponseException extends RuntimeException while
Fhp\Protocol\ServerException extends Exception - two unrelated hierarchies. Nearly
every catch here listed only CurlException|ServerException, so the whole
UnexpectedResponse family escaped uncaught.
The one that hurts is a rejected TAN: FinTs::submitTan() reports it as
UnexpectedResponseException("Bank has not accepted TAN: ..."), so mistyping a TAN
produced an error page and lost the pending action instead of saying "TAN nicht
akzeptiert" and letting the user try again. The same gap existed when fetching TAN
modes and TAN media, when executing an action, when saving a TAN mode, and on
logout - only login() had it right.
submitTan() additionally catches InvalidArgumentException, which is what the library
raises for a decoupled TAN mode ("Cannot submit TAN for a decoupled TAN mode",
because confirmation happens in the banking app). It now explains that the scheme is
not supported yet rather than showing an error page; implementing it properly is
OP#613.
A logout that cannot reach the bank now also drops the local session data, so the
user is not left with a connection that appears live but is not.
Refs: OP#607
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…de for While a statement request waits for a TAN, the action is held in the session. getStatements() picked it back up on nothing but its type, so the pending request of one account satisfied the call for another: start an import for account A, wait for the TAN prompt, open account B's import URL and enter the TAN there - and A's statements came back, which saveStatements() then wrote under B's konto_id. Wrong transactions in the wrong account, silently, in the accounting data. The IBAN and the date range a pending request was created for are now recorded alongside it, and it is only resumed when they match. Anything else is discarded with a warning and the request is made afresh, so the flow fails closed. The scope is recorded before execute(), because that is what caches the action and then throws NeedsTanException to end the request. Refs: OP#608 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
$options->productVersion = InstalledVersions::getRootPackage()['version'].DEV ? '-dev' : '';
Concatenation binds tighter than ?:, so this evaluated as (('4.4.3'.DEV) ? '-dev' : '')
- an always-truthy string - and every request announced the product version as
literally "-dev". Banks show that value in the user's list of registered products,
so it is what a treasurer sees when checking which application has access.
While here: an empty FINTS_REG_NR made FinTsOptions::validate() raise "Product name
required!" as an uncaught InvalidArgumentException. A missing registration number is
a configuration problem, so it now says so.
No changelog entry - nothing changes for users of an installation that is configured
correctly.
Refs: OP#611
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tion actionNewSepaKonto() wrote whatever arrived into konto_type: * The IBAN validator's result was destructured as [, $iban], throwing away the validity flag - and V_iban() returns [false, ''] for a bad IBAN, so an invalid one was stored as an empty string and the account could never sync. * The IBAN came from the form and was never held against the accounts the bank access actually holds, although getIbans() was right there. Any account at all could be registered for synchronisation. * Nothing enforced uniqueness of short or iban, even though short is documented as unique and serves as the prefix of every payment id. Two rows for one IBAN make the import pick an arbitrary one of them, because the lookup is keyed by IBAN. * The label was cut to 32 characters but never required to be non-empty, and short was taken as "first two letters of whatever arrived" instead of being required to be two letters. * The method treated *any* non-empty POST as a submit, so the TAN prompt - which posts back to the current URL - was mistaken for one and died in date_create(null). Presence of the form's own fields is now what identifies a submit. * sync-from was parsed with date_create(), which returns "now" for an empty string rather than false, so an empty date silently became today. Each failure now names itself instead of producing an unusable account. Two neighbouring crashes are gone as well: opening the import URL for an account that was never registered raised an undefined-array-key error page, and konto_type.sync_from is nullable while DateHelper::fromUntilLast() did "clone false" on it - a fatal. It falls back to the epoch and lets the bank decide how far back it goes. Refs: OP#609 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…endpoints These paths were not merely unused, they threw the moment they were reached. RestHandler called seven methods that do not exist on FintsConnectionHandler - saveDefaultTanMode, hasTanSessionInformation, deleteTanSessionInformation, lockCredentials, deleteCredential, changePassword and newKontoCredentials - so save-default-tan-mode, submit-tan, abort-tan, lock-credentials, delete-credentials, change-credential-password and save-new-konto-credentials all ended in "Call to undefined method". The class has no parent, no traits and no __call, and none of those names is defined anywhere in the repo. submit-tan was doubly broken: it destructured the bool from submitTan() as [$ret, $msg], so it could never report success. Most of them had no route entry either, making them unreachable twice over. importKonto() was a stale duplicate of FintsController::actionNewSepaKonto() (the live path, and the target of the form), still using the old validator and unsanitised input. HibiscusXMLRPCConnector (409 lines) builds on XML_RPC2_Client, which is not installed - no such package in composer.json or composer.lock, nothing in vendor/, no extension - and its HIBISCUS_* statics are read but never assigned. The update-konto endpoint called it anyway. It also embedded credentials in the request URL and disabled TLS verification (sslverify => false); patterns better deleted than left in the tree as an example. Its orphaned rest/hibiscus route is gone too. No callers were harmed: every one of these action names was searched across resources/, public/, legacy/, app/, routes/ and lang/, and nothing referenced them. rest/clear-session is kept - it works and a button uses it. The delete-credentials *page* route stays as well; it points at FintsController, not at the REST action, and implementing it properly is OP#618. No changelog entry: nothing that worked before stops working. Refs: OP#612 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The legacy route group is registered with ->withoutMiddleware(VerifyCsrfToken::class) (bootstrap/app.php), so Laravel's CSRF protection does not apply to any of these routes. Every form here does ship a hidden `nonce` field holding csrf_token() - but only RestHandler ever compared it; the actions in this controller never looked at it, so the field was decoration. That left the bank access reachable cross-site for anyone who could get a logged-in treasurer to load a page: repeated forced login attempts, which matter because three wrong PINs lock the online-banking access at the bank; creating credential records; and registering an arbitrary account for synchronisation. render() now verifies the nonce with hash_equals() before dispatching any POST, and refuses the request with an explanation otherwise. All FinTS forms already send it (HtmlForm adds it by default, and the two hand-rolled forms call renderNonce()), so no form needed changing. Keeping the check here rather than switching the middleware for the entire legacy route group is deliberate - that group contains a lot of other forms and is not something to flip in a patch release. Still open, and filed separately: importing statements and logging out are reachable via GET, so they remain triggerable by an <img> tag or a link prefetch. Converting them to POST means reworking the icon links into forms, which is UI work that wants its own testing. Refs: OP#610 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
new ReflectionClass(...)->method() without the wrapping parentheses, per NewMethodCallWithoutParenthesesRector. Pure formatting; the pre-commit hook runs pint but not rector, so this did not get caught when the file was added. Refs: OP#619 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Refs: OP#603 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lukas-staab
commented
Aug 11, 2026
| // The concatenation binds tighter than ?:, so this used to evaluate as | ||
| // (('4.4.3'.DEV) ? '-dev' : '') - an always-truthy string, which reported the | ||
| // version to the bank as literally "-dev" regardless of what is installed. | ||
| $options->productVersion = InstalledVersions::getRootPackage()['version'].(DEV ? '-dev' : ''); |
Member
Author
There was a problem hiding this comment.
use the one out of the config instead
Member
Author
There was a problem hiding this comment.
🤖 Claude Code (posting via @lukas-staab's account)
Done — config('stufis.version') now, in cf38557f.
Worth noting it was wrong twice over: getRootPackage()['version'] returns the normalised version, so even with the precedence fixed the bank would have been told 4.4.4.0. The config value goes through getPrettyVersion() and gives 4.4.4. Verified locally:
config(stufis.version) = '4.5.0-beta'
getRootPackage()['version'] = '4.5.0.0-beta' <- was being sent
getRootPackage()['pretty_version'] = '4.5.0-beta'
The Composer\InstalledVersions import is gone with it.
| $action = GetStatementOfAccount::create($account, $start, $end); | ||
| // Has to be recorded before execute(), which caches the action and then throws | ||
| // NeedsTanException, ending this request. | ||
| $this->setCache('action-scope', $scope); |
Member
Author
There was a problem hiding this comment.
🤖 Claude Code (posting via @lukas-staab's account)
Written, yes — the logger level is Info in production (DEV ? Debug : Info), and this is a warning. But read, realistically no: it goes to legacy/runtime/logs/fints.log, which nobody opens, and that was the actual problem — from the user's side the TAN they were about to enter just silently stops applying.
So in cf38557f it also says so on the page:
Der noch offene Umsatzabruf gehörte zu einem anderen Konto oder Zeitraum und wurde verworfen — Der Abruf für dieses Konto wird neu gestartet, dafür ist eine neue TAN nötig.
Separate question your comment raises: that whole fints.log channel sits outside Laravel's logging stack, so nothing there reaches central monitoring. Happy to fold it into Laravel's channels as its own work package if you want — say the word and I'll file it.
| if (ArrayHelper::allIn($post->keys(), ['iban', 'konto-name', 'konto-short', 'sync-from'])) { | ||
| $errors = []; | ||
|
|
||
| [$ibanValid, $iban] = (new NewValidator)->validate((string) $post->get('iban'), 'iban'); |
Member
Author
There was a problem hiding this comment.
feel free to use laravel validators or libaries here, if it has benefit
Member
Author
There was a problem hiding this comment.
🤖 Claude Code (posting via @lukas-staab's account)
Went one better and deleted the hand-rolled block entirely — see the reply on actionNewSepaKonto below. The Livewire page already had exactly these rules, including iban, which reports properly in German:
DE02120300000000202051 (valid) => PASSES
DE00120300000000202051 (bad checksum) => Der Wert IBAN ist keine International Bank Account Number (IBAN).
So rather than building a second set of rules next to the existing ones, the FinTS flow now uses those. ~50 lines removed.
| @@ -393,26 +394,75 @@ function ($actionName, $iban) use ($credId): string { | |||
|
|
|||
| protected function actionNewSepaKonto(): void | |||
Member
Author
There was a problem hiding this comment.
could we try to replace this action with the refactored livewire one? maybe with a prefilled iban?
Member
Author
There was a problem hiding this comment.
🤖 Claude Code (posting via @lukas-staab's account)
Done in cf38557f. The action is now a redirect to pages::new-banking-account with the IBAN prefilled, plus the two things you asked for after: it returns to the bank access afterwards, and manually_enterable is locked.
This turned out to be clearly the right call — that page also handles sync_until and manually_enterable, and the legacy insert never set manually_enterable at all. It only escaped being a bug because the column defaults to false.
Three details worth your eyes:
- The IBAN is locked (read-only, with a reason shown rather than hidden). The prefill comes from
lengthenIban(), i.e. the bank's own account list — so a bank-synced account still cannot be pointed at a foreign IBAN. That preserves the ownership check from OP#609, which a plain redirect would otherwise have lost. returnTois same-origin only. An absolute URL, a protocol-relative//host, orjavascript:all fall back to the Konto page — otherwise this would have been an open redirect. The value handed over is built from a named route as a relative path. Three tests cover the hostile inputs.- New exposure I handled: because that page also serves hand-kept accounts, an IBAN the credential does not hold can now reach the import, where
getSepaAccount()threwInvalidArgumentException— an error page. It reports the mismatch now.
One thing I did not change: the page's short rule is max:2, so a one-letter prefix is accepted (my deleted version required exactly two). Left it as the page's own convention — tell me if you want it tightened.
12 tests in tests/Pest/Accounting/NewBankingAccountTest.php.
| $syncUntil = $until === null ? false : DateTime::createFromFormat(DBConnector::SQL_DATE_FORMAT, $until); | ||
|
|
||
| // konto_type.sync_from is nullable, and "clone false" is a fatal error - so fall | ||
| // back to the epoch and let the bank decide how far back it will go. |
Member
Author
There was a problem hiding this comment.
this might be risky. Some banks where not very forgiving with dates in the future, and sometimes slightly branch the normed specification
Member
Author
There was a problem hiding this comment.
🤖 Claude Code (posting via @lukas-staab's account)
Agreed, and it is not a rare path either — three of the six accounts in the dev/testing data have sync_from NULL, so that epoch fallback would have been exercised in practice, not just in theory.
Changed in cf38557f to not invent a date at all: when there is no configured start and no last_sync, no start date is sent and the range is left to the bank's own default. The library supports it — GetStatementOfAccount::create() takes a nullable $from — so fromUntilLast() returns ?DateTime for the start and getStatements() accepts null. Both had exactly one caller, so the contract change is contained.
Your existing clamp for sync_until in the future is untouched:
no dates at all start=NULL(bank decides) until=2026-08-11
sync_from set start=2026-01-01 until=2026-08-11
sync_from + last_sync start=2026-08-01 until=2026-08-11
sync_until in future start=2026-01-01 until=2026-08-11
Product version from the config (OP#611)
config('stufis.version') instead of InstalledVersions::getRootPackage()['version'].
The latter is the *normalised* version, so the bank would have been told "4.4.4.0"
rather than "4.4.4" - the config value goes through getPrettyVersion(). The
InstalledVersions import is gone with it.
Discarded statement request is now visible (OP#608)
Dropping a pending request that belonged to another account only wrote a warning to
legacy/runtime/logs/fints.log. That file is written in production (the level is Info
there) but it is not somewhere anyone looks, and from the user's side the TAN they
were about to enter just silently stops applying. It says so on the page now.
No invented start date (OP#609)
Falling back to the epoch when sync_from is NULL was risky: banks are picky about
dates and only keep a limited history, and this is not a rare path - three of the six
accounts here have no start date. Instead no start date is sent at all and the range
is left to the bank, which the library supports (GetStatementOfAccount takes a
nullable from). fromUntilLast() returns ?DateTime for the start accordingly; both it
and getStatements() have exactly one caller each. A sync_until in the future is still
clamped to today, as before.
Account registration handed to the Livewire page (OP#609)
The hand-written create form is gone; the FinTS account list now links to
pages::new-banking-account with the IBAN prefilled. That page already validates with
Laravel rules - including the 'iban' rule, which reports properly in German - and it
also knows about sync_until and manually_enterable, a column the legacy insert never
set at all. So this replaces ~50 lines of hand-rolled checks with the form used
everywhere else, and answers the "use Laravel validators" note by using the ones that
already existed rather than building a second set.
The page returns to the bank access afterwards. returnTo is honoured only for
same-origin paths - an absolute URL, a protocol-relative "//host" or a "javascript:"
scheme falls back to the Konto page, so this cannot be turned into an open redirect -
and the value handed over is built from a named route as a relative path.
For an account coming from a bank access, the IBAN and the manual-entry switch are
locked: manual entry would rule out the very synchronisation the account is being set
up for. Both stay visible with a reason instead of being hidden, and store() forces
manually_enterable to false so a tampered request cannot flip it either.
Because that page also serves hand-kept accounts, an IBAN the credential does not hold
can now reach the import; getSepaAccount() threw InvalidArgumentException for it, which
was an error page. It reports the mismatch instead.
Covered by tests/Pest/Accounting/NewBankingAccountTest.php: prefill, storing, the return
path, the three open-redirect attempts, the manual-entry lock (including that a normal
cash account can still be flagged manual), and the validation rules.
Refs: OP#608
Refs: OP#609
Refs: OP#611
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The IBAN field on "Konto anlegen" carried only the label and an "optional" badge, so nothing said why it matters. It is what the FinTS import matches on: the statement lookup is keyed by IBAN (konto_type is fetched with the IBAN as the key), so an account without one cannot be synced automatically. The file import checks it too - see AccountIbanRule, which holds the stored IBAN against the one in the statement. The description says that now, and the variant shown for an account handed over by a bank access mentions the matching as well, since that is precisely why its IBAN is locked. Refs: OP#609 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was no config/logging.php at all, so the app ran on the framework defaults, and .env.example pinned LOG_STACK=single - one file that never rotates. Instances run on shared hosting without root, so there is no system logrotate to catch that either; storage/logs/laravel.log simply grew until someone noticed. The config is published now, with both LOG_CHANNEL and LOG_STACK defaulting to "daily" so an instance rotates even with no logging variables in its .env at all. Monolog prunes on write, so this needs no cron. LOG_DAILY_DAYS keeps a month. The unused slack and papertrail channels are dropped, the rest stays at framework parity. storage/logs is excluded from the backup source as well. The include list was storage_path() with no exclusion, so every stufis-update copied the whole log directory into its pre-update archive - a big log was being multiplied across every retained backup. Note for existing instances: an explicit LOG_STACK=single in a deployed .env beats the new default, so those have to be switched by hand, and the leftover unrotated laravel.log deleted once - the daily driver only prunes files matching its own laravel-<date>.log pattern. Both are in the install docs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nk by hand Picking a bank when creating an access was limited to whatever rows somebody had inserted into konto_bank directly in the database - the table had no seeder, no UI and no migration data, so a fresh instance could not create a bank access at all. The four columns it held (id, blz, name, url) were a hand-kept subset of public reference data. fints_institutes replaces it, filled by stufis:fints-institutes-update from the hbci4java blz.properties list (~4000 institutes). The Deutsche Kreditwirtschaft hands its own FinTS-Bankenliste to registered vendors only and forbids shipping it with a product, hence the public equivalent; the source is swappable through FINTS_INSTITUTE_LIST_URL. The BLZ is the primary key and what konto_credentials now references, so a bank's name and PIN/TAN endpoint have exactly one source. Banks move those endpoints every few weeks, and previously an access kept pointing at a stale URL until someone edited the database by hand. The command refuses implausibly short lists (--min-entries, default 1000) so a truncated download or an HTML error page cannot wipe the table, and offers --dry-run, --file and --prune. bin/stufis-update runs it after the migrations, deliberately non-fatal: reference data fetched over the network must not abort an otherwise fine deployment and leave the instance in maintenance mode. The migration carries existing accesses over before konto_bank is dropped, seeding the institutes actually in use from the retired rows - same name, same URL - so they keep working until the first real sync replaces them with authoritative data. It refuses to run if an access references a bank that is not there, is guarded against being re-run (MariaDB cannot roll back DDL), looks the foreign key up in information_schema rather than trusting the hardcoded legacy name, and has a working down(). Refs: OP#617
…unt over The account form serves both bank accounts and cash boxes, so its labels talk about "Konto bzw. Kasse" throughout. An account handed over by a FinTS bank access is tied to a real bank account and can never be a Kasse, so every mention of one is noise at that point. A label() helper picks the "-bank" variant of a key when bankSynced is set, which keeps the choice out of the template and makes the pairs explicit in lang/de/konto.php. The submit button says "Speichern und weiter zum automatischen Abruf" there, because saving hands the user back to the bank access to set the retrieval up rather than finishing the job. Refs: OP#609
validation.custom.name applies to every form with a field called "name", not just to projects, so a missing account name was reported as "Bitte gib einen Namen für das Projekt an." It is neutral now - the field's own label supplies the context. Refs: OP#609
The PIN and every TAN travel over konto_bank/fints_institutes' endpoint URL, so a plain http:// entry would hand them to anyone on the path. phpFinTS points this out in FinTsOptions but does not check it. FintsInstitute::hasSecurePinTanAddress() is the one place the rule lives. On the way in, the parser drops an address that is not https:// and keeps the institute without one - it then simply is not PIN/TAN capable, which is the truth of the matter - and the command reports how many it discarded. Nothing in today's list is affected: of the 2720 entries carrying a PIN/TAN address, none uses http://. The guard is a net, mainly for the address the retiring konto_bank table hands over, which nobody ever validated. Refs: OP#617
A legacy page renders inside two nested buffers: the one LegacyController opens and the one HTMLPageRenderer opens per Renderer. When a page broke off mid-render, at most one of them was closed - the redirect branch called ob_get_clean() not at all, the other two exactly once. The leftover buffer is flushed by PHP at shutdown, so a half-rendered page ended up behind the actual response - inside the body of a 302, for a redirect. In a process serving more than one request it also swallowed the next request's output, which is what made the first HTTP tests for the bank-access pages unusable. render() now remembers ob_get_level() before its own ob_start() and all three catch branches unwind to exactly that level - never below it, since that one belongs to the caller. Refs: OP#634
The credentials overview has always rendered a trash icon, but no method existed for the declared delete-credentials action, so the click ran into a 404 - a bank access could not be removed through the interface at all. That sits badly next to being able to create one from the bank list without touching SQL. Two steps: GET renders the confirmation, POST carries it out. The icon therefore stays a plain link - a GET that only renders a page - while the irreversible half is a nonce-checked POST, which is also what avoids the rebuild of the table columns that OP#621 expected to be coupled to this. The confirmation says what goes and what stays: accounts and their imported bookings survive, only the automatic retrieval ends. The icon now shows without an active bank session too. It used to hide behind hasActiveSession(), and the usual reason to delete an access is that logging in with it does not work - the same reason the constructor skips building a connection for this action: load() refuses a bank whose FinTS address is unusable, and that is exactly the access somebody wants rid of. Refs: OP#635
…k PIN With the default file driver a session is a PHP-serialized file under storage/framework/sessions/. During a FinTS dialog it carries the online-banking PIN and the bank's session state - FintsConnectionHandler keeps the password in the session on purpose, so it never reaches the database - and without encryption those sit there in cleartext for the session's lifetime. Set in .env.example with the reason next to it, and documented for existing instances, which have SESSION_ENCRYPT=false pinned and have to switch by hand. The price is stated too: turning it on invalidates every open session, so everyone logs in once more and a bank dialog in flight is lost. config/session.php is deliberately not shipped to flip the framework default; that would put another framework config file in the repo to keep in sync. The trade-off is that existing instances only get this once someone edits .env. Refs: OP#630
Deleting a bank access, the HTTPS check on a bank's FinTS address, and the SESSION_ENCRYPT recommendation for existing instances. Refs: OP#635
The bank dropdown held nothing but real banks, so the browser preselected the first of the ~4000 entries. Filling in the two text fields and submitting without touching the dropdown created the access at that bank without anyone noticing. The select now carries a title, which the selectpicker turns into a placeholder option with an empty value - the same way FormTemplater does it. An empty BLZ is refused with its own message rather than normalised into "00000000" and reported as an unknown BLZ. The legacy test helpers move to tests/Pest.php so a second FinTS test can use them, joined by legacyHtml(): legacy pages travel inside an iframe srcdoc, so their markup arrives htmlspecialchars-encoded and an assertion on a tag or an attribute would never match the raw response body.
saveAction() cleared the action-scope session key whenever an action stopped needing a TAN - which also covers the moment submitTan() has just completed one. getStatements() then found no scope to match the finished action against, discarded it as belonging to some other request, and started a brand-new statement request that asked for another TAN. Against a bank that requires one, an import could never finish. action-scope now only gets cleared when saveAction() drops the action outright (fresh start, or the existing "belongs to something else" guard); a completed action that is still handed in keeps its scope so getStatements() can resume it and clear the scope itself once it has returned the statement. Refs: OP#608 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Some banks use a "decoupled" approval procedure (e.g. pushTAN-Freigabe): the user confirms in their banking app instead of typing a TAN, and the library refused to submit a TAN for such a mode - which the interrupted- dialog flow had no way to handle and just reported as unsupported. The TAN-input page is now replaced by a confirmation page for a decoupled mode: it shows the bank's challenge text and a button that makes exactly one call to the bank asking whether the approval has arrived (FinTs:: checkDecoupledSubmission()). Deliberately minimal - no JavaScript, no timer, no automated polling - the user taps approve in the banking app and then presses the button here. A per-credential pacing state (seeded in saveAction(), consumed in the new confirmDecoupledTan()) keeps the app from asking the bank before the earliest allowed check, and stops it once the bank's attempt limit is used up. The actual HKTAN round trip against a decoupled procedure could not be covered by the tests added here (they mock Fhp\FinTs) and needs manual verification against a real bank access that offers one. Refs: OP#613 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The TAN prompt and the statement import share the route
konto/credentials/{id}/{shortIban}, and the page itself says nothing about
the account it belongs to - so somebody entering a TAN had no way to see
which account they were importing.
The trail now carries the account between the account list and
"Aktualisieren", resolved from the shortened IBAN in the URL. An account
this installation has not registered keeps the shortened IBAN as its label;
the bank lists accounts we do not know, so that is a normal state here.
Refs: OP#613
The breadcrumb now carries it, but the page under it still did not: a TAN prompt for a statement import looked exactly like one for any other account, and both TAN pages are drawn from the exception handler in render(), so neither knew what it was asking about. Both now say which account the pending import belongs to. The full IBAN comes from the account we already know rather than from the bank access - resolving it there would fetch the SEPA account list, i.e. talk to the bank in the middle of drawing a TAN prompt. Routes without an account (login, picking a TAN mode) stay silent instead of inventing one. Refs: OP#613
The "als .zip" button on the booking history ended in an error page. Two
things were wrong. Content-Length stat'ed the download name ('HHA.zip')
instead of the temp file the archive was written to, so PHP raised a warning
that Laravel turns into an ErrorException. Behind that, the handler echoed
the archive and returned - LegacyController hands whatever a legacy page
buffered to the app layout, so the binary would have been wrapped in HTML
rather than offered as a download.
The handler now throws the finished response out to the controller, the way
a redirect and a JSON reply already leave a legacy page, and the controller
drops the buffer and returns it untouched. An archive that cannot be built
reports through LegacyDieException instead of printing "Error :(" into the
page.
Refs: Ticket#715749
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
translations:check exits 1 on an empty value, so the workflow has been red on
this branch and on main alike. konto.csv-draganddrop-light-text was defined
but never referenced and budget-plan carried an '' => '' leftover - both are
gone. konto.hint.transaction.comment held a single space but is reached from
the manual CSV import via __("konto.hint.transaction.$attr"), so it keeps its
key and gets the example text its siblings have.
Refs: Ticket#715749
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
echoCSV() ended in exit(), which only worked by accident: the buffer and the queued header() calls happened to be flushed at shutdown, before Laravel could wrap them. Anything a page had already printed went out in front of the CSV, and the download could not be tested at all - exit() takes the test runner down with it. It now throws the response the way the zip export does, so the same buffer-discarding path applies. The Content-Type carries the charset the body is actually converted to; Laravel otherwise labels a text/* response utf-8 while these bytes are WINDOWS-1252. Refs: Ticket#715749 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The button already sits on the budget plan page and points at the same export, which takes the plan id anyway - so the booking history, where one goes to look at the bookings of a budget year, had no reason to omit it. Behind the "datev" setting, as there. It is rendered plain rather than disabled-for-non-finance like its twin: the budget plan page has no group restriction, while the booking routes require ref-finanzen - the same group the DATEV download itself is gated on. Refs: Ticket#715749 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
Patch release from a review of the FinTS bank-import path (controller, connection handler, the REST twin, the routing/CSRF wiring), checked against what
nemiah/php-fintsactually offers. One commit and one work package per fix.Fixed
import-transactionsbreadcrumb added.getAlnum()stripped every special character and umlaut before the PIN reached the bank. A mangled PIN is indistinguishable from a wrong one, and three wrong ones lock the online-banking access — this could cost a treasurer their bank access without them ever mistyping. Same stripping hit the TAN and two free-text labels.customer_ref(the SEPA end-to-end id), which MT940 routinely leaves empty — so the lookup failed routinely, and a failed lookup had no branch of its own: months of bookings were re-inserted while the UI reported success. Also repaired the saldo check against the stored value, which was dead code (guarded by$tryRewind === false, by which point the stored value had been overwritten).convertToCent()double-negated negative amounts. Latent today — see the note below — but fixed before OP#605 because repairing that guard makes it live. Covered by a new Pest test.Typed property must not be accessed before initializationon every click instead of offering re-login.UnexpectedResponseExceptionextendsRuntimeExceptionwhileServerExceptionextendsException, and nearly every catch listed only the latter. Decoupled TAN modes now report that they are unsupported (OP#613) rather than erroring.konto_id.short/ibanuniqueness unenforced, and the method treated any POST — including the TAN prompt — as a submit. Plus two crashes (unregistered account,NULL sync_fromdoingclone false).VerifyCsrfToken; forms shipped anoncefield that onlyRestHandlerever checked. Forced login attempts (→ bank lockout), credential creation and registering an arbitrary account were all reachable cross-site. Verified inrender()now, scoped to the FinTS pages rather than flipping the middleware for the whole legacy group.-dev(.binds tighter than?:).XML_RPC2_Client, embedded credentials in the URL,sslverify => false) and 7 REST endpoints that died with "Call to undefined method". 826 deletions, no callers.Note on the saldo sign bug
The consequence first suspected — "incremental import aborts for an overdrawn account" — does not occur, and the counter-intuition was right: MT940 carries unsigned magnitudes plus a separate credit/debit mark, so no bank value reaches the buggy branch, and the one caller that does is write-only. The arithmetic defect is real and is fixed, but it is latent; it would only have gone live once the dead saldo guard was repaired, which is exactly what OP#605 does. Hence the ordering.
Deferred (filed, no code here)
OP#613 decoupled TAN support (many banks now offer little else) · OP#614 persist state per credential in the DB · OP#615 CAMT statements · OP#616 unbooked transactions +
GetBalancecross-check · OP#617 maintainable bank registry (konto_bankhas no seeder or UI, and its URL is not forced to HTTPS) · OP#618 delete-credentials / change-password actions (routes and a UI icon exist, the methods never did) · OP#620 integration test forsaveStatements()· OP#621 state-changing GETs → POST.Verification
composer lint(PHPStan): no errorscomposer fix(rector + pint): clean, no unrelated churntests/Pest/Legacy/FintsAmountConversionTest.php(14 assertions, incl. the float-drift case)saveStatements()control flow simulated across 6 scenarios (empty account, normal incremental, restated bank value, tampered stored saldo, gap between statements,sync_untilstop) — an in-repo integration test needs legacy-DB fixtures that do not exist yet, filed as OP#620🤖 Generated with Claude Code