diff --git a/.planning/phases/18-payment-gateway/18-REVIEW-DISPOSITION.md b/.planning/phases/18-payment-gateway/18-REVIEW-DISPOSITION.md index a98ee37..dbd4ba9 100644 --- a/.planning/phases/18-payment-gateway/18-REVIEW-DISPOSITION.md +++ b/.planning/phases/18-payment-gateway/18-REVIEW-DISPOSITION.md @@ -1,19 +1,66 @@ --- phase: 18 review: 18-REVIEW.md -titles: json findings: -open: 0 -total: 0 -unparsed: 48 -recorded: 2026-10-13T16:00:48.989Z +open: 48 +total: 48 +unparsed: 0 +recorded: 2026-10-13T16:01:01.576Z --- # Phase 18: Code Review Disposition +Written by hand: the review uses part-prefixed IDs (A- for framework, pgstripe and layer; B- for sm-paymentgateway-plugin), which the gate parser does not match. All code was already pushed and tagged v0.1.25 when reviewed. + | Finding | Severity | Disposition | Source | |---------|----------|-------------|--------| +| A-CR-01 | critical | open | `payment_intent.payment_failed` rejects a payment the customer can still complete, so a paid order stays Rejected | +| A-CR-02 | critical | open | A "paid" signal for a Rejected or Canceled payment is dropped silently (no log, no refund, Checkout Session left open after a cancel) | +| A-CR-03 | critical | open | The admin money input reads a comma as the decimal separator in every locale, so `1,234` in `en` is saved as `1.234` | +| A-WR-01 | warning | open | Webhook verification, refunds and Stripe lookups always use the currently active mode's keys; the payment does not record its own mode | +| A-WR-02 | warning | open | Accepting an event does not check the event's amount, currency or session/intent id against the payment | +| A-WR-03 | warning | open | `stripeAmount` silently rounds amounts for non-two-decimal currencies | +| A-WR-04 | warning | open | The refund has no idempotency key; a Stripe success followed by a rollback leaves the payment stuck | +| A-WR-05 | warning | open | An unauthenticated GET makes outbound Stripe calls while the payment row lock is held | +| A-WR-06 | warning | open | Write-only secrets are stored untrimmed, and a whitespace-only value hides the env fallback | +| A-WR-07 | warning | open | `moneyColumnType` accepts any Scanner/Valuer struct, including `sql.NullFloat64` and `sql.NullString` | +| A-WR-08 | warning | open | A money field without `max` has no upper bound, so overflow is an opaque 500 instead of a 422 | +| A-WR-09 | warning | open | The layer navigates to any `redirect_url` without checking the scheme | +| A-WR-10 | warning | open | The layer's `formatMoney` throws on an unknown currency code and breaks the whole render | +| A-IN-01 | info | open | An authentic event for a payment that is not yet visible is answered 200 "ignored", so Stripe never retries it | +| A-IN-02 | info | open | `checkout.session.expired` is not handled | +| A-IN-03 | info | open | Method-backed dropdowns dispatch on the field name and ignore the declared method; submitted values are not checked against the options | +| A-IN-04 | info | open | The browser return answers an opaque 500 instead of redirecting when `accept` fails | +| A-IN-05 | info | open | Money values may be negative, and fields cannot declare a minimum | +| A-IN-06 | info | open | Enabling the operator without a secret key turns every checkout into a 500 | +| A-IN-07 | info | open | `readWinterFields` reports every read error as "File not found" | +| A-IN-08 | info | open | `isZeroMoney` treats `""`, `"-"` and `"."` as zero | +| B-CR-01 | critical | open | Guest access to an order is forgeable. `guest_code` is a deterministic hashids of the order id, and legacy rows used the public salt `KEIOS.PAYMENTGATEWAY` | +| B-CR-02 | critical | open | The PaymentAccepted bridge writes `successful_payment_id` before checking `Can(Pay)`, overwriting the real successful payment on Paid, Canceled or Finished orders (PHP did not persist it) | +| B-CR-03 | critical | open | `default_currency` can be changed at any time, which silently re-bases every stored exchange ratio and native amount | +| B-CR-04 | critical | open | The Fixer access key is written to the application log on any transport error | +| B-WR-01 | warning | open | Money arithmetic wraps int64 silently (`Add`, `Sub`, `Neg`, `FromMinor`, `Amount4.Scan(int64)`), and `Fits(MinInt64)` returns true | +| B-WR-02 | warning | open | The quote cart accepts an unbounded count, and `Mul` then panics with `ErrOverflow` in `orders.Service.Save` (500, a panic used as control flow) | +| B-WR-03 | warning | open | The customer API ignores `orders_enabled`. With orders off the bridge is a no-op, so `process` can be repeated and items edited after a payment exists | +| B-WR-04 | warning | open | The `RecordExternal` contract cannot be met for a disabled operator. The doc says "need not be enabled", but `Send` and `Accept` are guarded by `enabled` | +| B-WR-05 | warning | open | Operator side effects run before the state guard and inside the DB transaction, with no idempotency; lock order is inverted between the order and notification paths | +| B-WR-06 | warning | open | `POST /cancel` on an AwaitingPayment order with an in-flight online payment cancels the order and leaves the payment Sent; a later Accept charges the customer for a canceled order | +| B-WR-07 | warning | open | Payment rows are canceled from unlocked reads, and transitions are blind `UPDATE`s without a state predicate | +| B-WR-08 | warning | open | The native amount is never checked against `numeric(12,4)` in `Make` and `Save`, nor against `numeric(10,4)` in `RecordExternal` | +| B-WR-09 | warning | open | `import-winter` stores `mailer_from_email` and `admin_email` without the e-mail validation the settings form enforces | +| B-WR-10 | warning | open | Negative costs are not refused for shipping methods and quote items, and `add_quoteable` bypasses the quote cart's non-negative rule | +| B-IN-01 | info | open | The route comment is wrong; the API paths have three segments and also match the notification pattern | +| B-IN-02 | info | open | Per-user API throttling never applies | +| B-IN-03 | info | open | `Allocate` accepts negative ratios and can hand out more than the amount | +| B-IN-04 | info | open | Four-place ratios lose most of their precision for low-value currencies | +| B-IN-05 | info | open | The `golem15-paymentgateway-label` element never re-renders when `value` changes | +| B-IN-06 | info | open | `IsGuestEmail` matches any address that contains `guest@` | +| B-IN-07 | info | open | `process` returns the operator's raw message to an anonymous caller and turns every failure into a 422 | +| B-IN-08 | info | open | `readWinterFields` reports every read error as "File not found" | +| B-IN-09 | info | open | The stats sum is cast to `numeric(14,4)` | +| B-IN-10 | info | open | The guest token cache keeps a nil verifier forever and is never evicted | +| B-IN-11 | info | open | The notification route depends on `net/http/httptest` in production and replays the handler after commit | +| B-IN-12 | info | open | The quotes controller has no `FormExtendQuery` | +| B-IN-13 | info | open | A huge `page` overflows the offset in `GET /orders` | Dispositions: `open` (recorded, not yet triaged), `fixed`, `skipped`, `deferred`. -Set `deferred` by hand and put the reason in the Source cell; both are preserved. A `|` in the reason is kept as prose and escaped on the next run. -Re-running the gate keeps every row it can. A row the current review no longer reports is kept and its Source cell flagged, so a finding does not leave this record silently. ONE exception: when a finding id is REUSED by a different finding, the earlier decision cannot keep a row — the id is taken — and it is dropped. A RECORDED decision (anything but `open`) is named on the console when that happens; a row still at `open` is replaced silently, because `open` records no decision to lose. diff --git a/.planning/phases/18-payment-gateway/18-REVIEW.md b/.planning/phases/18-payment-gateway/18-REVIEW.md new file mode 100644 index 0000000..d554cf2 --- /dev/null +++ b/.planning/phases/18-payment-gateway/18-REVIEW.md @@ -0,0 +1,630 @@ +--- +phase: 18-payment-gateway +reviewed: 2026-10-13T16:00:36Z +depth: standard +files_reviewed: 106 +files_reviewed_list: + - ../sm-pgstripe-plugin/admin.go + - ../sm-pgstripe-plugin/browser_return.go + - ../sm-pgstripe-plugin/client.go + - ../sm-pgstripe-plugin/config/config.yaml + - ../sm-pgstripe-plugin/console/import_winter.go + - ../sm-pgstripe-plugin/models/settings/fields.yaml + - ../sm-pgstripe-plugin/models/settings.go + - ../sm-pgstripe-plugin/operator.go + - ../sm-pgstripe-plugin/plugin.go + - ../sm-pgstripe-plugin/scripts/winter-export-settings.sql + - ../sm-pgstripe-plugin/updates/202610130001_create_golem15_pgstripe_settings.go + - ../sm-pgstripe-plugin/webhook.go + - admin/src/app/money.ts + - admin/src/components/form/fields/MoneyField.vue + - admin/src/components/form/fields/PasswordField.vue + - admin/src/components/form/formContext.ts + - admin/src/components/form/PreviewField.vue + - admin/src/components/form/RecordActions.vue + - admin/src/components/form/registry.ts + - admin/src/components/list/CellValue.vue + - admin/src/components/list/DataTable.vue + - admin/src/views/PreviewView.vue + - admin/src/views/SettingsFormView.vue + - modules/cabana/admin_openapi.go + - modules/cabana/crud.go + - modules/cabana/field_dependent.go + - modules/cabana/field_money.go + - modules/cabana/form_schema.go + - modules/cabana/http.go + - modules/cabana/list_schema.go + - modules/cabana/registry.go + - modules/cabana/relation_child.go + - modules/cabana/relation_form.go + - modules/cabana/schema_types.go + - modules/cabana/settings.go + - modules/cabana/settings_secret.go + - modules/pact/capabilities.go + - scripts/check-phase18.sh + - ../vue-paymentgateway-layer/app/composables/useOrders.ts + - ../vue-paymentgateway-layer/app/composables/useOrder.ts + - ../vue-paymentgateway-layer/app/composables/usePaymentGatewayApi.ts + - ../vue-paymentgateway-layer/app/utils/checkout.ts + - ../vue-paymentgateway-layer/app/utils/client.ts + - ../vue-paymentgateway-layer/app/utils/money.ts + - ../vue-paymentgateway-layer/app/utils/poller.ts + - ../vue-paymentgateway-layer/app/utils/states.ts + - ../vue-paymentgateway-layer/testing/load.ts + - ../sm-paymentgateway-plugin/admin.go + - ../sm-paymentgateway-plugin/admin_navigation.go + - ../sm-paymentgateway-plugin/admin_permissions.go + - ../sm-paymentgateway-plugin/admin_settings.go + - ../sm-paymentgateway-plugin/api/actions.go + - ../sm-paymentgateway-plugin/api/auth.go + - ../sm-paymentgateway-plugin/api/errors.go + - ../sm-paymentgateway-plugin/api/items.go + - ../sm-paymentgateway-plugin/api/methods.go + - ../sm-paymentgateway-plugin/api/order_json.go + - ../sm-paymentgateway-plugin/api/orders.go + - ../sm-paymentgateway-plugin/assets/js/quote-cart.js + - ../sm-paymentgateway-plugin/boot.go + - ../sm-paymentgateway-plugin/cart/legacy.go + - ../sm-paymentgateway-plugin/cart/v2.go + - ../sm-paymentgateway-plugin/console/import_winter.go + - ../sm-paymentgateway-plugin/controllers/admin_registry.go + - ../sm-paymentgateway-plugin/controllers/base.go + - ../sm-paymentgateway-plugin/controllers/orders.go + - ../sm-paymentgateway-plugin/controllers/orders_partials.go + - ../sm-paymentgateway-plugin/controllers/payments.go + - ../sm-paymentgateway-plugin/controllers/quoteables.go + - ../sm-paymentgateway-plugin/controllers/quotes_cart.go + - ../sm-paymentgateway-plugin/controllers/quotes.go + - ../sm-paymentgateway-plugin/controllers/shippings.go + - ../sm-paymentgateway-plugin/currency/converter.go + - ../sm-paymentgateway-plugin/currency/fixer.go + - ../sm-paymentgateway-plugin/currency/nbp.go + - ../sm-paymentgateway-plugin/events/mailers.go + - ../sm-paymentgateway-plugin/events/recipients.go + - ../sm-paymentgateway-plugin/field_types.go + - ../sm-paymentgateway-plugin/fsm/fsm.go + - ../sm-paymentgateway-plugin/mail.go + - ../sm-paymentgateway-plugin/models/hashids.go + - ../sm-paymentgateway-plugin/models/labels.go + - ../sm-paymentgateway-plugin/models/mailer_settings.go + - ../sm-paymentgateway-plugin/models/null_id.go + - ../sm-paymentgateway-plugin/models/order.go + - ../sm-paymentgateway-plugin/models/payment.go + - ../sm-paymentgateway-plugin/models/quoteable.go + - ../sm-paymentgateway-plugin/models/settings/fields.yaml + - ../sm-paymentgateway-plugin/models/settings.go + - ../sm-paymentgateway-plugin/models/settings/mailer_fields.yaml + - ../sm-paymentgateway-plugin/models/shipping.go + - ../sm-paymentgateway-plugin/money/amount.go + - ../sm-paymentgateway-plugin/money/money.go + - ../sm-paymentgateway-plugin/money/stored.go + - ../sm-paymentgateway-plugin/operators/operator.go + - ../sm-paymentgateway-plugin/orders/bridge.go + - ../sm-paymentgateway-plugin/orders/service.go + - ../sm-paymentgateway-plugin/payments/notification.go + - ../sm-paymentgateway-plugin/payments/service.go + - ../sm-paymentgateway-plugin/plugin.go + - ../sm-paymentgateway-plugin/routes.go + - ../sm-paymentgateway-plugin/schedule.go + - ../sm-paymentgateway-plugin/support/format.go + - ../sm-paymentgateway-plugin/support/locale.go + - ../sm-paymentgateway-plugin/updates/202610130007_create_golem15_paymentgateway_mailer_settings.go + - ../sm-paymentgateway-plugin/views/mail/layouts/paymentgateway.htm +findings: + critical: 7 + warning: 20 + info: 21 + total: 48 +status: issues_found +parts: [a, b] +note: Reviewed after push and tag v0.1.25 (advisory). Paths are relative to summercms.go; ../ marks sibling repos. Part B finding paths are relative to sm-paymentgateway-plugin. +--- + +# Phase 18 code review + +Two parallel standard-depth reviews over 106 source files. Part A: framework (cabana, pact, admin SPA), sm-pgstripe-plugin and vue-paymentgateway-layer. Part B: sm-paymentgateway-plugin. + +| Part | Critical | Warning | Info | +|---|---|---|---| +| A | 3 | 10 | 8 | +| B | 4 | 10 | 13 | + +# Part A + +## Phase 18: Code Review Report (part A) + +**Reviewed:** 2026-10-13T12:00:00Z +**Depth:** standard +**Files Reviewed:** 47 +**Status:** issues_found + +Paths without `../` are relative to `summercms.go/`. Paths with `../` are the sibling repos `sm-pgstripe-plugin` and `vue-paymentgateway-layer`. + +## Summary + +The review covered three areas: the cabana money field, write-only settings secrets and method-backed dropdowns; the Stripe operator (webhook signatures, idempotency, browser return, refunds, amount conversion); and the Nuxt layer's auth token and redirect handling. Signature verification itself is sound: `webhook.ValidatePayload` runs on the raw bytes before any lookup, the browser return never accepts from request input, and secrets never reach JSON (`json:"-"`, `withoutSecrets`, redacting `MarshalJSON`). The auth token goes only into an `Authorization: Bearer` header and never into a query or cookie. + +The serious problems are in the payment state handling around Stripe. A `payment_intent.payment_failed` event moves the payment to a final Rejected state even though Stripe lets the customer retry the same PaymentIntent or Checkout Session. If the customer then pays, Stripe takes the money and the local payment stays rejected. `accept` also drops any "paid" signal for a Rejected or Canceled payment without a log line, so these cases cannot even be found afterwards. The admin money input reads a comma as the decimal separator in every locale, so an English-locale admin who types `1,234` saves `1.234`. Smaller issues: key selection depends on the active mode, amounts are rounded for non-two-decimal currencies, the refund has no idempotency key, and Stripe is called while the row lock is held. + +Tracing note: the Stripe operator runs inside `sm-paymentgateway-plugin/payments/notification.go`. That file was read for context (lock, ErrNotificationIgnored → 200, Enabled gate) but is outside this part's scope. + +## Critical Issues + +### A-CR-01: `payment_intent.payment_failed` rejects a payment the customer can still complete, so a paid order stays Rejected + +**File:** `../sm-pgstripe-plugin/webhook.go:161-162` (also `webhook.go:136`, doc comment) +**Issue:** `handleEvent` maps `payment_intent.payment_failed` to `o.reject`. In Stripe this event fires on every failed attempt, for example a card decline inside Checkout or a failed `confirmCardPayment` in PaymentIntent mode. The PaymentIntent returns to `requires_payment_method`, and the customer can retry on the same Checkout page or with the same client secret. Reject is final (`payments/machine.go`: Rejected is in `PaymentFinalStates`, and Accept is allowed only from Created or Sent). So the sequence is: decline, webhook, payment Rejected (order mail and events fire), customer retries, payment succeeds, `checkout.session.completed` or `payment_intent.succeeded` arrives, `Can(Accept)` returns false, and the event is a silent no-op. Stripe has captured the money while the order is Rejected. The PHP operator (`Golem15\PGStripe\Operators\Stripe`) never rejected on a failure event. This behavior is new in the port. Both PaymentIntent metadata (`PaymentIntentData.Metadata` at `operator.go:181-183`) and `paymentMetadata` carry `payment_uuid`, so the failure event always resolves to the payment. +**Fix:** Do not treat `payment_intent.payment_failed` as terminal. Log it and leave the payment Sent. Reject only on terminal signals: `checkout.session.async_payment_failed`, `checkout.session.expired`, and `payment_intent.canceled`. +```go +case EventPaymentIntentPaymentFailed: + // Not terminal: the intent returns to requires_payment_method and the + // customer may retry on the same Checkout page or card form. + o.logger.Warn("golem15.pgstripe: payment attempt failed", "event", ev.ID, "payment", pc.Payment.ID) + return nil +``` + +### A-CR-02: A "paid" signal for a Rejected or Canceled payment is dropped silently (no log, no refund, Checkout Session left open after a cancel) + +**File:** `../sm-pgstripe-plugin/operator.go:439-442`, `../sm-pgstripe-plugin/browser_return.go:60-69` +**Issue:** `accept` returns `nil` when `Can(Accept)` is false. That is correct for a replay of an already Accepted payment. It also covers a payment that Stripe reports as paid while the local state is Rejected (A-CR-01) or Canceled. The customer API allows `cancel` on an `AwaitingPayment` order (`api/order_json.go:162`, `api/actions.go:366-384`), and that cancels the Sent payment. The Stripe Checkout Session the customer has open stays payable for up to 24 h, because nothing expires it. If the customer pays it, the webhook and the browser return both reach `accept`, see `Can=false`, and return 200 with no log line. The money is captured, the order is Canceled, and nothing records it. +**Fix:** In `accept`, tell a replay (`pc.Payment.State == Accepted`) apart from a paid-at-Stripe payment in any other final state. For the second case, log at Error level with the payment id, event id and PaymentIntent id, and fire an event or admin notice (or refund automatically). Also expire the stored Checkout Session (`client.V1CheckoutSessions.Expire`) when a Stripe payment is canceled, through a `PaymentCanceled` listener in the plugin. +```go +ok, err := svc.Can(ctx, pc.Tx, pc.Payment, pgmodels.PaymentTransitionAccept) +if err != nil { return err } +if !ok { + if pc.Payment.State != pgmodels.PaymentStateAccepted { + o.logger.Error("golem15.pgstripe: Stripe reports the payment paid but its state refuses Accept; manual refund needed", + "payment", pc.Payment.ID, "state", pc.Payment.State, "intent", extra[DataPaymentIntentID]) + // optionally: record the data and fire a dedicated event + } + return nil +} +``` + +### A-CR-03: The admin money input reads a comma as the decimal separator in every locale, so `1,234` in `en` is saved as `1.234` + +**File:** `admin/src/app/money.ts:39-51` (used by `admin/src/components/form/fields/MoneyField.vue:169-185`) +**Issue:** `parseMoneyInput` treats a single `,` or `.` as the decimal separator whatever the locale. In `en`, `,` is the grouping separator, so an administrator who types `1,234` (one thousand two hundred thirty-four) gets the canonical value `"1.234"`. The default scale is 4, so the result is not `too_precise`. It is sent, accepted by the server, and saved as an amount 1000 times too small, with no error. Grouping is documented as `invalid`, but a single group separator is never detected. The field then shows the typed text, not the parsed value, until the page reloads. +**Fix:** Make the parser locale-aware. Accept the locale's decimal separator (`decimalSeparator(locale)`) and `.`. Refuse the locale's grouping separator, or refuse any `d,ddd` pattern with exactly three fraction digits when `,` is not the locale's decimal separator, as `invalid`. +```ts +export function parseMoneyInput(text: string, scale: number, decimal = '.'): MoneyParseResult { + // ... + const allowed = decimal === ',' ? /[.,]/g : /[.]/g + if (decimal !== ',' && body.includes(',')) return { value: null, error: 'invalid' } + // ... +} +``` + +## Warnings + +### A-WR-01: Webhook verification, refunds and Stripe lookups always use the currently active mode's keys; the payment does not record its own mode + +**File:** `../sm-pgstripe-plugin/client.go:51-65`, `../sm-pgstripe-plugin/operator.go:286, 342`, `../sm-pgstripe-plugin/browser_return.go:87` +**Issue:** `ModeKeys` chooses the live or test triple from the `test_mode` setting at call time. After `test_mode` is toggled, webhooks for payments created under the other mode fail verification with a 403, so Stripe retries for 3 days and then gives up and the payments are never accepted. Refunds and browser-return lookups for those payments use the wrong secret key and fail with "No such payment_intent". Nothing in `payments.data` records the mode used at Purchase. +**Fix:** In `Purchase`, store the mode (`stripe_mode: live|test`, or Stripe's `livemode`) in `payments.data`. In `Refund` and `paidAtStripe`, choose the keys from that stored mode. In `VerifyNotification`, try both configured signing secrets (or decode `livemode` first and pick the matching secret), so a mode switch does not lose in-flight events. + +### A-WR-02: Accepting an event does not check the event's amount, currency or session/intent id against the payment + +**File:** `../sm-pgstripe-plugin/webhook.go:149-160` +**Issue:** `checkout.session.completed` and `payment_intent.succeeded` accept the payment based only on `metadata.payment_uuid` or `client_reference_id`. `amount_total` / `amount_received`, `currency`, and whether `data.object.id` equals the stored `stripe_checkout_session_id` / `payment_intent_id` are never compared. If a second session exists for the same payment, or the order total changed after Purchase, a lower payment still accepts the order. Any other integration on the same Stripe account that reuses the `payment_uuid` metadata key could do the same. +**Fix:** Decode `amount_total`, `amount_received` and `currency` in `EventObject`. Accept only when the amount equals `stripeAmount(paymentTotal(pc))`, the currency matches, and the object id matches the stored id (when one is stored). Otherwise log at Error level and leave the payment unchanged. + +### A-WR-03: `stripeAmount` silently rounds amounts for non-two-decimal currencies + +**File:** `../sm-pgstripe-plugin/operator.go:562-588` +**Issue:** Zero-decimal currencies use `(cents + 50) / 100`, so a JPY total of `1000.50` is charged as 1001 with no error. Three-decimal currencies use `Minor()*10`, which rounds a scale-4 `1.2345 KWD` to two decimals first (charged 1.230 or 1.240). Stripe's currency docs also list UGX as a special case that must be sent as a two-decimal value (×100), but the code puts UGX in `zeroDecimal`, which would charge 1/100 of the amount. Please check this against Stripe's current currency table before relying on it. +**Fix:** Refuse an amount that the currency cannot represent exactly instead of rounding it (return an error when `cents%100 != 0` for zero-decimal currencies). Compute three-decimal amounts from the scale-4 units, and refuse when the last digit is not zero. Confirm the UGX, ISK, HUF and TWD rules against Stripe's current table, and add tests for each. + +### A-WR-04: The refund has no idempotency key; a Stripe success followed by a rollback leaves the payment stuck + +**File:** `../sm-pgstripe-plugin/operator.go:302-314` +**Issue:** `V1Refunds.Create` is called inside the payment service's transaction (`payments/service.go` Refund). If Stripe refunds and then `SaveData`, the `Refund` transition or an event listener fails, the transaction rolls back and the payment stays Accepted. The admin's next attempt sends a new refund request, which Stripe refuses with `charge_already_refunded`. The payment can then never reach Refunded through the UI, even though the money has been returned. `Purchase` has the same weakness: a session created before a rollback is orphaned. +**Fix:** Set a deterministic idempotency key, for example `params.SetIdempotencyKey("pgstripe-refund-" + pc.Payment.UUID)`. Also treat the Stripe error code `charge_already_refunded` as success (`operators.Complete()`). + +### A-WR-05: An unauthenticated GET makes outbound Stripe calls while the payment row lock is held + +**File:** `../sm-pgstripe-plugin/browser_return.go:60-69, 82-121`; `../sm-pgstripe-plugin/client.go:28-33` +**Issue:** `browserReturn` runs inside the route's `lagoon.Transaction`, after `SELECT ... FOR UPDATE` on the payment (and order). It calls `V1CheckoutSessions.Retrieve` / `V1PaymentIntents.Retrieve`, each with a 30 s timeout and up to 2 retries. Anyone holding the success URL (it travels in browser history and Referer) can repeat the GET to hold the lock and a database connection for tens of seconds at a time. That blocks the real webhook and every order action for that payment and uses up the connection pool. +**Fix:** Retrieve the Stripe object before taking the lock, or use a short dedicated timeout (for example 5 s, no retries) for the browser-return lookup. Then lock and apply only the state change. Also skip the Stripe call when the payment is already in a final state. + +### A-WR-06: Write-only secrets are stored untrimmed, and a whitespace-only value hides the env fallback + +**File:** `modules/cabana/settings_secret.go:121-138` +**Issue:** `liftSecretValues` drops only `""`. A pasted key with a leading or trailing space, or a string of only spaces, is encrypted and stored as given. The state then reads `stored`, and `ModeKeys` (`fallback`, `client.go:68-76`) prefers it over the `SUMMER_GOLEM15__PGSTRIPE__*` value. Every Stripe call then fails authentication, and every webhook fails its signature check (403), with no hint in the UI. +**Fix:** Trim the value in `liftSecretValues` and treat a blank result as "keep" (delete the key), or refuse it as a 422. The import already trims (`import_winter.go:261`), so the form would then match it. +```go +case string: + if strings.TrimSpace(v) == "" { delete(body, name); continue } + body[name] = strings.TrimSpace(v) +``` + +### A-WR-07: `moneyColumnType` accepts any Scanner/Valuer struct, including `sql.NullFloat64` and `sql.NullString` + +**File:** `modules/cabana/field_money.go:235-247` +**Issue:** The boot gate is meant to ensure the "never parsed into a float" guarantee, but it only checks for `*T implements sql.Scanner` and `T implements driver.Valuer`. `sql.NullFloat64` passes, so the decimal string is converted to `float64` on Fill and on save. That breaks the exactness the field promises. `sql.NullString` also passes, but it marshals as `{"String":..,"Valid":..}`. MoneyField shows it as empty, and an unedited save sends the object back, which `validateMoneyValue` rejects as `money.invalid` (422), so the record cannot be saved without retyping the amount. +**Fix:** Require that the type also round-trips as a JSON string (implements `json.Marshaler`/`encoding.TextMarshaler` producing a string), or keep an explicit allowlist (string, *string, and types implementing a `cabana.MoneyValue` marker interface). Refuse `sql.Null*` and any float-backed type at boot. + +### A-WR-08: A money field without `max` has no upper bound, so overflow is an opaque 500 instead of a 422 + +**File:** `modules/cabana/field_money.go:93-107, 180-185` +**Issue:** `max` is optional, and without it `validateMoneyValue` accepts any number of integer digits. On a `numeric(10,4)` column behind a string field, Postgres raises `numeric field overflow` inside the save transaction, and the admin gets the generic server error instead of `money.too_large` on the field. T-18-10 names the column ceiling as a mitigation, but nothing enforces declaring it. +**Fix:** Either require `max` on every money field, or derive a default from the scale and a conservative precision (for example `999999.9999` for scale 4). Alternatively, map a `numeric field overflow` (SQLSTATE 22003) to a 422 on the field. + +### A-WR-09: The layer navigates to any `redirect_url` without checking the scheme + +**File:** `../vue-paymentgateway-layer/app/utils/checkout.ts:134-138` (wired to `window.location.assign` in `Pg/Checkout.vue:51`) +**Issue:** `isRedirect` checks only that `redirect_url` is a non-empty string, and `deps.assign` hands it to `location.assign`. A `javascript:` or `data:` URL from any operator, whether misconfigured, written by a third party, or the result of a compromised upstream response, would run in the shop's origin, where the JWT cookie the auth hook reads is available. +**Fix:** Accept only `http:`/`https:` absolute URLs (or same-origin relative paths), and show `pg.error.process` otherwise. +```ts +export function isRedirect(r: PgProcessResult | null | undefined): r is { redirect_url: string } { + const u = (r as { redirect_url?: unknown } | null)?.redirect_url + if (typeof u !== 'string' || u === '') return false + try { return ['https:', 'http:'].includes(new URL(u, 'https://x.invalid').protocol) } catch { return false } +} +``` + +### A-WR-10: The layer's `formatMoney` throws on an unknown currency code and breaks the whole render + +**File:** `../vue-paymentgateway-layer/app/utils/money.ts:48-55` +**Issue:** `new Intl.NumberFormat(locale, {style:'currency', currency})` throws `RangeError` for a malformed code (legacy rows with `zł`, an empty-padded `"PLN "`, and so on). The admin version (`admin/src/app/money.ts:89-96`) catches this, but the layer does not. One bad row makes the SSR render of the order page or order list fail with a 500. +**Fix:** Wrap the formatter as the admin does: in the `catch`, format as `style: 'decimal'` and append the code. + +## Info + +### A-IN-01: An authentic event for a payment that is not yet visible is answered 200 "ignored", so Stripe never retries it + +**File:** `../sm-pgstripe-plugin/operator.go:373-381` +**Issue:** `knownPayment` reads outside the purchase transaction. If an event arrives before the transaction that created the payment commits (for example a PaymentIntent confirmed in `Purchase` that settles quickly), the event is answered 200 and Stripe never sends it again. +**Fix:** Answer a retryable status (for example 404 or 409) for a uuid that matches the uuid shape but is not found, and keep the 200 "ignored" for events without a uuid. + +### A-IN-02: `checkout.session.expired` is not handled + +**File:** `../sm-pgstripe-plugin/webhook.go:149-164` +**Issue:** An abandoned Checkout Session leaves the payment Sent and the order `AwaitingPayment` until someone acts on it by hand. The layer then polls 20 times and shows "slow". +**Fix:** Handle `checkout.session.expired` (when its id matches the stored session) with Cancel or Reject. + +### A-IN-03: Method-backed dropdowns dispatch on the field name and ignore the declared method; submitted values are not checked against the options + +**File:** `../sm-pgstripe-plugin/models/settings.go:90-95`; `modules/cabana/form_schema.go:434-447` +**Issue:** `options: getCheckoutModeOptions` is only checked to be an identifier. `DropdownOptions(field)` receives the field name, so a typo in the method name is never caught. The settings Put does not check the value against the served options, so only the model's `in:` rule protects `checkout_mode`. +**Fix:** Pass the method name to the provider (or check at boot that the provider answers a non-empty list for the field), and validate submitted dropdown values against the options. + +### A-IN-04: The browser return answers an opaque 500 instead of redirecting when `accept` fails + +**File:** `../sm-pgstripe-plugin/browser_return.go:66-68` +**Issue:** The customer returning from Stripe lands on a JSON 500 page. +**Fix:** Log the error and still redirect to `target`. The webhook will settle the payment. + +### A-IN-05: Money values may be negative, and fields cannot declare a minimum + +**File:** `modules/cabana/field_money.go:128-138`; `admin/src/app/money.ts:35-38` +**Issue:** Both parsers accept a leading minus, and `max` limits only the magnitude. A price or shipping-cost field cannot refuse negatives. +**Fix:** Add a `min` key (or `unsigned: true`) and check it on both sides. + +### A-IN-06: Enabling the operator without a secret key turns every checkout into a 500 + +**File:** `../sm-pgstripe-plugin/operator.go:127-131`; `../sm-pgstripe-plugin/models/settings.go:69-73` +**Issue:** `ErrNoSecretKey` is returned as a Go error, not as `StatusError`, and the settings save does not check that `enabled` has a key (column or env) for the active mode. +**Fix:** Return `operators.Failed("Stripe is not configured")`, and/or add a settings rule refusing `enabled` without a key. + +### A-IN-07: `readWinterFields` reports every read error as "File not found" + +**File:** `../sm-pgstripe-plugin/console/import_winter.go:338-341` +**Issue:** A permission error or a directory path is misreported. +**Fix:** `return nil, fmt.Errorf("%s: %w", file, err)`. + +### A-IN-08: `isZeroMoney` treats `""`, `"-"` and `"."` as zero + +**File:** `../vue-paymentgateway-layer/app/utils/money.ts:58-60` +**Issue:** The regex `^-?0*(\.0*)?$` matches strings that are not amounts, so a malformed value is shown as "free". +**Fix:** `/^-?0+(\.0+)?$/`. + +--- + +_Reviewed: 2026-10-13T12:00:00Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_ + + +# Part B + +## Phase 18: Code Review Report (part B: sm-paymentgateway-plugin) + +**Reviewed:** 2026-10-13T15:58:16Z +**Depth:** standard +**Files Reviewed:** 59 (all in `/media/nvme/dev/golem15/summercms.io/summercms/sm-paymentgateway-plugin`; paths below are relative to that repo) +**Status:** issues_found + +## Summary + +I reviewed the money package, the order and payment machines with their services and bridge, the notification route, the customer API (authorization, state guards, projection), the legacy cart reader, the currency providers, `paymentgateway:import-winter`, and the admin controllers. To check behaviour I also read the helpers these files call: `money/parse.go`, `cart/item.go`, `cart/cart.go`, `cart/details.go`, `support/hashids.go`, `support/guest_token.go`, `support/redactor.go` and the cabana file/upload handlers. For parity questions I compared against the PHP plugin in `examples/golem15-wintercms-starter/plugins/golem15/paymentgateway`. + +Several parts hold up: +- The notification route verifies the request before it looks up a payment, caps the body at 1 MiB and redacts its logs. +- The payment row is locked `FOR UPDATE` before `Can`. +- API mutations re-check their state guard on the locked order row. +- The order JSON projection is a real allowlist. +- Unknown and forbidden orders both answer 404. +- The legacy reader fails loudly on unknown types. + +The main problems: +- **Guest codes can be forged.** `guest_code` is derived deterministically from the order id, and legacy rows used a salt that is public in the PHP source. +- **The bridge diverges from PHP and corrupts `successful_payment_id`.** +- **The default currency can be changed after setup**, which silently re-bases every stored exchange ratio. +- **The Fixer access key leaks into logs.** + +Below the blockers there are integer-overflow and panic paths in money arithmetic, external operator calls made before the state guards and inside DB transactions, and the customer API ignoring `orders_enabled`. + +## Critical Issues + +### B-CR-01: Guest access to an order is forgeable. `guest_code` is a deterministic hashids of the order id, and legacy rows used the public salt `KEIOS.PAYMENTGATEWAY` + +**File:** `api/auth.go:106-110`, `models/hashids.go:61-69` (encoder in `support/hashids.go:96-98,181-184`) +**Issue:** `canActOnOrder` gives full order access (read, cancel, change the payment method, remove items, request a refund) to anyone who presents `?guest_code=` equal to the stored `guest_code`. That code is not random: it is `hashids(salt, id × 118, 32)` (`GuestCodeFactor`). +- **Legacy orders.** The PHP plugin hard-coded the salt `KEIOS.PAYMENTGATEWAY` in `support/HashIdsHelper.php`, and the Go port keeps it as `LegacySalt`. So for every order PHP created, both `hash_id` and `guest_code` can be computed by anyone. For each id 1..N, compute `hash = hashids(LegacySalt, 8).encode(id)` and `code = hashids(LegacySalt, 8|32).encode(id*118)`. PHP's static cache reused the length-8 encoder, so try both lengths. Then call `GET /_paymentgateway/api/v1/orders/{hash}?guest_code={code}`. This enumerates every legacy guest order, and `POST .../cancel` / `.../refund-request` act on them. +- **New orders.** The secret is the hashids salt, which is also what protects the public `hash_id`. Hashids is an obfuscation scheme, not a MAC. The salt-derived alphabet shuffle can be recovered from enough known (id, hash) pairs, and a customer can collect pairs for consecutive ids by placing orders. Once the shuffle is known, every guest code follows. + +T-18-37 says the guest code is an authorization factor. As implemented it is only as strong as hashids. The PHP original had the same weakness, but the port re-exposes it on a new, headless, enumerable API. +**Fix:** Make `guest_code` a random bearer secret and stop deriving it from the id: +```go +// models/hashids.go AfterCreate, for a guest order +b := make([]byte, 24) +if _, err := rand.Read(b); err != nil { return err } +code := base64.RawURLEncoding.EncodeToString(b) // 32 chars +updates["guest_code"] = code +``` +For legacy rows, add a migration or console step that rotates every non-null `guest_code` to a random value. Alternatively, refuse a presented code that equals `hashids(LegacySalt, id*118)` at either length, and require the HMAC `guest_token` for those orders. Document the rotation in the import notes. Store a hash of the code (SHA-256) instead of the code itself if the column is ever exposed to admins. + +### B-CR-02: The PaymentAccepted bridge writes `successful_payment_id` before checking `Can(Pay)`, overwriting the real successful payment on Paid, Canceled or Finished orders (PHP did not persist it) + +**File:** `orders/bridge.go:98-110` +**Issue:** `onPaymentAccepted` runs `UPDATE ... SET successful_payment_id = ` unconditionally and only then checks `Can(Pay)`. When `Pay` is not allowed, it logs and returns `nil`, so the write commits. In PHP (`OrderPaymentEventSubscriber::onPaymentAccepted`) `$order->successful_payment = ...` is only an in-memory assignment, persisted by `performTransition(Pay)` through `saveWithState`. When `can(pay)` is false, nothing is saved. + +Consequences: +- **Duplicate payment on a Paid order.** A second accepted payment (a double Stripe session, or an admin accepting a stale cash payment) re-points the order at payment B. `PerformTransition(GrantRefund/DirectRefund/RefundAfterReturn)` then refunds B only. Payment A stays Accepted, is no longer referenced, and is never refunded through the order flow. +- **Late payment on a Canceled order.** A Stripe payment confirmed after the API or cron canceled the order is recorded as the order's successful payment. The customer is charged, the order stays Canceled, and only an error log line records it. The admin "create payment" buttons are hidden because `SuccessfulPaymentID != nil`. + +**Fix:** Persist only together with the transition, as PHP does, and handle the not-payable case explicitly: +```go +ok, err := b.orders.Can(ctx, e.Tx, order, models.OrderTransitionPay) +if err != nil { return err } +if !ok { + b.logger.Error("paymentgateway: order cannot accept the payment", ...) + return nil // optionally fire an event / flag for manual refund +} +id := e.Payment.ID +if err := e.Tx.WithContext(ctx).Model(order).Update("successful_payment_id", id).Error; err != nil { return err } +order.SuccessfulPaymentID = &id +return b.orders.PerformTransition(ctx, e.Tx, order, models.OrderTransitionPay) +``` +Consider treating an Accepted payment on a Canceled order like the Rejected branch (refund, or at least an admin notification). + +### B-CR-03: `default_currency` can be changed at any time, which silently re-bases every stored exchange ratio and native amount + +**File:** `models/settings/fields.yaml:10-18`, `models/settings.go:209-232,244-276`, `currency/converter.go:175-206,267-283`, `console/import_winter.go:141-160` +**Issue:** `golem15_paymentgateway_currencies.ratio` stores "units of the default currency per unit of ISO" relative to the default currency at the time each row was fetched. PHP made `defaultCurrency` a disabled field: it was set once by the SetUp wizard and then locked. The Go settings form makes it a required, freely editable dropdown with no guard, and `import-winter --force` overwrites it too. + +After a change from PLN to EUR: +- `ConvertToNative` multiplies a USD amount by the old USD-to-PLN ratio (≈3.7) and labels the result EUR. +- The PLN rows of existing orders have no ratio row, so a new one is fetched against EUR. + +Every new `native_amount_stored` is then wrong by the PLN/EUR factor. The payments and orders "sales this month" scoreboards sum `native_amount_stored WHERE native_currency = `, which mixes in the wrong amounts and drops the earlier ones. Nothing warns the administrator. +**Fix:** Make the field write-once: +- In `Settings.BeforeValidate`, load the stored row and reject a change of a non-empty `default_currency` (422 `settings.default_currency_locked`), or allow it only when no orders or payments exist. +- Add `attributes: { disabled: true }` (or a `dependsOn`/readOnly preset) after setup in `fields.yaml`. +- Make `import-winter --force` refuse a different code when orders exist. +- If a change must ever be supported, truncate and refetch `golem15_paymentgateway_currencies` in the same transaction. + +### B-CR-04: The Fixer access key is written to the application log on any transport error + +**File:** `currency/fixer.go:50,66-69` +**Issue:** The access key goes into the query string (`q.Set("access_key", f.AccessKey)`, then `root+"/latest?"+q.Encode()`). When `client.Do` fails (timeout, DNS, TLS, connection refused), Go returns a `*url.Error` whose `Error()` is `Get "https://…/latest?access_key=&base=…": `. `net/http` strips only userinfo passwords, not query values. `Rates` then logs that error at Warn level (`slog.Default().Warn("paymentgateway: fixer rate", ..., "err", err)`), and the scheduled `pg:update-currencies` runs every ten minutes, so an outage writes the key into the logs repeatedly. Also, when `fixer_url` is unset the key is sent to the default `https://exchange.viamage.com`, a third-party proxy, rather than fixer.io. +**Fix:** Do not log the raw `*url.Error`, and strip the query: +```go +resp, err := client.Do(req) +if err != nil { + var ue *url.Error + if errors.As(err, &ue) { + return "", fmt.Errorf("%w: fixer request failed: %v", ErrNoRate, ue.Err) + } + return "", fmt.Errorf("%w: fixer request failed", ErrNoRate) +} +``` +Also refuse to send `AccessKey` unless `FixerBaseURL` is explicitly configured (or default the URL to `https://data.fixer.io/api` when a key is set). + +## Warnings + +### B-WR-01: Money arithmetic wraps int64 silently (`Add`, `Sub`, `Neg`, `FromMinor`, `Amount4.Scan(int64)`), and `Fits(MinInt64)` returns true + +**File:** `money/money.go:83-85,138-154`, `money/amount.go:48-49,62-70,79-84` +**Issue:** `Mul`, `Div` and `Allocate` were hardened to panic on overflow, but: +- `Add` and `Sub` do `m.units + o.units` and `m.units - o.units` with no check. +- `FromMinor` does `cents * 100`. +- `Amount4.Scan` does `v * scaleUnits` for an int64 source. +- `Neg` and `Abs` of `MinInt64` return `MinInt64`. +- `Amount4.Fits` computes `-a`, which for `MinInt64` stays negative, so `Fits` returns true. +- `Amount4.String` of `MinInt64` prints garbage (`%04d` of a negative remainder). + +`cart.totalGross` sums item totals with `Add`. Two lines that each fit int64 can sum to a wrapped value that lands inside `±MaxNumeric12` and passes the `Fits` ceiling check in `orders.Make`, `orders.Save` and `payments.Create`. The stored total then has nothing to do with the lines. +**Fix:** Check overflow in `Add` and `Sub` and return `ErrOverflow` (their signatures already return an error): +```go +s := m.units + o.units +if (o.units > 0 && s < m.units) || (o.units < 0 && s > m.units) { + return Money{}, ErrOverflow +} +``` +Guard `FromMinor`, `Scan(int64)` and `Neg` the same way (return an error, or panic consistently with `Mul`). Make `Fits` reject `math.MinInt64` explicitly. + +### B-WR-02: The quote cart accepts an unbounded count, and `Mul` then panics with `ErrOverflow` in `orders.Service.Save` (500, a panic used as control flow) + +**File:** `controllers/quotes_cart.go:229-234` (with `cart/item.go` `TotalNetPrice`/`TotalGrossPrice`, `money/money.go:209-212`) +**Issue:** `change_item` with `field: "count"` only checks `n >= 1`. With a unit price of `999999.9999` (the allowed maximum), any count above about 9.2×10⁸ makes `Mul(count, 1)` exceed int64 after the scale-5 step. `fromScale5` then panics. Nothing recovers it before `svc.Save` → `TotalGross`, so the request dies with a panic instead of the `money.too_large` 403 the code intends to return just below. Smaller counts that fit individually can still wrap the cart sum (B-WR-01). +**Fix:** Bound the count before applying it, and have `Save` turn arithmetic overflow into `ErrAmountTooLarge`: +```go +const maxCount = 1_000_000 +if !ok || n < 1 || n > maxCount { return c.refuse(ctx, "quote.count_min") } +// and in orders.Service.Save / Make: compute totals via a helper that recovers money.ErrOverflow +``` +Prefer making `Item.TotalNetPrice` and `TotalGrossPrice` return `(Money, error)` over panicking. + +### B-WR-03: The customer API ignores `orders_enabled`. With orders off the bridge is a no-op, so `process` can be repeated and items edited after a payment exists + +**File:** `api/actions.go:62-115`, `api/items.go:51-75`, `orders/bridge.go:60-62` +**Issue:** The doc comment says a second `process` answers 409 because "the bridge moves the order to AwaitingPayment". `bridge.order()` returns nil when `settings.OrdersEnabled` is false, so the order stays `Placed` after a successful `process`. A second `process` then passes `requireState(Placed)` and creates and sends a second payment (another Stripe Checkout session). `AddPiece`, `RemoveItem` and `SelectShipping` also keep working on an order that already has a Sent payment for the old total. `resolveOrder` and every handler skip the switch, while the admin side does check it (`ordersAdminController.ordersOn`). +**Fix:** In `resolveOrder` (or a middleware on the API group), answer 404 or 409 when `!settings.OrdersEnabled`. Also refuse `process` when the order already has a payment in `Created` or `Sent`: +```go +if !settings.OrdersEnabled { writeNotFound(w); return nil, false } +``` + +### B-WR-04: The `RecordExternal` contract cannot be met for a disabled operator. The doc says "need not be enabled", but `Send` and `Accept` are guarded by `enabled` + +**File:** `payments/service.go:458-470` (guard in `payments/machine.go` `NewMachine`, `enabledGuard` at `payments/service.go:117-129`) +**Issue:** `RecordExternal` accepts any registered operator, enabled or not, "the record is a fact, then calls Send and Accept". Both `Send` and `Accept` carry `Guard: enabled`, which returns false for a disabled operator. A consumer following the documented D-04 b path therefore gets `ErrTransition` and a payment stuck in `Created`, and with the default cron it is canceled after `payment_created_to_canceled_timeout` days. +**Fix:** Either require `GetEnabled` in `RecordExternal` (and update the doc), or add an explicit `AcceptExternal` that bypasses the enabled guard only for externally recorded payments (for example a `Subject.External` flag checked in `enabledGuard`). + +### B-WR-05: Operator side effects run before the state guard and inside the DB transaction, with no idempotency; lock order is inverted between the order and notification paths + +**File:** `payments/service.go:260-291,325-353`, `orders/service.go:323-337`, `orders/bridge.go:63,96` +**Issue:** +1. `Purchase` calls `op.Purchase` (an outbound Stripe request) before it checks that `Send` is allowed. A payment that is not `Created`, or whose operator is registered but disabled (`s.Operator` does not check enabled), still triggers the external call, and only then fails on `apply(Send)`. +2. `Refund` calls `op.Refund` inside the caller's transaction. If anything later in the transaction fails (an event listener, the bridge, a serialization or deadlock error), the money has been refunded at the operator while the row rolls back to `Accepted`. An admin retry then issues a second refund request. No idempotency key derived from the payment UUID is passed. +3. The admin order refund (`PerformTransition` → `payments.Lock`) takes the order lock and then the payment lock. The notification path (`ProcessNotification` → `Lock(payment)` → bridge `orders.Lock`) takes them in the opposite order. A concurrent refund webhook and admin refund on the same order can deadlock. Postgres aborts one side, possibly the admin side after the operator refund was already issued (point 2). + +**Fix:** +- Check `Can(Send)` (with the guard) before `op.Purchase`. +- Pass an idempotency key (the payment UUID plus the transition) to operator calls. +- Lock order before payment everywhere: in `ProcessNotification`, when `p.OrderID != nil`, lock the order row before the payment row. + +### B-WR-06: `POST /cancel` on an AwaitingPayment order with an in-flight online payment cancels the order and leaves the payment Sent; a later Accept charges the customer for a canceled order + +**File:** `api/actions.go:345-388` (with `orders/bridge.go:103-110`) +**Issue:** `Cancel` cancels only cash and manual payments, then applies `Cancel` to the order directly, even when a Stripe payment is still `Sent` (the customer may be on the Checkout page). When the webhook later accepts that payment, `onPaymentAccepted` finds the order Canceled, cannot `Pay`, logs an error and (B-CR-02) records it as the successful payment. Nothing refunds it. The comment's claim that "the sweep handles" this is not backed by anything in the reviewed code. +**Fix:** +- Refuse `cancel` (409) while an API-mode payment of the order is `Sent`, or cancel that payment at the operator first (expire the Checkout session or cancel the intent). +- In the bridge, refund an Accepted payment on a Canceled order the same way the Rejected branch does. + +### B-WR-07: Payment rows are canceled from unlocked reads, and transitions are blind `UPDATE`s without a state predicate + +**File:** `api/actions.go:356-376`, `orders/service.go:338-359`, `payments/service.go:362-372` +**Issue:** `ForOrderRows` reads the order's payments without `FOR UPDATE`, then `Can` and `Cancel` act on that snapshot. `apply` writes `UPDATE ... SET state = next WHERE id = ?` with no `AND state = `. Correctness depends on every concurrent writer also locking the order row first. The bridge does this only when `orders_enabled` is on, and `payments.Accept` from the admin payments screen locks only the payment. Interleaving: the admin accepts a manual payment while the API cancel holds the order, with orders off. Both commit, and the last write wins: an Accepted payment is flipped to Canceled. +**Fix:** Lock each payment row before deciding (`s.payments.Lock(ctx, tx, p.ID)` inside the loop). In `apply`, use an optimistic predicate: +```go +res := tx.WithContext(ctx).Model(&models.Payment{}).Where("id = ? AND state = ?", p.ID, p.State).Update("state", next) +if res.RowsAffected != 1 { return fmt.Errorf("%w: payment %d changed concurrently", ErrTransition, p.ID) } +``` +Apply the same to `orders.Service.apply`. + +### B-WR-08: The native amount is never checked against `numeric(12,4)` in `Make` and `Save`, nor against `numeric(10,4)` in `RecordExternal` + +**File:** `orders/service.go:106-111,211-216`, `payments/service.go:472-474` +**Issue:** `Make` and `Save` check `total` against `MaxNumeric12` but not `native`. A EUR order of 50,000,000 converts to about 213,000,000 PLN, which overflows `native_amount_stored numeric(12,4)`. Postgres raises `numeric field overflow`, which surfaces as an opaque 500 in the API (and as a 500 from the quote cart) instead of the 422 or 403 `too_large` that the same code returns for the gross total. `RecordExternal` checks `order.AmountStored` but copies `NativeAmountStored` into a `numeric(10,4)` payment column unchecked. `payments.Create` does both checks correctly. +**Fix:** After `ConvertToNative`, add `if !native.Amount4().Fits(money.MaxNumeric12) { return ErrAmountTooLarge }` in `Make` and `Save`, and `if !order.NativeAmountStored.Fits(money.MaxNumeric10)` in `RecordExternal`. + +### B-WR-09: `import-winter` stores `mailer_from_email` and `admin_email` without the e-mail validation the settings form enforces + +**File:** `console/import_winter.go:349-377,431-434` +**Issue:** `mailer.credentials.email` is copied verbatim (only trimmed). `mailgroup` is accepted as an e-mail whenever it contains `@`. The values then become the mail `ReplyTo` and the staff `To` address (`events/mailers.go:133-135`, `events/recipients.go:85-86`). An exported value such as `"a@b.c, other@x"` or one containing CR/LF bypasses the "e-mail rules validate mailer_from_email and admin_email" mitigation of T-18-24. postcard's header checks will probably refuse such a value at send time, which then breaks every mail silently: the error is only logged. +**Fix:** Validate with `net/mail.ParseAddress` and require that the parsed address equals the trimmed input (single address, no display name). Report invalid values as ignored lines, as the timeouts already do. + +### B-WR-10: Negative costs are not refused for shipping methods and quote items, and `add_quoteable` bypasses the quote cart's non-negative rule + +**File:** `models/shipping.go:58-64`, `models/quoteable.go:39-45`, `controllers/quotes_cart.go:198` +**Issue:** The `money` admin field compares magnitude against `max` and accepts a leading minus, and neither model's `Rules()` refuses a negative `cost_stored`. A negative shipping cost lowers every order that selects it (`SelectShipping`). A negative quoteable is turned into a negative cart line by `addQuoteable` (`q.CostStored.Money(currency)`), while `change_item` refuses the same value with `quote.price_negative`. `payments.Create` uses `Fits` (absolute value), so a negative total passes and becomes a negative cash or manual payment. +**Fix:** Refuse `cost_stored < 0` in `shippingsAdminController.beforeSave` and `quoteablesAdminController.beforeSave` (or add a `min: "0"` to the money field if cabana supports it). Check `q.CostStored < 0` in `addQuoteable`. Refuse a non-positive total in `payments.Create` (`ErrAmountTooLarge`'s sibling `ErrAmountInvalid`). + +## Info + +### B-IN-01: The route comment is wrong; the API paths have three segments and also match the notification pattern + +**File:** `routes.go:59-60` (patterns at `routes.go:15-19`) +**Issue:** `/_paymentgateway/api/v1/orders`, `/payment-methods` and `/shippings` have three segments after `/_paymentgateway`, so they also match `/_paymentgateway/{op}/{mode}/{result}`. GET works only because the literal pattern is more specific. `POST /_paymentgateway/api/v1/orders` is routed to `notify` and answers 404 "Operator not found". +**Fix:** Correct the comment, or reserve the `api` segment (register explicit 405 handlers for the three API paths). + +### B-IN-02: Per-user API throttling never applies + +**File:** `plugin.go:119-128` +**Issue:** The `paymentgateway-api` bucket keys on `bouncer.User`, but the throttle is group middleware that runs before the handler-level `api.OptionalJWT`/`RequireJWT`, which are the only places the principal is set. The key is therefore always the client IP, contrary to the "per user or IP" claim in T-18-41. +**Fix:** Run the optional JWT guard as group middleware ahead of the throttle, or document IP-only keying. + +### B-IN-03: `Allocate` accepts negative ratios and can hand out more than the amount + +**File:** `money/money.go:252-303` +**Issue:** With ratios `[-1,-1,4]` on `0.01`, truncation toward zero gives shares `[0,0,0.02]`. The remainder is then negative and the `remainder > 0` loop never corrects it. `Allocate` is not used in production code yet. +**Fix:** Refuse negative ratios (return nil or panic), as MoneyRight expects positive ratios. + +### B-IN-04: Four-place ratios lose most of their precision for low-value currencies + +**File:** `currency/converter.go:117-133` +**Issue:** `numeric(8,4)` means IDR→EUR (≈0.0000568) is stored as 0.0001, a 76 % error. A ratio below 0.00005 rounds to 0 and is refused (`ErrBadRatio`), which makes the currency unusable. This matches PHP, but it matters for money exactness. +**Fix:** Document the limitation, or widen `currencies.ratio` (for example `numeric(18,10)`) in a later phase. + +### B-IN-05: The `golem15-paymentgateway-label` element never re-renders when `value` changes + +**File:** `assets/js/quote-cart.js:23-28` +**Issue:** The element reads `value` only in `connectedCallback` and has no `observedAttributes` or `attributeChangedCallback`. If Vue patches the attribute on a reused row element, the list keeps showing the old state or operator label. +**Fix:** Add `static get observedAttributes() { return ['value'] }` and an `attributeChangedCallback` that updates `textContent`. + +### B-IN-06: `IsGuestEmail` matches any address that contains `guest@` + +**File:** `events/recipients.go:38-40` +**Issue:** An address such as `myguest@example.com` or `theguest@firm.pl` is treated as the guest marker, so the user's own address is skipped. This matches PHP `str_contains`. +**Fix:** Match the actual guest-user domain or prefix the user plugin uses. + +### B-IN-07: `process` returns the operator's raw message to an anonymous caller and turns every failure into a 422 + +**File:** `api/actions.go:103-114` +**Issue:** +- `resp.Message` (for example a Stripe error text) is returned to whoever holds the guest code. +- Database errors and deadlocks returned by `orders.Purchase` also become 422 "payment could not be started" instead of a logged 500. + +**Fix:** Return the generic `msgPaymentFailed` and log `resp.Message`. Map only domain errors (`ErrUnsupportedCurrency`, `MissingFieldError`, `ErrAmountTooLarge`, operator errors) to 422. + +### B-IN-08: `readWinterFields` reports every read error as "File not found" + +**File:** `console/import_winter.go:309-313` +**Issue:** Permission denied, a directory path or an I/O error are all reported as a missing file. +**Fix:** Use `fmt.Errorf("%s: %w", file, err)`. + +### B-IN-09: The stats sum is cast to `numeric(14,4)` + +**File:** `controllers/orders_partials.go:155-158` +**Issue:** A monthly sum above 9,999,999,999.9999 makes Postgres fail the stats partial with `numeric field overflow`. +**Fix:** Cast to unconstrained `numeric` and parse the text. `ParseAmount4` already reports overflow. + +### B-IN-10: The guest token cache keeps a nil verifier forever and is never evicted + +**File:** `api/auth.go:155-177` +**Issue:** If `app.key` is unavailable on the first call, `nil` is cached for that app permanently. The map keyed by `*backpack.App` also grows in tests that build many apps. +**Fix:** Do not cache nil. Hold the verifier on the plugin instance, or publish it on the app at boot. + +### B-IN-11: The notification route depends on `net/http/httptest` in production and replays the handler after commit + +**File:** `payments/notification.go:12,158-167` +**Issue:** +- A production package imports `net/http/httptest`, and responses are fully buffered. +- `r.Body` has already been consumed when the operator's handler runs, so a handler that re-reads it sees EOF. +- `op.Enabled` (a settings DB read) runs before `VerifyNotification`, and its 404-versus-403 answer reveals which operators are enabled. + +**Fix:** +- Use a small custom `ResponseWriter` recorder. +- Rewind `r.Body` (`bytes.NewReader(body)`) before `handler.ServeHTTP`. +- Optionally answer 403 for a disabled operator. + +### B-IN-12: The quotes controller has no `FormExtendQuery` + +**File:** `controllers/quotes.go:60-63` (and `controllers/admin_registry.go:273-287`) +**Issue:** Only the list is scoped to `state = 'Quote'`. The quotes preview, update form, `process_quote` and the `quote_cart` action all load any order id under the quotes controller. Saves are refused by `FormBeforeUpdate`, but the screens and the read-only cart of non-quote orders are served there. +**Fix:** Add `FormExtendQuery` with the same `state = 'Quote'` scope, as `ordersAdminController` does for the inverse. + +### B-IN-13: A huge `page` overflows the offset in `GET /orders` + +**File:** `api/orders.go:33-46` +**Issue:** `(page-1)*OrdersPerPage` overflows for a `page` near `MaxInt`, and the negative offset is ignored, so the first page is served. +**Fix:** Clamp `page` to something like `total/OrdersPerPage + 1` before computing the offset. + +--- + +_Reviewed: 2026-10-13T15:58:16Z_ +_Reviewer: Claude (msd-code-reviewer)_ +_Depth: standard_ +