amadan
Multi-currency: the sixth review's findings
The quote signature is keyed by a secret derived from the billing secret key, which only the server holds, and covers the session, the account, the book, the quoted country and currency, the choice flag and the member count the page priced at; the door compares that count with the one it charges under the membership lock and treats a count that moved as the conflict the page already explains. The pass judges a session it read against the intent it is working on before dispatching on it. The refusal review is keyed by the intent and filed only while that intent stands; resolving it retires exactly that intent, closes its quantity events and records what it was. The provisioning pager fails when a page fails. The comments at the bind and the trigger say what the code does.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
15 files changed,
+676
−54
docs/superpowers/specs/2026-10-01-multi-currency-price-book-design.md+1 −1docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review6.md+147 −0docs/superpowers/specs/DISPOSITIONS-multi-currency.md+24 −4docs/superpowers/specs/OPUS-REVIEW-multi-currency-review6.md+176 −0implementation/carloku-paid-launch/stripe-provision.sh+12 −8internal/console/billingcurrency_test.go+169 −3internal/console/billinghandlers.go+12 −6internal/console/billingpage.go+20 −13internal/console/billingpass.go+9 −1internal/console/billingquantityaudit.go+1 −1internal/console/billingservice.go+20 −7internal/console/billingservice_test.go+8 −1internal/console/billingstore.go+74 −9internal/console/templates/billing.html+1 −0internal/plans/provision_test.go+2 −0
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 4b2416c1f..578484619 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 |
| @@ -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, 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. |
| +- **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 session token, the account, the book, the quoted country, the quoted currency, the choice flag and the member count the page priced at, keyed by a secret derived from the billing secret key, which only the server holds), 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. The member count the page priced at is compared again under the membership lock when the door charges; a count that moved is a conflict (`?error=conflict`, the existing banner), not a higher charge. 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-review6.md b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review6.md| new file mode 100644 |
| index 000000000..c3b821d21 |
| --- /dev/null |
| +++ b/docs/superpowers/specs/CODEX-FINDINGS-multi-currency-review6.md |
| @@ -0,0 +1,147 @@ |
| +All platform references are at **`b51c2a26`**; pricing-site references are at **`9427c22`**. The requested `git diff b51c2a26 b51c2a26` is empty. Fifth-review closure below is assessed against the reports and dispositions embedded in the pinned commit. No files were modified; no tests, network commands or AWS commands were run. Test counterfactuals are deductions from source. |
| + |
| +**Fifth-review findings** |
| + |
| +“Without its fix” means removing the particular behavior, not reverting the whole commit. |
| + |
| +| Opus finding | Closed? | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **P3-1 — Ownership omitted on listing/reconciliation/binding paths** | **Reported cases closed; universal ownership remains incomplete.** Listing now checks ownership at `internal/console/billingservice.go:1168`, reconciliation at `:776`, binding at `:929`. The completed-session pass retains a different bypass: finding 4 below. | **No** for removing those guards: `internal/console/billingcurrency_test.go:2580`, `:2597`, `:2619` distinguish them. **Yes** for the remaining cross-account dispatch case, which is absent. | |
| +| **P3-2 — Non-atomic trigger replacement** | **Closed in implementation.** The new trigger is created before the transaction that checks its existence and removes the old trigger: `internal/console/billingstore.go:196`, `:776`. | **Yes.** The test at `internal/console/billingcurrency_test.go:2356` checks successful migration’s final state; unsafe DROP-then-CREATE also passes when uninterrupted. | |
| +| **P3-3 — Tautological inclusion assertion** | **Closed.** `internal/console/billingcurrency_test.go:2462` checks actual base-unit divisibility and display precision. | **Yes if those assertions are removed:** today’s terms remain exact. An inexact-terms mutation distinguishes the improved test; removing the assertion does not. | |
| +| **P3-4 — Webhook and ownership test gaps** | **Named gaps closed.** Individual ownership clauses have cases at `internal/console/billingcurrency_test.go:2517`; the fresh webhook independently creates its review at `:2650`. | **No** for removing individual predicate clauses or making the checkout webhook a successful no-op. The new resolution race and cross-account dispatch remain untested. | |
| +| **P3-5 — Account-wide products and one-page price count** | **Reported normal-response cases closed.** Pagination and metadata filters exist at `implementation/carloku-paid-launch/stripe-provision.sh:393`. Failed pagination can nevertheless report success: finding 5. | **No** for literal restoration: `internal/plans/provision_test.go:27` checks the relevant strings. **Yes** for broken pagination behavior retaining those strings. | |
| + |
| +| Astra finding | Closed? | Would its test pass without its fix? | |
| +|---|---|---| |
| +| **1 — Incomplete session ownership** | **Partly**, as Opus P3-1 above. | Original guard reversions fail; finding 4 survives. | |
| +| **2 — Fabricated quote fields** | **No.** The signature uses a key disclosed to the buyer. Finding 2. | Removing signature verification fails `internal/console/billingcurrency_test.go:1968`. Recomputing a valid signature from the disclosed key is untested and still works. | |
| +| **3 — Trigger replacement gap** | **Yes**, as Opus P3-2 above. | **Yes:** the migration test does not distinguish safe replacement from uninterrupted unsafe replacement. | |
| +| **4 — Resolution leaves refused intent standing** | **Sequential case closed; safe retirement incomplete.** Resolution deletes the pending intent, but can reopen the review and later delete a newer intent. Finding 1. | **No** for removing deletion: `internal/console/billingcurrency_test.go:2695` fails. **Yes** for the unsafe account-only deletion and concurrent reopening. | |
| +| **5 — Ordinary site build bypasses draft gate** | **Closed.** Site `package.json:6` runs the checker before Eleventy. This also closes Opus’s carried review-four P3-6. | **No** for literal restoration: site `test/pricing-pricebook.test.mjs:153` checks the build script. It does not execute that entry point. | |
| +| **6 — Catalogs exceeding 100 prices fail** | **Reported case closed**, subject to finding 5’s failed-page handling. | Literal restoration fails the source-string check; behavioral breakage can pass. | |
| +| **7 — Empty own-option tax behavior rejected** | **Closed.** `implementation/carloku-paid-launch/stripe-provision.sh:430` accepts null, empty and `exclusive`, matching the console. | **No** for restoring the old expression. The test checks text rather than evaluating the expression. | |
| + |
| +On the fifth-review disagreement, **Astra’s reading was right about quote provenance**: showing an amount later at Stripe does not satisfy the console’s explicit quote rule. Its **P2 treatment of ownership and the old migration gap was also justified** by their possible effects: acting for the wrong purchase and freezing incomplete monetary history. The ownership scenarios require inconsistent provider responses; they are not demonstrated buyer-controlled exploits. Opus correctly found no ordinary GBP conversion path, but that does not close these separate boundaries. |
| + |
| +**Findings, most severe first** |
| + |
| +1. **P2 — Resolving a refused purchase can delete a newer checkout intent.** |
| + |
| + **Where:** `internal/console/billingstore.go:896`; `internal/console/billingservice.go:920`, `:942`; `internal/console/billingquantity.go:1348`. |
| + |
| + A concrete interleaving: |
| + |
| + - Intent **I1**, session **S1**, has an open wrong-currency completion review. |
| + - Another bind reads I1, then pauses before filing its review. |
| + - An operator resolves the existing review. Resolution deletes I1. |
| + - The paused bind resumes. `fileBillingReviewOnce` checks only for an **open review**, not whether I1 still exists. It reopens the resolved review as occurrence 2. |
| + - A new checkout creates **I2**. No subscription was recorded from the refused purchase, and checkout creation does not reject an outstanding review. |
| + - Resolving occurrence 2 for S1 executes `DELETE ... WHERE account_id=?`, deleting I2 and leaving I2’s payable Stripe session without its intent. |
| + |
| + This can occur within one console: the resolution handler at `internal/console/billingoperator.go:221` does not take the binder’s billing lock. Multiple processes also defeat an in-process-lock-only fix. |
| + |
| + The resolution audit at `internal/console/billingstore.go:886` records the review, actor and note, **not the deleted intent or session identity**. |
| + |
| + **Smallest acceptable fix:** Associate refusal reviews with the exact intent/session; condition both filing and retirement on that identity within their transactions. Record the retired identity in the resolution audit. Add an interleaving test that preserves I2 after resolving S1. |
| + |
| +2. **P2 — The buyer can forge the new quote signature.** |
| + |
| + **Where:** `internal/console/billingpage.go:435`; `internal/console/billinghandlers.go:91`; `internal/console/templates/billing.html:45`. |
| + |
| + `billingQuoteSignature` uses the session token as its HMAC key. That same token is rendered as `csrf`. |
| + |
| + Load `?country=IN¤cy=gbp`, showing Personal at **£7.70**. Using the form’s `csrf`, compute the HMAC for the configured book, `IN`, `usd`, and choice flag `0`. Submit: |
| + |
| + ```text |
| + country=IN |
| + quoted_country=IN |
| + currency=usd |
| + quoted_currency=usd |
| + currency_chosen= |
| + quote_sig=<recomputed signature> |
| + ``` |
| + |
| + With valid consent and postal fields, the handler creates a **USD 9.80** checkout without rendering that console quote. The existing tests alter fields while retaining an invalid signature; they never recompute it using the disclosed key. |
| + |
| + **Smallest acceptable fix:** Use a server-only signing secret, authenticating the session and account alongside the quote inputs, or store server-issued quote records. Add a test that attempts forgery using only values available in the rendered form. |
| + |
| +3. **P2 — A valid quote can purchase a higher team total after membership changes.** |
| + |
| + **Where:** `internal/console/billingpage.go:264`, `:265`; `internal/console/billingservice.go:399`; `internal/console/templates/billing.html:94`. |
| + |
| + An IN/GBP Community page for ten members displays **£15.80 annually**. An eleventh member joins before submission. The unchanged signed form passes, but checkout recounts members and purchases **£17.38**. |
| + |
| + Quantity affects the displayed annual price, yet the signature authenticates neither the displayed membership count nor charged quantities. The membership transaction protects the count read during POST, not the count used to render the page. |
| + |
| + **Smallest acceptable fix:** Authenticate the displayed quantity information and compare it with the quantity being committed under the membership lock. Redirect to a refreshed quote when the charged quantity changes. Test an unchanged form across a ten-to-eleven-member change. |
| + |
| +4. **P2 — The completed-session pass checks ownership against a response-selected intent, not necessarily the intent it was processing.** |
| + |
| + **Where:** `internal/console/billingpass.go:187`, `:192`; `internal/console/billingservice.go:905`, `:920`, `:929`. |
| + |
| + Account A’s intent IA is bound to `cs_A`. A provider response retains ID `cs_A` but contains account B’s reference, customer, intent IB and subscription. IB is ready but unbound. |
| + |
| + The pass checks only the returned session ID, then calls the binder. The binder resolves **B** from the response metadata and checks `ownSession` against **IB**. Because IB is unbound, that predicate accepts `cs_A`. The console can then file B’s review or bind and reconcile B’s purchase while processing IA. |
| + |
| + Applying the predicate later against an intent selected from the response does not establish ownership against the original intent. |
| + |
| + **Smallest acceptable fix:** Load the account belonging to the original pending intent and apply `ownSession` immediately after the pass’s read, before dispatching on status. Add a two-account test that preserves the requested session ID while replacing all other ownership fields. |
| + |
| +5. **P3 — Failed pagination can still print “counts ok.”** |
| + |
| + **Where:** `implementation/carloku-paid-launch/stripe-provision.sh:396`. |
| + |
| + Suppose the first page contains all 36 expected current-book prices and reports `has_more=true`. The next Stripe command fails with empty stdout. |
| + |
| + Its exit status is ignored. The accumulator retains the earlier array, the empty `has_more` result breaks the loop, and the count can pass. Additional current-book entries on unread pages are never checked. The generated shell enables `pipefail` at line 222, but the function ultimately returns its successful `printf`. |
| + |
| + **Smallest acceptable fix:** Explicitly propagate Stripe and JSON-processing failures, and validate each list envelope before accumulating it. Test a successful first page followed by a failed second page. The current substring test cannot detect this defect. |
| + |
| +For the remaining requested paths: |
| + |
| +- **A — Ordinary GBP purchase:** `?country=IN¤cy=gbp` produces a chosen GBP quote at band D (`billingpage.go:263`). The form comparison passes GBP to checkout (`billinghandlers.go:80`, `:101`). `startBillingCheckout` prices and stores GBP independently of the band (`billingservice.go:404`, `:509`); `checkoutParams:221` requests fixed GBP. Binding and reconciliation preserve the intent’s terms; account and assignment writes reject same-subscription currency/book changes (`billingstore.go:582`, `:708`). History preserves both fields (`billinginvoicestore.go:530`), `AllowanceForMonth` rejects incompatible paid spans, and calculation version 2 carries GBP (`billingcalculation.go:174`). Invoice creation, recovery, verification and hashes use that frozen currency; quantity operations use the account’s stored catalog identity. I found no additional path converting or repricing that purchase’s money. |
| + |
| +- **B/C/M/O — Forms, defaults and cookies:** Present currency fields require exactly one sold, lowercase value; empty, repeated, trimmed or case-folded alternatives receive 400 on both doors after their authentication gates (`billinghandlers.go:160`). A missing field uses the country’s effective default and must match the quote; legacy no-control forms use EUR. Changing country/currency ordinarily redirects before purchase. Cookies cannot independently alter an accepted POST. Present-empty clears; absent falls back; invalid leaves the old cookie untouched, so an old valid choice can reappear on a later absent-parameter GET. Held currency overrides URL/cookie suggestions, including legacy EUR (`billingpage.go:295`). The quote door checks session, token and membership and constructs only the escaped account billing URL (`billinghandlers.go:121`); it performs no billing mutation. |
| + |
| +- **Q — Signature reuse and omitted inputs:** Without recomputing it, a signature fails for a changed covered quote, book or session token. It remains reusable for the same covered tuple and is not account-bound. Knowing the disclosed key defeats its provenance claim. Plan changes select among prices already rendered; quantity changes are finding 3. Postal code affects tax, while the page explicitly quotes prices before VAT. |
| + |
| +- **D/L/R — Other ownership paths:** Both create sites, interactive resume reads, listing recovery and initial reconciliation now contain the intended guards. Expired-intent retirement performs equivalent ownership checks before committing (`billingstore.go:425`, `:448`). The already-recorded subscription shortcut at `billingservice.go:917` still returns before `ownSession`; downstream reconciliation independently checks the stored subscription, and I found no separate monetary corruption from that shortcut. Sequential wrong-currency completion handling avoids expiry and deduplicates the open review. Finding 1 defeats permanent resolution under concurrency. |
| + |
| +- **E/H/N/T — Barrier and rollout:** For valid writer-produced history, the trigger and allowance calculation agree on a positive paid span: |
| + |
| + ```text |
| + max(effective_at, month_start) |
| + < min(next_assignment_effective_at, period_end, month_end) |
| + ``` |
| + |
| + Both tables participate in next-assignment ordering, including higher revisions in the same second. Free/inactive rows contribute nothing but terminate preceding spans. Renewal and quantity dating change `effective_at`; the assignment writer copies the provider’s period end as `PaidThrough`. I found no requested history distinguishing the refined trigger from the calculation. Migration creates the new trigger first, then transactionally checks its presence and drops the old one. Missing-new failure retains the old trigger; a second startup is harmless. An interruption can leave **both**, never remove the last existing barrier through this sequence. Earlier branch builds can recreate the old trigger, so coordinated rollout remains necessary. |
| + |
| +- **F/G/J/K — Figures, books and site:** Current pooled inclusions produce exact per-member displays at every valid quantity; unavailable Personal uses quantity one, and band A matches the asserted approved sentences. Book validation, checked arithmetic, legacy EUR preservation and optional complete catalogs have no further identified defect. The public export contains no account or provider secrets and matches the site’s copy. Site rates, band scaling, precision checks, country options/defaults and static/dynamic link parameters agree with it. The draft marker, exact `"1"` override and ordinary-build gate are present. A locale-only GBP display followed by empty link suggestions can become console USD; that is the distinction required by ruling 9. |
| + |
| +**Claims the code contradicts** |
| + |
| +- **Quote provenance remains false:** `billingpage.go:430` says only a rendered console page can produce the signature. Findings 2 and 3 contradict the stronger quote claims in the design and dispositions. |
| +- **Universal ownership remains false:** the design and dispositions’ review-five ownership row overstate the completed-session pass, as finding 4 shows. The already-recorded binding shortcut is also an intentional exception. |
| +- **Permanent “once” remains false under concurrency:** the new sequential resolution test does not cover finding 1. Conversely, `billingservice.go:939` still says the retirement door does not exist, although resolution now deletes intents. |
| +- **Migration comments describe different code:** `billingstore.go:192` and dispositions review-four line 116 say the old trigger is dropped first; the source creates the new one first. The comment also claims CREATE and DROP share one transaction; they do not. The actual ordering is safe. |
| +- **Review summaries still overstate agreement:** dispositions lines 108 and 135 say earlier findings were unanimously closed. The embedded Astra reports explicitly mark ownership, crafted quotes and migration cases partly closed. Opus accepted fabricated matching quote fields in review five; it did not make Astra’s provenance finding. |
| +- **Coverage claims remain too broad:** the migration test passes an uninterrupted unsafe replacement. Provisioning tests read shell source, not “generated checks,” and execute neither pagination nor `jq`. The site build test checks script text. These limitations matter even where the implementation is fixed. |
| +- **The inclusion mutation descriptions conflict:** dispositions line 143 says removing the check was killed; removing those assertions passes current exact fixtures. Line 135 instead describes making a terms figure inexact, which the new assertions would detect. |
| +- **Mutation history cannot be established from source:** the named review-three and review-four behavioral mutations generally have distinguishing assertions now; the current incoming-country-cookie case is present. The default-as-explicit mutation is equivalent for accepted ordinary forms. None of that proves historical runs occurred or establishes forged-quote provenance. |
| +- **“400 before anything” remains overstated:** checkout step-up and consent handling precede currency validation. No purchase is created on those earlier exits. |
| +- The design’s legacy Adaptive Pricing and own-only `currency_options` exceptions are now stated correctly. The previously vacuous webhook coverage is also genuinely repaired by the fresh fixture. |
| + |
| +I found no concrete calendar-date, ordering, temporary-root or shared-cookie-jar failure. Authentication/webhook fixtures use current timestamps; the one-form browser helper explicitly avoids a cookie jar. |
| + |
| +**What must be verified against Stripe test mode before enablement** |
| + |
| +1. **Catalogs:** Read all 36 prices with expanded currency options. Verify exact amount, currency, metadata, recurrence and tax behavior. Run provisioning checks with historical/unrelated catalogs exceeding 100 objects; confirm pagination failures stop verification. |
| +2. **Checkout matrix:** Complete USD/EUR/GBP purchases across IE, GB, US, IN and JP, including IN/GBP. Verify unit × quantity, fixed session/subscription/item currency, disabled Adaptive Pricing and absent conversion. Attempt incompatible price/session currency and record the refusal. |
| +3. **Renewal and quantity changes:** Exercise renewals and both quantity policies in each currency. Verify proration currency, effective date, amount and the quantity-fence/idempotency behavior. |
| +4. **Overages:** Exercise every meter, rounding boundary and invoice floor through draft creation, items, finalization and payment. Compare every line and subtotal with the frozen calculation; include canceled subscriptions and automatic tax. |
| +5. **Recovery:** Lose responses around checkout and invoice operations, then recover through replay and listing within and beyond retention windows. Confirm original identities, unchanged currencies and no duplicate charges; include legacy request compatibility. |
| +6. **Customer currency constraints:** Establish behavior during a live subscription, after cancellation and with outstanding old-currency invoices. Confirm the console’s stricter held-currency rule remains effective. |
| +7. **Refusals, discounts and events:** Record actual open/completed-session expiry responses; test incompatible fixed-amount discounts, percentage/house discounts, and duplicated or out-of-order webhooks. Confirm no 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 d5a74f8d8..2e327eedb 100644 |
| --- a/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| +++ b/docs/superpowers/specs/DISPOSITIONS-multi-currency.md |
| @@ -132,17 +132,17 @@ The fixes are in the commit that adds this section and have not been read by a r |
| ## 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. |
| +Reports: `OPUS-REVIEW-multi-currency-review5.md`, `CODEX-FINDINGS-multi-currency-review5.md`. Both found the GBP purchase consistent end to end and the fourth round's code changes present; Astra marked three of them partly closed (ownership, crafted quotes, migration), which is what its findings below are about. Its 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 (a data mutant, since removing a test assertion over exact data can show nothing): 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 2 (P2): quote fields forged to agree with each other bought a price no console page had shown. | Fixed in part here, keyed wrongly: the page signed its quote with the session's own token, which the form discloses, so the buyer could forge it (review six, both reviewers). Review six re-keys it with a server secret and covers the account and the member count. | `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 afterwards in a transaction that first confirms the new one exists; no moment has neither (the ordering is the guarantee; the two do not share a transaction, and no test can show the atomicity itself, only the result). 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` | |
| +| 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 script's text for the pager and the filters (it does not run them: no Stripe here). | `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 | | |
| @@ -150,3 +150,23 @@ Reports: `OPUS-REVIEW-multi-currency-review5.md`, `CODEX-FINDINGS-multi-currency |
| 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. |
| + |
| +## Review six (at b51c2a26; Opus accepted with fixes, Astra rejected) |
| + |
| +Reports: `OPUS-REVIEW-multi-currency-review6.md`, `CODEX-FINDINGS-multi-currency-review6.md`. Both found the fifth round's fixes present and the GBP purchase consistent end to end, and both found the same defect at the top: the quote signature was keyed by the session token the form discloses. Everything of either is fixed below with a test that fails without it; six mutants were killed in a scratch copy. |
| + |
| +| Finding | Disposition | Where | Reproduction | |
| +| --- | --- | --- | --- | |
| +| Opus P2-1, Astra 2 (P2): the quote signature was keyed by the buyer's own session token, so the buyer could sign any quote from what the form disclosed. | Fixed: the key is a secret derived from the billing secret key (as `locationDigest` derives its own), which only the server holds; the MAC covers the session token, the account, the book, the quoted country, the quoted currency, the choice flag and the member count. | `billingpage.go` `billingQuoteSignature`, `billinghandlers.go` | `TestBillingFormPostsWhatItShows` (a forgery computed exactly as the fifth build signed, from the form's own csrf: refused; the rendered quote buys) | |
| +| Astra 3 (P2): a member who joined between the page and the post bought a higher team total under a valid quote. | Fixed: the page's member count is in the signature and the form; the door compares it with the count it charges, under the membership lock, and a count that moved is `errBillingConflict`, which the page already explains ("the account changed while you were on this page, for example a member was added"); a forged count fails the signature. | `billingservice.go` `startBillingCheckoutQuoted`, `billinghandlers.go` | `TestBillingFormPostsWhatItShows` ("a member who joins between the page and the post": eleven priced, a twelfth joins, the post is a conflict; the refreshed page buys twelve) | |
| +| Astra 1 (P2), Opus P3-1: the refusal review was keyed by the subscription, so a binding paused across a resolution could reopen it, and the resolution deleted the account's intent by account alone, closed none of its quantity events and recorded nothing of it. | Fixed: the review is keyed by the intent (its subscription field holds the intent id) and is filed only while that intent stands, in one transaction; the resolution retires exactly that intent, closes its quantity events (`checkout_refused_resolved`) and records what it was in `billing_retired_intents`; a later intent is untouched. | `billingstore.go` `fileCheckoutRefusedReview`, `resolveBillingReview` | `TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution` (the record, the events, a new purchase after the resolution, a late filing for the retired intent files nothing, a second resolution named for the old intent leaves the new one) | |
| +| Astra 4 (P2): the pass judged ownership against the intent the response named, not the one it was working on. | Fixed: the pass applies `ownSession` to the session it read against the intent it is working on, with that intent's account, before dispatching on status. | `billingpass.go` | `TestOwnershipIsEnforcedOnEveryPath` ("the bound session answered with another intent's everything") | |
| +| Astra 5 (P3): a failed page in the provisioning pager could still print "counts ok". | Fixed: a page that fails, or is not a list page, fails the whole; the plans test reads the script's text for it. | `stripe-provision.sh` | `TestStripeProvisionScriptMatchesThePriceBook` | |
| +| Opus P3-2, Astra claims: comments at the bind ("an operator's door that does not exist yet") and at the trigger ("in one transaction"); the dispositions' "every finding fixed" summaries, the Opus P3-5 "generated checks" wording, the inclusion-mutation description. | Corrected in this commit. | comments, this file | | |
| +| Opus: the quantity is a price input the signature did not cover. | Covered now (Astra 3). | | | |
| + |
| +Mutation checks on these fixes in a scratch copy: the quote keyed by the session token again (caught by a forgery over the current message with the disclosed token); the member count not compared under the lock; the pass judging ownership against nothing (caught by the two-account case); a late filing for a retired intent accepted; the quantity events left open (caught once the fixture had an event: they are kept for live, team-plan intents): all killed. The resolution deleting by account alone survived and is equivalent under the invariants: a refusal review is open only while its intent stands, and no second intent can open while it does, so the named delete and the account-wide one meet the same row; the named predicate is kept because Opus asked for the invariant not to be the only thing holding it. The processing guard admits the new outcome `checkout_refused_resolved`; its text changed, so a database with the older guard gets this one at the next start (billingquantityaudit.go's rule). |
| + |
| +Open after this round: the site's `bands.p3` sentence; `billing.quote.unknown` and `billing.currency.unknown` as unreviewed system copy; everything that needs Stripe test mode. |
| + |
| +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-review6.md b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review6.md| new file mode 100644 |
| index 000000000..094ffbdec |
| --- /dev/null |
| +++ b/docs/superpowers/specs/OPUS-REVIEW-multi-currency-review6.md |
| @@ -0,0 +1,176 @@ |
| +# Sixth review of `feat/carloku-multi-currency-20261001` at `b51c2a26` (base `12e70d95`) |
| + |
| +**Verdict: accept with fixes.** One finding is P2: the quote signature added for Astra 2 is keyed by a value the buyer holds, so it authenticates nothing. |
| + |
| +I found no path that computes, sends, verifies or stores money in a currency other than the purchase's stored one. |
| + |
| +How I worked: source only, by `git show b51c2a26:` and site `9427c22`. I ran no tests and modified nothing. Your prompt asked for the delta `b51c2a26..b51c2a26`, which is empty; I read `1f005804..b51c2a26`, the fifth-review commit, as the intended delta. |
| + |
| +## Fifth-review findings |
| + |
| +| Finding | Closed? | Would its test pass without the fix? | |
| +|---|---|---| |
| +| **Opus P3-1 / Astra 1**: ownership not checked on every path | **Closed.** `ownSession` now runs on the listing (`billingservice.go:1168`), reconcile (`:776`) and bind (`:929`). The pass checks the returned id (`billingpass.go:187`). | **No** for listing, reconcile and bind. The `TestOwnershipIsEnforcedOnEveryPath` cases expire, record or review without each fix. **Yes** for the pass's id check (admitted belt and braces; the bind's bound-id clause catches it). | |
| +| **Opus P3-2 / Astra 3**: the trigger swap was not atomic | **Closed in effect.** CREATE of the new trigger, then a transaction that checks the new one exists and drops the old. Some moment may have **both**, which fails closed. No moment has neither. | **Yes.** "a database that carries the earlier, wider trigger" is unchanged since round five and checks only the end state. The old DROP-then-CREATE passes it. See I-2. | |
| +| **Opus P3-3**: tautological inclusion check | **Closed.** `per×bps % 10000` and the three-decimal unit check are correct for floor scaling. Dead placeholder at `billingcurrency_test.go:2460-2461`. | **No.** | |
| +| **Opus P3-4**: test gaps | **Mostly closed.** The webhook alone files the review (`TestRefusedPaid…Resolution`, first `send`). `TestOwnSessionClauses` covers each clause. Bind's customer check is covered. | Each would fail without its fix. The negative webhook legs in `TestOwnershipIsEnforcedOnEveryPath` still pass on a no-op webhook (I-3). | |
| +| **Opus P3-5 / Astra 6**: Check 2 paging and filters | **Closed.** `all_of` pager, metadata filters; `TERMS`/`BOOK` are printed at `stripe-provision.sh:381`. | **No** to a literal revert, but the test is substring-only (I-4). | |
| +| **Astra 2**: forged agreeing quote fields | **Not closed.** See P2-1. | The test fails without the fix only because it forges without the key. `TestCheckoutFormCurrencyField:204` signs a quote from `raw` itself, which is exactly the forgery. | |
| +| **Astra 4 / carried Opus P3-3**: a resolved review reopens | **Closed** (`billingstore.go:896-900`). See P3-1 for scope. | **No.** Without the DELETE, the next pass reopens the review (occurrences 2). | |
| +| **Astra 5 / Opus P3-6**: `npm run build` bypassed the draft check | **Closed** (site `package.json`). | **No.** The site test asserts that the build script starts with the check. | |
| +| **Astra 7**: empty `tax_behavior` | **Closed** (`stripe-provision.sh:430`). | Substring test only. | |
| + |
| +**Who was right where the two disagreed:** |
| +- **Ownership:** Astra was right that the paths existed. Opus's P3 was the right severity: each needs an inconsistent provider response, and the worst effect was expiring or reviewing a foreign session. |
| +- **The trigger swap:** P3 was right. The gap is a crash between two startup statements, and the next start's migration closes it before this binary freezes anything. Only a pre-book binary started inside that gap could exploit it. |
| +- **The fabricated quote:** Astra was right that the rule as written was broken, and it still is (P2-1). Opus was right that no reachable purchase is at a price outside the book or unselectable on the page. |
| + |
| +## Findings |
| + |
| +### P2-1. The quote signature is keyed by the buyer's own session token, so the buyer can forge any quote |
| +- **Where:** |
| + - Signed at `billingpage.go:265` with `p.CSRF`, which is `raw`, the session token (`server.go:1722`). |
| + - The HMAC is at `billingpage.go:435-447`. |
| + - It is verified at `billinghandlers.go:91` with `r.PostForm.Get("csrf")`, which `billingOwner` (`:35`) requires to equal the session cookie. |
| +- **Scenario:** On `?country=IN¤cy=gbp` (Personal at £7.70), the buyer reads their `csrf` from the form and posts: |
| + - `country=IN`, `quoted_country=IN`, `currency=usd`, `quoted_currency=usd`, `currency_chosen=` |
| + - `quote_sig=HMAC-SHA256(csrf, "carloku-billing-quote\0<book>\0IN\0usd\00")`, with the book id public in the export and the algorithm in the open-source tree. |
| + |
| + The door buys USD 9.80, which no page rendered. That is the round-five Astra 2 post plus one computable field. The repo's own test does exactly this (`billingcurrency_test.go:204`). |
| +- **What the signature does not cover:** the account (a quote from account A's page verifies on account B's door for the same user) and the quantity. The quantity is a price input: a member who joins between render and post changes the amount. That predates this branch and the amount is shown at Stripe. |
| +- **Smallest fix:** |
| + - Key the MAC with a server secret, derived like `locationDigest` (`billingservice.go:1061-1068`) from `cfg.SecretKey` under `carloku/billing/quote/v1`. |
| + - MAC over the session token, `acct.Sqid`, the book, the country, the currency and the flag. |
| + - Change `TestCheckoutFormCurrencyField:204` to take `quote_sig` from a rendered page. |
| + - Add a case where the token-keyed signature is refused. |
| +- **Alternatively:** withdraw the claim in the design (`:152`, "nothing is bought at a price a page did not show this session") and in the dispositions. |
| + |
| +### P3-1. The resolution deletes the account's intent without naming it, skips the retirement bookkeeping, and records nothing about it |
| +- **Where:** `billingstore.go:896-900`. |
| +- **Which intent goes:** `DELETE … WHERE account_id=?`. The table is keyed by `account_id` (`:124`), and a refused-paid intent cannot be replaced while it stands: |
| + - The resume path refuses again (`billingservice.go:542-546`). |
| + - The listing path does too (`:1176`). |
| + - `retireIntent` needs an expired, unpaid session. |
| + |
| + So today it deletes the refused intent. That is safe only by those invariants, not by the predicate. |
| +- **The bookkeeping skipped:** `retireIntent` (`:451`) and `discardUnreadyIntent` (`:483`) both close the intent's `billing_quantity_events` (`processed_at`, outcome). This path leaves them at `processed_at=0` forever. |
| +- **What is lost:** the intent id, the session id and the price terms. The resolution row keeps only account, subscription, reason, actor and note. This was the only row tying the refused session to the account. |
| +- **Scenario:** the operator refunds and resolves. The intent row, carrying `session_id=cs_…` and `currency=gbp`, vanishes. Its pending quantity events stay unprocessed, and nothing records which session was settled. |
| +- **Fix:** |
| + - Delete `WHERE account_id=? AND id=?`, with the id read in the same transaction and checked against a session whose subscription is `r.Subscription`. |
| + - Mark its quantity events processed (`'checkout_refused_resolved'`). |
| + - Record the intent and session ids in the resolution's note or a retired-intents row. |
| + |
| +### P3-2. Stale and inaccurate comments on authority paths |
| +- `billingservice.go:939-941` says a resolved review "is reopened … retiring such an intent is an operator's door that does not exist yet". The resolution now retires the intent. |
| +- `billingstore.go:193-195` says "the DROP … and the CREATE … run in one transaction". The CREATE runs before, in autocommit (`:196`, `:211`); the transaction holds only the existence check and the DROP (`:776-793`). The ordering is what guarantees "never neither", not a shared transaction. |
| +- **Fix:** correct both comments. |
| + |
| +## Section notes |
| + |
| +### A. One GBP purchase from the one form |
| +`?country=IN¤cy=gbp` gives `currency_chosen=1`, `quoted_currency=gbp`, `quoted_country=IN` and `quote_sig`. The rest of the path is unchanged since round five and stays in GBP: |
| +1. `billingFormChoice` returns gbp, chosen; `effective=gbp`; the signature verifies. |
| +2. `startBillingCheckout` gets `posted=gbp`; the band is D, and the price is checked on currency and amount. |
| +3. `CreateCheckout` sends `currency=gbp`, Adaptive Pricing off. |
| +4. Bind and reconcile both call `ownSession` now. |
| +5. `applyBillingStateDated`, `billingInvoiceHistory`, `AllowanceForMonth`, `priceBillingCalculation` (v2), the engine and the quantity worker follow. |
| + |
| +### B, M. The one-form rule |
| +- Every legitimate path still buys what was shown. |
| +- A legacy console signs and verifies the same book and `eur`, unchosen. A held account signs its held currency as chosen. |
| +- An erased flag on a GB page fails the signature, then quotes `?country=GB¤cy=` and buys GBP: one extra round trip, harmless. |
| +- The quote door is unchanged: an escaped redirect to the account's own billing page, with no writes. |
| +- Repeated `currency` gets 400; a field present and empty gets 400; repeated `quoted_*` fields read the first value on both sides. |
| +- Only P2-1 defeats the rule. |
| + |
| +### C. Cookies |
| +Unchanged and correct: present-empty clears, absent falls back, invalid sets nothing. Checkout never reads a cookie. |
| + |
| +### D, L, R. The ownership predicate |
| +- **Every path that acts on a session now gates on `ownSession`:** both create sites, both resume reads, the listing, the pass's open branch, bind and reconcile. |
| +- **Two places act without calling it, both safe:** |
| + - The pass's expired branch (`billingpass.go:197`) retires through `retireIntent`, whose store-side checks (`billingstore.go:425, 448`, plus the `session_id` in the DELETE) match it. |
| + - Bind's early return (`billingservice.go:917`) applies only to a subscription already recorded, and only hands it to reconcile, which checks again. |
| +- **Its clauses are sufficient:** mode, intent id, client reference, the intent's customer, and the bound id. |
| +- **A refused-paid review cannot be recorded, expired or multiplied** through any path while it is open. After resolution the intent is gone, so bind fails at `billingCheckoutIntent`. |
| + |
| +### E, N. The trigger |
| +- The body is unchanged since round five, and I agree with both round-five readings: it matches `AllowanceForMonth` on: |
| + - a supersession in the same second |
| + - a renewal that moves `effective_at` |
| + - a quantity change |
| + - a free assignment |
| + - `period_end`, whose sole writer is `applyBillingStateDated`, from the provider's `CurrentPeriodEnd` |
| +- **The old trigger name:** "neither" is no longer possible; "both" is possible, and fails closed: |
| + - after a crash between the CREATE and the commit, until the next start |
| + - after an earlier branch build runs (roll note) |
| + |
| +### T. `retireEarlierBarrierTrigger` |
| +- SQLite DDL inside a transaction is fine. |
| +- A second start is idempotent: the new trigger exists and the DROP is a no-op. |
| +- The "missing" error is unreachable in practice, because a failed CREATE has already returned. If it ever fired, the console would refuse to start, which is loud and closed. |
| + |
| +### F. Inclusions |
| +Exact at every paid plan, band and quantity. The hard-coded factors in the test equal `expectedBandFactorsBps`, which `Validate` pins. |
| + |
| +### G, H, J, O. Validation, schema, export, held currency |
| +- No change since round five, apart from Check 2 and the `tax_behavior` rule. Both are correct. |
| +- O: legacy EUR is held as EUR and signed as such. |
| + |
| +### Q. The quote signature |
| +- Its MAC message does separate books, country, currency and flag, and two sessions do get different signatures. |
| +- After a token change, the old form fails csrf with 403 before the signature is read. |
| +- None of that matters while the key is client-held (P2-1). |
| + |
| +### S. The intent the resolution retires |
| +Covered in P3-1. |
| + |
| +### K. The pricing site |
| +- Unchanged since `4feb1cd` apart from `build` now running `check-pricebook.mjs` first. `serve` is ungated, which is fine for development. |
| +- Rates, band scaling, link parameters, typed figures, the country list and defaults are as round five found them. |
| +- The one visible difference is that a locale guess shows £ on the site and $ in the console. That is ruling 9 as intended. |
| + |
| +### I. Tests |
| +1. `TestCheckoutFormCurrencyField:204` forges signatures from `raw`. This is evidence for P2-1, and the test needs changing with the fix. |
| +2. The trigger-swap test cannot fail on the non-atomic version (table above). |
| +3. In `TestOwnershipIsEnforcedOnEveryPath`, every leg ignores errors (`_ = billingPass`, a webhook with no status check). The negative assertions also pass if the pass errors early or the webhook is a no-op. |
| + - Fix: assert the listing or read was called, and that the webhook returned its expected status. |
| +4. `provision_test.go` checks the script's source by substring: it neither generates the commands nor runs jq. The dispositions' "reads the generated checks for the pager" overstates this. |
| +5. Dead code at `billingcurrency_test.go:2460-2461`. |
| +6. No new wall-clock hazards. The webhook timestamps use `time.Now()`, and bind's review time uses `time.Now()` rather than the pass's clock. That predates this branch and does not break the asserts. |
| + |
| +## Claims the code contradicts |
| +- **Design `:152`, and the comments at `billingpage.go:100-104` and `:430-434`:** "only a page this console rendered for this session can produce it" and "nothing is bought at a price a page did not show this session" are false (P2-1). |
| +- **Dispositions, review five:** |
| + - "each is fixed below with a test that fails without it" is not true of the trigger swap (I-2). |
| + - The Astra 2 row says it is fixed. It is not. |
| +- **Comments:** `billingservice.go:939-941` and `billingstore.go:193-195` (P3-2). |
| +- **Dispositions, Opus P3-5 row:** "the plans test reads the generated checks". It reads script source (I-4). |
| +- **Review-four table (P):** as amended in round five, its rows and its eight mutation claims match the code. Each named fix has a distinguishing assertion. |
| + |
| +## Before enabling: Stripe test mode |
| +1. **Read all 36 prices** with `expand[]=currency_options`. Expect an empty map or only the price's own currency. Run Checks 2 and 3 against an account holding more than 100 active prices, so the pager is exercised. |
| +2. **A session per currency** (USD, EUR, GBP) for buyers in IE, GB, US, IN and JP, including IN with GBP. Expect: |
| + - the session currency to equal the price's |
| + - no `currency_conversion` |
| + - Adaptive Pricing off |
| + - a subtotal of unit × quantity |
| +3. **A session whose currency differs from its price's.** Expect a 400 at create; record the error shape. |
| +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, then after cancelling. |
| + - A GBP overage invoice for an old month 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 frozen calculation. |
| +7. **A GBP quantity change and a GBP renewal.** Expect the proration lines and the renewal invoice in GBP. |
| +8. **Lost-response replays:** |
| + - 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. |
| +9. **`ExpireCheckout` on a completed session.** Record the error shape. |
| +10. **A replayed `checkout.session.completed`** for a session whose refused review was resolved. Expect no review and no record. |
| +11. **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.** Before enabling: P2-1 (server-keyed quote MAC, or the claim withdrawn), and P3-1's named delete. The rest can follow. |
diff --git a/implementation/carloku-paid-launch/stripe-provision.sh b/implementation/carloku-paid-launch/stripe-provision.sh| index 222fb5306..eb56f6a17 100755 |
| --- a/implementation/carloku-paid-launch/stripe-provision.sh |
| +++ b/implementation/carloku-paid-launch/stripe-provision.sh |
| @@ -390,18 +390,22 @@ print_check() { |
| 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 |
| +all_of() { # all_of <path> <query...>: every object of a list endpoint, as a JSON array; a page that fails or is not a list fails the whole |
| + local path="$1"; shift; local all='[]' after='' page more |
| 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")" |
| + page="$(stripe get "$path" @FLAGS@ -d limit=100 ${after:+-d "starting_after=$after"} "$@")" || { echo "LIST FAILED: $path" >&2; return 1; } |
| + jq -e '.object == "list" and (.data | type) == "array" and (.has_more | type) == "boolean"' <<<"$page" >/dev/null || { echo "LIST FAILED: $path returned no list page" >&2; return 1; } |
| + all="$(jq -cs '.[0] + .[1].data' <<<"$all $page")" || return 1 |
| + more="$(jq -r .has_more <<<"$page")" || return 1 |
| + [ "$more" = true ] || break |
| + after="$(jq -er '.data[-1].id' <<<"$page")" || return 1 |
| 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; } |
| +products="$(all_of /v1/products -d active=true)" || exit 1 |
| +jq -e --arg terms "$TERMS" '([.[] | select(.metadata.terms_version == $terms)] | length) == 3' <<<"$products" >/dev/null || { echo "PRODUCT COUNT FAILED" >&2; exit 1; } |
| +prices="$(all_of /v1/prices -d active=true)" || exit 1 |
| +jq -e --arg book "$BOOK" '([.[] | select(.metadata.price_book == $book)] | length) == 36' <<<"$prices" >/dev/null || { echo "PRICE COUNT FAILED" >&2; exit 1; } |
| echo "counts ok: 3 products of $TERMS, 36 prices of $BOOK" |
| EOF |
| ) |
diff --git a/internal/console/billingcurrency_test.go b/internal/console/billingcurrency_test.go| index 54bbf2994..9cd07af7e 100644 |
| --- a/internal/console/billingcurrency_test.go |
| +++ b/internal/console/billingcurrency_test.go |
| @@ -200,8 +200,10 @@ 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")) |
| + // The quote as the page would have signed it for this session, this |
| + // account and its one member. |
| + form.Set("quoted_members", "1") |
| + form.Set("quote_sig", fx.srv.billingQuoteSignature(raw, acct.Sqid, fx.srv.billing.cfg.PriceBook, "IE", form.Get("quoted_currency"), form.Get("currency_chosen") == "1", 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}) |
| @@ -1993,6 +1995,33 @@ func TestBillingFormPostsWhatItShows(t *testing.T) { |
| 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 page quoted", intent, err) |
| } |
| + // A forgery from everything the form discloses: the session token |
| + // (csrf), the book (public), the fields, and the algorithm, keyed |
| + // as the fifth build keyed it. Refused: the key is the server's. |
| + _, fields = b.page("?country=IN¤cy=gbp") |
| + forged := hmac.New(sha256.New, []byte(fields.Get("csrf"))) |
| + fmt.Fprintf(forged, "carloku-billing-quote\x00%s\x00IN\x00usd\x000", fx.srv.billing.cfg.PriceBook) |
| + fields.Set("currency", "usd") |
| + fields.Set("currency_chosen", "") |
| + fields.Set("quoted_currency", "usd") |
| + fields.Set("quote_sig", hex.EncodeToString(forged.Sum(nil))) |
| + 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("a quote signed with the disclosed token: %d %s", w.Code, w.Header().Get("Location")) |
| + } |
| + // And a forgery with the current message, keyed by the token the |
| + // form discloses: refused, because the key is the server's secret. |
| + _, fields = b.page("?country=IN¤cy=gbp") |
| + disclosed := hmac.New(sha256.New, []byte(fields.Get("csrf"))) |
| + fmt.Fprintf(disclosed, "%s\x00%s\x00%s\x00%s\x00%s\x00%s\x00%s", fields.Get("csrf"), fx.sqid, fx.srv.billing.cfg.PriceBook, "IN", "usd", "0", fields.Get("quoted_members")) |
| + fields.Set("currency", "usd") |
| + fields.Set("currency_chosen", "") |
| + fields.Set("quoted_currency", "usd") |
| + fields.Set("quote_sig", hex.EncodeToString(disclosed.Sum(nil))) |
| + 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("a quote signed with the disclosed token over the current message: %d %s", w.Code, w.Header().Get("Location")) |
| + } |
| // A signature is for one session: another owner's token does not |
| // sign this one's quote. |
| other := newBillingBrowser(t, fx) |
| @@ -2003,6 +2032,46 @@ func TestBillingFormPostsWhatItShows(t *testing.T) { |
| t.Fatal("two sessions share a quote signature") |
| } |
| }) |
| + t.Run("a member who joins between the page and the post is a conflict, not a higher charge", func(t *testing.T) { |
| + fx, f, acct := newCurrencyFixture(t) |
| + f.customerCountry = "IE" |
| + for i := 0; i < 10; i++ { |
| + if err := fx.st.AddMember(acct.ID, fmt.Sprintf("m%d@keymail.dev", i), RoleMember, time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + } |
| + b := newBillingBrowser(t, fx) |
| + m, fields := b.page("?country=IE") |
| + if !strings.Contains(m, "You have 11 members") || fields.Get("quoted_members") != "11" { |
| + t.Fatalf("eleven members: %v in:\n%s", fields, m) |
| + } |
| + if err := fx.st.AddMember(acct.ID, "late@keymail.dev", RoleMember, time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + fields.Set("plan", plans.PlanCommunity) |
| + fields.Set("consent", plans.CurrentTermsVersion) |
| + if w := b.post("/billing/checkout", fields); w.Code != http.StatusSeeOther || w.Header().Get("Location") != fx.path("/billing")+"?error=conflict" { |
| + t.Fatalf("a twelfth member before the post: %d %s", w.Code, w.Header().Get("Location")) |
| + } |
| + if _, err := fx.st.billingCheckoutIntent(acct.ID); err == nil { |
| + t.Fatal("an intent was opened at a quantity the page did not price") |
| + } |
| + // A forged quoted_members does not help: the signature covers it. |
| + fields.Set("quoted_members", "12") |
| + if w := b.post("/billing/checkout", fields); w.Code != http.StatusSeeOther || !strings.HasPrefix(w.Header().Get("Location"), fx.path("/billing")+"?country=IE¤cy=") { |
| + t.Fatalf("a forged member count: %d %s", w.Code, w.Header().Get("Location")) |
| + } |
| + // The refreshed page prices twelve, and buys twelve. |
| + _, fields = b.page("?country=IE¤cy=") |
| + fields.Set("plan", plans.PlanCommunity) |
| + fields.Set("consent", plans.CurrentTermsVersion) |
| + if w := b.post("/billing/checkout", fields); w.Code != http.StatusSeeOther || !strings.HasPrefix(w.Header().Get("Location"), "https://checkout.stripe.com/") { |
| + t.Fatalf("the refreshed quote: %d %s %s", w.Code, w.Header().Get("Location"), w.Body.String()) |
| + } |
| + if intent, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || intent.Quantity != 12 { |
| + t.Fatalf("intent %+v %v; want twelve members", intent, err) |
| + } |
| + }) |
| t.Run("a currency that is not sold is refused before anything", func(t *testing.T) { |
| fx, _, acct := newCurrencyFixture(t) |
| b := newBillingBrowser(t, fx) |
| @@ -2616,6 +2685,50 @@ func TestOwnershipIsEnforcedOnEveryPath(t *testing.T) { |
| } |
| } |
| }) |
| + t.Run("the bound session answered with another account's everything", func(t *testing.T) { |
| + // Account A's intent is bound to the session. Account B has a ready, |
| + // unbound intent. The provider answers the read of A's session with |
| + // the session's id and B's everything else. The pass working on A's |
| + // intent must do nothing: in particular it must not bind B's intent |
| + // to A's session, which B's own ownership test would allow (B's |
| + // intent is unbound). |
| + 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("A's click: dest %q err %v", dest, err) |
| + } |
| + intentA, _ := fx.st.billingCheckoutIntent(acct.ID) |
| + if err := fx.st.CreateAccount(acct.ID+100, "cdef", "Other", "paul@keymail.dev", time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + other, err := fx.st.AccountBySqid("cdef") |
| + if err != nil || other == nil { |
| + t.Fatal(err) |
| + } |
| + f.mu.Lock() |
| + f.failCreate = true |
| + f.mu.Unlock() |
| + if _, err := fx.srv.startBillingCheckout(context.Background(), other, plans.PlanPersonal, "GB", "", "", f.now); err == nil { |
| + t.Fatal("fixture: B's create response was not lost") |
| + } |
| + intentB, err := fx.st.billingCheckoutIntent(other.ID) |
| + if err != nil || intentB.SessionID != "" { |
| + t.Fatalf("fixture: B's intent %+v %v", intentB, err) |
| + } |
| + f.mu.Lock() |
| + f.failCreate, f.sessionStatus, f.sessionSqid, f.subscriptionPrice = false, "complete", "cdef", intentA.Price |
| + f.meta["checkout_intent"], f.meta["account_sqid"] = intentB.ID, "cdef" |
| + f.mu.Unlock() |
| + if err := fx.srv.reconcilePendingIntent(context.Background(), intentA, f.now.Add(time.Minute)); err == nil { |
| + t.Fatal("the pass on A's intent accepted a session that is B's in everything but its id") |
| + } |
| + stateB, _ := fx.st.billingAccount(other.ID) |
| + afterB, _ := fx.st.billingCheckoutIntent(other.ID) |
| + stateA, _ := fx.st.billingAccount(acct.ID) |
| + if stateB.Subscription != "" || afterB.SessionID != "" || stateA.Subscription != "" || f.expireCalls != 0 || reviews(t, fx, acct) != 0 || reviews(t, fx, other) != 0 { |
| + t.Fatalf("B %+v / %+v, A %+v, %d expiries; want nothing bound, recorded, expired or reviewed for either account", stateB, afterB, stateA, f.expireCalls) |
| + } |
| + }) |
| t.Run("a read of the bound session answered with another", func(t *testing.T) { |
| fx, f, acct := newCurrencyFixture(t) |
| f.customerCountry = "GB" |
| @@ -2685,16 +2798,69 @@ func TestRefusedPaidSessionReviewThroughTheWebhookAndItsResolution(t *testing.T) |
| 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) |
| } |
| + // A member joins while the refused intent stands: a quantity event |
| + // against the intent, which the resolution must close. (Such events |
| + // are kept for live, team-plan intents; the fixture's intent is made |
| + // one here, in the store, for that alone.) |
| + if _, err := fx.st.db.Exec(`UPDATE billing_checkout_intents SET live_mode=1, plan='community', quantity=10 WHERE account_id=? AND id=?`, acct.ID, intent.ID); err != nil { |
| + t.Fatal(err) |
| + } |
| + if err := fx.st.AddMember(acct.ID, "kim@keymail.dev", RoleMember, time.Now()); err != nil { |
| + t.Fatal(err) |
| + } |
| + var pending int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_quantity_events WHERE account_id=? AND intent_id=? AND processed_at=0`, acct.ID, intent.ID).Scan(&pending); err != nil || pending == 0 { |
| + t.Fatalf("fixture: %d pending quantity events against the intent (%v)", pending, err) |
| + } |
| // 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 { |
| + // The review is keyed by the intent, which is what the resolution |
| + // retires: exactly that intent, with its quantity events closed and a |
| + // record of what it was. |
| + if err := fx.st.resolveBillingReview(billingReviewResolution{AccountID: acct.ID, Subscription: intent.ID, 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") |
| } |
| + var retiredSession, retiredCurrency string |
| + if err := fx.st.db.QueryRow(`SELECT session_id, currency FROM billing_retired_intents WHERE account_id=? AND intent_id=?`, acct.ID, intent.ID).Scan(&retiredSession, &retiredCurrency); err != nil || retiredSession != intent.SessionID || retiredCurrency != "gbp" { |
| + t.Fatalf("the retired intent's record: %q %q %v", retiredSession, retiredCurrency, err) |
| + } |
| + var unprocessed int |
| + if err := fx.st.db.QueryRow(`SELECT count(*) FROM billing_quantity_events WHERE account_id=? AND intent_id=? AND processed_at=0`, acct.ID, intent.ID).Scan(&unprocessed); err != nil || unprocessed != 0 { |
| + t.Fatalf("%d quantity events of the retired intent left unprocessed (%v)", unprocessed, err) |
| + } |
| + // A new purchase opens a new intent; a binding of the old session that |
| + // read it before the resolution files nothing now, and a resolution |
| + // named for the old intent touches the new one not at all. |
| + f.mu.Lock() |
| + f.sessionStatus, f.sessionCurrency = "open", "" |
| + f.mu.Unlock() |
| + // (The fake's sessions expire an hour after its clock: the new purchase |
| + // is made within that hour.) |
| + // (A team plan: the account has two members now.) |
| + if dest, err := fx.srv.startBillingCheckout(context.Background(), acct, plans.PlanCommunity, "GB", "", "", f.now.Add(30*time.Minute)); err != nil || dest == "" { |
| + t.Fatalf("a new purchase after the resolution: dest %q err %v", dest, err) |
| + } |
| + second, err := fx.st.billingCheckoutIntent(acct.ID) |
| + if err != nil || second.ID == intent.ID { |
| + t.Fatalf("second intent %+v %v", second, err) |
| + } |
| + if err := fx.st.fileCheckoutRefusedReview(intent, intent.SessionID, f.now.Add(40*time.Minute)); err != nil { |
| + t.Fatal(err) |
| + } |
| + if open, _ := review(); open != 0 { |
| + t.Fatal("a late filing for the retired intent reopened its review") |
| + } |
| + if err := fx.st.resolveBillingReview(billingReviewResolution{AccountID: acct.ID, Subscription: intent.ID, Reason: billingCheckoutRefusedReason, Occurrence: 1, Actor: "paul@keymail.dev", Note: "refunded"}, f.now.Add(50*time.Minute)); err != nil { |
| + t.Fatalf("resolving again, idempotently: %v", err) |
| + } |
| + if after, err := fx.st.billingCheckoutIntent(acct.ID); err != nil || after.ID != second.ID { |
| + t.Fatalf("the new intent after a resolution named for the old: %+v %v", after, err) |
| + } |
| _ = fx.srv.billingPass(context.Background(), f.now.Add(2*time.Hour)) |
| send("evt_again") |
| if open, n := review(); open != 0 || n != 1 { |
diff --git a/internal/console/billinghandlers.go b/internal/console/billinghandlers.go| index f2a3e6d89..6c403c47e 100644 |
| --- a/internal/console/billinghandlers.go |
| +++ b/internal/console/billinghandlers.go |
| @@ -10,6 +10,7 @@ import ( |
| "log" |
| "net/http" |
| "net/url" |
| + "strconv" |
| "strings" |
| "time" |
| @@ -84,21 +85,26 @@ func (s *Server) handleBillingCheckout(w http.ResponseWriter, r *http.Request) { |
| effective = string(c) |
| } |
| } |
| - // 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. |
| + // And the quote has to be one a page rendered for this session and |
| + // this account, at the member count it was priced at (its signature, |
| + // keyed by a secret only the server holds), 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")) { |
| + quotedMembers, merr := strconv.Atoi(r.PostForm.Get("quoted_members")) |
| + if merr != nil || country != quotedCountry || effective != quotedCurrency || !s.billingQuoteSigned(r.PostForm.Get("csrf"), acct.Sqid, s.billing.cfg.PriceBook, quotedCountry, quotedCurrency, quotedChosen, quotedMembers, r.PostForm.Get("quote_sig")) { |
| http.Redirect(w, r, back+billingQuoteQuery(country, currency, chosen), http.StatusSeeOther) |
| return |
| } |
| // The currency is given to the door only when it is a choice. With |
| - // none, the billing country decides. |
| + // none, the billing country decides. The member count the page priced |
| + // at goes with it: the door compares it with the count it charges, |
| + // under the membership lock, and a count that moved is a conflict |
| + // (the page says the account changed and shows the new price). |
| posted := "" |
| if chosen { |
| posted = currency |
| } |
| - dest, err := s.startBillingCheckout(r.Context(), acct, r.PostForm.Get("plan"), country, r.PostForm.Get("postal"), posted, time.Now()) |
| + dest, err := s.startBillingCheckoutQuoted(r.Context(), acct, r.PostForm.Get("plan"), country, r.PostForm.Get("postal"), posted, quotedMembers, time.Now()) |
| if errors.Is(err, errBillingCurrency) { |
| http.Error(w, "currency_invalid", http.StatusBadRequest) |
| return |
diff --git a/internal/console/billingpage.go b/internal/console/billingpage.go| index 9ec1b741e..16bc91be8 100644 |
| --- a/internal/console/billingpage.go |
| +++ b/internal/console/billingpage.go |
| @@ -98,10 +98,12 @@ type billingPageView struct { |
| 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. |
| + // (chosen or not) under the configured book, for this session, this |
| + // account and this member count: an HMAC keyed by a secret only the |
| + // server holds (derived from the billing secret key). The checkout |
| + // door takes a post only with a quote the page rendered, so fields |
| + // arranged to agree with each other, however they are signed with |
| + // what the form discloses, buy nothing a page did not show. |
| QuoteSig string |
| // Error is the ?error= key (unavailable|pending|conflict), "" otherwise. |
| @@ -262,7 +264,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) |
| + v.QuoteSig = s.billingQuoteSignature(p.CSRF, acct.Sqid, terms.PriceBook, v.QuotedCountry, v.Currency, v.CurrencyChosen, v.Members) |
| } |
| p.Billing = v |
| @@ -429,22 +431,27 @@ func billingPlanRows(terms plans.PriceTerms, band string, members int64) []billi |
| // 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)) |
| +// is sessionToken, the account sqid, and members members (the quantity a |
| +// team plan is priced at follows from it): HMAC-SHA256 over those, hex, |
| +// keyed by a secret derived from the billing secret key, which only the |
| +// server holds. The form discloses the session token (as csrf) and every |
| +// quoted field, and none of them is the key, so only a page this console |
| +// rendered can produce the signature for a quote. |
| +func (s *Server) billingQuoteSignature(sessionToken, sqid, book, country, currency string, chosen bool, members int) string { |
| + root := hmac.New(sha256.New, []byte(s.billing.cfg.SecretKey)) |
| + root.Write([]byte("carloku/billing/quote/v1")) |
| + mac := hmac.New(sha256.New, root.Sum(nil)) |
| flag := "0" |
| if chosen { |
| flag = "1" |
| } |
| - fmt.Fprintf(mac, "carloku-billing-quote\x00%s\x00%s\x00%s\x00%s", book, country, currency, flag) |
| + fmt.Fprintf(mac, "%s\x00%s\x00%s\x00%s\x00%s\x00%s\x00%d", sessionToken, sqid, book, country, currency, flag, members) |
| 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)) |
| +func (s *Server) billingQuoteSigned(sessionToken, sqid, book, country, currency string, chosen bool, members int, sig string) bool { |
| + return hmac.Equal([]byte(s.billingQuoteSignature(sessionToken, sqid, book, country, currency, chosen, members)), []byte(sig)) |
| } |
| // formatFigure renders num/den as the approved sentences write a figure: |
diff --git a/internal/console/billingpass.go b/internal/console/billingpass.go| index c76dfd90a..447e0d140 100644 |
| --- a/internal/console/billingpass.go |
| +++ b/internal/console/billingpass.go |
| @@ -184,7 +184,15 @@ func (s *Server) reconcilePendingIntent(ctx context.Context, intent billingInten |
| if err != nil { |
| return err |
| } |
| - if session.ID != intent.SessionID { |
| + // The intent's own session, judged against the intent this pass is |
| + // working on, before anything is dispatched on it: a response |
| + // carrying the right id and another account's everything else is |
| + // acted on for nobody. |
| + owner, err := s.store.AccountByID(intent.AccountID) |
| + if err != nil || owner == nil { |
| + return err |
| + } |
| + if !b.ownSession(session, intent, owner) { |
| return errBillingIdentity |
| } |
| switch session.Status { |
diff --git a/internal/console/billingquantityaudit.go b/internal/console/billingquantityaudit.go| index 34199aec2..318186e42 100644 |
| --- a/internal/console/billingquantityaudit.go |
| +++ b/internal/console/billingquantityaudit.go |
| @@ -19,7 +19,7 @@ func (s *Store) migrateQuantityHistoryGuards() error { |
| // the create, and a database that is already current is not touched. |
| const processingGuard = `CREATE TRIGGER billing_quantity_events_processing_guard BEFORE UPDATE ON billing_quantity_events |
| WHEN NEW.processed_at<=0 OR NEW.processed_at<OLD.occurred_at_ns/1000000000 |
| - OR NEW.processing_outcome NOT IN ('charged','included','landed_unverified','checkout_expired','checkout_abandoned') |
| + OR NEW.processing_outcome NOT IN ('charged','included','landed_unverified','checkout_expired','checkout_abandoned','checkout_refused_resolved') |
| OR length(trim(NEW.processing_reference))=0 OR length(NEW.processing_reference)>256 |
| BEGIN SELECT RAISE(ABORT,'billing_quantity_processing_invalid'); END` |
| if _, err := s.db.Exec(`CREATE TRIGGER IF NOT EXISTS billing_quantity_events_insert_guard BEFORE INSERT ON billing_quantity_events |
diff --git a/internal/console/billingservice.go b/internal/console/billingservice.go| index 5d76313d3..a3d7e9d0e 100644 |
| --- a/internal/console/billingservice.go |
| +++ b/internal/console/billingservice.go |
| @@ -291,7 +291,7 @@ func (b *billingRuntime) ownSession(session *billingstripe.Session, intent billi |
| // own (ownSession): nothing here expires a session on its id alone. |
| func (s *Server) refuseSession(ctx context.Context, intent billingIntent, sessionID string, session *billingstripe.Session, now time.Time) error { |
| if session != nil && (session.Status == "complete" || session.PaymentStatus == "paid") { |
| - if err := s.store.fileBillingReviewOnce(intent.AccountID, session.SubscriptionID, billingCheckoutRefusedReason, now); err != nil { |
| + if err := s.store.fileCheckoutRefusedReview(intent, session.ID, now); err != nil { |
| return err |
| } |
| return errBillingIdentity |
| @@ -359,6 +359,16 @@ func priceTermsFromMetadata(md map[string]string) (plans.PriceTerms, error) { |
| // it is never replaced by a default. The band comes from the country alone, so the |
| // choice of currency changes the money and nothing else. |
| func (s *Server) startBillingCheckout(ctx context.Context, acct *Account, plan, country, postal, currency string, now time.Time) (string, error) { |
| + return s.startBillingCheckoutQuoted(ctx, acct, plan, country, postal, currency, -1, now) |
| +} |
| + |
| +// startBillingCheckoutQuoted is startBillingCheckout for a form that was |
| +// priced at quotedMembers members: the door compares that with the count |
| +// it charges, read under the membership lock, and refuses (errBillingConflict) |
| +// a count that moved, so a member who joined between the page and the post |
| +// is never charged for unseen. A negative quotedMembers asks for no such |
| +// check. |
| +func (s *Server) startBillingCheckoutQuoted(ctx context.Context, acct *Account, plan, country, postal, currency string, quotedMembers int, now time.Time) (string, error) { |
| b := s.billing |
| if b == nil { |
| return "", errBillingUnavailable |
| @@ -400,6 +410,9 @@ func (s *Server) startBillingCheckout(ctx context.Context, acct *Account, plan, |
| if err != nil { |
| return "", err |
| } |
| + if quotedMembers >= 0 && len(members) != quotedMembers { |
| + return "", errBillingConflict |
| + } |
| qty := max(int64(len(members)), def.MinQuantity) |
| chosen, err := plans.ChooseCurrency(b.cfg.PriceBook, currency, country) |
| if err != nil { |
| @@ -934,12 +947,12 @@ func (s *Server) bindBillingCheckout(ctx context.Context, sessionID string) (str |
| // band), completed on other price terms or in another currency |
| // than it was asked for. Nothing is recorded from it, and a person |
| // is told, the same as when the background pass finds it unbound |
| - // (refuseSession): one open review, however many passes, webhooks |
| - // or bindings meet it; the intent stays as it is so no second |
| - // purchase opens beside it. (A resolved review is reopened by the |
| - // next pass while the intent stands: retiring such an intent is an |
| - // operator's door that does not exist yet.) |
| - if err := s.store.fileBillingReviewOnce(acct.ID, session.SubscriptionID, billingCheckoutRefusedReason, time.Now()); err != nil { |
| + // (refuseSession): one open review, keyed by the intent, however |
| + // many passes, webhooks or bindings meet it, and filed only while |
| + // that intent stands; the intent stays as it is so no second |
| + // purchase opens beside it, until the operator resolves the review, |
| + // which retires exactly that intent. |
| + if err := s.store.fileCheckoutRefusedReview(intent, session.ID, time.Now()); err != nil { |
| return "", err |
| } |
| return "", errBillingIdentity |
diff --git a/internal/console/billingservice_test.go b/internal/console/billingservice_test.go| index 7c9fa9951..15eaf3ccf 100644 |
| --- a/internal/console/billingservice_test.go |
| +++ b/internal/console/billingservice_test.go |
| @@ -48,6 +48,9 @@ type fakeBillingStripe struct { |
| // the id the session reports in place of cs_test_paid (the provider |
| // answering a read of one session with another). |
| sessionCustomer, sessionID string |
| + // sessionSqid, when set, is the client_reference_id the session names |
| + // in place of the fixture account's. |
| + sessionSqid string |
| // prices are extra price objects, by id, beside price_personal. |
| prices map[string]map[string]any |
| subscriptionPrice, subscriptionCurrency string |
| @@ -155,7 +158,11 @@ func (f *fakeBillingStripe) session() map[string]any { |
| 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"}}} |
| + sqid := f.sqid |
| + if f.sessionSqid != "" { |
| + sqid = f.sessionSqid |
| + } |
| + 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": 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 481234a3c..bd319f195 100644 |
| --- a/internal/console/billingstore.go |
| +++ b/internal/console/billingstore.go |
| @@ -170,6 +170,16 @@ func (s *Store) migrateBilling() error { |
| )`, |
| `CREATE TRIGGER IF NOT EXISTS billing_review_resolutions_no_update BEFORE UPDATE ON billing_review_resolutions BEGIN SELECT RAISE(ABORT,'billing_review_resolution_immutable'); END`, |
| `CREATE TRIGGER IF NOT EXISTS billing_review_resolutions_no_delete BEFORE DELETE ON billing_review_resolutions BEGIN SELECT RAISE(ABORT,'billing_review_resolution_immutable'); END`, |
| + // The intents retired by an operator's resolution of a refused, |
| + // paid session's review: what the intent and the session were, so |
| + // the row that tied the refused session to the account is not lost |
| + // with the intent. |
| + `CREATE TABLE IF NOT EXISTS billing_retired_intents ( |
| + account_id INTEGER NOT NULL, intent_id TEXT NOT NULL, session_id TEXT NOT NULL, |
| + plan TEXT NOT NULL, band TEXT NOT NULL, currency TEXT NOT NULL, price_book TEXT NOT NULL, |
| + reason TEXT NOT NULL, actor TEXT NOT NULL, retired_at INTEGER NOT NULL, |
| + PRIMARY KEY(account_id, intent_id) |
| + )`, |
| // The barrier against a build from before price books. Such a |
| // build reads legacy assignments only, prices what it reads in EUR |
| // and writes the month as a version 1 calculation. For a month in |
| @@ -190,9 +200,11 @@ 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. |
| - // (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.) |
| + // (This CREATE runs first; the DROP of the earlier name runs after |
| + // it, in a transaction that first confirms this one exists |
| + // (retireEarlierBarrierTrigger), so no moment leaves the database |
| + // with neither: the ordering is the guarantee, not a shared |
| + // transaction.) |
| `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 |
| @@ -888,15 +900,68 @@ func (s *Store) resolveBillingReview(r billingReviewResolution, now time.Time) e |
| 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. |
| + // operator saying the money is settled. The review is keyed by the |
| + // intent (its subscription field holds the intent id), and exactly |
| + // that intent, which was left standing so no second purchase opened |
| + // beside the refused one, is retired in the same transaction: its |
| + // quantity events are closed as retireIntent closes them, what it was |
| + // is recorded, and the row goes, so the next pass finds nothing to |
| + // bind and the review does not come back. An intent that is not that |
| + // one (a later purchase's) is left alone. |
| if r.Reason == billingCheckoutRefusedReason { |
| - if _, err := tx.Exec(`DELETE FROM billing_checkout_intents WHERE account_id=?`, r.AccountID); err != nil { |
| + var sessionID, plan, band, currency, book string |
| + err := tx.QueryRow(`SELECT session_id,plan,band,currency,price_book FROM billing_checkout_intents WHERE account_id=? AND id=?`, r.AccountID, r.Subscription).Scan(&sessionID, &plan, &band, ¤cy, &book) |
| + if err != nil && !errors.Is(err, sql.ErrNoRows) { |
| return err |
| } |
| + if err == nil { |
| + reference := sessionID |
| + if reference == "" { |
| + reference = "checkout_refused_resolved" |
| + } |
| + if _, err := tx.Exec(`UPDATE billing_quantity_events SET processed_at=?,processing_outcome='checkout_refused_resolved',processing_reference=? WHERE account_id=? AND intent_id=? AND subscription='' AND processed_at=0`, at, reference, r.AccountID, r.Subscription); err != nil { |
| + return err |
| + } |
| + if _, err := tx.Exec(`INSERT INTO billing_retired_intents(account_id,intent_id,session_id,plan,band,currency,price_book,reason,actor,retired_at) VALUES(?,?,?,?,?,?,?,?,?,?)`, |
| + r.AccountID, r.Subscription, sessionID, plan, band, currency, book, r.Reason, r.Actor, at); err != nil { |
| + return err |
| + } |
| + if _, err := tx.Exec(`DELETE FROM billing_checkout_intents WHERE account_id=? AND id=?`, r.AccountID, r.Subscription); err != nil { |
| + return err |
| + } |
| + } |
| + } |
| + return tx.Commit() |
| +} |
| + |
| +// fileCheckoutRefusedReview files the review of a session that completed |
| +// in a currency or on price terms other than its intent asked for: once |
| +// while open, keyed by the intent (the review's subscription field holds |
| +// the intent's id, which is what the operator's resolution then retires), |
| +// and only while that intent still stands, so a binding that read the |
| +// session before the resolution and files after it files nothing. |
| +func (s *Store) fileCheckoutRefusedReview(intent billingIntent, sessionID string, observed time.Time) error { |
| + tx, err := s.db.Begin() |
| + if err != nil { |
| + return err |
| + } |
| + defer tx.Rollback() |
| + var stands bool |
| + if err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM billing_checkout_intents WHERE account_id=? AND id=?)`, intent.AccountID, intent.ID).Scan(&stands); err != nil { |
| + return err |
| + } |
| + if !stands { |
| + return nil |
| + } |
| + var open bool |
| + if err := tx.QueryRow(`SELECT EXISTS(SELECT 1 FROM billing_reviews WHERE account_id=? AND subscription=? AND reason=? AND resolved_at=0)`, intent.AccountID, intent.ID, billingCheckoutRefusedReason).Scan(&open); err != nil { |
| + return err |
| + } |
| + if open { |
| + return nil |
| + } |
| + if err := fileBillingReviewTx(tx, intent.AccountID, intent.ID, billingCheckoutRefusedReason, observed); err != nil { |
| + return err |
| } |
| return tx.Commit() |
| } |
diff --git a/internal/console/templates/billing.html b/internal/console/templates/billing.html| index d675b3987..2d11539d4 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="quoted_members" value="{{.Members}}"> |
| <input type="hidden" name="quote_sig" value="{{.QuoteSig}}"> |
| <div class="signin billing-quote"> |
| <label><!-- billing.country.label -->Billing country |
diff --git a/internal/plans/provision_test.go b/internal/plans/provision_test.go| index 37f20ef66..8eca60097 100644 |
| --- a/internal/plans/provision_test.go |
| +++ b/internal/plans/provision_test.go |
| @@ -28,6 +28,8 @@ func TestStripeProvisionScriptMatchesThePriceBook(t *testing.T) { |
| `select(.metadata.price_book == $book)] | length) == 36`, |
| `select(.metadata.terms_version == $terms)] | length) == 3`, |
| `starting_after=$after`, |
| + `|| { echo "LIST FAILED: $path" >&2; return 1; }`, |
| + `products="$(all_of /v1/products -d active=true)" || exit 1`, |
| `-ge 36 ]`, |
| `to_entries | all(.key == $currency and .value.unit_amount == $cents and (.value.tax_behavior == null or .value.tax_behavior == "" or .value.tax_behavior == "exclusive"))`, |
| } { |