Payment: a second checkout can open while the first is still live

Problem/Motivation

A step configured to go straight to the gateway discards the payment already pending on it and starts a new one, so a payer can be charged twice. Plus a settle step that checks the amount against the hold without checking the currency.

Found by a complete file-by-file audit of the whole module at commit bc205f4f: 1130 tracked files, 1126 read in full, 4 binary assets recorded as skipped. Every linter was already clean at that commit, so none of this is something a tool reports: phpcs Drupal over 1008 files and DrupalPractice over 814 both at zero, PHPStan level 5 with no errors, cspell against the CI configuration over 1061 files with no issues, eslint with no errors, stylelint with no problems, composer validate, and the per-project translation check reporting every string translated in the catalog of the project that ships it.

Findings

High

1. A skip-landing pay click discards the live pending payment and opens a second provider checkout (modules/orchestra_payment/src/Plugin/Interaction/PaymentInteraction.php line 530, correctness). When the pay click carries no signed displayed amount ($shown === NULL, the skip_landing path), line 530 overwrites the found $pending with NULL, so the deferred reuse check at 606-608 always calls keepReusablePayment(NULL, ...) and startPayment() always creates a brand-new payment. A payer on a step configured to go straight to the gateway who returns to the step before the provider has answered is handed a second kessai payment and a second live provider checkout while the first is still completable, so they can be charged twice; the second capture finds the token already consumed and is recorded only at debug level, so nothing raises it for an operator. Fix: Leave $pending alone when there is no shown amount (if ($shown !== NULL) { $pending = keepReusablePayment($pending, $shown); }), and add a kernel test for the skip-landing second arrival asserting exactly one payment exists.

Medium

1. The settle step checks the capture amount against the hold without checking the currency (modules/orchestra_payment/src/Plugin/TaskType/SettleTask.php line 152, correctness). execute() compares the re-resolved captureAmount with getHoldAmount() and captures it, never comparing the payable's currency with the payment's, though the settle node's resolver is configured independently of the payment node's. A run whose settle-step resolver prices in a different currency captures that number in the authorized currency, and neither the incident guard nor the log says anything. Fix: Treat a currency mismatch exactly as the over-hold case: an incident naming both currencies, before the bccomp against the hold.

Low

1. The $timeout docblock promises a window the payment reuse path does not deliver (modules/orchestra_payment/src/Plugin/Interaction/PaymentInteraction.php line 1038, documentation). startPayment()'s @param says the timeout is the same value the opening event carried so the two cannot drift, but it is only passed to create(); a reused pending payment keeps the expires stamp from the earlier click while the fresh window is announced to subscribers. A domain subscriber that sizes its hold from the opening event holds its subject past the reused payment's real deadline, and the invariant the docblock states is not the one the code keeps. Fix: Re-stamp the reused payment's expires time from the timeout, or narrow the docblock.
2. CheckoutRefuser::getAskedAmounts() is dead, misnamed, and its docblock claims a distinction no test makes (modules/orchestra_payment/tests/modules/orchestra_payment_route_test/src/CheckoutRefuser.php line 69, tests). The asked counter and its accessor are never read by any test, the accessor is named 'amounts' while returning a call count, and the class docblock asserts a capability nothing exercises. A maintainer believes 'nothing objected' and 'nobody was asked' are told apart somewhere and does not add the assertion that would tell them apart. Fix: Assert in the refusal test that the subscriber was asked exactly once and rename it getAskedCount(), or delete the counter, the accessor and the docblock sentence.

Proposed resolution

One merge request for this issue, one commit per finding, each commit naming the finding it closes. Every defect gets a test that fails against the unfixed code, so the ground is closed and the next audit pass has to look somewhere new.

Remaining tasks

• Fix each finding above, with its test.
• Run every linter the pipeline runs, and the impacted test classes, before pushing.
• Play the next-major lane by hand, since its composer job is manual and a skipped lane reads as green.

User interface changes

Named per finding above; no new screens.

API changes

Pre-1.0, so a signature is changed where the better shape needs it rather than preserved; each is named in its finding.

AI-Generated: Yes (Claude Code was used to run this audit and to draft this issue summary. I reviewed the findings against the source myself before posting; the code and tests will follow on the merge request.)