docs(18): add code review report (re-review after gap closure 18-08)
This commit is contained in:
Jakub Zych
@@ -1,21 +1,25 @@
|
||||
---
|
||||
phase: 18-payment-gateway
|
||||
reviewed: 2026-10-13T16:00:36Z
|
||||
reviewed: 2026-10-13T22:33:10Z
|
||||
depth: standard
|
||||
files_reviewed: 106
|
||||
files_reviewed: 110
|
||||
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/host_test.go
|
||||
- ../sm-pgstripe-plugin/models/settings/fields.yaml
|
||||
- ../sm-pgstripe-plugin/models/settings.go
|
||||
- ../sm-pgstripe-plugin/operator.go
|
||||
- ../sm-pgstripe-plugin/operator_test.go
|
||||
- ../sm-pgstripe-plugin/plugin.go
|
||||
- ../sm-pgstripe-plugin/README.md
|
||||
- ../sm-pgstripe-plugin/scripts/winter-export-settings.sql
|
||||
- ../sm-pgstripe-plugin/updates/202610130001_create_golem15_pgstripe_settings.go
|
||||
- ../sm-pgstripe-plugin/webhook.go
|
||||
- ../sm-pgstripe-plugin/webhook_test.go
|
||||
- admin/src/app/money.ts
|
||||
- admin/src/components/form/fields/MoneyField.vue
|
||||
- admin/src/components/form/fields/PasswordField.vue
|
||||
@@ -111,23 +115,84 @@ files_reviewed_list:
|
||||
- ../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
|
||||
critical: 6
|
||||
warning: 21
|
||||
info: 28
|
||||
total: 55
|
||||
resolved: 1
|
||||
partly_resolved: 1
|
||||
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.
|
||||
first_reviewed: 2026-10-13T16:00:36Z
|
||||
rereview_note: "Incremental re-review after gap-closure plan 18-08 (../sm-pgstripe-plugin d488834..0313820, seven files). The findings counts are the findings still open or partly open. resolved (A-CR-01) is not in them. partly_resolved (A-CR-02) is counted in critical. Findings on files outside the seven are carried over unchanged and were not re-judged. The Docker-backed TestHostStripeFlow was read, not run."
|
||||
---
|
||||
|
||||
# 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 |
|
||||
| Part | Critical | Warning | Info | Resolved |
|
||||
|---|---|---|---|---|
|
||||
| A | 2 | 11 | 15 | 1 |
|
||||
| B | 4 | 10 | 13 | 0 |
|
||||
|
||||
The table counts the findings that are open or partly open after the 18-08 re-review. The first review counted A 3 / 10 / 8 and B 4 / 10 / 13 (48 findings).
|
||||
|
||||
## Re-review after 18-08
|
||||
|
||||
**Re-reviewed:** 2026-10-13T22:33:10Z
|
||||
**Scope:** gap-closure plan 18-08, `git -C ../sm-pgstripe-plugin diff d488834..0313820` (commits `0c4fde7`, `f5c10e8`, `be75108`, `0313820`), seven files
|
||||
**Depth:** standard
|
||||
|
||||
This is an incremental update of the full-phase review below, not a new review of the phase.
|
||||
|
||||
**Re-read in full at `0313820`, together with the diff:** `../sm-pgstripe-plugin/webhook.go`, `operator.go`, `browser_return.go`, `host_test.go`, `operator_test.go`, `webhook_test.go` and `README.md`. The three test files and the README are new in `files_reviewed_list` (106 to 110 files).
|
||||
|
||||
**Read for tracing only, not reviewed:** `../sm-pgstripe-plugin/client.go`, `stripefake_test.go`, `plugin.go:60-70` and `lang/*/lang.yaml:30`; in `../sm-paymentgateway-plugin` (at `f55320a`, clean tree) `payments/notification.go`, `payments/service.go:95-456`, `payments/machine.go`, `fsm/fsm.go:63-88`, `models/payment.go`, `api/actions.go:340-388`, `console/pg_cancel_sent_payments.go`, `schedule.go` and the timeout rule in `models/settings.go`; in the framework `modules/sunscreen/sunscreen.go` and `internal/build/build.go:105-112`.
|
||||
|
||||
**Not re-read:** everything else. All Part B findings, and the Part A findings on the framework, the admin SPA, the Nuxt layer and the other pgstripe files (A-CR-03, A-WR-06 to A-WR-10, A-IN-03, A-IN-05, A-IN-07, A-IN-08), are carried over exactly as written and were not re-judged.
|
||||
|
||||
**Commands run in `../sm-pgstripe-plugin` at `0313820`:** `go vet ./...` (exit 0) and `go test -count=1 -short ./...` (ok). The three new Docker-free subtests pass: `TestStripeOperator/a_declined_attempt_is_logged_and_never_rejected`, `TestStripeOperator/expired_sessions_and_canceled_intents_change_nothing` and `TestStripeWebhook/last_payment_error_decodes_its_codes`. `TestHostStripeFlow` needs Docker and was not run, as instructed, so every statement about `host_test.go` comes from reading it together with the Stripe fake.
|
||||
|
||||
### Existing findings on the seven files
|
||||
|
||||
| Finding | Status after 18-08 | Commit | Checked by |
|
||||
|---|---|---|---|
|
||||
| A | 3 | 10 | 8 |
|
||||
| B | 4 | 10 | 13 |
|
||||
| A-CR-01 | resolved | `0c4fde7` | reading, unit subtest run |
|
||||
| A-CR-02 | partly resolved (still counted as critical) | `f5c10e8` | reading |
|
||||
| A-WR-01 | unchanged | | reading |
|
||||
| A-WR-02 | unchanged | | reading |
|
||||
| A-WR-03 | unchanged | | reading |
|
||||
| A-WR-04 | unchanged | | reading |
|
||||
| A-WR-05 | unchanged | | reading |
|
||||
| A-IN-01 | unchanged | | reading |
|
||||
| A-IN-02 | unchanged, by plan decision 3 | | reading |
|
||||
| A-IN-04 | unchanged | | reading |
|
||||
| A-IN-06 | unchanged (`operator.go` part; `models/settings.go` not re-read) | | reading |
|
||||
|
||||
Each of these findings carries a **Re-review (18-08)** paragraph with the details.
|
||||
|
||||
### New findings (found in the 18-08 re-review)
|
||||
|
||||
No new critical finding. One warning and seven info items, all in Part A:
|
||||
|
||||
- A-WR-11: a payment whose asynchronous Checkout payment is still processing is canceled by the sweep, and the later success is refused.
|
||||
- A-IN-09: the decline Warn line says the payment stays open whatever its state.
|
||||
- A-IN-10: the refused-accept Error line is written per signal, and the success URL can repeat it.
|
||||
- A-IN-11: the decline text is still logged on the purchase-time decline path.
|
||||
- A-IN-12: `PaymentError.Type` is decoded and exported but nothing reads it.
|
||||
- A-IN-13: the `completedEvent` fixture names the first payment's session and intent, and `paid_after_final_state` uses it for the third purchase.
|
||||
- A-IN-14: test-quality notes (state assertions that cannot fail on their own, a mislabelled scenario, an untested branch).
|
||||
- A-IN-15: the README and the settings page list different webhook events.
|
||||
|
||||
### What held up
|
||||
|
||||
- **Log content.** The new Warn and Error lines carry ids, the payment state and Stripe's `code` and `decline_code`. `PaymentError` has no field for the message text, the body never reaches the logger, and no e-mail or key material is in reach of `attemptFailed` or `refusedAccept`. Two things to know: the payment uuid is new in log lines, and it is all a caller needs for the unauthenticated browser return (which changes state only after re-checking at Stripe); `decline_code` values such as `insufficient_funds` are tied to a payment and through it to a user, so the log's retention should allow for that.
|
||||
- **Redaction.** None of the new attribute keys (`payment`, `uuid`, `state`, `event`, `intent`, `session`, `code`, `decline_code`) is on sunscreen's redaction list, so the ids needed to find the charge reach the production log.
|
||||
- **Logger capture.** The generated `main` calls `sunscreen.InstallDefault` before `party.Activate`, so the `slog.Default()` the operator takes at activation is the application's handler.
|
||||
- **Row lock.** `attemptFailed` and `refusedAccept` run inside the route's transaction on the payment row locked `FOR UPDATE`, read the state of that locked row and add no query, write or Stripe call. Lock hold time and lock order are as before (A-WR-05 and B-WR-05 stand as written).
|
||||
- **Proof method of the decline unit test.** Its control case (`operator_test.go:442-447`) shows that a rejecting event fails without a payment service, so the nil error of the decline does prove that no transition was attempted.
|
||||
|
||||
# Part A
|
||||
|
||||
@@ -138,6 +203,8 @@ Two parallel standard-depth reviews over 106 source files. Part A: framework (ca
|
||||
**Files Reviewed:** 47
|
||||
**Status:** issues_found
|
||||
|
||||
**Re-review (18-08):** 51 files after the re-review added `host_test.go`, `operator_test.go`, `webhook_test.go` and `README.md` of sm-pgstripe-plugin. The second paragraph of the summary below describes the code before 18-08; see the re-review paragraphs of A-CR-01 and A-CR-02 for the current state.
|
||||
|
||||
Paths without `../` are relative to `summercms.go/`. Paths with `../` are the sibling repos `sm-pgstripe-plugin` and `vue-paymentgateway-layer`.
|
||||
|
||||
## Summary
|
||||
@@ -163,6 +230,8 @@ case EventPaymentIntentPaymentFailed:
|
||||
return nil
|
||||
```
|
||||
|
||||
**Re-review (18-08):** resolved by `0c4fde7` (sm-pgstripe-plugin). `handleEvent` gives `payment_intent.payment_failed` its own case, which calls `attemptFailed` and returns nil (`webhook.go:189-191`); `attemptFailed` only logs (`webhook.go:200-216`). `o.reject(` has one call site left, `checkout.session.async_payment_failed` (`webhook.go:185-186`). Checked by reading the code and by running `TestStripeOperator/a_declined_attempt_is_logged_and_never_rejected`, which passes. `TestHostStripeFlow/decline_then_succeed`, `/decline_then_succeed_payment_intent` and `/event_table` assert the end-to-end outcome (payment Sent after the decline, order Paid after the retry); they were read, not run. The second half of the fix above (reject on `checkout.session.expired` and `payment_intent.canceled`) was not taken, by plan decision 3; that part is tracked under A-IN-02. Related new findings: A-WR-11 and A-IN-09.
|
||||
|
||||
### 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`
|
||||
@@ -181,6 +250,19 @@ if !ok {
|
||||
}
|
||||
```
|
||||
|
||||
**Re-review (18-08):** partly resolved by `f5c10e8` (sm-pgstripe-plugin). Checked by reading; `TestHostStripeFlow/paid_after_final_state` was read, not run.
|
||||
|
||||
Resolved: `accept` now calls `refusedAccept` when `Can(Accept)` is false (`operator.go:447-454`). For every state except Accepted and Refunded, `refusedAccept` writes an Error record with the payment id, uuid, state, the event id or `browser_return`, the PaymentIntent id and the stored session id (`operator.go:484-501`). The browser return passes its own trigger (`browser_return.go:71`). A paid signal for a Rejected or Canceled Stripe payment is no longer dropped without a trace.
|
||||
|
||||
What remains:
|
||||
|
||||
1. **Nothing prevents the charge.** No code expires the Checkout Session or cancels the PaymentIntent when a payment is canceled: there is no `V1CheckoutSessions.Expire` call and no `PaymentCanceled` listener in the plugin. A Checkout Session expires at Stripe 24 h after creation (the code sets no `expires_at`). A PaymentIntent in `requires_payment_method` has, as far as I know, no expiry of its own, so in PaymentIntent mode a payment canceled by the sweep stays payable with its client secret; confirm this against Stripe's documentation.
|
||||
2. **The log line is the only signal.** No event, admin notice, stored marker or refund follows (plan decision 5). The line is written per signal, not per payment (A-IN-10).
|
||||
3. **The customer-cancel scenario described above does not reach the new line.** `api/actions.go:361-365` of sm-paymentgateway-plugin (read for tracing) skips payments whose operator mode is not cash or manual, so `POST /orders/{hash}/cancel` cancels the order and leaves the Stripe payment Sent. A later paid signal passes `Can(Accept)`, the payment becomes Accepted and the outcome is the one of B-WR-06 and B-CR-02 (gateway, not re-read). The sentence "that cancels the Sent payment" in the issue text holds for cash and manual payments only. The payments that do reach `refusedAccept` are those canceled by `pg:cancel-sent-payments` or by an administrator, and those rejected by `checkout.session.async_payment_failed`.
|
||||
4. **A replay is inferred from the state alone.** Accepted and Refunded are silent without a comparison of the signal's PaymentIntent id with the stored `payment_intent_id` (`operator.go:485-488`). A paid signal for a different PaymentIntent carrying the same payment uuid would be treated as a replay. I found no path in the code I read that gives one payment two payable Stripe objects; closing this is the id check A-WR-02 asks for.
|
||||
|
||||
The silent drop that made this finding critical is fixed, and the remainder is of the same kind as B-WR-06. It stays counted in the critical tier because this re-review does not re-grade existing findings.
|
||||
|
||||
### 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`)
|
||||
@@ -203,30 +285,40 @@ export function parseMoneyInput(text: string, scale: number, decimal = '.'): Mon
|
||||
**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.
|
||||
|
||||
**Re-review (18-08):** unchanged. `ModeKeys` is still called with the settings of the moment in `Refund` (`operator.go:286`), `VerifyNotification` (`operator.go:342`) and `paidAtStripe` (`browser_return.go:92`, formerly `:87`), and nothing stores the mode at Purchase. `client.go` is not part of the 18-08 diff. `TestHostStripeFlow/wrong_secret` asserts the behaviour (`host_test.go:819-823`: the live secret does not verify while test mode is on).
|
||||
|
||||
### 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.
|
||||
|
||||
**Re-review (18-08):** unchanged. The accepting cases are now at `webhook.go:178-188`. `EventObject` (`webhook.go:63-73`) still decodes no amount or currency, and no case compares `data.object.id` with the stored session or intent id. 18-08 names this missing check as its reason for not acting on `checkout.session.expired` (`webhook.go:157-163`). The new test `TestHostStripeFlow/paid_after_final_state` shows the effect in its part 4 (A-IN-13), and the replay classification added by 18-08 relies on the state alone (A-CR-02, remaining item 4).
|
||||
|
||||
### 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.
|
||||
|
||||
**Re-review (18-08):** unchanged. `stripeAmount` is now at `operator.go:620-633` with the currency tables at `operator.go:607-614`; UGX is still in `zeroDecimal` (`operator.go:609`). `operator_test.go:304-306` asserts the rounding (`500.50 JPY` becomes 501).
|
||||
|
||||
### 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()`).
|
||||
|
||||
**Re-review (18-08):** unchanged. `operator.go:302-314` builds `RefundCreateParams` without an idempotency key and does not treat `charge_already_refunded` as success.
|
||||
|
||||
### 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.
|
||||
|
||||
**Re-review (18-08):** unchanged. The lines are now `browser_return.go:65-75` and `87-127`; `client.go:28-33` is not part of the 18-08 diff. One thing changed around this finding: the Stripe lookup is what produces the new Error line when the browser return finds a Canceled or Rejected payment paid (`TestHostStripeFlow/paid_after_final_state`, part 3). The "skip the Stripe call when the payment is already in a final state" part of the fix should therefore skip only Accepted and Refunded. Each such GET also writes one Error record (A-IN-10).
|
||||
|
||||
### 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`
|
||||
@@ -269,6 +361,21 @@ export function isRedirect(r: PgProcessResult | null | undefined): r is { redire
|
||||
**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.
|
||||
|
||||
### A-WR-11: A payment whose asynchronous Checkout payment is still processing is canceled by the sweep, and the later `async_payment_succeeded` is refused
|
||||
|
||||
**Found in:** 18-08 re-review. The behaviour predates 18-08; 18-08 documents the sweep as what closes a payment left Sent, and it made the outcome visible as an Error line.
|
||||
**File:** `../sm-pgstripe-plugin/webhook.go:178-184` (with `webhook.go:157-163`, `../sm-pgstripe-plugin/operator.go:166-184`, `../sm-pgstripe-plugin/README.md:36`)
|
||||
**Issue:** `checkout.session.completed` with `payment_status` unpaid returns nil and writes nothing (`webhook.go:179-181`). The payment stays Sent, its `updated_at` stays at purchase time, and nothing records that the customer finished Checkout and an asynchronous payment is running. The gateway's daily `pg:cancel-sent-payments` cancels every Sent payment whose `updated_at` is older than `payment_sent_to_canceled_timeout` days (settings rule 1 to 30, default 7; `console/pg_cancel_sent_payments.go:56-66` of sm-paymentgateway-plugin, read for tracing). `purchaseCheckout` sets no `payment_method_types`, so the session offers the methods enabled on the Stripe account, including delayed-notification ones. When such a payment settles after the timeout, `checkout.session.async_payment_succeeded` reaches `accept`, `Can(Accept)` is false for the Canceled payment, and the result is the `refuses Accept` Error line: the customer is charged, the bridge has already canceled the order, and someone has to refund or settle by hand. With the timeout at 1 day this concerns every delayed method. With the default of 7 days it needs a settlement slower than a week; Stripe documents up to 14 business days for SEPA Direct Debit (from memory; confirm against the methods enabled on the account).
|
||||
**Fix:** Mark the payment when its session completes unpaid, and tell operators how the timeout relates to delayed methods. Storing a marker with `SaveData` also moves `updated_at`, which restarts the sweep's clock from the day the customer finished Checkout:
|
||||
```go
|
||||
case EventCheckoutSessionCompleted:
|
||||
if obj.PaymentStatus != PaymentStatusPaid {
|
||||
// An asynchronous method is running: record it once, no transition.
|
||||
return o.asyncPending(ctx, pc, string(obj.PaymentIntent))
|
||||
}
|
||||
```
|
||||
where `asyncPending` sets `stripe_async_pending` (and the PaymentIntent id) only when the payment is Sent and the marker is absent. Add a README sentence that `payment_sent_to_canceled_timeout` must be longer than the settlement time of the delayed methods enabled on the account, or set `PaymentMethodTypes` to immediate methods until the sweep can skip pending payments.
|
||||
|
||||
## Info
|
||||
|
||||
### A-IN-01: An authentic event for a payment that is not yet visible is answered 200 "ignored", so Stripe never retries it
|
||||
@@ -277,12 +384,16 @@ export function isRedirect(r: PgProcessResult | null | undefined): r is { redire
|
||||
**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.
|
||||
|
||||
**Re-review (18-08):** unchanged. `ExtractUUID` still answers `ErrNotificationIgnored` for a uuid that `knownPayment` does not find outside the purchase transaction (`operator.go:373-381`, `385-393`).
|
||||
|
||||
### 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.
|
||||
|
||||
**Re-review (18-08):** unchanged, by plan decision 3. `checkout.session.expired` and `payment_intent.canceled` stay 200 no-ops. This is now documented (`webhook.go:157-163`, `README.md:36`) and asserted (`TestHostStripeFlow/event_table`, `TestStripeOperator/expired_sessions_and_canceled_intents_change_nothing`). One correction to the issue text: the payment is not left Sent until someone acts by hand. The gateway's daily `pg:cancel-sent-payments` cancels a Sent payment whose `updated_at` is older than `payment_sent_to_canceled_timeout` days (1 to 30, default 7; `console/pg_cancel_sent_payments.go` and `schedule.go`, read for tracing). So the payment is closed after that many days, not when the session expires. The early cancel on an expired event whose id matches the stored session is still open. See A-WR-11 for a case where the sweep closes a payment too early.
|
||||
|
||||
### 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`
|
||||
@@ -295,6 +406,8 @@ export function isRedirect(r: PgProcessResult | null | undefined): r is { redire
|
||||
**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.
|
||||
|
||||
**Re-review (18-08):** unchanged. `browserReturn` still returns the error of `accept` (`browser_return.go:71-73`, formerly `:66-68`), which the route answers with a 500.
|
||||
|
||||
### 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`
|
||||
@@ -307,6 +420,8 @@ export function isRedirect(r: PgProcessResult | null | undefined): r is { redire
|
||||
**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.
|
||||
|
||||
**Re-review (18-08):** unchanged for `operator.go:127-131`; `models/settings.go` was not re-read. `TestHostStripeFlow/no_secret_key` asserts that `ErrNoSecretKey` comes back as a Go error (`host_test.go:1221-1241`).
|
||||
|
||||
### A-IN-07: `readWinterFields` reports every read error as "File not found"
|
||||
|
||||
**File:** `../sm-pgstripe-plugin/console/import_winter.go:338-341`
|
||||
@@ -319,11 +434,65 @@ export function isRedirect(r: PgProcessResult | null | undefined): r is { redire
|
||||
**Issue:** The regex `^-?0*(\.0*)?$` matches strings that are not amounts, so a malformed value is shown as "free".
|
||||
**Fix:** `/^-?0+(\.0+)?$/`.
|
||||
|
||||
### A-IN-09: The decline Warn line says "the payment stays open for a retry" whatever the payment's state
|
||||
|
||||
**Found in:** 18-08 re-review
|
||||
**File:** `../sm-pgstripe-plugin/webhook.go:200-216`
|
||||
**Issue:** `attemptFailed` runs for every `payment_intent.payment_failed` of an owned payment and never reads `pc.Payment.State`; the record has no `state` attribute. A decline delivered after the payment was accepted (the late delivery that `TestHostStripeFlow/decline_then_succeed` sends at `host_test.go:650-658`), or for a Rejected, Canceled or Refunded payment, is logged as "the payment stays open for a retry". Someone reading the log for a Canceled payment is told the opposite of its state.
|
||||
**Fix:** Add `"state", pc.Payment.State` to the attributes and use a neutral message when `pc.Payment.IsFinal()` (for example "payment attempt failed; the payment is already final").
|
||||
|
||||
### A-IN-10: The refused-accept Error line is written per signal, and the success URL can repeat it
|
||||
|
||||
**Found in:** 18-08 re-review
|
||||
**File:** `../sm-pgstripe-plugin/operator.go:484-501`, `../sm-pgstripe-plugin/browser_return.go:70-73`, `../sm-pgstripe-plugin/README.md:39`
|
||||
**Issue:** The README says a paid signal for such a payment "writes one Error log line". That is one line per signal. In Checkout mode one charge produces both `checkout.session.completed` and `payment_intent.succeeded`, and both resolve to the payment because the uuid is in the session's and the PaymentIntent's metadata (`operator.go:180-183`). Each load of the success URL adds another line: it is an unauthenticated GET, limited only by the route's `throttle:paymentgateway-notify` bucket (part 3 of `TestHostStripeFlow/paid_after_final_state` expects the second record). Plan decision 5 makes this line the alert, so an alert rule that counts records overcounts, and anyone holding the success URL can raise the alert again. Nothing is stored on this path, by decision, so the operator cannot deduplicate.
|
||||
**Fix:** Say "one Error line per signal" in the README and advise keying alerts on the `payment` attribute. If a single alert per payment is wanted, store a marker in `payments.data` on the first refusal and log at Info level afterwards.
|
||||
|
||||
### A-IN-11: The decline text is still logged on the purchase-time decline path
|
||||
|
||||
**Found in:** 18-08 re-review
|
||||
**File:** `../sm-pgstripe-plugin/operator.go:255-260` and `operator.go:533-540` (logged at `payments/service.go:285` of sm-paymentgateway-plugin, read for tracing); `../sm-pgstripe-plugin/README.md:161`
|
||||
**Issue:** 18-08 keeps the decline text out of the webhook log lines by not decoding it, and its threat T-18-51 and plan prohibition state that logs never carry the decline message. For a decline confirmed at Purchase, `purchaseIntent` puts `pi.LastPaymentError.Msg` into `operators.Failed(msg)`, and the payment service logs that response at Error level as `message`. `failure` does the same with `se.Msg`, although its own log line leaves the message out on purpose (`operator.go:536`). The guarantee therefore holds for webhook events only. Stripe's message texts are not personal data on their own; the point is that the stated rule and the behaviour differ.
|
||||
**Fix:** Keep the customer-facing message in the response (the D-17 contract) and have the gateway log Stripe's code instead of `resp.Message` (this goes with B-IN-07), or narrow the wording in the README and the threat model to the webhook path.
|
||||
|
||||
### A-IN-12: `PaymentError.Type` is decoded and exported but nothing reads it
|
||||
|
||||
**Found in:** 18-08 re-review
|
||||
**File:** `../sm-pgstripe-plugin/webhook.go:75-83`
|
||||
**Issue:** The doc comment says the operator reads the error type, but `attemptFailed` logs `code` and `decline_code` only (`webhook.go:207-214`). The only reader is the decode test (`webhook_test.go:114`). The field is new exported API of a shared plugin, where removing it later is a breaking change.
|
||||
**Fix:** Log it as `type` (it separates a card decline from an `api_error` on Stripe's side), or drop the field before the plugin's first tag.
|
||||
|
||||
### A-IN-13: `completedEvent` names the first payment's session and intent, and `paid_after_final_state` uses it for the third purchase
|
||||
|
||||
**Found in:** 18-08 re-review (from reading; the test needs Docker and was not run)
|
||||
**File:** `../sm-pgstripe-plugin/host_test.go:1190-1213`, `host_test.go:1245-1252`
|
||||
**Issue:** `completedEvent` always builds an event for `cs_test_1` with `pi_test_1`. In part 4 the payment is the third purchase on that host, so the fake gave it `cs_test_3` and `pi_test_3` (`stripefake_test.go:111-121`) and `purchaseCheckout` stored both (`operator.go:192-195`). The event `evt_ok_3` therefore names another payment's session and intent with p3's uuid. The operator accepts it, and `accept` overwrites the stored `payment_intent_id` with `pi_test_1` (`operator.go:455-459`). The refund that follows in the same part is then sent for `pi_test_1`, the first payment's intent, and the fake refunds any id (`stripefake_test.go:218-228`). The part passes only because A-WR-02 is open, it will fail once the object id is matched against the stored id, and it would not notice a refund sent for the wrong intent. `decline_then_succeed` guards the same fixture by asserting `cs_test_1` first (`host_test.go:610-613`).
|
||||
**Fix:** Give `completedEvent` the session and intent ids as parameters and take them from the stored payment data. In part 4, assert that the refund form's `payment_intent` equals the id stored at purchase.
|
||||
|
||||
### A-IN-14: Test-quality notes on the 18-08 tests
|
||||
|
||||
**Found in:** 18-08 re-review
|
||||
**File:** `../sm-pgstripe-plugin/operator_test.go:455-457`, `operator_test.go:526-528`, `../sm-pgstripe-plugin/host_test.go:1094-1104`, `../sm-pgstripe-plugin/operator.go:479-483`
|
||||
**Issue:**
|
||||
- **State assertions that cannot fail on their own.** The operator in these two subtests has no payment service, so every path that could change `pc.Payment.State` returns an error first. Once `handleEvent` has returned nil, the state is necessarily still Sent. The real proof is the error check together with the control case (`operator_test.go:442-447`); the state checks read like a second, independent proof and are not one.
|
||||
- **A mislabelled scenario.** The comment on part 1 of `paid_after_final_state` says "The customer cancels the order", but the test cancels the payment with `env.payments.Cancel`. The customer API does not cancel a Stripe payment (A-CR-02, remaining item 3), so the path the comment names ends with an Accepted payment on a Canceled order and is not covered by this test.
|
||||
- **An untested branch.** The Error line for "Created or Sent with the operator's guard refusing" has no test. Through the notification route it is also shadowed: `ProcessNotification` answers 404 for a disabled operator before it calls the operator (`payments/notification.go:71-80`), so the branch needs the switch to change between that check and `Can`.
|
||||
|
||||
**Fix:** Drop the two state checks or replace them with an assertion that can fail (for example a published payment service whose calls are counted). Reword the part 1 comment to "the sweep or an administrator cancels the payment". Add a unit case for the guard-refusing branch with a payment service whose operator is disabled.
|
||||
|
||||
### A-IN-15: The README and the settings page list different webhook events
|
||||
|
||||
**Found in:** 18-08 re-review
|
||||
**File:** `../sm-pgstripe-plugin/README.md:68` (against `lang/en/lang.yaml:30` and `lang/pl/lang.yaml:30` of sm-pgstripe-plugin, outside this scope, read for cross-reference)
|
||||
**Issue:** Step 2 of the README's Dashboard section now lists four events to subscribe to and marks `payment_intent.payment_failed` optional. The comment shown under both signing-secret fields of the settings page (`webhook_comment`, `models/settings/fields.yaml:48,68`) lists three: `checkout.session.completed`, `checkout.session.async_payment_succeeded` and `payment_intent.succeeded`. It leaves out `checkout.session.async_payment_failed`, the only event that still rejects after 18-08. An administrator who follows the in-app text never receives the terminal failure, and the payment stays Sent until the sweep. The 18-08 plan lists this text as a known follow-up.
|
||||
**Fix:** Add `checkout.session.async_payment_failed` to `webhook_comment` in both language files.
|
||||
|
||||
---
|
||||
|
||||
_Reviewed: 2026-10-13T12:00:00Z_
|
||||
_Reviewer: Claude (msd-code-reviewer)_
|
||||
_Depth: standard_
|
||||
_Re-reviewed (18-08 scope, seven sm-pgstripe-plugin files): 2026-10-13T22:33:10Z_
|
||||
|
||||
|
||||
# Part B
|
||||
|
||||
Reference in New Issue
Block a user