amadan
Multi-currency: the fifth review's findings
Ownership is the one test every session passes before anything is done with it: on the listing recovery, on reconcile (the bound session and the intent's customer), on a direct binding and in the pass, as well as on the create sites. The page signs its quote with the session's own token, so fields arranged to agree with each other buy nothing a page did not show this session. The earlier barrier trigger is dropped in a transaction that first confirms the current one exists. Resolving the review of a session paid in the wrong currency retires the intent, so the review does not come back. The provisioning check reads every page and counts products and prices by their metadata, and accepts the same own-option shapes the console does. The inclusions test asserts the divisibility that makes the shares exact instead of a tautology, and the webhook, the bind's customer clause and every ownership clause have cases of their own. The design and the dispositions say what the code does, and what review five showed.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
14 files changed,
+754
−42
docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md+2 −2docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review5.md+188 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+24 −3docs/superpowers/specs/OPUS-REVIEW-multi-currency-review5.md+145 −0implementation/carloku-paid-launch/stripe-provision.sh+22 −6internal/console/billingcurrency_test.go+254 −14internal/console/billinghandlers.go+5 −1internal/console/billingpage.go+35 −3internal/console/billingpass.go+3 −0internal/console/billingservice.go+20 −7internal/console/billingservice_test.go+9 −3internal/console/billingstore.go+42 −1internal/console/templates/billing.html+1 −0internal/plans/provision_test.go+4 −2
CI passed — run details
diff --git a/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md b/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md| index 713e21771..4b2416c1f 100644 |
| --- a/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md |
| +++ b/docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md |
| @@ -4,7 +4,7 @@ Status: built on `feat/carloku-multi-currency-20261001` (off the paid-launch bra |
| The amounts: Paul confirmed the annual base prices on 2026-10-01 for implementation, not for production (Personal 28 USD / 24 EUR / 22 GBP; Community 6 / 5 / 4.50 per member; Business 60 / 50 / 45 per member; team minimum ten). The overage rates and the invoice floors are proposed and not approved. So the book that carries them is still marked `draft`, and live mode refuses a draft book: approving it for production is a new book version with `status: approved`. |
| -It amends the approved pricing spec (`2026-09-07-carloku-pricing-plans-design.md`) in one place, on Paul's instruction of 2026-10-01: prices are fixed in USD, EUR and GBP, and Stripe's Adaptive Pricing is switched off on every Checkout Session. The spec's "no `currency_options`" rule stays. |
| +It amends the approved pricing spec (`2026-09-07-carloku-pricing-plans-design.md`) in one place, on Paul's instruction of 2026-10-01: prices are fixed in USD, EUR and GBP, and Stripe's Adaptive Pricing is switched off on every Checkout Session a price book opens (a legacy intent, replayed under its original key, is sent as that build sent it). The spec's "no `currency_options`" rule stays, with one reading: an expanded single-currency price may carry its own currency as its only option, at its own amount. |
| ## Rules |
| @@ -146,7 +146,7 @@ func PublicPriceBook(book string) ([]byte, error) // the export below, byte-stab |
| New columns, both `TEXT NOT NULL DEFAULT ''`, on `billing_checkout_intents` and `billing_accounts`: `currency`, `price_book`. Empty in both means a legacy row. Assignments of a purchase priced from a book go in a new table, `billing_book_assignments`, with the same columns plus `currency` and `price_book` required non-empty; `billing_assignments` keeps only legacy assignments. The split is for rollback: a build from before books reads `billing_assignments` alone, so it finds no history for a book-priced purchase. For an account with both kinds of history it would find the legacy half, so a database trigger (`billing_months_no_legacy_calc_over_paid_book_history`; the earlier, wider `..._over_book_history` is dropped at migration) refuses to freeze a calculation whose top-level `v` is not 2 (read with `json_extract`) for any month into which an active book-priced assignment of the account was priced: its span, from its effective instant to the earliest of the next assignment's effective instant (by time then revision, over both tables), its paid-through instant and the month's end, has length inside the month, which is `AllowanceForMonth`'s own rule. A book assignment that never counted for a month (cancelled before its first reconciliation, or superseded in the same second) is left out, so this build's own version 1 for a legacy-only month after it still goes in. The environment and the binary change together, and no binary from before books runs against a database with a book-priced row (roll note in the dispositions). |
| -- **Checkout** (`POST /a/{sqid}/billing/checkout`): one form, with the country and currency controls, the plan, the postal code and the consent. The form carries what the page priced (`quoted_country`, `quoted_currency`) and whether the currency shown was a choice (`currency_chosen`). A post whose country or currency is not what the page priced is not a purchase: it is answered with a redirect to the page quoting the posted country (and the posted currency when it is a choice), so nothing is bought at a price the buyer has not seen. A currency shown as the country's default and not chosen is given to the door as no currency, and the billing country's own currency is bought in; a chosen one (the URL's, the pricing page's, the account's, or changed on the form) is the purchase's. A currency field that is present and is not exactly `usd`, `eur` or `gbp` (no trimming, no case folding, not empty): 400, nothing is created, before the quote is looked at; on the page, a `?currency=` that names such a thing is said (`billing.currency.unknown`, unreviewed system copy like `billing.quote.unknown`) and the default shown is not a choice. The quote comparison is made on what would be bought (the chosen currency, else the posted country's own), so a form whose fields disagree with each other, a choice flag erased, or a form with no currency control against a page that priced a choice, is answered with the quote for what it would buy. The intent stores the chosen currency and book. A retry with a different currency while an intent is open is a different intent, exactly as a different plan is today. `POST /a/{sqid}/billing/quote` is the form's "Show prices for this country" button: the same form, answered with the same redirect, buying nothing; it asks for the session and the form's token, no step-up. |
| +- **Checkout** (`POST /a/{sqid}/billing/checkout`): one form, with the country and currency controls, the plan, the postal code and the consent. The form carries what the page priced (`quoted_country`, `quoted_currency`) and whether the currency shown was a choice (`currency_chosen`). A post whose country or currency is not what the page priced, judged on what would be bought (the chosen currency, else the posted country's own), or whose quote the page did not sign (`quote_sig`: an HMAC over the book, the quoted country, the quoted currency and the choice flag, keyed by the session's own token, which the form double-submits as `csrf`), is not a purchase: it is answered with a redirect to the page quoting the posted country (and the posted currency when it is a choice), so nothing is bought at a price a page did not show this session. A currency shown as the country's default and not chosen is given to the door as no currency, and the billing country's own currency is bought in; a chosen one (the URL's, the pricing page's, the account's, or changed on the form) is the purchase's. A currency field that is present and is not exactly `usd`, `eur` or `gbp` (no trimming, no case folding, not empty): 400, nothing is created, before the quote is looked at; on the page, a `?currency=` that names such a thing is said (`billing.currency.unknown`, unreviewed system copy like `billing.quote.unknown`) and the default shown is not a choice. The quote comparison is made on what would be bought (the chosen currency, else the posted country's own), so a form whose fields disagree with each other, a choice flag erased, or a form with no currency control against a page that priced a choice, is answered with the quote for what it would buy. The intent stores the chosen currency and book. A retry with a different currency while an intent is open is a different intent, exactly as a different plan is today. `POST /a/{sqid}/billing/quote` is the form's "Show prices for this country" button: the same form, answered with the same redirect, buying nothing; it asks for the session and the form's token, no step-up. |
| - **A currency already held**: checkout is refused while the account's subscription has not ended (`canceled` or `incomplete_expired`), and after it has ended a checkout in any currency but the one bought in is refused, the billing country's default included; the page offers that currency alone, as a choice, for a purchase from before price books too (EUR). A change of currency waits on what Stripe permits for a customer whose subscription ended (test-mode item 4) and is then an operator's door, not the form's. |
| - **Stripe price identity**: the configured price for (book, plan, band, currency) must have that `currency`, `unit_amount` equal to the book's unit price, no `currency_options` but, at most, an entry under its own currency at its own amount and tax behaviour (which an expanded read of a single-currency price may carry), and metadata `price_book`, `currency`, `terms_version`, `plan`, `band` that all match. The checks that exist today (per-unit, yearly, licensed, exclusive tax, active, mode) stay. |
| - **Checkout Session**: created with `currency` and `adaptive_pricing[enabled]=false`, and with `currency`, `price_book` in the session and subscription metadata. A purchase with no book (only a console with no book configured opens one, and an intent from before books is replayed as that build sent it) is asked for exactly as before: no currency, Adaptive Pricing on. Every session the console creates, replays, lists, reads back or binds passes one check against its intent; one that is this intent's and not its purchase (another currency, a conversion, Adaptive Pricing) is expired and the intent retired, and one already paid is left for a person with a review filed once. A subscription whose price is not the configured one for its stored (book, plan, band, currency), or that the provider reports in another currency, is an identity mismatch. |
diff --git a/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review5.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review5.md| new file mode 100644 |
| index 000000000..6d52fe546 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review5.md |
| @@ -0,0 +1,188 @@ |
| +All platform references below are at `1f005804`; site references are at `4feb1cd`. This was source-only: no modifications, tests, network commands or AWS commands. Statements about tests failing are deductions from their assertions, not execution results. |
| + |
| +## Fourth-review findings |
| + |
| +“Without fix” means reverting the particular behavior, rather than reverting the entire commit. |
| + |
| +| Opus finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **P3-1 — Existing trigger not replaced** | **Partly.** Successful migration replaces it, but replacement is not atomic. Finding 3 below. | **No** if the old trigger remains. **Yes** with the current interruption gap: the test checks only successful completion. | |
| +| **P3-2 — Legacy EUR not held** | **Yes.** `billingpage.go:284` derives the held currency through `Money()`, including legacy EUR. | **No.** `billingcurrency_test.go:1998` distinguishes it. That case checks the form, not its submission. | |
| +| **P3-3 — Resolved paid-refusal review reopens** | **No; explicitly left open.** Finding 4. | No regression test establishes permanent resolution. | |
| +| **P3-4 — Static site links omit parameters** | **Yes.** Site `build.mjs:193` and `:346` include both empty parameters. | **No.** Site `pricing-pricebook.test.mjs:134` checks them. | |
| +| **P3-5 — Unsold URL currency silently replaced** | **Yes for the reported omission.** The page displays a notice; invalid submitted fields receive 400. | **No.** `billingcurrency_test.go:1975` checks refusals and `:1993` checks the notice. | |
| +| **P3-6 — Ordinary site build bypasses draft gate** | **No; explicitly unchanged.** Finding 5. | **Yes.** Existing tests invoke the generator/checker directly, not the ordinary build as a publication gate. | |
| +| **P3-7 — Test gaps** | **Partly.** Customer, incoming country cookie, occurrence count and actual webhook calls were added. Independent ownership/webhook coverage remains incomplete. | **Yes** for removing several ownership clauses or making checkout-webhook handling a successful no-op. Details below. | |
| + |
| +| Astra finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **1 — Incomplete session ownership** | **Partly.** Both create sites now reject another customer. Other paths still bypass the predicate. Finding 1. | **No** for removing the customer clause at creation; **yes** with the remaining listing/reconciliation/binding gaps. | |
| +| **2 — Quote fields can authorize unseen currency** | **Partly.** Erased flags and absent controls are compared against the effective purchase currency. Mutually consistent forged quote fields still buy without a console quote. Finding 2. | **No** for reverting effective-currency comparison. The new test explicitly **expects the forged-fields purchase to succeed**, at `billingcurrency_test.go:1960`. | |
| +| **3 — Invalid explicit currency bypasses refusal** | **Yes.** Present currency requires exactly one sold value, without normalization. | **No** for the scalar-validation reversion. Duplicate-field rejection has no corresponding case in the new form test. | |
| +| **4 — Same-second assignment makes trigger too wide** | **Yes.** The next assignment ends the span, including a higher revision in the same second. | **No.** `billingcurrency_test.go:2300` distinguishes it. | |
| +| **5 — Existing database retains old trigger** | **Partly.** Successful replacement works; interrupted replacement remains unsafe. | **No** for retaining the old trigger; **yes** for non-atomic replacement. | |
| +| **6 — Changed currency metadata bypasses review** | **Yes for that scenario.** `billingservice.go:923` reviews either terms or actual-currency mismatch. | **No.** The changed-metadata fixture at `billingcurrency_test.go:2231` distinguishes it. | |
| +| **7 — Legacy held EUR missing** | **Yes.** Same closure as Opus P3-2. | **No.** | |
| +| **8 — Static links omit both parameters** | **Yes.** Same closure as Opus P3-4. | **No.** | |
| +| **9 — Provisioning demands exactly 36 global prices** | **Closed for the reported 48-price case; incomplete for larger catalogs.** Finding 6. | **No** for literal restoration of the old expression: source-string assertions catch it. They do not exercise pagination or execute the generated checks. | |
| +| **10 — `PRICEBOOK_ALLOW_DRAFT=0` permits publication** | **Yes.** Both generator and checker require exactly `"1"`. | **No.** Site `pricing-pricebook.test.mjs:140` includes unset, empty, `0`, `false` and `yes`. | |
| + |
| +The fourth-review claims resolve as follows: |
| + |
| +- Astra was right about the customer boundary, forged quote fields, same-second history and changed-metadata review. Opus’s broader “closed” readings missed reachable code paths. |
| +- For trigger migration, both identified the same defect. **P2 is justified** because it blocks legitimate monthly calculations after upgrade; billing being disabled limits exposure, not the defect’s eventual effect. |
| +- The legacy selector, static links and unsold-URL notice fit **P3**. Astra’s invalid-POST case warranted **P2** because whitespace could become an unintended default purchase. |
| +- The nonexistent test name, demo configuration, static parameters and preview-write ordering are corrected. |
| +- Claims of universal ownership checking, complete closure and independently tested webhook behavior remain overstated. |
| + |
| +## Findings, most severe first |
| + |
| +### 1. P2 — Session ownership is still not enforced on every path |
| + |
| +**Where:** `internal/console/billingservice.go:1163`, `:769`, `:923`; `internal/console/billingpass.go:187`. |
| + |
| +The predicate at `billingservice.go:257` now checks the requested mode, intent identifier, account reference, known customer and bound session ID. Those checks are useful, but several consumers do not apply them before acting: |
| + |
| +- **Listing recovery:** An unbound GBP intent belongs to `cus_A`. The customer-scoped listing returns a session carrying its intent ID and account reference but `CustomerID=cus_B`. `intentSession` rejects it; line 1164 immediately calls `refuseSession` anyway. An open foreign session is therefore expired. This happens even when its currency is correct. |
| +- **Subscription reconciliation:** An intent is bound to `cs_A`. `GetCheckout(cs_A)` returns a completed `cs_B` with otherwise matching subscription/customer/reference/intent fields. The manual checks at line 769 never compare the returned ID with `intent.SessionID`; reconciliation can record the subscription. It also compares the session customer with the subscription customer, rather than the intent’s customer. |
| +- **Direct binding/webhook:** A completed `cs_B` carries the expected account, customer and intent metadata but differs from the bound `cs_A`. If its currency is wrong, line 923 files an account review **before** the SQL binding condition at line 938 rejects a different session ID. Different reported subscription IDs can create multiple review rows for sessions the predicate would reject. |
| +- The background completed-session branch dispatches using the returned session ID before checking it against the pending intent. |
| + |
| +These are inconsistent-provider-response cases, not demonstrated buyer-controlled exploits. They nevertheless cross the exact boundary this round asks to enforce. |
| + |
| +**Smallest acceptable fix:** Apply `ownSession` immediately after each session read/list result, before expiry, review, binding or subscription recording. Apply `intentSession` separately for purchase identity. Keep the existing subscription/payment checks and store-side conditional writes. Add independent wrong-customer listing, wrong-bound-ID reconciliation and wrong-bound-ID binding cases. |
| + |
| +The two create sites and both interactive resume guards now perform the ownership check correctly. Expired-intent retirement also checks equivalent identity conditions in the store. |
| + |
| +### 2. P2 — Matching forged quote fields still bypass the one-form quote rule |
| + |
| +**Where:** `internal/console/billinghandlers.go:87`; `internal/console/billingcurrency_test.go:1960`. |
| + |
| +**Scenario:** Load `?country=IN¤cy=gbp`, showing Personal at **£7.70**. Post: |
| + |
| +```text |
| +country=IN |
| +quoted_country=IN |
| +currency=usd |
| +quoted_currency=usd |
| +currency_chosen= |
| +``` |
| + |
| +With the normal consent and postal fields, the handler creates a **USD 9.80** checkout without first rendering that console quote. No server-issued value proves what the page showed. The new test deliberately expects this purchase. |
| + |
| +The effective-currency fix correctly stops merely erasing `currency_chosen` or removing `currency` from the original GBP form. It does not close the fabricated-quote part of Astra 2. Stripe subsequently displaying the price does not satisfy the requested console quote rule. |
| + |
| +**Smallest acceptable fix:** Authenticate the rendered quote’s account, country, currency and book/terms identity, using a signed value or independent quote ID. Compare effective purchase inputs against that quote. Preserve independent valid quotes across tabs rather than storing one mutable account-wide quote. |
| + |
| +### 3. P2 — Trigger replacement can leave the database without either barrier |
| + |
| +**Where:** `internal/console/billingstore.go:193`, `:194`, `:209`. |
| + |
| +**Scenario:** A database contains legacy September history followed by paid book history. Migration commits `DROP TRIGGER`, then crashes or fails before committing `CREATE TRIGGER`. Neither barrier remains until another successful migration. |
| + |
| +A pre-book process running against that database can now freeze its legacy-only version 1. The successful-migration test cannot detect this gap. |
| + |
| +**Smallest acceptable fix:** Replace the trigger within one explicit write transaction, preferably `BEGIN IMMEDIATE`. Verify rollback preserves the old barrier if replacement fails. |
| + |
| +After a successful migration, only the new trigger remains. A subsequent startup of an earlier branch build that creates the old name can recreate it alongside the new trigger; the base pre-book binary does not create that name. This reinforces the documented coordinated-rollout requirement. |
| + |
| +### 4. P3 — Resolving a paid-refusal review does not settle the intent |
| + |
| +**Where:** `internal/console/billingservice.go:933`; `internal/console/billingstore.go:743`. |
| + |
| +**Scenario:** An owned GBP checkout completes in another currency. An operator settles the payment and resolves its review. The intent remains pending; the next pass encounters it again and reopens the review, incrementing its occurrence count. The account cannot complete recovery through the existing resolution operation. |
| + |
| +**Smallest acceptable fix:** Add a narrowly scoped operator operation that records settlement and retires that exact refused intent atomically. Test resolution followed by another pass and webhook. |
| + |
| +Repeated encounters are deduplicated **while the same review remains open**. That is narrower than “reviewed once.” |
| + |
| +### 5. P3 — The site’s ordinary build still bypasses its draft-publication check |
| + |
| +**Where, site:** `package.json:6`; `hack/check-pricebook.mjs:7`. |
| + |
| +**Scenario:** The committed pricing HTML carries its first-line draft marker. Running `npm run build` invokes Eleventy directly and publishes that HTML without running the checker. |
| + |
| +**Smallest acceptable fix:** Run `hack/check-pricebook.mjs` before the ordinary build, or install an equivalent Eleventy hook. Test that build entry point. |
| + |
| +The generator’s exact `"1"` override, first-line marker and preview-before-refusal ordering are otherwise corrected. |
| + |
| +### 6. P3 — Provisioning still rejects valid catalogs once active prices exceed 100 |
| + |
| +**Where:** `implementation/carloku-paid-launch/stripe-provision.sh:391`. |
| + |
| +**Scenario:** The account contains the current book’s 36 active prices plus enough historical or unrelated active prices to exceed 100. Check 2 requests one global page with `limit=100` and requires `has_more == false`. It fails despite a complete, valid current catalog. |
| + |
| +**Smallest acceptable fix:** Paginate before counting, or restrict the request to the current book’s expected identities. Exercise the generated verification against a paginated fixture. |
| + |
| +The 36-plus-12 legacy case is fixed; arbitrary supported historical catalogs are not. |
| + |
| +### 7. P3 — Console and provisioning disagree on empty own-option tax behavior |
| + |
| +**Where:** `internal/console/billingservice.go:197`; `implementation/carloku-paid-launch/stripe-provision.sh:414`. |
| + |
| +**Scenario:** An otherwise valid own-only option contains: |
| + |
| +```json |
| +{"gbp":{"unit_amount":770,"tax_behavior":""}} |
| +``` |
| + |
| +The console accepts it. The provisioning expression defaults missing/null tax behavior, but rejects an explicit empty string. |
| + |
| +**Smallest acceptable fix:** Give both validators the same accepted shapes and test those shapes. This is a code-contract discrepancy; source inspection does not establish that Stripe emits this spelling. |
| + |
| +## Remaining requested checks and claims |
| + |
| +**The ordinary GBP purchase is consistent end to end.** The IN/GBP page posts band D and GBP; effective quote comparison passes. `startBillingCheckout:400` selects GBP independently of the country’s affordability band and verifies Personal’s 770-pence unit price at `:443`. `CreateCheckout`, `internal/billingstripe/client.go:621`, sends GBP with Adaptive Pricing disabled. |
| + |
| +Binding and reconciliation retain the intent’s terms. `applyBillingStateDated`, `billingstore.go:577`, refuses same-subscription book/currency changes and copies those terms into assignments at `:703`. `billingInvoiceHistory` preserves them; `AllowanceForMonth`, `internal/plans/month.go:167`, rejects incompatible paid spans. `billingcalculation.go:174` writes version 2 in GBP. Invoice creation, recovery, verification and request hashes use the frozen calculation’s currency. The quantity worker checks the stored price/currency at `billingquantity.go:1013` and `:1102`. I found no additional conversion or historical-repricing path. |
| + |
| +**Form and cookies.** `billingFormChoice:158` distinguishes absence from a present empty field and rejects repeated currency fields. Invalid scalar values receive 400 on both doors after their authentication/consent gates. A missing currency field on a book console buys the country default only if that effective currency matches the submitted quote; otherwise it redirects. A legacy console’s genuine no-control form continues to buy legacy EUR. |
| + |
| +An unchanged stale tab uses its own fields, not another tab’s cookies. A forged `"1"` preserves the posted currency; an erased flag now causes re-quotation when the effective default differs. The held currency overrides URL/cookie suggestions, including legacy EUR. |
| + |
| +The quote endpoint checks session, token and membership and constructs an escaped account-local billing redirect at `billinghandlers.go:145`. I found no open redirect or billing/provider mutation there. |
| + |
| +Present-empty parameters clear cookies; absence falls back; invalid values leave existing cookies untouched (`billingcurrency.go:48`). Thus an invalid visit followed by an absent-parameter visit can reuse an old valid choice. A cleared choice does not return that way. Cookies influence the rendered form, not checkout independently. |
| + |
| +**Trigger spans and rollout.** For valid writer-produced histories, `billingstore.go:197` now matches `AllowanceForMonth`’s positive paid span: |
| + |
| +```text |
| +max(effective_at, month_start) |
| + < min(next_assignment_effective_at, period_end, month_end) |
| +``` |
| + |
| +Both order assignments by effective time then revision across both tables. Same-second supersession contributes nothing. Free/inactive assignments contribute nothing but still terminate preceding spans. Renewal and quantity dating move `effective_at`; `period_end` remains the provider’s paid-through boundary, copied by the sole assignment-writing routine at `billingstore.go:700`. |
| + |
| +Once migration completes, I found no remaining valid-history example where this predicate permits an old version 1 over a priced book span, or rejects this build’s legitimate legacy-only version 1. The interruption gap above is separate. A fresh database receives the schema through startup migration; running older binaries after book purchases remains unsupported for the account/intent reasons documented in the roll note. |
| + |
| +**Inclusions, arithmetic and export.** Current inclusions match pooled allowances at every valid quantity. Unavailable Personal is displayed at quantity one; teams use the charged quantity. Current band products are integral in base units and display exactly within three decimals. Band A matches the asserted approved sentences. `formatFigure` truncates nonterminating shares, but no current plan produces one. |
| + |
| +I found no further book-validation, checked-arithmetic, catalog-configuration or public-disclosure defect. The site’s copied export matches the platform export at these commits. Exact rates, band scaling, country options/defaults and dynamic/static link parameters agree. A locale can display GBP while sending empty parameters and therefore lead to a USD console default; that is the deliberate distinction required by ruling 9, not an explicit choice being changed. |
| + |
| +The following claims still need correction: |
| + |
| +- **Universal closure:** `DISPOSITIONS-multi-currency.md:108` says every fourth-review finding is fixed, while `:122` and `:123` explicitly leave two open. It also misstates Astra’s fourth report as finding every earlier issue closed. |
| +- **Universal ownership:** The design at line 152 and `billingservice.go:238` claim every session passes one check. Finding 1 disproves this. |
| +- **Quote provenance:** Dispositions line 113 says signing adds nothing. It would establish precisely the rendered-quote provenance that matching client-controlled fields cannot establish. |
| +- **Ownership test coverage:** Dispositions line 124 overstates account-clause coverage. The named ownership test varies intent/customer, not wrong client reference, mode or bound session ID. Removing those latter clauses survives that test. |
| +- **Independent webhook coverage:** `billingcurrency_test.go:2201` now sends webhooks, but only after passes/direct binding have created the review. A successful no-op webhook still leaves its final row/occurrence assertions passing. The changed-metadata fixture uses passes only. |
| +- **Generated provisioning checks:** `internal/plans/provision_test.go:27` searches script-source substrings. It does not generate commands or evaluate jq. Exact textual reversions now fail; broken surrounding behavior can still pass. |
| +- **Vacuous quantity assertion:** `billingcurrency_test.go:2429` is an algebraic identity after the preceding divisibility assertion. Current figures remain exact, but this extra assertion proves nothing. |
| +- **Unqualified “once”:** `billingservice.go:281` and design line 152 still promise a review once, despite reopening after resolution. |
| +- **Remaining stale absolutes:** The design’s opening “every Checkout Session” Adaptive Pricing statement and blanket no-options statement omit the later legacy/own-only exceptions. |
| + |
| +All eight specifically listed fourth-round mutations at dispositions line 127 have distinguishing assertions in the source. That supports their plausibility, not the historical claim that they were executed. The previously missing incoming-country-cookie case now exists at `billingcurrency_test.go:1681`. The review-three default-as-explicit mutation is equivalent for forms accepted by the **new effective-currency comparison**; that does not authenticate fabricated quotes. |
| + |
| +I found no concrete calendar-date, test-order, temporary-root or shared-cookie-jar failure. Session, step-up and webhook fixtures intentionally use current timestamps. |
| + |
| +## Stripe test-mode verification before enablement |
| + |
| +1. **Catalog identity:** Read all 36 prices using the pinned API version and expanded options. Verify exact amounts, currency, metadata, recurrence and tax behavior. Exercise empty/own-only options and historical catalogs, including pagination. |
| +2. **Checkout matrix:** Complete USD, EUR and GBP purchases across matching and differing country defaults, including IN/GBP. Verify unit × quantity, session/subscription/item currency, disabled Adaptive Pricing and absent conversion. Attempt incompatible price/session currency and record the actual refusal. |
| +3. **Renewal and quantities:** Renew each currency and exercise both quantity policies and the quantity fence. Verify proration dates, amounts, currencies and duplicate-request behavior. |
| +4. **Overages:** Exercise every meter, rounding boundaries and invoice floors in each currency through draft, items, finalization and payment. Compare subtotal/lines against the frozen calculation, including canceled subscriptions and automatic-tax completion. |
| +5. **Recovery:** Lose responses at checkout and invoice-operation boundaries. Recover through replay and listing, within and beyond replay windows. Verify original identities/currencies and no duplicate purchases or charges; include legacy request compatibility. |
| +6. **Customer currency constraints:** Determine Stripe’s behavior during a live subscription, after cancellation and while old-currency invoices remain outstanding. Confirm the console’s refusal remains effective. |
| +7. **Refusals and discounts:** Establish the real response shapes for expiring open/completed sessions, incompatible fixed-amount discounts, percentage discounts and house coupons. Confirm replayed/out-of-order webhooks do not create duplicate accounting effects. |
| + |
| +**Verdict: reject.** |
| \ No newline at end of file |
diff --git a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md| index 80ea3c2fa..d5a74f8d8 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -105,12 +105,12 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## Review four (at 8958392b; Opus accepted with fixes, Astra rejected) |
| -Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency-review4.md`. Both found every third-review finding closed in the code and no path that stores or sends money in a currency other than the purchase's; the disputed trigger reading went Astra's way. Every finding of either is fixed below, each with a test that fails without it; eight mutants, one per fix, were killed in a scratch copy. |
| +Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency-review4.md`. Both found every third-review finding closed in the code and no path that stores or sends money in a currency other than the purchase's; the disputed trigger reading went Astra's way. Every finding of either that is marked fixed below has a test that fails without it; eight mutants, one per fix, were killed in a scratch copy; two items are left open and say so. |
| | Finding | Disposition | Where | Reproduction | |
| | --- | --- | --- | --- | |
| | Astra 1 (P2): `ownSession` did not compare the customer, so a session carrying the intent's id but another customer could be expired, or saved, before a later check refused it. | Fixed: `ownSession` also requires the intent's customer (once it has one) and, once bound, the bound session's id; `intentSession` starts from it; `bindBillingCheckout` checks the customer before anything. | `billingservice.go` | `TestCurrencyRefusalExpiresOnlyTheIntentsOwnSession` (another customer, in another currency and in the right one: zero expiries, nothing kept) | |
| -| Astra 2 (P2): posted quote fields and the choice flag could make the door buy a currency the page did not show (the flag erased on a chosen GBP page bought India's dollar). | Fixed: the handler compares the quote with what would be bought (the chosen currency, else the posted country's own), not with the posted field. A form whose fields disagree with each other, or has no currency control against a chosen quote, is answered with the quote for what it would buy. Fields forged to agree with each other buy exactly what they describe, which the buyer sees at the provider; a signed quote would add nothing to that, since the posted country and currency determine the price. | `billinghandlers.go` | `TestBillingFormPostsWhatItShows` ("a choice flag erased buys nothing the page did not show") | |
| +| Astra 2 (P2): posted quote fields and the choice flag could make the door buy a currency the page did not show (the flag erased on a chosen GBP page bought India's dollar). | Fixed: the handler compares the quote with what would be bought (the chosen currency, else the posted country's own), not with the posted field. A form whose fields disagree with each other, or has no currency control against a chosen quote, is answered with the quote for what it would buy. Fields forged to agree with each other bought exactly what they described, which the buyer saw at the provider; review five held that the console's own quote rule asks for more, and the quote is signed from review five on (below). | `billinghandlers.go` | `TestBillingFormPostsWhatItShows` ("a choice flag erased buys nothing the page did not show") | |
| | Astra 3 (P2), Opus P3-5: an invalid currency reached the quote redirect instead of a 400, `" "` was trimmed into absence, and an unsold `?currency=` was silently replaced on the page. | Fixed: a present currency field must be exactly a currency sold (no trimming, no folding, not empty), else 400 before anything, on the checkout and the quote door; the page says when `?currency=` names something not sold and does not post the default as a choice. | `billinghandlers.go` `billingFormChoice`; `billingpage.go`; `billing.html` (`billing.currency.unknown`, unreviewed system copy, flagged) | `TestBillingFormPostsWhatItShows` ("a currency that is not sold is refused before anything") | |
| | Astra 4 (P2): the trigger counted an active book row superseded in the same second, which `AllowanceForMonth` skips. | Fixed: the trigger's span ends at the earliest of the next assignment's effective instant (time, then revision, over both tables), `period_end` and the month's end, and must have length inside the month: `AllowanceForMonth`'s rule. | `billingstore.go` | `TestBarrierTriggerLetsThisBuildsLegacyMonthsThrough` ("superseded in the same second") | |
| | Astra 5 (P2), Opus P3-1: a database opened by an earlier build kept the old trigger. | Fixed: the trigger has a new name and the old one is dropped first at migration. | `billingstore.go` | same test ("a database that carries the earlier, wider trigger": the old body installed, migration run, the new name alone remains, the legacy month freezes) | |
| @@ -121,7 +121,7 @@ Reports: `OPUS-REVIEW-multi-currency-review4.md`, `CODEX-FINDINGS-multi-currency |
| | Astra 10 (P3): `PRICEBOOK_ALLOW_DRAFT=0` allowed a draft. | Fixed on the site: exactly `"1"` in the build and the check; the preview is written before the refusal, as the comment said. | site | site test (unset, empty, `0`, `false`, `yes` refuse) | |
| | Opus P3-6: the draft gate holds only through `npm run check`. | Stated, not changed: the runbook runs `check`; a `build` hook is a site-build decision for the morning. | | | |
| | Opus P3-3: a resolved refused-paid review is reopened by the next pass while the intent stands. | Open: an operator's door that retires such an intent once the money is settled does not exist; the comment and the design say "once while open". | | | |
| -| Opus P3-7, Astra claims: test gaps (webhook path, `occurrences`, `ownSession` clauses, console divisibility check, the script's jq, the empty-country mutation claim, the token match on the site). | Fixed: the webhook and `occurrences` in the review test; the customer and account clauses; divisibility of every figure at the minimum in the console test; the script's checks read by the plans test; `?country=` with a country cookie in the page test; token matching in `build.mjs`. | tests | as named | |
| +| Opus P3-7, Astra claims: test gaps (webhook path, `occurrences`, `ownSession` clauses, console divisibility check, the script's jq, the empty-country mutation claim, the token match on the site). | Fixed: the webhook and `occurrences` in the review test (review five showed the webhook leg could not fail and the account clauses were untested; both corrected there); divisibility of every figure at the minimum in the console test (review five showed one clause was a tautology; corrected there); the script's checks read by the plans test; `?country=` with a country cookie in the page test; token matching in `build.mjs`. | tests | as named | |
| | Claims corrected: the nonexistent test name at `billingpage.go`; "every finding has a test" (the script changes had none); "once" is once while open; the trigger "exactly the set"; the design's demo-without-a-book sentence; `build.mjs`'s "preview always written". | Corrected in this commit. | | | |
| Mutation checks on these fixes, eight in a scratch copy, each run against its test: all eight failed their tests (the customer clause removed; the posted currency compared instead of the effective one; an invalid currency normalised instead of refused; the trigger ignoring the next assignment; the old trigger left in place; a legacy purchase not held; a terms mismatch on a bound completion not reviewed; the unsold-currency notice removed). |
| @@ -129,3 +129,24 @@ Mutation checks on these fixes, eight in a scratch copy, each run against its te |
| Open after this round: the operator's door for a refused, paid session's intent (Opus P3-3); the site's `bands.p3` sentence; `billing.currency.unknown` and `billing.quote.unknown` are unreviewed system copy. |
| The fixes are in the commit that adds this section and have not been read by a reviewer. |
| + |
| +## Review five (at 1f005804; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review5.md`, `CODEX-FINDINGS-multi-currency-review5.md`. Both found every fourth-review finding marked fixed closed in the code and the GBP purchase consistent end to end. Astra's rejection rests on ownership not being applied on every path, on the fabricated-quote case, and on the trigger swap; Opus found the same three, lower. The code decides: each was reachable, and each is fixed below with a test that fails without it. Mutation checks in a scratch copy: the listing acting without ownership, reconcile ignoring the bound id and customer, the bind without ownership, the quote signature not checked, the resolution leaving the intent, the webhook alone filing nothing, and a per-member figure in the terms made not whole in base units: all killed. The pass's own check of the returned id against the intent's survived its removal and is kept as belt and braces: the bind refuses such a session on its bound-id clause, so the outcome is the same by construction. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Astra 1 (P2), Opus P3-1: `ownSession` was not applied on the listing recovery (a listed session with the intent's id but another customer was expired or reviewed), on reconcile (the session read back was not required to be the bound one, nor the intent's customer), on a direct binding (a different completed session with matching metadata could file a review before the bound-id condition), or in the pass (the returned id was handed on unchecked). | Fixed: `ownSession` is applied immediately after each read or listing, before expiry, review, binding or recording; the pass checks the returned id against the intent's; `intentSession` begins from `ownSession`. The design's "every session passes one check" is true now. | `billingservice.go`, `billingpass.go` | `TestOwnershipIsEnforcedOnEveryPath` (a listed session of another customer, in the right currency and the wrong one: nothing expired or reviewed; a read of the bound session answered with another: not bound, recorded or reviewed through the pass, the webhook, a direct binding and reconcile), `TestOwnSessionClauses` (each clause alone) | |
| +| Astra 2 (P2): quote fields forged to agree with each other bought a price no console page had shown. | Fixed: the page signs its quote (`quote_sig`: HMAC-SHA256 over the book, the quoted country, the quoted currency and the choice flag, keyed by the session's own token, which the form double-submits as `csrf`); the checkout door takes a post only with the signature of exactly that quote, else answers with the quote. Opus had accepted the earlier argument; the code now meets Astra's reading, which is the rule as written ("nothing is bought at a price the page did not show"). | `billingpage.go`, `billinghandlers.go`, `billing.html` | `TestBillingFormPostsWhatItShows` ("a choice flag erased": forged agreeing fields, a tampered signature and a missing one all redirect to the quote and open nothing; the rendered dollar quote buys; two sessions' signatures differ) | |
| +| Astra 3 (P2), Opus P3-2: the trigger swap ran as two statements, so a crash between them left neither trigger. | Fixed: the new trigger is created in the migration list as before, and the old one is dropped in a transaction that first confirms the new one exists; no moment has neither. The roll note gains: builds of this branch from before the rename must not run after this one (they would create the old name beside the new). | `billingstore.go` `retireEarlierBarrierTrigger` | `TestBarrierTriggerLetsThisBuildsLegacyMonthsThrough` ("a database that carries the earlier, wider trigger") | |
| +| Astra 4 (P3), Opus carried P3-3: resolving a refused, paid session's review left the intent standing, so the next pass reopened the review. | Fixed: resolving a review of that reason retires the account's pending intent in the same transaction; the next pass and webhook find nothing to bind and the review stays resolved. | `billingstore.go` `resolveBillingReview` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` | |
| +| Opus P3-3: one clause of the inclusions test was a tautology. | Fixed: the test asserts, for every paid plan, band factor and axis, that the figure times the factor is whole in base units and that the per-member share shows in three decimals of its unit; the tautology is gone; the page comment says what the test checks. | `billingcurrency_test.go`, `billingpage.go` | the test itself (a mutant removing the check is killed) | |
| +| Opus P3-4, Astra claims: the webhook leg ran after the review was already open; the bind customer check and three `ownSession` clauses were untested. | Fixed: the webhook alone files the review on a fresh fixture; each `ownSession` clause has a case; the bound-id and customer paths are exercised. | tests | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution`, `TestOwnSessionClauses`, `TestOwnershipIsEnforcedOnEveryPath` | |
| +| Opus P3-5, Astra 6 (P3): Check 2 counted products account-wide and read one page of 100 prices. | Fixed: every page is read (`starting_after`) and products and prices are counted by their metadata (`terms_version`, `price_book`); the plans test reads the generated checks for the pager and the filters. | `stripe-provision.sh`; `plans/provision_test.go` | `TestStripeProvisionScriptMatchesThePriceBook` | |
| +| Astra 7 (P3): the verifier refused an own-only option whose `tax_behavior` is the empty string, which the console accepts. | Fixed: the verifier accepts null, empty or `exclusive`, as the console does. | `stripe-provision.sh` | the same test (the accepted shapes are in the asserted expression) | |
| +| Astra 5 (P3), Opus P3-6: `npm run build` bypassed the draft check. | Fixed on the site (`9427c22`): the ordinary build runs the check first; the test reads both scripts. | site `package.json` | site test | |
| +| Opus claims: the design's "once"; the dispositions' "every finding fixed", the signing argument, the coverage claim; the design's Adaptive Pricing and no-options absolutes. | Corrected in this commit. | design, this file | | |
| + |
| +Open after this round: nothing marked open on this branch except what needs Stripe test mode (both reports' lists) and the site's `bands.p3` sentence; `billing.quote.unknown` and `billing.currency.unknown` remain unreviewed system copy. |
| + |
| +The fixes are in the commit that adds this section and have not been read by a reviewer. |
diff --git a/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review5.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review5.md| new file mode 100644 |
| index 000000000..17a230f0b |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review5.md |
| @@ -0,0 +1,145 @@ |
| +# Opus authority review, round five: `feat/carloku-multi-currency-20261001` at `1f005804` |
| + |
| +**Verdict: accept with fixes.** Every fourth-review finding that was marked fixed is closed in the code. Each one's test fails without its fix, with two exceptions noted below: the webhook leg of the review test, and the inclusion check's second clause. Two items remain open by declaration: Opus P3-3 and P3-6. |
| + |
| +I found no path that computes, sends, verifies or stores money in a currency other than the purchase's stored one. What remains is all P3: |
| +- The listing-recovery path still expires a session without `ownSession`. |
| +- The trigger swap is not atomic. |
| +- Some tests are vacuous. |
| +- A few claims overstate. |
| + |
| +How I worked: read-only, with `git show` at `1f005804` and site `4feb1cd`. I ran no tests. My scratch copies under `$TMPDIR` have been deleted. |
| + |
| +## Fourth-review findings |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Astra 1 / Opus P3-7 (customer)**: `ownSession` ignored the customer | **Closed at both create sites and both resume reads** (`billingservice.go:257-265`). **Not on the listing path**: see P3-1. | **No.** At `billingcurrency_test.go:2148-2161`, the `"eur"` case expires the session and the `""` case saves it. The client does not check the customer, so the service clause is the only one. | |
| +| **Astra 2**: an erased flag, or no control, bought a default the page did not show | **Closed.** The comparison is now on `effective` (`billinghandlers.go:80-91`), which is what `ChooseCurrency` buys. | **No.** "a choice flag erased…" (`:1937`) buys USD under the old comparison. | |
| +| **Astra 3 / Opus P3-5**: invalid currency redirected, `" "` became absence, unsold `?currency=` silently replaced | **Closed.** `billingFormChoice` returns not-ok for repeated, empty or non-exact values, and both doors answer 400. The page sets `CurrencyError`. | **No.** `"GBP"`, `"gbp "` and `" "` each fail under trimming or folding. The page assertion fails without the notice. | |
| +| **Astra 4**: trigger counted a row superseded in the same second | **Closed.** The span formula is exactly `AllowanceForMonth`'s (section N). | **No.** The old body refuses the September freeze. | |
| +| **Astra 5 / Opus P3-1**: old trigger kept on existing databases | **Closed for this binary.** See P3-3 for atomicity and branch rollback. | **No.** Without the DROP, `group_concat` names two triggers. | |
| +| **Astra 6**: a metadata+currency change skipped the review | **Closed** (`billingservice.go:923`). | **No.** The old code returned before the review, giving zero reviews. The webhook leg cannot fail (P3-4). | |
| +| **Astra 7 / Opus P3-2**: legacy purchase not held | **Closed** (`billingpage.go:284-287`). The page predicate `Subscription!="" && Plan!=""` equals the door's `Plan!=""`, because `applyBillingStateDated` writes both together. | **No.** | |
| +| **Astra 8 / Opus P3-4**: static site links | **Closed** (`build.mjs` card and `start.cta.console`). | **No.** `pricing-pricebook.test.mjs:136` counts 5 links carrying the empty parameters. | |
| +| **Astra 9**: Check 2 demanded exactly 36 in the account | **Closed.** The count is now by `metadata.price_book`. | **No.** `provision_test.go` reads the generated strings and refuses the old ones. See P3-5 for what remains. | |
| +| **Astra 10**: `PRICEBOOK_ALLOW_DRAFT=0` | **Closed.** `=== "1"` in both the build and the check. The preview is now written before the throw. | **No** (`:145` tests the values). | |
| +| **Opus P3-3**: resolved review reopens | **Open, as declared.** The comment at `billingservice.go:924-929` says so. | n/a | |
| +| **Opus P3-6**: draft gate only via `check` | **Open, as declared.** `npm run build` is bare `eleventy`, and the committed `src/pricing/index.html` starts with the draft marker. | n/a | |
| +| **Opus P3-7, other gaps** | Partly closed: the webhook is now called and `occurrences` is checked. The `client_reference_id` and live-mode clauses are still untested (the fake hard-codes both). The customer check in `bindBillingCheckout` has no test. | See P3-4. | |
| + |
| +**Where the fourth review's two reviewers disagreed, the code sides with Astra on substance:** |
| +- **The trigger (Astra 4).** Opus said "this build cannot hit that". It can: the test reproduces it with two `applyBillingState` calls in one second. |
| +- **The metadata-change review (Astra 6).** Opus missed it. |
| +- **The forged and invalid-currency posts (Astra 2 and 3).** Both broke owner rules 1 and 9 at `8958392b`. |
| + |
| +On severity, Opus's P3 was the better label for Astra 1, 2 and 3: |
| +- Astra 1 needs an inconsistent provider response. |
| +- Astra 2 and 3 need the buyer to craft their own post, and the amount is shown at Stripe before anything is paid. |
| + |
| +## Findings, most severe first |
| + |
| +**P3-1. The listing recovery expires a session without the ownership test.** |
| +- **Where:** `billingservice.go:1163`, in `recoverReadyIntentSession`. |
| +- **Scenario:** an intent bound to customer `cus_A` loses its create response. The pass lists `customer=cus_A`, and the provider (inconsistently) returns a session with the intent's `checkout_intent` and `client_reference_id` but `customer=cus_B`. Lines 1160-1162 pass, and `intentSession` fails only because `ownSession` fails. `refuseSession` then expires it, or, if it was paid, files a review. This is the Astra 1 class, on the path the design (`:152`) and the dispositions say passes the one test. `listCheckouts` (`billingstripe/catalog.go:137`) does not filter by customer on the client side. |
| +- **Fix:** before line 1163, add `if !b.ownSession(&session, intent, acct) { return nil, errBillingIdentity }`, and add a listing case with another customer. |
| +- **Related, no money moves:** |
| + - `reconcileBillingSubscription:769` compares the bound session's customer with the subscription's, not with `intent.Customer`. |
| + - The pass's `complete` branch (`billingpass.go:190`) hands `session.ID` to `bindBillingCheckout` without checking that it equals `intent.SessionID`. The bind's UPDATE prevents a wrong binding, but a review can be filed for another session. |
| + |
| +**P3-2. The trigger swap is not atomic, and an earlier branch build reinstalls the old trigger.** |
| +- **Where:** `billingstore.go:193-207`. These are separate `Exec` calls with no transaction. |
| +- **Scenarios:** |
| + - A crash between the DROP and the CREATE leaves **neither** trigger until this binary opens the database again. |
| + - Running `a88a39e6`, `de81dd70` or `8958392b` after this binary recreates `..._over_book_history` by `IF NOT EXISTS`, leaving **both**. The wider one then refuses this build's legitimate version 1 (the cancelled-purchase September). |
| +- Only dev, demo or test-mode databases can carry those builds. |
| +- **Fix:** run the DROP and CREATE in one `BEGIN IMMEDIATE`, and add a roll note that those three branch builds must not run after this one. |
| + |
| +**P3-3. The inclusion test's "stable across quantities" check is a tautology.** |
| +- **Where:** `billingcurrency_test.go:2429`. After the check at `:2423`, `share*MinQuantity == pooled`, so `f.per*7*share*MinQ != f.per*7*f.pooled` can never be true. |
| +- **What can still slip through:** |
| + - `pooled(min) % min == 0` does not imply `per*bps % 10000 == 0`. For example, `per*bps = 10000k + r` with `r < 1000` passes at a minimum of 10. |
| + - The display units (3600 s, 1e9 bytes) at three decimals are not checked either. |
| +- **Today's figures are exact.** I computed `per*bps % 10000 == 0`, and at most three decimals, for every paid plan, band and axis. Free is not band-scaled. |
| +- The comment at `billingpage.go:405-408` ("a future figure that is not would fail there") overstates the test. |
| +- **Fix:** assert `per*bps % 10000 == 0` and `(per*bps/10000*1000) % unit == 0` per paid plan, band and axis, and delete line 2429. |
| + |
| +**P3-4. Remaining test gaps.** |
| +- **The webhook leg** (`:2200-2213`) runs after the review is already open. `fileBillingReviewOnce` is a no-op while it is open, so the leg passes even if the webhook never reached the review code. |
| + - Fix: resolve the review first, or run the webhook on a fresh fixture. |
| +- **The customer check in `bindBillingCheckout`** (`billingservice.go:920`) has no test. |
| +- **`ownSession`'s clauses** for the bound session id, `client_reference_id` and live mode have no case. |
| + |
| +**P3-5. The provisioning script's Check 2 still fails closed on a larger account.** |
| +- **Where:** `stripe-provision.sh:388-392`. |
| +- **Scenarios:** |
| + - "Exactly 3 active products" is counted account-wide, so any unrelated product fails it. |
| + - `limit=100` with `has_more == false` fails once more than 100 prices are active, for example three books plus the legacy twelve. |
| +- **Fix:** select products by metadata, and paginate or filter prices with `-d lookup_keys` or metadata. |
| + |
| +**Carried, declared open:** Opus P3-3 (a reopened review needs an operator door) and Opus P3-6 (the draft gate holds only through `check`). |
| + |
| +## Section notes |
| + |
| +- **A. One GBP purchase, end to end.** The page at `?country=IN¤cy=gbp` sets `currency_chosen=1`, `quoted_currency=gbp`, `quoted_country=IN`. In the handler, `effective=gbp` matches the quote, so `posted=gbp`. `ChooseCurrency` gives GBP and the band is D. The price is checked for currency and amount, and `CreateCheckout` sends `currency=gbp` with Adaptive Pricing off. From there, bind, reconcile, `applyBillingStateDated`, `billingInvoiceHistory`, `AllowanceForMonth`, `priceBillingCalculation` (version 2, GBP), the engine and the quantity worker are unchanged since review four and stay in GBP. |
| +- **B. The one-form rule and the quote door.** |
| + - A changed country or currency, an erased flag, or no control against a chosen quote all redirect to a quote. |
| + - A stale tab buys its own quote. |
| + - A forged `currency_chosen=1` on a default page buys the same default. |
| + - Fields forged to agree with each other buy exactly what they describe, with the amount shown at Stripe. I accept the dispositions' argument against a signed quote. |
| + - The quote door redirects only to `/a/{PathEscape(sqid)}/billing?{Encode}` and writes nothing. |
| + - The default is bought in every path. With no cookie, with a cleared cookie, and on a default page, `effective == CurrencyForCountry(book, country)`, which is what the door picks. A held account is offered its currency alone, whatever the URL or cookie says. |
| +- **C. Cookies.** Present and empty clears, absent falls back, invalid sets nothing. An invalid visit followed by an absent one brings back the earlier cookie choice. It is shown before purchase, so this is by design. Checkout never reads cookies. |
| +- **D. Refusals.** Both create sites and both resume reads use `ownSession`. The listing path does not (P3-1). The webhook, the pass and a direct bind reach one open review. No path records or expires a non-owned session, apart from P3-1. |
| +- **E and N. The trigger.** It equals `AllowanceForMonth`: `max(eff, start) < min(period_end, month end, next (eff, rev) over both tables)`, `status='active'`, not free, and revisions share one counter. Each case: |
| + - **Same-second supersession:** an empty span in both. |
| + - **Renewal:** the dated renewal begins at the old `period_end`. A late, undated renewal leaves a gap in both. |
| + - **Quantity change:** dated at the operation's instant in both. |
| + - **Free:** excluded in both. |
| + - **`period_end`:** its only writer is `applyBillingStateDated`, from `sub.CurrentPeriodEnd`. It is the provider's period end, read as `PaidThrough`. |
| + - **The remaining gap:** the trigger runs only at freeze time. A first assignment dated `sub.Created` can land in a month already frozen. This build does the same for legacy purchases, and the held currency keeps that month's currency the same, so this is not a currency path. |
| +- **F. Inclusions.** Exact at every paid plan, band and quantity. The unavailable Personal row is priced at quantity 1. Band A is unchanged. |
| +- **H. Schema and rollout.** Sound, apart from P3-2. |
| +- **J. The export.** Unchanged. |
| +- **K. The pricing site.** |
| + - The static links now carry empty parameters. The script sends a lower-case currency only when the visitor chose one, and the country as selected. |
| + - The default comes from `data-default-currency`, and `must()` matches whole tokens. |
| + - One visible difference remains, consistent with ruling 9: a locale guess (en-GB with no country selected) shows £ on the site and $ in the console. |
| +- **M. The quote comparison.** |
| + - **Repeated `currency`:** 400. Repeated `quoted_*`, `country` or `currency_chosen` take the first value, consistently on both sides. |
| + - **Present but empty:** 400. |
| + - **No field, book console:** the quote is compared with the country default. |
| + - **No field, legacy console:** this is test-only, because `cmd/console` refuses an empty book. `CurrencyForCountry("")` returns EUR, which equals the page's `quoted_currency=eur`. |
| +- **O. Held currency.** A legacy EUR account is held in EUR and buys book EUR. A month that holds both the legacy span and the new book span returns `ErrPriceTerms` and goes to review, as designed. |
| + |
| +## Claims the code contradicts |
| + |
| +- **The design (`:152`):** |
| + - "Every session the console … lists … passes one check": not on the listing path (P3-1). |
| + - "a review filed once": it is once *while open*. The dispositions corrected this; the design did not. |
| +- **The dispositions' review-four row for Opus P3-7** says "the customer and account clauses" are tested. Only the customer clause is. |
| +- **"The webhook path is now exercised":** it is called, but its leg cannot fail (P3-4). |
| +- **`billingpage.go:405-408` and the inclusion test's comment:** the divisibility check does not catch a future inexact figure (P3-3). |
| +- **"400 before anything":** the consent redirect and step-up come before it. Nothing is created either way. |
| +- **"eight mutants killed":** I agree all eight are killed. No mutant was claimed for the bind customer check or the bound-session clause of `ownSession`, and neither has a test. |
| + |
| +## To run in Stripe test mode before enabling |
| + |
| +1. **All 36 prices,** read with `expand[]=currency_options`. Expect an empty map or only the price's own currency, and expect Check 2 (with other books present) and Check 3 to pass. |
| +2. **A session per currency** (with `adaptive_pricing[enabled]=false`) for buyers in IE, GB, US, IN and JP. Expect the session currency to equal the price's, no `currency_conversion`, and a subtotal of unit × quantity. |
| +3. **A session whose currency differs from its price's.** Expect a 400 at create; note the error code. |
| +4. **The completed subscription.** Expect the subscription and item currency, and the `price_book` and `currency` metadata, to match the purchase. |
| +5. **The customer's currency lock:** |
| + - A USD checkout while a GBP subscription is live: expect it refused, and note at which call. |
| + - After cancelling, on the same customer: is USD then allowed? |
| + - Is a GBP overage invoice for an old month still accepted afterwards? |
| +6. **Overage invoices in GBP and USD** (draft, items, finalize, pay) with only the IE tax registrations. Expect tax to complete and the subtotal to equal the calculation. |
| +7. **A GBP quantity change and a GBP renewal.** Expect the proration lines and the renewal invoice in GBP. |
| +8. **Replays after a lost response:** |
| + - The same key and parameters: expect the same session. |
| + - A legacy request under its original key: expect it accepted. |
| + - `GET /v1/checkout/sessions?customer=` must return only that customer's sessions (the assumption behind P3-1). |
| +9. **`ExpireCheckout` on a completed session.** Note the error shape. |
| +10. **Promotion codes.** A EUR `amount_off` code on a GBP session: expect it refused. The 50% code and the house coupon: expect them applied. |
| + |
| +**Verdict: accept with fixes.** Fix P3-1 and P3-2 before enabling; the rest can follow. |
diff --git a/implementation/carloku-paid-launch/stripe-provision.sh b/implementation/carloku-paid-launch/stripe-provision.sh| index 64e210649..222fb5306 100755 |
| --- a/implementation/carloku-paid-launch/stripe-provision.sh |
| +++ b/implementation/carloku-paid-launch/stripe-provision.sh |
| @@ -385,11 +385,27 @@ print_check() { |
| words stripe get /v1/account "${MODE_WORDS[@]}" |
| printf ' \\\n | jq -e --arg a "$EXPECTED_ACCOUNT" %s\n\n' "'.id == \$a and .country == \"IE\" and .default_currency == \"eur\" and .charges_enabled == true'" |
| - section "Check 2. Counts: exactly 3 active products, and exactly 36 active recurring prices of this book (other books' and legacy prices may stand beside them)" |
| - words stripe get /v1/products "${MODE_WORDS[@]}" -d active=true -d limit=100 |
| - printf ' \\\n | jq -e %s\n' "'(.data | length) == 3 and (.has_more == false)'" |
| - words stripe get /v1/prices "${MODE_WORDS[@]}" -d active=true -d limit=100 |
| - printf ' \\\n | jq -e --arg book "%s" %s\n\n' "$BOOK" "'([.data[] | select(.metadata.price_book == \$book)] | length) == 36 and (.has_more == false)'" |
| + section "Check 2. Counts: exactly 3 active products of these terms, and exactly 36 active recurring prices of this book (other books', legacy and unrelated objects may stand beside them, in any number)" |
| + note "Every page is read (starting_after) before counting; a single page of 100 would fail a larger account for no fault of the catalog." |
| + local pager |
| + pager=$( |
| + cat <<'EOF' |
| +all_of() { # all_of <path> <query...>: every object of a list endpoint, as a JSON array |
| + local path="$1"; shift; local all='[]' after='' page |
| + while :; do |
| + page="$(stripe get "$path" @FLAGS@ -d limit=100 ${after:+-d "starting_after=$after"} "$@")" |
| + all="$(jq -cs '.[0] + .[1].data' <<<"$all $page")" |
| + [ "$(jq -r .has_more <<<"$page")" = true ] || break |
| + after="$(jq -r '.data[-1].id' <<<"$page")" |
| + done |
| + printf '%s' "$all" |
| +} |
| +all_of /v1/products -d active=true | jq -e --arg terms "$TERMS" '([.[] | select(.metadata.terms_version == $terms)] | length) == 3' >/dev/null || { echo "PRODUCT COUNT FAILED" >&2; exit 1; } |
| +all_of /v1/prices -d active=true | jq -e --arg book "$BOOK" '([.[] | select(.metadata.price_book == $book)] | length) == 36' >/dev/null || { echo "PRICE COUNT FAILED" >&2; exit 1; } |
| +echo "counts ok: 3 products of $TERMS, 36 prices of $BOOK" |
| +EOF |
| + ) |
| + printf '%s\n\n' "${pager//@FLAGS@/$MODE_TEXT}" |
| section "Check 3. Every price and its product (the GetPrice and GetProduct fields the console compares)" |
| note "Console rules: active, livemode matches mode, the key's currency, exclusive, year x1, licensed, per_unit, no transform_quantity," |
| @@ -411,7 +427,7 @@ for key in "${!CENTS[@]}"; do |
| and .tax_behavior == "exclusive" and .billing_scheme == "per_unit" and .transform_quantity == null |
| and .unit_amount == $cents and .recurring.interval == "year" and .recurring.interval_count == 1 |
| and .recurring.usage_type == "licensed" |
| - and ((.currency_options // {}) | to_entries | all(.key == $currency and .value.unit_amount == $cents and ((.value.tax_behavior // "exclusive") == "exclusive"))) |
| + and ((.currency_options // {}) | to_entries | all(.key == $currency and .value.unit_amount == $cents and (.value.tax_behavior == null or .value.tax_behavior == "" or .value.tax_behavior == "exclusive"))) |
| and .metadata.terms_version == $terms and .metadata.price_book == $book and .metadata.currency == $currency |
| and .metadata.plan == $plan and .metadata.band == $band' \ |
| <<<"$price" >/dev/null || { echo "PRICE CHECK FAILED: $key $id" >&2; exit 1; } |
diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go| index 7733902ab..54bbf2994 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -4,6 +4,7 @@ import ( |
| "context" |
| "crypto/hmac" |
| "crypto/sha256" |
| + "database/sql" |
| "encoding/hex" |
| "encoding/json" |
| "errors" |
| @@ -17,6 +18,7 @@ import ( |
| "testing" |
| "time" |
| + "github.com/carlosframework/platform/internal/billingstripe" |
| "github.com/carlosframework/platform/internal/plans" |
| "github.com/carlosframework/platform/internal/usage" |
| ) |
| @@ -198,6 +200,8 @@ func TestCheckoutFormCurrencyField(t *testing.T) { |
| form.Set("quoted_currency", currency) |
| form.Set("currency_chosen", "1") |
| } |
| + // The quote as the page would have signed it for this session. |
| + form.Set("quote_sig", billingQuoteSignature(raw, fx.srv.billing.cfg.PriceBook, "IE", form.Get("quoted_currency"), form.Get("currency_chosen") == "1")) |
| r := httptest.NewRequest(http.MethodPost, fx.path("/billing/checkout"), strings.NewReader(form.Encode())) |
| r.Header.Set("Content-Type", "application/x-www-form-urlencoded") |
| r.AddCookie(&http.Cookie{Name: sessionCookie, Value: raw}) |
| @@ -1957,19 +1961,46 @@ func TestBillingFormPostsWhatItShows(t *testing.T) { |
| if w := b.submit(fields); w.Code != http.StatusSeeOther || w.Header().Get("Location") != fx.path("/billing")+"?country=IN¤cy=" { |
| t.Fatalf("no control: %d %s", w.Code, w.Header().Get("Location")) |
| } |
| - // Forged quote fields that agree with each other buy exactly what |
| - // they describe, which the buyer then sees at the provider: the |
| - // dollar at band D, not the pound the page showed. |
| - _, fields = b.page("?country=IN¤cy=gbp") |
| - fields.Set("currency", "usd") |
| - fields.Set("currency_chosen", "") |
| - fields.Set("quoted_currency", "usd") |
| + // Quote fields forged to agree with each other, from a page that |
| + // priced something else: the page's signature does not cover them, |
| + // so the post is answered with the quote for what it describes and |
| + // buys nothing; the same fields with a tampered signature, likewise. |
| + for _, tamper := range []func(url.Values){ |
| + func(f url.Values) { |
| + f.Set("currency", "usd") |
| + f.Set("currency_chosen", "") |
| + f.Set("quoted_currency", "usd") |
| + }, |
| + func(f url.Values) { f.Set("quote_sig", "0000") }, |
| + func(f url.Values) { f.Del("quote_sig") }, |
| + } { |
| + _, fields = b.page("?country=IN¤cy=gbp") |
| + tamper(fields) |
| + fields.Set("postal", "110001") |
| + if w := b.submit(fields); w.Code != http.StatusSeeOther || !strings.HasPrefix(w.Header().Get("Location"), fx.path("/billing")+"?country=IN¤cy=") { |
| + t.Fatalf("forged or unsigned quote: %d %s", w.Code, w.Header().Get("Location")) |
| + } |
| + if _, err := fx.st.billingCheckoutIntent(acct.ID); err == nil { |
| + t.Fatal("an intent was opened on a quote no page rendered") |
| + } |
| + } |
| + // And the quote the page did render for the dollar buys the dollar. |
| + _, fields = b.page("?country=IN¤cy=") |
| fields.Set("postal", "110001") |
| if w := b.submit(fields); w.Code != http.StatusSeeOther || !strings.HasPrefix(w.Header().Get("Location"), "https://checkout.stripe.com/") { |
| - t.Fatalf("forged agreeing fields: %d %s", w.Code, w.Header().Get("Location")) |
| + t.Fatalf("the rendered dollar quote: %d %s", w.Code, w.Header().Get("Location")) |
| } |
| if intent, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || intent.Currency != "usd" || intent.Band != plans.BandD { |
| - t.Fatalf("intent %+v %v; want the dollar the fields described", intent, err) |
| + t.Fatalf("intent %+v %v; want the dollar the page quoted", intent, err) |
| + } |
| + // A signature is for one session: another owner's token does not |
| + // sign this one's quote. |
| + other := newBillingBrowser(t, fx) |
| + _, fields = other.page("?country=IN¤cy=gbp") |
| + theirs := fields.Get("quote_sig") |
| + _, fields = b.page("?country=IN¤cy=gbp") |
| + if fields.Get("quote_sig") == theirs { |
| + t.Fatal("two sessions share a quote signature") |
| } |
| }) |
| t.Run("a currency that is not sold is refused before anything", func(t *testing.T) { |
| @@ -2423,11 +2454,25 @@ func TestBillingInclusionsFollowTheQuotedBand(t *testing.T) { |
| if f.pooled%def.MinQuantity != 0 { |
| t.Errorf("%s band %s: %s pooled %d is not a whole multiple of %d members", plan, band, f.name, f.pooled, def.MinQuantity) |
| } |
| - // The per-member share at the minimum must equal the share at |
| - // any other quantity: that holds exactly when figure x factor |
| - // is whole, which the quote at one member's worth shows. |
| - if share := f.pooled / def.MinQuantity; share*7 != 0 && (f.per*7*share*def.MinQuantity != f.per*7*f.pooled) { |
| - t.Errorf("%s band %s: %s share is not stable across quantities", plan, band, f.name) |
| + // Exact at every quantity, not only the minimum: the figure |
| + // times the band factor is whole in base units, and its |
| + // per-member share shows in three decimals of its unit. |
| + bps := q1.Allowances.AwakeSeconds // placeholder, replaced below |
| + _ = bps |
| + for _, factor := range []int64{10000, 7500, 5000, 3500} { |
| + if (f.per*factor)%10000 != 0 { |
| + t.Errorf("%s: %s %d x %d bps is not whole in base units; the page would have to state the pooled figure", plan, f.name, f.per, factor) |
| + } |
| + unit := int64(1) |
| + switch f.name { |
| + case "awake": |
| + unit = 3600 |
| + case "storage", "transfer": |
| + unit = 1_000_000_000 |
| + } |
| + if share := f.per * factor / 10000; (share*1000)%unit != 0 { |
| + t.Errorf("%s: %s share %d at %d bps does not show in three decimals of its unit", plan, f.name, share, factor) |
| + } |
| } |
| } |
| } |
| @@ -2464,3 +2509,198 @@ func TestBillingInclusionsFollowTheQuotedBand(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// ownSession is the one test every session passes before it is expired, |
| +// offered, bound or recorded from: this mode, the intent's id, this |
| +// account's reference, the intent's customer once it has one, and, once |
| +// bound, the bound session's id. |
| +func TestOwnSessionClauses(t *testing.T) { |
| + fx, _, acct := newCurrencyFixture(t) |
| + b := fx.srv.billing |
| + intent := billingIntent{ID: "0123456789abcdef0123456789abcdef0123456789abcdef", Customer: "cus_paid"} |
| + good := func() *billingstripe.Session { |
| + return &billingstripe.Session{ID: "cs_a", LiveMode: b.cfg.LiveMode, CustomerID: "cus_paid", ClientReferenceID: acct.Sqid, Metadata: map[string]string{"checkout_intent": intent.ID}} |
| + } |
| + if !b.ownSession(good(), intent, acct) { |
| + t.Fatal("the intent's own session is not its own") |
| + } |
| + for name, spoil := range map[string]func(s *billingstripe.Session, i *billingIntent){ |
| + "no id": func(s *billingstripe.Session, _ *billingIntent) { s.ID = "" }, |
| + "the other mode": func(s *billingstripe.Session, _ *billingIntent) { s.LiveMode = !s.LiveMode }, |
| + "another intent": func(s *billingstripe.Session, _ *billingIntent) { |
| + s.Metadata["checkout_intent"] = "fedcba9876543210fedcba9876543210fedcba9876543210" |
| + }, |
| + "another account": func(s *billingstripe.Session, _ *billingIntent) { s.ClientReferenceID = "zzz" }, |
| + "another customer": func(s *billingstripe.Session, _ *billingIntent) { s.CustomerID = "cus_other" }, |
| + "not the bound session": func(_ *billingstripe.Session, i *billingIntent) { i.SessionID = "cs_b" }, |
| + } { |
| + s, i := good(), intent |
| + spoil(s, &i) |
| + if b.ownSession(s, i, acct) { |
| + t.Errorf("%s: taken for the intent's own", name) |
| + } |
| + } |
| + // Before the intent has a customer, any customer; once bound, the |
| + // bound id and no other. |
| + i := intent |
| + i.Customer = "" |
| + if !b.ownSession(good(), i, acct) { |
| + t.Error("an intent with no customer yet refused its session") |
| + } |
| + i = intent |
| + i.SessionID = "cs_a" |
| + if !b.ownSession(good(), i, acct) { |
| + t.Error("the bound session refused") |
| + } |
| +} |
| + |
| +// Ownership is enforced on every path the provider's answer reaches: a |
| +// listed session with the intent's id but another customer is neither |
| +// expired nor reviewed; a read of the bound session answered with another |
| +// session is neither bound, recorded nor reviewed, through the pass, the |
| +// webhook, a direct binding and reconcile. |
| +func TestOwnershipIsEnforcedOnEveryPath(t *testing.T) { |
| + reviews := func(t *testing.T, fx *consoleFixture, acct *Account) int { |
| + var n int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_reviews WHERE account_id=? AND reason=?`, acct.ID, billingCheckoutRefusedReason).Scan(&n); err != nil { |
| + t.Fatal(err) |
| + } |
| + return n |
| + } |
| + webhook := func(t *testing.T, fx *consoleFixture, id string) { |
| + payload := fmt.Sprintf(`{"id":%q,"type":"checkout.session.completed","created":%d,"livemode":false,"data":{"object":{"id":"cs_test_paid"}}}`, id, time.Now().Unix()) |
| + ts := time.Now().Unix() |
| + mac := hmac.New(sha256.New, []byte("whsec_test")) |
| + fmt.Fprintf(mac, "%d.%s", ts, payload) |
| + r := httptest.NewRequest(http.MethodPost, "/billing/webhook", strings.NewReader(payload)) |
| + r.Header.Set("Stripe-Signature", fmt.Sprintf("t=%d,v1=%s", ts, hex.EncodeToString(mac.Sum(nil)))) |
| + w := httptest.NewRecorder() |
| + fx.srv.ServeHTTP(w, r) |
| + } |
| + t.Run("a listed session of another customer", func(t *testing.T) { |
| + for _, currency := range []string{"eur", ""} { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry, f.failCreate, f.sessionCustomer, f.sessionCurrency = "GB", true, "cus_other", currency |
| + if _, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err == nil { |
| + t.Fatal("fixture: the create response was not lost") |
| + } |
| + f.mu.Lock() |
| + f.failCreate, f.sessionStatus = false, "open" |
| + f.mu.Unlock() |
| + _ = fx.srv.billingPass(context.Background(), f.now.Add(time.Minute)) |
| + intent, err := fx.st.billingCheckoutIntent(acct.ID) |
| + if err != nil || intent.SessionID != "" || f.expireCalls != 0 || reviews(t, fx, acct) != 0 { |
| + t.Fatalf("currency %q: intent %+v %v, %d expiries, %d reviews; want nothing done with another customer's session", currency, intent, err, f.expireCalls, reviews(t, fx, acct)) |
| + } |
| + } |
| + }) |
| + t.Run("the bound session completed for another customer", func(t *testing.T) { |
| + for _, currency := range []string{"", "eur"} { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + intent, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + f.mu.Lock() |
| + f.sessionStatus, f.sessionCustomer, f.sessionCurrency, f.subscriptionPrice = "complete", "cus_other", currency, intent.Price |
| + f.mu.Unlock() |
| + if _, err := fx.srv.bindBillingCheckout(context.Background(), intent.SessionID); err == nil { |
| + t.Fatalf("currency %q: a session of another customer bound", currency) |
| + } |
| + _ = fx.srv.billingPass(context.Background(), f.now.Add(time.Minute)) |
| + webhook(t, fx, "evt_customer_"+currency) |
| + state, _ := fx.st.billingAccount(acct.ID) |
| + if state.Subscription != "" || f.expireCalls != 0 || reviews(t, fx, acct) != 0 { |
| + t.Fatalf("currency %q: state %+v, %d expiries, %d reviews; want nothing recorded, expired or reviewed for another customer's session", currency, state, f.expireCalls, reviews(t, fx, acct)) |
| + } |
| + } |
| + }) |
| + t.Run("a read of the bound session answered with another", func(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + intent, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + for _, currency := range []string{"", "eur"} { |
| + f.mu.Lock() |
| + f.sessionStatus, f.sessionID, f.sessionCurrency, f.subscriptionPrice = "complete", "cs_other", currency, intent.Price |
| + f.mu.Unlock() |
| + _ = fx.srv.billingPass(context.Background(), f.now.Add(time.Minute)) |
| + webhook(t, fx, "evt_other_"+currency) |
| + if _, err := fx.srv.bindBillingCheckout(context.Background(), "cs_test_paid"); err == nil { |
| + t.Fatalf("currency %q: another session bound directly", currency) |
| + } |
| + if err := fx.srv.reconcileBillingSubscription(context.Background(), "sub_paid", "", f.now.Add(time.Minute)); err == nil { |
| + t.Fatalf("currency %q: reconciled from another session", currency) |
| + } |
| + state, _ := fx.st.billingAccount(acct.ID) |
| + if state.Subscription != "" || f.expireCalls != 0 || reviews(t, fx, acct) != 0 { |
| + t.Fatalf("currency %q: state %+v, %d expiries, %d reviews; want nothing recorded, expired or reviewed from another session", currency, state, f.expireCalls, reviews(t, fx, acct)) |
| + } |
| + } |
| + }) |
| +} |
| + |
| +// The webhook alone files the review of a bound session completed in |
| +// another currency (no pass or binding before it), and resolving that |
| +// review retires the intent, so the next pass and webhook find nothing and |
| +// the review does not come back. |
| +func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "GB" |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanPersonal, "GB", "", "", f.now); err != nil || dest == "" { |
| + t.Fatalf("first click: dest %q err %v", dest, err) |
| + } |
| + intent, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + f.mu.Lock() |
| + f.sessionStatus, f.sessionCurrency, f.subscriptionPrice = "complete", "eur", intent.Price |
| + f.mu.Unlock() |
| + send := func(id string) { |
| + payload := fmt.Sprintf(`{"id":%q,"type":"checkout.session.completed","created":%d,"livemode":false,"data":{"object":{"id":%q}}}`, id, time.Now().Unix(), intent.SessionID) |
| + ts := time.Now().Unix() |
| + mac := hmac.New(sha256.New, []byte("whsec_test")) |
| + fmt.Fprintf(mac, "%d.%s", ts, payload) |
| + r := httptest.NewRequest(http.MethodPost, "/billing/webhook", strings.NewReader(payload)) |
| + r.Header.Set("Stripe-Signature", fmt.Sprintf("t=%d,v1=%s", ts, hex.EncodeToString(mac.Sum(nil)))) |
| + w := httptest.NewRecorder() |
| + fx.srv.ServeHTTP(w, r) |
| + } |
| + review := func() (open int, occurrences int64) { |
| + var resolved int64 |
| + err := fx.st.db.QueryRow(`SELECT occurrences, resolved_at FROM billing_reviews WHERE account_id=? AND reason=?`, acct.ID, billingCheckoutRefusedReason).Scan(&occurrences, &resolved) |
| + if errors.Is(err, sql.ErrNoRows) { |
| + return 0, 0 |
| + } |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if resolved == 0 { |
| + return 1, occurrences |
| + } |
| + return 0, occurrences |
| + } |
| + send("evt_first") |
| + if open, n := review(); open != 1 || n != 1 { |
| + t.Fatalf("after the webhook alone: %d open, %d occurrences; want the review filed by it", open, n) |
| + } |
| + // The operator settles the money and resolves the review. |
| + if err := fx.st.SetOperator("paul@keymail.dev", true); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.resolveBillingReview(billingReviewResolution{AccountID: acct.ID, Subscription: "sub_paid", Reason: billingCheckoutRefusedReason, Occurrence: 1, Actor: "paul@keymail.dev", Note: "refunded"}, f.now.Add(time.Hour)); err != nil { |
| + t.Fatalf("resolve: %v", err) |
| + } |
| + if _, err := fx.st.billingCheckoutIntent(acct.ID); err == nil { |
| + t.Fatal("the refused intent still stands after its review was resolved") |
| + } |
| + _ = fx.srv.billingPass(context.Background(), f.now.Add(2*time.Hour)) |
| + send("evt_again") |
| + if open, n := review(); open != 0 || n != 1 { |
| + t.Fatalf("after resolution, a pass and a webhook: %d open, %d occurrences; want the review to stay resolved", open, n) |
| + } |
| + if state, _ := fx.st.billingAccount(acct.ID); state.Subscription != "" || f.expireCalls != 0 { |
| + t.Fatalf("state %+v, %d expiries; want nothing recorded or expired", state, f.expireCalls) |
| + } |
| +} |
diff --git a/internal/console/billinghandlers.go b/internal/console/billinghandlers.go| index 83fe5ae64..f2a3e6d89 100644 |
| --- a/internal/console/billinghandlers.go |
| +++ b/internal/console/billinghandlers.go |
| @@ -84,7 +84,11 @@ func (s *Server) handleBillingCheckout(w http.ResponseWriter, r *http.Request) { |
| effective = string(c) |
| } |
| } |
| - if country != r.PostForm.Get("quoted_country") || effective != r.PostForm.Get("quoted_currency") { |
| + // And the quote has to be one a page rendered for this session (its |
| + // signature, keyed by the session token the form double-submits), so |
| + // fields arranged to agree with each other buy nothing unseen. |
| + quotedCountry, quotedCurrency, quotedChosen := r.PostForm.Get("quoted_country"), r.PostForm.Get("quoted_currency"), r.PostForm.Get("currency_chosen") == "1" |
| + if country != quotedCountry || effective != quotedCurrency || !billingQuoteSigned(r.PostForm.Get("csrf"), s.billing.cfg.PriceBook, quotedCountry, quotedCurrency, quotedChosen, r.PostForm.Get("quote_sig")) { |
| http.Redirect(w, r, back+billingQuoteQuery(country, currency, chosen), http.StatusSeeOther) |
| return |
| } |
diff --git a/internal/console/billingpage.go b/internal/console/billingpage.go| index 703bed9a0..9ec1b741e 100644 |
| --- a/internal/console/billingpage.go |
| +++ b/internal/console/billingpage.go |
| @@ -1,6 +1,10 @@ |
| package console |
| import ( |
| + "crypto/hmac" |
| + "crypto/sha256" |
| + "encoding/hex" |
| + "fmt" |
| "log" |
| "net/http" |
| "sort" |
| @@ -93,6 +97,12 @@ type billingPageView struct { |
| QuotedCountry string |
| QuoteError bool |
| CurrencyError bool |
| + // QuoteSig is the page's word that it priced QuotedCountry in Currency |
| + // (chosen or not) under the configured book, for this session: an HMAC |
| + // keyed by the session's own token. The checkout door takes a post |
| + // only with a quote the page rendered, so fields arranged to agree |
| + // with each other buy nothing a page did not show. |
| + QuoteSig string |
| // Error is the ?error= key (unavailable|pending|conflict), "" otherwise. |
| Error string |
| @@ -252,6 +262,7 @@ func (s *Server) handleBillingPage(w http.ResponseWriter, r *http.Request) { |
| } |
| terms := s.pageCurrency(r, v, country, state) |
| v.Plans = billingPlanRows(terms, band, int64(v.Members)) |
| + v.QuoteSig = billingQuoteSignature(p.CSRF, terms.PriceBook, v.QuotedCountry, v.Currency, v.CurrencyChosen) |
| } |
| p.Billing = v |
| @@ -403,9 +414,10 @@ func billingPlanRows(terms plans.PriceTerms, band string, members int64) []billi |
| // the quote grants at its quantity, per member. What the account |
| // is granted is the pooled figure; the page states its per-member |
| // share, which is exact for every plan, band and quantity in the |
| - // terms (TestBillingInclusionsFollowTheQuotedBand checks the |
| - // figures' divisibility, so a future figure that is not would fail |
| - // there). |
| + // terms today (TestBillingInclusionsFollowTheQuotedBand checks |
| + // that each figure times each band factor is whole in base units |
| + // and shows in three decimals; a future figure that is not would |
| + // fail there and the page would have to state the pooled figure). |
| row.Hours = formatFigure(price.Allowances.AwakeSeconds, price.Quantity*3600) |
| row.StorageGB = formatFigure(price.Allowances.StorageByteMonths, price.Quantity*1_000_000_000) |
| row.TransferGB = formatFigure(price.Allowances.TransferBytes, price.Quantity*1_000_000_000) |
| @@ -415,6 +427,26 @@ func billingPlanRows(terms plans.PriceTerms, band string, members int64) []billi |
| return rows |
| } |
| +// billingQuoteSignature is the page's word that it priced country in |
| +// currency (chosen or a default) under book, for the session whose token |
| +// is key: HMAC-SHA256 over those, hex. The token is the session's own |
| +// secret, double-submitted as the form's csrf field, so only a page this |
| +// console rendered for this session can produce it. |
| +func billingQuoteSignature(key, book, country, currency string, chosen bool) string { |
| + mac := hmac.New(sha256.New, []byte(key)) |
| + flag := "0" |
| + if chosen { |
| + flag = "1" |
| + } |
| + fmt.Fprintf(mac, "carloku-billing-quote\x00%s\x00%s\x00%s\x00%s", book, country, currency, flag) |
| + return hex.EncodeToString(mac.Sum(nil)) |
| +} |
| + |
| +// billingQuoteSigned reports whether sig is the signature of that quote. |
| +func billingQuoteSigned(key, book, country, currency string, chosen bool, sig string) bool { |
| + return hmac.Equal([]byte(billingQuoteSignature(key, book, country, currency, chosen)), []byte(sig)) |
| +} |
| + |
| // formatFigure renders num/den as the approved sentences write a figure: |
| // the whole part with thousands separators, then up to three decimals |
| // with trailing zeros dropped ("85", "0.7", "29.75", "30,000"). A share |
diff --git a/internal/console/billingpass.go b/internal/console/billingpass.go| index f9e27fa02..c76dfd90a 100644 |
| --- a/internal/console/billingpass.go |
| +++ b/internal/console/billingpass.go |
| @@ -184,6 +184,9 @@ func (s *Server) reconcilePendingIntent(ctx context.Context, intent billingInten |
| if err != nil { |
| return err |
| } |
| + if session.ID != intent.SessionID { |
| + return errBillingIdentity |
| + } |
| switch session.Status { |
| case "complete": |
| id, err := s.bindBillingCheckout(ctx, session.ID) |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index fd3fb2880..5d76313d3 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -237,7 +237,9 @@ func (s *Server) checkoutParams(acct *Account, intent billingIntent) billingstri |
| // price book, in the intent's currency with no Adaptive Pricing and no |
| // conversion on it. Every session the console binds to an intent, offers |
| // to a buyer or records a purchase from passes through here, however it |
| -// was come by: created, replayed, listed, or read back by its id. |
| +// was come by: created, replayed, listed, or read back by its id; and |
| +// every session the console expires, reviews, binds or records from |
| +// passes ownSession first. |
| func (b *billingRuntime) intentSession(session *billingstripe.Session, intent billingIntent, acct *Account) bool { |
| if !b.ownSession(session, intent, acct) { |
| return false |
| @@ -278,9 +280,11 @@ func (b *billingRuntime) ownSession(session *billingstripe.Session, intent billi |
| // A completed one cannot be expired: somebody paid for it, in the wrong |
| // currency, which the provider should not have let happen. Nothing is |
| // recorded from it, the intent is left as it is so no second purchase is |
| -// opened beside it, a review is filed once for the account, and nothing |
| -// is written at the provider again (later passes still read the session, |
| -// and file nothing more). A person has to look at what was paid for. |
| +// opened beside it, a review is filed for the account (once while it is |
| +// open; resolving it retires the intent, so it does not come back), and |
| +// nothing is written at the provider again (later passes still read the |
| +// session, and file nothing more). A person has to look at what was paid |
| +// for. |
| // |
| // session is the session as last read, or as the client refused it when |
| // it was created. The caller has established that it is this intent's |
| @@ -766,7 +770,10 @@ func (s *Server) reconcileBillingSubscription(ctx context.Context, id, eventID s |
| if session.Status != "complete" { |
| return errBillingPending |
| } |
| - if session.LiveMode != b.cfg.LiveMode || session.SubscriptionID != id || session.CustomerID != sub.CustomerID || session.ClientReferenceID != acct.Sqid || session.Metadata["checkout_intent"] != intent.ID { |
| + // The session read back must be the intent's own (its id, this |
| + // account, the intent's customer, this mode), and must be the |
| + // session of this subscription and its customer. |
| + if !b.ownSession(session, intent, acct) || session.SubscriptionID != id || session.CustomerID != sub.CustomerID { |
| return errBillingIdentity |
| } |
| // A purchase priced from a book was paid in the book's currency for |
| @@ -917,7 +924,9 @@ func (s *Server) bindBillingCheckout(ctx context.Context, sessionID string) (str |
| if intent.ID != session.Metadata["checkout_intent"] || intent.Plan != session.Metadata["plan"] || intent.Terms != session.Metadata["terms_version"] || intent.Band != session.Metadata["band"] { |
| return "", errBillingIdentity |
| } |
| - if intent.Customer != "" && session.CustomerID != intent.Customer { |
| + // The intent's own session in every way (id once bound, account, |
| + // customer, mode) before anything is recorded or reviewed from it. |
| + if !b.ownSession(session, intent, acct) { |
| return "", errBillingIdentity |
| } |
| if sessionTerms, err := priceTermsFromMetadata(session.Metadata); err != nil || sessionTerms != intent.priceTerms() || (!sessionTerms.Legacy() && !session.FixedCurrency(string(sessionTerms.Currency))) { |
| @@ -1152,7 +1161,11 @@ func (s *Server) recoverReadyIntentSession(ctx context.Context, acct *Account, i |
| if session.Metadata["checkout_intent"] != intent.ID { |
| continue |
| } |
| - if session.LiveMode != b.cfg.LiveMode || session.ClientReferenceID != acct.Sqid { |
| + // A listed session carrying the intent's id is acted on only when |
| + // it is the intent's own in every other way too (mode, account, |
| + // customer): the listing was asked for by customer, and the answer |
| + // is checked, not trusted. |
| + if !b.ownSession(&session, intent, acct) { |
| return nil, errBillingIdentity |
| } |
| // A session found by listing was not checked when it was created: |
diff --git a/internal/console/billingservice_test.go b/internal/console/billingservice_test.go| index 2851af279..7c9fa9951 100644 |
| --- a/internal/console/billingservice_test.go |
| +++ b/internal/console/billingservice_test.go |
| @@ -44,8 +44,10 @@ type fakeBillingStripe struct { |
| // another currency than it was asked for. |
| currency, sessionCurrency string |
| // sessionCustomer, when set, is the customer the session names in |
| - // place of the one the intent was opened for. |
| - sessionCustomer string |
| + // place of the one the intent was opened for. sessionID, when set, is |
| + // the id the session reports in place of cs_test_paid (the provider |
| + // answering a read of one session with another). |
| + sessionCustomer, sessionID string |
| // prices are extra price objects, by id, beside price_personal. |
| prices map[string]map[string]any |
| subscriptionPrice, subscriptionCurrency string |
| @@ -149,7 +151,11 @@ func (f *fakeBillingStripe) session() map[string]any { |
| if currency == "-" { // a session that reports no currency at all |
| currency = "" |
| } |
| - return map[string]any{"currency": currency, "id": "cs_test_paid", "url": "https://checkout.stripe.com/c/paid", "status": f.sessionStatus, "expires_at": f.now.Add(time.Hour).Unix(), "livemode": false, "customer": customer, "subscription": sub, "payment_status": payment, "client_reference_id": f.sqid, "metadata": f.meta, "customer_details": map[string]any{"address": map[string]string{"country": "IE"}}} |
| + id := "cs_test_paid" |
| + if f.sessionID != "" { |
| + id = f.sessionID |
| + } |
| + return map[string]any{"currency": currency, "id": id, "url": "https://checkout.stripe.com/c/paid", "status": f.sessionStatus, "expires_at": f.now.Add(time.Hour).Unix(), "livemode": false, "customer": customer, "subscription": sub, "payment_status": payment, "client_reference_id": f.sqid, "metadata": f.meta, "customer_details": map[string]any{"address": map[string]string{"country": "IE"}}} |
| } |
| func (f *fakeBillingStripe) serve(w http.ResponseWriter, r *http.Request) { |
diff --git a/internal/console/billingstore.go b/internal/console/billingstore.go| index 1901f0722..481234a3c 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -190,7 +190,9 @@ func (s *Store) migrateBilling() error { |
| // substring. Earlier builds of this branch created a wider trigger |
| // under the name billing_months_no_legacy_calc_over_book_history; |
| // it is dropped first, so a database they opened gets this one. |
| - `DROP TRIGGER IF EXISTS billing_months_no_legacy_calc_over_book_history`, |
| + // (The DROP of the earlier name and the CREATE of this one run in |
| + // one transaction, below, so no moment leaves the database with |
| + // neither.) |
| `CREATE TRIGGER IF NOT EXISTS billing_months_no_legacy_calc_over_paid_book_history BEFORE INSERT ON billing_months |
| WHEN (CASE WHEN json_valid(CAST(NEW.calculation AS TEXT)) THEN json_extract(CAST(NEW.calculation AS TEXT), '$.v') IS NOT 2 ELSE 1 END) |
| AND EXISTS (SELECT 1 FROM billing_book_assignments b WHERE b.account_id = NEW.account_id |
| @@ -210,6 +212,9 @@ func (s *Store) migrateBilling() error { |
| return err |
| } |
| } |
| + if err := s.retireEarlierBarrierTrigger(); err != nil { |
| + return err |
| + } |
| // Existing rows predate the resolution workflow; resolved_at=0 marks every |
| // one of them, and every new one, as an open hold until a resolution |
| // procedure exists to clear it. |
| @@ -762,6 +767,31 @@ func (s *Store) billingReview(accountID int64, subscription, reason string) (bil |
| return x, err |
| } |
| +// retireEarlierBarrierTrigger drops the wider barrier trigger earlier builds |
| +// of this branch created, in the same transaction that makes sure the |
| +// current one exists, so a crash between the two cannot leave a database |
| +// with neither. (A build of this branch from before the rename, run after |
| +// this one, would create the old name again beside the new: such builds |
| +// are not to run after this one, as the roll note says.) |
| +func (s *Store) retireEarlierBarrierTrigger() error { |
| + tx, err := s.db.Begin() |
| + if err != nil { |
| + return err |
| + } |
| + defer tx.Rollback() |
| + var n int |
| + if err := tx.QueryRow(`SELECT count(*) FROM sqlite_master WHERE type='trigger' AND name='billing_months_no_legacy_calc_over_paid_book_history'`).Scan(&n); err != nil { |
| + return err |
| + } |
| + if n != 1 { |
| + return errors.New("billing: the barrier trigger is missing") |
| + } |
| + if _, err := tx.Exec(`DROP TRIGGER IF EXISTS billing_months_no_legacy_calc_over_book_history`); err != nil { |
| + return err |
| + } |
| + return tx.Commit() |
| +} |
| + |
| type billingReviewResolution struct { |
| AccountID int64 |
| Subscription, Reason string |
| @@ -857,6 +887,17 @@ func (s *Store) resolveBillingReview(r billingReviewResolution, now time.Time) e |
| r.AccountID, r.Subscription, r.Reason, r.Occurrence, lastSeen, r.Actor, r.Note, at); err != nil { |
| return err |
| } |
| + // Resolving the review of a session paid in the wrong currency is the |
| + // operator saying the money is settled: the account's pending intent, |
| + // which was left standing so no second purchase opened beside the |
| + // refused one, is retired in the same transaction, so the next pass |
| + // finds nothing to bind and the review does not come back. The next |
| + // purchase starts clean. |
| + if r.Reason == billingCheckoutRefusedReason { |
| + if _, err := tx.Exec(`DELETE FROM billing_checkout_intents WHERE account_id=?`, r.AccountID); err != nil { |
| + return err |
| + } |
| + } |
| return tx.Commit() |
| } |
diff --git a/internal/console/templates/billing.html b/internal/console/templates/billing.html| index 95a37d982..d675b3987 100644 |
| --- a/internal/console/templates/billing.html |
| +++ b/internal/console/templates/billing.html |
| @@ -50,6 +50,7 @@ |
| <input type="hidden" name="quoted_country" value="{{.QuotedCountry}}"> |
| <input type="hidden" name="quoted_currency" value="{{.Currency}}"> |
| <input type="hidden" name="currency_chosen" value="{{if .CurrencyChosen}}1{{end}}"> |
| + <input type="hidden" name="quote_sig" value="{{.QuoteSig}}"> |
| <div class="signin billing-quote"> |
| <label><!-- billing.country.label -->Billing country |
| <select name="country" required> |
diff --git a/internal/plans/provision_test.go b/internal/plans/provision_test.go| index 37f064210..37f20ef66 100644 |
| --- a/internal/plans/provision_test.go |
| +++ b/internal/plans/provision_test.go |
| @@ -25,9 +25,11 @@ func TestStripeProvisionScriptMatchesThePriceBook(t *testing.T) { |
| // configured map may hold more than the 36 keys, and a price may carry |
| // its own currency as its only currency option. |
| for _, want := range []string{ |
| - `select(.metadata.price_book == \$book)] | length) == 36`, |
| + `select(.metadata.price_book == $book)] | length) == 36`, |
| + `select(.metadata.terms_version == $terms)] | length) == 3`, |
| + `starting_after=$after`, |
| `-ge 36 ]`, |
| - `to_entries | all(.key == $currency and .value.unit_amount == $cents`, |
| + `to_entries | all(.key == $currency and .value.unit_amount == $cents and (.value.tax_behavior == null or .value.tax_behavior == "" or .value.tax_behavior == "exclusive"))`, |
| } { |
| if !strings.Contains(script, want) { |
| t.Errorf("the script's verification lacks %q", want) |