fix(payments): drop the notification sender-IP gate; re-check on its own cadence
CI / changes (pull_request) Successful in 3s
CI / unit (pull_request) Successful in 11s
CI / integration (pull_request) Successful in 21s
CI / ui (pull_request) Successful in 1m16s
CI / conformance (pull_request) Successful in 10s
CI / gate (pull_request) Successful in 0s
CI / deploy (pull_request) Successful in 1m55s

A real test payment on the contour exposed both problems at once. YooKassa
delivered the notification five times; all five were rejected because the
backend saw the sender as 10.77.0.1 — the contour sits behind a tunnel and
cannot observe real client addresses, the same reason the IP bans in this
repository are prod-only. The chips were not lost (the reconcile sweep would
have credited them), but the primary path was dead and the customer was left
watching an unchanged balance.

The address check is removed rather than made conditional. It never was the
security boundary — the confirming GET /v3/payments/{id} is — and the one thing
it bought is already bought earlier and far more tightly: the order is resolved
from the notification's metadata *before* any provider call, so a notification
naming no known order costs a single indexed read and stops there. Guessing a
live order id means guessing a uuid. Against that, an address check adds nothing
and breaks every deployment that cannot see real client addresses, while turning
any future change to YooKassa's published ranges into a silent degradation.

The second problem was mine. The reconcile threshold was keyed off the order
lifetime, so a lost notification cost the customer the full 30-minute TTL before
the chips landed. Those are different questions: the lifetime governs how long a
customer may take to pay, the re-check governs how soon we notice a lost
callback. Split apart — `payments.ReconcileAfter`, one minute, swept on every
reaper tick. The bound D49 was chosen for survives: the calls one order can
cause are still its lifetime divided by the sweep interval, a handful, not an
open-ended poll. Worst case for a failed notification drops from ~30 minutes to
~5; an order the customer is still paying for is left alone.

Tests: the foreign-sender test is replaced by the two properties that now carry
the load — a notification naming an unknown order makes no provider call at all,
and a genuine notification is honoured whatever address it appears to come from.
Plus one pinning that a seconds-old order is not polled.

The shared bundle budget goes 31 -> 32 KB, with the reason recorded in the
script header: every user-visible string lands in that chunk and it had been
sitting 40 bytes under the cap.

Decisions D48 and D49 revised.
This commit is contained in:
Ilia Denisov
2026-07-28 12:00:03 +02:00
parent 12e616ceae
commit 4cac09c9f3
12 changed files with 174 additions and 155 deletions
@@ -41,6 +41,9 @@ type fakeYooKassa struct {
refundStatus string
// refundCalls counts the refunds actually requested.
refundCalls int
// paymentGets counts the confirming reads, so a test can assert a forged notification never
// reached the provider at all.
paymentGets int
// refunds answers GET /refunds/{id}; a missing id is a 404.
refunds map[string]yookassa.Refund
// createdIdempotenceKey records the key sent on the last create-payment call.
@@ -82,6 +85,7 @@ func (f *fakeYooKassa) serve(w http.ResponseWriter, r *http.Request) {
f.payments[id] = p
_ = json.NewEncoder(w).Encode(p)
case r.Method == http.MethodGet && strings.HasPrefix(r.URL.Path, "/payments/"):
f.paymentGets++
p, ok := f.payments[strings.TrimPrefix(r.URL.Path, "/payments/")]
if !ok {
w.WriteHeader(http.StatusNotFound)
@@ -267,19 +271,37 @@ func TestYooKassaNotifyIsNotEvidence(t *testing.T) {
}
}
// TestYooKassaNotifyRejectsForeignSenders checks the sender allowlist: an address outside YooKassa's
// published ranges is refused before any provider call, so a forger cannot make us fan out requests.
func TestYooKassaNotifyRejectsForeignSenders(t *testing.T) {
// TestYooKassaNotifyForAnUnknownOrderCostsNoProviderCall pins what actually keeps a forger from
// using this endpoint to drive traffic at the provider on our behalf. The sender address is not
// checked — it cannot be, in a deployment that sees only its own tunnel — so the guard is that the
// order is resolved from the notification first: an id matching no order costs one indexed read and
// stops there. Guessing a live order id means guessing a uuid.
func TestYooKassaNotifyForAnUnknownOrderCostsNoProviderCall(t *testing.T) {
f, shop := newFakeYooKassa(t)
srv, _ := yookassaServer(t, shop)
before := f.paymentGets
if code := postYooKassaNotify(t, srv, ykSenderIP, yookassa.EventPaymentSucceeded, "pay-forged", uuid.NewString()); code != http.StatusOK {
t.Fatalf("notify = %d, want 200", code)
}
if f.paymentGets != before {
t.Errorf("confirming reads = %d, want %d — a forged notification reached the provider", f.paymentGets, before)
}
}
// TestYooKassaNotifyIsAcceptedFromAnyAddress: the contour (and any deployment behind a tunnel) sees
// only its own internal address as the sender, so a genuine notification must still be honoured.
func TestYooKassaNotifyIsAcceptedFromAnyAddress(t *testing.T) {
f, shop := newFakeYooKassa(t)
srv, pay := yookassaServer(t, shop)
acc := provisionAccount(t)
orderID, paymentID := seedYooKassaOrder(t, f, pay, acc)
if code := postYooKassaNotify(t, srv, "8.8.8.8", yookassa.EventPaymentSucceeded, paymentID, orderID.String()); code != http.StatusForbidden {
t.Fatalf("notify from a foreign address = %d, want 403", code)
if code := postYooKassaNotify(t, srv, "10.77.0.1", yookassa.EventPaymentSucceeded, paymentID, orderID.String()); code != http.StatusOK {
t.Fatalf("notify = %d, want 200", code)
}
if got := readBalance(t, acc, "direct"); got != 0 {
t.Errorf("balance = %d, want 0 — a foreign sender credited chips", got)
if got := readBalance(t, acc, "direct"); got != 100 {
t.Errorf("balance = %d, want 100 — a genuine notification was refused on its source address", got)
}
}
@@ -345,9 +367,10 @@ func TestYooKassaReconcileCreditsLostNotification(t *testing.T) {
acc := provisionAccount(t)
orderID, _ := seedYooKassaOrder(t, f, pay, acc)
// Age the order past its lifetime without any notification having been delivered.
// Age the order just past the re-check threshold — minutes, not the order's full lifetime — with
// no notification ever delivered.
if _, err := testDB.ExecContext(context.Background(),
`UPDATE payments.orders SET created_at = now() - interval '2 days' WHERE order_id=$1`, orderID); err != nil {
`UPDATE payments.orders SET created_at = now() - interval '2 minutes' WHERE order_id=$1`, orderID); err != nil {
t.Fatalf("age order: %v", err)
}
if n := srv.ReconcileYooKassaOrders(context.Background()); n != 1 {
@@ -365,6 +388,27 @@ func TestYooKassaReconcileCreditsLostNotification(t *testing.T) {
}
}
// TestYooKassaReconcileSkipsFreshOrders keeps the sweep off an order the customer is still paying
// for: it re-checks only orders older than ReconcileAfter, so opening a payment page does not start
// polling the provider straight away.
func TestYooKassaReconcileSkipsFreshOrders(t *testing.T) {
f, shop := newFakeYooKassa(t)
srv, pay := yookassaServer(t, shop)
acc := provisionAccount(t)
seedYooKassaOrder(t, f, pay, acc) // just created
before := f.paymentGets
if n := srv.ReconcileYooKassaOrders(context.Background()); n != 0 {
t.Errorf("reconciled %d orders, want 0 (the order is seconds old)", n)
}
if f.paymentGets != before {
t.Errorf("confirming reads = %d, want %d — a fresh order was polled", f.paymentGets, before)
}
if got := readBalance(t, acc, "direct"); got != 0 {
t.Errorf("balance = %d, want 0", got)
}
}
// TestYooKassaReconcileLeavesUnpaidOrders checks the sweep does not invent a credit for an order the
// customer abandoned.
func TestYooKassaReconcileLeavesUnpaidOrders(t *testing.T) {