Skip to content

fix: explain an unbuyable checkout instead of a dead button, and keep an inverted credit range out of the DOM - #1438

Merged
sakibsadmanshajib merged 4 commits into
mainfrom
fix/buy-credits-rails
Aug 29, 2026
Merged

sakibsadmanshajib merged 4 commits into
mainfrom
fix/buy-credits-rails

Conversation

@sakibsadmanshajib

@sakibsadmanshajib sakibsadmanshajib commented Aug 29, 2026 •

Copy link
Copy Markdown
Owner

What a live QA sweep found

On the deployed console the buy-credits modal is unusable and says nothing about why:

  • GET /api/v1/accounts/current/checkout/rails returns one rail, {"rail":"stripe","enabled":false}. No bKash, no SSLCommerz.
  • The modal correctly filters out disabled rails, so the Payment method fieldset renders empty and Continue to payment is permanently disabled with no explanation.
  • The same response carries min_credits: 10000000 alongside max_credits: 0, and both go straight into the amount input's HTML min and max. max < min is an invalid HTML5 number range regardless of any rail.

Established cause

Three candidate explanations were on the table (absent credentials, a feature gate, or a reporting bug). It is the first, and here is the evidence rather than a guess.

  1. No rail credential exists on the box. A direct read of the deployment's .env shows STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET, BKASH_APP_KEY, SSLCOMMERZ_STORE_ID, HIVE_PAYMENTS_STUB and HIVE_ENV all absent. Control-plane's own boot line confirms the consequence: 2026/08/29 16:14:15 payments: 0 rail(s) active: []. The rails map in apps/control-plane/cmd/server/main.go is populated only from credentials.
  2. A feature gate is not involved, in either direction. The payments package contains zero references to featuregate, and the featuregate package's only billing mention is its admin category list. Issue Seven billing feature gates have no runtime reader, so disabling a payment rail leaves it live #756 (billing gates with no runtime reader) does not reach this path, so nothing here depends on gate plumbing being trustworthy.
  3. The endpoint is reporting correctly. GetCheckoutOptions marks a rail enabled exactly when the deployment registered it. Only Stripe is listed because AvailableRails("") returns the non-BD set and the account's country is not BD.
  4. max_credits: 0 is a deliberate server sentinel. MostRestrictiveMaxCredits skips disabled options and documents 0 as its answer when nothing is selectable.

So the disabled state is correct. The defects are the silence about it, and the console treating that sentinel as a purchase ceiling.

What changed

Two guards, placed at the single choke point every caller routes through (app/api/v1/accounts/current/[...path]/route.ts calls getCheckoutRails(), and the modal is its only consumer).

lib/control-plane/client.ts — getCheckoutRails now rejects an unusable purchase bound set when a rail is actually selectable: a ceiling below the floor, a non-positive floor, a non-positive step, or any of the three simply missing. The bounds are read raw and judged on what the server actually sent, and the fallback defaults apply only afterwards, on the path where nothing is purchasable and no bound is ever rendered. Judging after defaulting would fabricate a coherent range out of an omitted field and wave it through, which is the quiet half of this same defect and is how issue #1386 shipped a fabricated 1.00 USD ceiling against a real 100.00 USD one with nothing complaining.

The guard is conditional on a selectable rail on purpose: a zero ceiling with nothing selectable is a real deployment state, not corruption, and raising a server error there would replace one dishonest message with another.

components/billing/checkout-modal.tsx — the modal branches on whether a purchase is possible at all. With no selectable rail, or with incoherent bounds, it renders a plain-words explanation that names the next move and renders neither the amount input nor a Continue button, so there is no dead control and no attribute pair that can be inverted. Keep balance still closes the modal. The purchase path is untouched when a rail is available.

Before and after, from the user's seat

Before After
No rail configured Empty Payment method fieldset, permanently disabled Continue to payment, no explanation "No payment method is available for this account yet, so credits cannot be bought here. Your balance and your existing API keys are unaffected. Contact support to have a payment method enabled for this account." No amount input, no dead button
Amount input attributes min="10000000" max="0" The input is not rendered at all in that state
Selectable rail with a broken or incomplete bound set Rendered as an invalid range, or silently defaulted Load fails honestly with the existing "Unable to load payment options" message
Selectable rail, healthy bounds Works Unchanged

Money semantics

No pricing, charge, ledger, credit-unit or FX code is touched. No amount moves in any direction. The diff is three source files on the billing and checkout surface, one new test file, and one proof log.

The underlying cause is out of scope here, and is filed

This pull request fixes the console's handling of an unbuyable state. It does not make credits buyable on the demo box, and it must not be read as having done so.

The reason no rail is enabled is a deployment configuration fact: the box carries no payment rail credentials and no payment stub either, so control-plane registers zero rails at boot. That is filed as #1449, with the three options for resolving it and the note that the BD rails need a BD-country account to be exercised at all. The graceful message this PR adds is not intended to become the permanent answer.

Related issues

Review streams

Stream Status
Mandatory security review (money path) Ran. No blocking findings. Confirmed the authoritative guard runs server side in the proxy route, the modal check is defence in depth, and Go's ValidatePurchaseAmount and maxCreditsForRail remain independently authoritative at initiate time. One LOW finding, fixed rather than accepted.
Adversarial TypeScript and React review Ran. One LOW finding on a vacuous assertion loop in the new test, fixed.
CodeRabbit CLI Ran, on a retry. The first attempt refused with "Review limit reached" and was reported as SKIPPED at the time; after the window reset it reviewed the full diff. Two findings: an unvalidated credit_increment, real and fixed; and "move this buglog entry to the PR body" aimed at docs/proof/.../log.md, a false positive, rebutted in a PR comment. The CodeRabbit GitHub App check separately reports "Review rate limited" on the same account.
Plain adversarial read of the diff Ran by the author, folded into the fixes above.

Review follow-ups, in order

  1. The range check ran after the fallback defaults were applied, so an omitted bound was filled in with a fabricated coherent value and passed. Bounds are now judged raw, before defaulting.
  2. The unavailable message named the state but not the next move. It now says what to do.
  3. The DOM range assertion iterated an empty node list on every pass, a green that could not go red. A healthy case joined the list so it runs against a real node.
  4. The purchase predicate checked the floor and the ceiling but not the step. A credit_increment of 0 survives the nullish fallback as a literal 0, renders as step="0" and freezes both stepper buttons; a negative one makes the decrement button raise the amount. Both now take the explained-unavailable path, with test cases.

Live verification

Captured against the live demo box with two console images built from this repository, one from this branch's parent commit and one from this branch, both talking to the box's real control-plane and real GoTrue. Identical server response on both runs:

GET /api/v1/accounts/current/checkout/rails -> 200
{"rails":[{"rail":"stripe","currency":"USD","label":"Card","enabled":false}],
 "credit_increment":10000000,"min_credits":10000000,"max_credits":0, ...}

Before: credit-amount renders with min="10000000" max="0", Continue to payment present and permanently disabled, no explanation.
After: no credit-amount input in the DOM at all, no Continue control, the state explained with a next step, Keep balance still closes the modal.

Re-run against a rebuild from the final commit after all four follow-ups: identical output. Full capture log, with the substrate stated plainly: docs/proof/buy-credits-rails-2026-08-29/log.md. Screenshots posted in a comment below.

Tests

__tests__/checkout-rails-range-guard.test.ts covers the decoder: a coherent bound set passes; an inverted, non-positive, non-finite or omitted bound with a selectable rail throws; a zero ceiling with nothing selectable is preserved as the honest answer, as is an omitted bound where nothing can be bought.

components/billing/checkout-modal-ui.test.tsx carries the regression guard for the exact defect shape. Across a no-rail fixture, two inverted-range fixtures, a zero-step and a negative-step fixture, and one healthy fixture, every rendered input[type=number] must carry a finite, positive, non-inverted range, and the presence of the field must match whether the case is purchasable. The healthy case is what stops the attribute loop from being a green that cannot go red.

The pre-existing "pay button is gated" test is replaced, because the behaviour it locked in (render a dead disabled button) is the defect this PR removes.

Buglog entry

{"id":"bug-2026-08-29-checkout-rails-sentinel","date":"2026-08-29","title":"Buy-credits modal renders a dead button and an inverted min/max range when no payment rail is configured","error_message":"checkout/rails returns min_credits=10000000 with max_credits=0; console writes min=\"10000000\" max=\"0\" onto the amount input and leaves Continue to payment permanently disabled with no explanation","root_cause":"The deployed box has no payment rail credentials, so control-plane reports every rail enabled=false and MostRestrictiveMaxCredits returns its documented 0 sentinel for 'nothing is selectable'. The console treated that sentinel as a purchase ceiling and wrote it straight into the HTML min/max attributes, rendered the purchase form regardless of whether any rail could complete a purchase, and applied its fallback defaults before checking coherence so an omitted bound was fabricated rather than refused.","fix":"getCheckoutRails judges the purchase bounds raw and rejects an unusable set when a rail is selectable; CheckoutModal branches on whether a purchase is possible and renders an explanation with a next step, plus no amount input and no Continue button, when it is not.","tags":["billing","checkout","console","payments","silent-failure","frontend"]}

… an inverted credit range out of the DOM

On the deployed box the buy-credits modal renders an empty Payment method
fieldset, a permanently disabled Continue to payment button that explains
nothing, and an amount input carrying min="10000000" max="0".

The cause is not a bug in the rails endpoint and not a feature gate. The box
has no payment rail credentials at all (control-plane logs "payments: 0
rail(s) active: []"), so every rail reports enabled=false, which is the honest
answer. The top-level max_credits is the most restrictive ceiling among the
rails the payer can actually select, and the control plane documents 0 as its
answer when nothing is selectable. The console treated that sentinel as a
purchase ceiling and wrote it straight into the HTML min and max attributes.

Two guards, at the one path every caller routes through:

getCheckoutRails now refuses a purchase range whose ceiling is below its floor,
or whose floor is not positive, whenever a rail is actually selectable. That is
a broken response and belongs on the error path rather than in the DOM. The
guard is deliberately conditional on a selectable rail, because a zero ceiling
with nothing selectable is a real deployment state, not corruption.

CheckoutModal now branches on whether a purchase is possible at all. With no
selectable rail it says so in plain words and renders neither the amount input
nor a Continue button, so there is no dead control and no attribute pair to
invert. Keep balance still closes the modal. The purchase path is unchanged
when a rail is available.

No pricing, charge, ledger or credit-unit arithmetic is touched, and no
monetary amount moves in any direction.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1
@coderabbitai

coderabbitai Bot commented Aug 29, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

Next included review available in 54 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: a83f3079-410c-46f9-8727-c6a70b7a6165

📥 Commits

Reviewing files that changed from the base of the PR and between 908e48a and 0533dac.

📒 Files selected for processing (5)
  • apps/web-console/__tests__/checkout-rails-range-guard.test.ts
  • apps/web-console/components/billing/checkout-modal-ui.test.tsx
  • apps/web-console/components/billing/checkout-modal.tsx
  • apps/web-console/lib/control-plane/client.ts
  • docs/proof/buy-credits-rails-2026-08-29/log.md

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Comment thread apps/web-console/lib/control-plane/client.ts
@sakibsadmanshajib

Copy link
Copy Markdown
Owner Author

Security review (money path: buy-credits checkout modal).

Checked against the five points in the review request:

  1. Malformed/hostile control-plane response reaching a purchase the server would refuse, or bypassing a bound. No bypass found. Posted one LOW/informational inline finding: the new coherence guard in getCheckoutRails() applies fallback defaults (?? 10_000_000 / ?? 1_000_000_000) to min_credits/max_credits before checking coherence, so a response that omits those fields (rather than sending an explicit non-positive or inverted value) slips past the guard with fabricated-but-coherent numbers instead of being rejected. It does not let a purchase clear a bound the server would refuse, because ValidatePurchaseAmount (apps/control-plane/internal/payments/types.go) independently re-validates credits > 0, the CreditIncrement multiple, and the real per-rail ceiling at initiate time, with no dependency on anything the console computed.

  2. Client-side range check mistaken for enforcement. Confirmed the two checks are correctly layered and neither is being treated as authority: the coherence guard added to getCheckoutRails() in apps/web-console/lib/control-plane/client.ts runs server-side, inside the Next.js route handler (app/api/v1/accounts/current/[...path]/route.ts) that proxies the browser's rails request, so a malformed upstream payload is rejected before it ever reaches the browser. The rangeIsCoherent/canPurchase check added to CheckoutModal is genuinely browser-side and is documented in its own comment as "defence in depth," not enforcement; it only decides what renders. Nothing server-side (ValidatePurchaseAmount, maxCreditsForRail) was touched by this diff, confirmed by grep across apps/control-plane/internal/payments/.

  3. Information disclosure in the new user-facing copy. None. The two unavailableReason strings ("No payment method is available for this account yet…" / "The payment options for this account came back unusable…") name no rail, provider, or deployment configuration fact. The existing enabled-rail list (unchanged by this PR) already only ever showed generic labels like "Card"/"bKash", not touched here.

  4. NaN / Infinity / negative / > Number.MAX_SAFE_INTEGER reaching the guard or the amount field. Handled. readNumberField type-checks to number (which admits NaN and ±Infinity), and the new guard explicitly gates on Number.isFinite(...) plus <= 0 / < minCredits, matching the pre-existing FX-17 pattern a few lines above it for price_per_block_minor/credit_block_size. A value past MAX_SAFE_INTEGER that's still a finite float passes the coherence check, but that's pre-existing, unrelated to this diff: computeBlockSplitAmountMinor already routes the actual money math through BigInt to avoid precision loss at that magnitude, and the real ceiling is still Go int64 arithmetic in ValidatePurchaseAmount/maxCreditsForRail.

  5. New branch letting a checkout fire with an amount the user didn't choose. No. handleCheckout is unchanged and still POSTs the current creditAmount/selectedRail state. The diff only removes a path to firing it: previously "Continue to payment" rendered disabled when nothing was purchasable, now it doesn't render at all when canPurchase is false, which is strictly tighter than before.

One LOW/informational finding posted inline on apps/web-console/lib/control-plane/client.ts (non-blocking). No HIGH/CRITICAL findings.

Comment thread apps/web-console/components/billing/checkout-modal-ui.test.tsx Outdated
@sakibsadmanshajib

Copy link
Copy Markdown
Owner Author

Adversarial TypeScript/React review

Scope note: CI was still running at review time (Web console (type + unit + build), Go tests, CodeQL all IN_PROGRESS; none of the completed checks had failed). I did not check out the branch; I read the pushed blobs directly and traced the request path (route handler -> getCheckoutRails() -> CheckoutModal) instead of running the Docker build locally, to avoid duplicating the in-flight CI job. Re-check this review's conclusions against that job once it lands.

1. Type safety

No as, any, or unsafe casts introduced. readNumberField/readBooleanField (pre-existing, unchanged) already gate on typeof, so nothing in this diff weakens the decode boundary. One thing worth confirming explicitly: the guard uses ?? (not ||) when defaulting min_credits/max_credits in checkout-modal.tsx (lines 202-203, unchanged by this diff but load-bearing for the fix) — || would silently turn a real 0 ceiling back into the 1,000,000,000 fallback and defeat the whole guard. ?? is correct here.

2. Guard-conditionality hole enumeration (item 4 of the brief)

Traced every payload shape that can reach the DOM given the full pipeline (getCheckoutRails() -> app/api/v1/accounts/current/[...path]/route.ts GET handler -> client fetch -> CheckoutModal):

  • No rail decodes as enabled (real defect being fixed): server guard skipped (anyRailSelectable false), range passed through as-is. On the client, canPurchase = selectableRails.length > 0 && rangeIsCoherent short-circuits to false on the first clause regardless of range — the amount input is never rendered. No hole.
  • A rail is enabled and the range is incoherent (max < min, or min <= 0): getCheckoutRails() throws before ever returning. The route handler's catch converts this to a non-2xx response; the client's fetch sees !response.ok and sets fetchError instead of options. CheckoutModal's own canPurchase check is never reached in production for this shape — it's true defense-in-depth (matters only if the modal is ever fed options from something other than this endpoint, e.g. a future refactor or a unit test), not a live path today.
  • A malformed rail entry that fails decodeCheckoutRail (missing label, non-boolean enabled, etc.) is dropped from the rails array entirely, on both the guard's anyRailSelectable check and the client's selectableRails filter, since they read the same decoded array. No divergence between what the guard sees and what the modal sees.
  • NaN/Infinity in the wire payload: not representable — JSON.parse has no NaN/Infinity literal, so readNumberField's typeof value === "number" can never see one. The Number.isFinite checks in both the guard and the modal are defensive but not exercisable via this HTTP boundary; not a hole, just unreachable insurance.
  • Both min_credits/max_credits absent from an otherwise-valid payload with an enabled rail: falls back to the hardcoded 10,000,000 / 1,000,000,000 defaults (pre-existing behavior, untouched by this PR), which is coherent by construction and does not trigger the new guard. Not a regression introduced here.

No enumerated shape puts an invalid range or a NaN into the DOM. The client-side isCheckoutOptions type guard in checkout-modal.tsx (pre-existing, not touched by this PR) is structurally loose — it only checks Array.isArray(value.rails), not that min_credits/max_credits are numbers — but it's not exploitable today because the only producer of that response is getCheckoutRails(), which already guarantees the shape or throws. Worth knowing if that route handler is ever changed to proxy a payload from anywhere else.

3. Accessibility

role="status" is uniquely used (no getByRole collision with the existing role="alert" on fetchError), announces politely without extra aria-live wiring, and the dialog's aria-labelledby="checkout-title" is untouched since the title renders unconditionally in every branch. "Keep balance" stays outside the canPurchase conditional, so the modal is always exitable — no unreachable control in the new !canPurchase branch. No regression found.

4. Test replacement (item 6 of the brief)

Legitimate, not a coverage drop. The removed test ("pay button is gated until a rail is selectable...") asserted the exact defect this PR removes: a permanently disabled Continue button with the Payment method heading still visible and no explanation. Keeping it would have meant asserting the old broken UX. The two replacement tests are a strict superset for that scenario: they check the status message text, absence of the "Payment method" heading, absence of the Continue button (not just disabled — actually absent), and presence of "Keep balance". No behavior the old test verified is left unverified.

Minor gap, not worth a blocking finding: none of the new tests isolate "no rail selectable, but the accompanying range happens to be coherent" as its own fixture (the closest, noRailFixture(), pairs the disabled rail with the same 10M/0 inverted range every time). The short-circuit && in canPurchase makes this a non-issue by construction, so it's a nice-to-have, not a requirement.

5. Left one inline comment

On checkout-modal-ui.test.tsx, the input[type=number] min/max loop inside "an inverted or zero purchase range never reaches the DOM" never actually executes its assertion body against a real element under the current three fixtures, since canPurchase is false in all of them — see the inline comment for detail and a suggested positive-control fixture.

No other findings. React derived-state, effect-dependency, and closure checks (item 2 of the brief) turned up nothing: canPurchase/rangeIsCoherent/selectableRails are plain consts recomputed every render (not stuffed into useEffect+useState), the one pre-existing useEffect (mount-time rails fetch) is untouched by this diff, and decrementAmount/incrementAmount close over fresh per-render values with no memoization to go stale.

…o do next

Review follow-ups on the buy-credits guard.

The range check ran after the fallback defaults were applied, so a response
that omitted min_credits or max_credits was filled in with fabricated values
that were coherent by construction and sailed straight through. The guard
caught a loudly wrong response and missed a silently incomplete one, which is
the same shape as the defect it exists for: issue #1386 shipped a fabricated
1.00 USD ceiling against a real 100.00 USD one and nothing complained. The
bounds are now read raw and judged on what the server actually sent, and the
defaults apply only afterwards, on the path where nothing is purchasable and
no bound is ever rendered. credit_increment joins the check for the same
reason, since a fabricated step is the same class of quiet failure.

The unavailable message now names the next move rather than only the state. A
dead end that explains itself is still a dead end, and being left with nothing
to do was the original complaint.

The DOM range assertion also gained a healthy case. With only unpurchasable
fixtures it iterated an empty node list on every pass, so it would have gone
on reporting success against a modal that rendered no amount field for any
payload at all. It now runs against a real node once and stays a tripwire for
a regression that starts rendering the field again on the broken payloads.

Adds the live before and after capture log under docs/proof.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@sakibsadmanshajib

Copy link
Copy Markdown
Owner Author

Visual proof

Buy credits on the live demo box, same account and same server response on both runs (one Stripe rail, disabled, min_credits 10000000 against max_credits 0). Left, on this branch's parent commit: empty Payment method fieldset, Continue to payment present but permanently disabled with no explanation, and the amount input rendered with min=10000000 max=0. Right, on this branch: no amount input in the DOM at all, no dead Continue control, the state explained with a next step, and Keep balance still closes the modal. Full capture log in docs/proof/buy-credits-rails-2026-08-29/log.md

pr1438-20260829173920-13383-before-02-modal.png

pr1438-20260829173923-14783-after-02-modal.png

CodeRabbit caught that the modal's purchase predicate checked the floor and
the ceiling but not the step. A `credit_increment` of 0 survives the nullish
fallback as a literal 0, renders as step="0", and freezes both stepper buttons
on an amount the payer cannot change. A negative one is the same fault with a
sign, where the decrement button raises the amount. Either way the form is a
dead control, which is the thing this change exists to remove.

The step now sits in the same predicate as the bounds, so such a response
takes the same explained-unavailable path rather than rendering a frozen
field. An absent increment is deliberately still allowed at this layer: the
fallback substitutes a real one-cent step, and getCheckoutRails already
refuses an absent increment upstream whenever a rail is selectable, so the
modal never sees that case from the real path.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1
@sakibsadmanshajib

Copy link
Copy Markdown
Owner Author

CodeRabbit CLI stream

Earlier reported as SKIPPED on this PR because the CLI refused with "Review limit reached". It was retried after the window reset and did run against the full diff. Two findings, both major by its rating. One is fixed, one is a false positive and is rebutted below.

1. checkout-modal.tsx — validate credit_increment before purchase is available. FIXED in 2276646.

Correct and worth having. The purchase predicate checked the floor and the ceiling but not the step. A credit_increment of 0 survives the nullish fallback as a literal 0, renders as step="0", and freezes both stepper buttons on an amount the payer cannot change. A negative one is the same fault with a sign: the decrement button would raise the amount. Either way the field is a dead control, which is exactly what this PR exists to remove, so leaving that one shape rendering would have been a half fix.

The step now sits in the same predicate as the bounds, and two cases were added to the DOM guard test: zero increment and negative increment, both expected unpurchasable. An absent increment is deliberately still tolerated at this layer, and the test says so in a comment: the fallback substitutes a real one-cent step there, and getCheckoutRails already refuses an absent increment upstream whenever a rail is selectable, so the modal never receives that shape from the real path.

2. docs/proof/buy-credits-rails-2026-08-29/log.md — "move this buglog entry to the PR body". REBUTTED, no change.

This is a false positive: it has matched the wrong rule against the wrong file.

The repository rule it quotes governs .wolf/buglog.jsonl, the tracked bug-memory file, which must never be appended to on a feature branch because GitHub's server-side merge ignores the merge=union driver and every branch that appended one line then conflicts serially (issue #873). This PR does not touch .wolf/ at all, and the buglog entry for this defect is carried in the PR body under a "Buglog entry" heading exactly as that rule requires.

docs/proof/<slug>/log.md is a different artifact under a different, and opposite, rule. The visual-proof rule requires the capture's text log to be committed under docs/proof/, precisely because npm run lint:proof-tokens scans that directory and nothing else. A log left in a scratch directory, a PR comment or a release asset is unscanned, and the linter then goes on reporting green over the frozen legacy corpus rather than failing. Moving this file into the PR body would trade a loud CI check for a quiet absence, which is the specific trade that rule exists to prevent. npm run lint:proof-tokens passes on this branch: 223 files scanned, self-test ok.

The before and after capture was taken before the review follow-ups landed.
Re-ran the after half against a rebuild from the final commit, on the same
account and the same server response, and got identical output. Says so in the
log rather than leaving the reader to assume the screenshots match the code
that will merge.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WyEwUxZCArdn1ZUDkTvuQ1
@sakibsadmanshajib
sakibsadmanshajib merged commit 472dab6 into main Aug 29, 2026
30 checks passed
@sakibsadmanshajib
sakibsadmanshajib deleted the fix/buy-credits-rails branch August 29, 2026 18:25
sakibsadmanshajib added a commit that referenced this pull request Aug 29, 2026
…g is created (issue #1449) (#1470)

Fixes #1449

## What the issue said, and what I found instead

The issue concluded this is configuration, not a code defect: the demo
box holds no rail credentials, so control-plane registers no rail, `GET
/checkout/rails` reports `enabled: false`, and PR #1438 already fixed
the console's presentation of that state. I verified the configuration
half and it holds. I do not agree it is the whole story, because the API
half of the same flow is a defect, and one of the issue's three proposed
remedies is not actually available.

### The flow, hop by hop

1. Console entry:
`apps/web-console/components/billing/checkout-launcher.tsx` opens
`checkout-modal.tsx`, which reads the rail list at
`checkout-modal.tsx:80` and posts the purchase at
`checkout-modal.tsx:155`.
2. Rail list: `apps/control-plane/internal/platform/http/router.go:332`
routes to `payments/http.go:80` `handleGetRails`, which calls
`payments/service.go:618` `GetCheckoutOptions`. That function marks a
rail `enabled` only if `s.rails[rail]` exists (`service.go:631`), which
is accurate.
3. Registration: `apps/control-plane/cmd/server/main.go:919-927`
registered each rail from a single environment variable, then logged
`payments: N rail(s) active`.
4. Purchase: `payments/http.go:147` `handleInitiateCheckout` calls
`payments/service.go:88` `InitiateCheckout`.

### What the customer got before this change

Not a spinner and not a silent dead end. On a box with no credentials
the console itself never posts, because no rail is selectable, so a
console user sees the #1438 message. Anything that does post, an SDK
caller, a curl, or a future surface that trusts the endpoint, walked the
whole path: country lookup, billing profile read, FX snapshot on a BD
rail, `InsertPaymentIntent`, and only then the rail lookup at the old
`service.go:197`, which failed with `payments: no rail implementation
for stripe`. `classifyInitiateError` has no case for that string, so it
fell through to the default and answered **400 "checkout failed"**,
blaming the payer for a deployment fault, and leaving a stranded
`created` intent row plus a burned FX snapshot behind on every attempt.

### Test mode

No code branches on live versus test anywhere.
`payments/stripe/rail.go:24` hands `STRIPE_SECRET_KEY` straight to the
Stripe SDK, so `sk_test_...` with the `whsec_...` from a test-mode
webhook endpoint is a complete, demonstrable configuration and needs no
code change. That part of the issue is correct: it is config.

### The remedy the issue proposes that does not exist

Option 2, turning on the demo stub, cannot be done from the box's
`.env`. `deploy/docker/docker-compose.yml` passes control-plane an
explicit environment list, and neither `HIVE_PAYMENTS_STUB` nor
`HIVE_ENV` is on it. Neither string appears anywhere under `deploy/`. So
the container never sees either variable, `paymentStub.IsEnabled()` is
always false under compose, and setting them on the box does nothing. I
deliberately did not wire them through: that would make instant credit
reachable from a single variable, and this change is about refusing
honestly, not about granting credit without a payment.

## What changed

1. `payments/service.go`: the rail implementation lookup moved up to sit
immediately after the country-availability check, so it now precedes the
profile read, the FX snapshot, the insert and the provider call (it was
moved ahead of the country check in the first cut of this PR, and moved
back one step in review, see the review response below). It returns a
new sentinel, `ErrRailNotConfigured`, whose error text names the
environment variables an operator must set, the same way
`ErrReturnURLNotConfigured` already does.
2. `payments/http.go`: `classifyInitiateError` maps that sentinel to
**503** with `this payment method is unavailable right now and nothing
was charged`. Provider blind, and a deployment fault rather than a
customer one. The rail name and the variable names stay in the existing
server-side log line.
3. `payments/rail_credentials.go` (new): `RailCredentialEnvs` declares
the full credential set per rail in one place, and
`MissingRailCredentials` reports which of them are unset, treating
whitespace as unset.
4. `cmd/server/main.go`: a rail is now registered only when its whole
credential set is present. Each incomplete rail is skipped and logged as
`WARNING: payments: <rail> rail not registered, buying credits on it
will be refused. Unset credential(s): ...`, alongside the existing
active-rail count. This closes a worse failure than the one in the
issue: a Stripe secret key with no webhook secret used to register a
rail that could redirect a payer and take their money, then fail
signature verification on every settlement event, so the charge
succeeded and the credit never arrived.
5. `.env.example`: documented the payment rail section, which previously
named none of these variables at all. Empty placeholders only, no
credentials of any kind, plus the note that Stripe test-mode keys work
unchanged and that `HIVE_PAYMENTS_STUB` is not plumbed through compose.

No credit is granted anywhere in this diff, and no purchase is faked.
`math/big` arithmetic is untouched.

## Tests

New file
`apps/control-plane/internal/payments/checkout_unconfigured_rail_test.go`:

-
`TestInitiateCheckout_UnconfiguredRailGrantsNoCreditAndCreatesNothing`:
with a complete billing profile and zero rails registered, the call
fails with `ErrRailNotConfigured`, the ledger records **zero** grants,
**zero** intents are inserted, and the server-side error names both
Stripe variables.
- `TestInitiateCheckout_UnconfiguredBDRailTakesNoFXSnapshot`: same for a
BD account on bKash, additionally asserting no FX snapshot is taken and
no intent is stranded.
- `TestInitiateEndpoint_UnconfiguredRailRefusesProviderBlind`: through
the real handler, asserts 503, and that the wire body contains none of
`stripe`, `bkash`, `sslcommerz`, `secret_key`, `webhook`, `credential`,
`env`, while still saying `unavailable`. Zero ledger grants, zero
intents.
- `TestMissingRailCredentials`: a complete set reports nothing missing;
a Stripe key with no webhook secret reports exactly
`STRIPE_WEBHOOK_SECRET`; a whitespace-only value counts as missing;
nothing set reports the whole set.

RED first: the four tests failed to build against `main` on the three
undefined symbols. Then GREEN:

```
docker compose --profile tools run toolchain "cd /workspace && go test ./apps/control-plane/... -count=1 -short"
```

passes, whole control-plane module, exit 0. `go vet` clean on the
touched packages. `gofmt` reports no diff on any file this PR adds or
edits; the pre-existing struct-alignment complaints in `service.go` and
`http.go` are untouched and out of scope.

## Visual proof

None, deliberately. No console file is in this diff, and the console's
rendering on the demo box is byte identical before and after: it still
shows the #1438 message, because the box still has zero rails. The
behaviour this PR changes is what the API answers to a caller that posts
anyway, which no screenshot can show.

## Security review response (2026-08-29)

A security review returned WARNING with two HIGH and four MEDIUM
findings, all posted inline. Every one is answered on its own thread;
the summary and the one design question that needed a direct answer are
here.

### The defect's history, which explains its shape

A rail was registered from a single environment variable, in an
environment where all twelve payment variable names are always present.
`deploy/docker/docker-compose.yml` injects each of them at lines 508 to
519 through a `${VAR:-}` default, so on the deployed box the twelve
names exist and the twelve values are empty. "Is it set" was therefore
never a meaningful question on this deployment, and the code was asking
it anyway. That is why the gate has to be an emptiness question rather
than a presence question, and why the tests now pin the
present-and-empty case by name: a port to `os.LookupEnv` would read
three fully configured rails on a box that has none.

### What the review changed

1. HIGH, forged webhook path. `stripe.NewRail` now returns an error and
refuses when either credential is empty, so the boot-time gate is
defence in depth rather than the whole defence. An empty webhook secret
made `ConstructEventWithOptions` HMAC with a key an attacker can also
compute, which turns a forged `payment.succeeded` into a credit grant
for a payment that never happened. `sslcommerz.NewRail` gets the same
guard for the same reason (its IPN hash is computed locally from the
store password, so an empty one verifies against `md5("")`), and
`bkash.NewRail` for the weaker reason (its settlement is a live provider
call, so it fails closed rather than accepting a forgery).
2. HIGH, a test that could not go red. The registration loop moved out
of `main()` into `payments.RegisterRails`, a pure function over a lookup
and a builder list.
`TestRegisterRails_HalfACredentialSetRegistersNothing` asserts a Stripe
secret key with no webhook secret registers nothing, never runs the
constructor, and produces a refusal naming exactly
`STRIPE_WEBHOOK_SECRET`. Confirmed red against the relaxed gate before
it was made green.
3. MEDIUM, present-and-empty was unpinned.
`TestMissingRailCredentials_PresentButEmptyIsUnconfigured` uses the
box's own shape as the fixture, with a lookup that fails the test if a
variable is absent rather than empty, and asserts both the predicate and
the registration decision.
4. MEDIUM, BD rails advertised themselves without FX credentials.
`XE_ACCOUNT_ID` and `XE_API_KEY` joined both BD credential sets, so a BD
rail that cannot price a checkout is no longer registered and no longer
reported as enabled.
5. MEDIUM, stranded `created` intents. Answered directly below.
6. LOW, ordering, trim asymmetry, and the stub documentation. The
configuration check now follows the country check, so a rail the caller
was never entitled to select is answered as customer input rather than
as a deployment fault, and the 503-versus-400 configuration oracle is
gone. All twelve variables are read through `strings.TrimSpace` in
`main()`, so the value checked is the value used. The `.env.example`
stub block now leads with the consequence (setting it grants real ledger
credits with no payment) and states plainly that both variables are
inert in this file, since neither appears anywhere under `deploy/`;
nothing in it implies the stub works on any deployment this repo ships.

### Can the intent insert move after the point of no return

No, and the reason is specific rather than a preference.

The insert is the durable record that any provider-side session this
request opens is attributable to an account, and the unique index
`idx_payment_intents_account_idempotency` on `(account_id,
idempotency_key)` is the only concurrency gate on this endpoint.
Insert-first is what makes a double submit or a retry on the same key
fail locally before a second provider session exists. Moving the insert
after `Initiate` would trade a stranded local row for an orphaned
session at the provider, and would open a window in which a session
exists with no row for the settlement path to resolve against.

So the ordering stays and the missing half is supplied instead: every
post-insert error path now drives the intent to `failed` through
`CompareAndSetStatus(created -> failed)`, covering both the `Initiate`
failure and the `UpdateProviderDetails` failure. A failed attempt
reaches a terminal state under its own power, so no population of aging
`created` rows accumulates and no reaper is needed to own one. If the
transition itself fails, its error is joined onto the original rather
than swallowed.

This is deliberately the opposite lesson from issue #600, where stranded
reservation holds with no reaper produced a three-day billing outage.
The fix there was a sweeper for rows that were already stranded; the fix
here is that the failure path is terminal, so the rows are never created
in that state to begin with.

### Test evidence for this round

Red first, against a build with all four gates deliberately relaxed
(registration gate reduced to a presence check, XE removed from the BD
sets, the configuration check moved back ahead of the country check, the
terminal transition removed):

```
--- FAIL: TestMissingRailCredentials_BDRailsNeedTheirFXCredentials
--- FAIL: TestRegisterRails_HalfACredentialSetRegistersNothing
--- FAIL: TestInitiateCheckout_RailNotAvailableForCountryIsACustomerFault
--- FAIL: TestInitiateCheckout_FailedInitiateLeavesNoCreatedIntent
```

Green after restoring them:

```
docker compose --profile tools run toolchain "cd /workspace && go test ./apps/control-plane/... -count=1 -short"
```

passes across the whole control-plane module, zero FAIL lines, with `go
vet ./apps/control-plane/...` clean and `gofmt` reporting no diff on any
file this PR touches.

## What the box needs to actually sell credits

Exactly one of these three sets, in full, then restart control-plane and
read the boot log. A partial set registers nothing and names what is
unset.

- Stripe, non-BD accounts, test mode is fine: `STRIPE_SECRET_KEY`
**and** `STRIPE_WEBHOOK_SECRET`. Nothing else. The webhook endpoint in
the Stripe dashboard points at
`https://control-hive.scubed.co/webhooks/stripe`, which the tunnel
already exposes on purpose.
- bKash, BD accounts only: the four, `BKASH_APP_KEY`,
`BKASH_APP_SECRET`, `BKASH_USERNAME`, `BKASH_PASSWORD`, **and
additionally** `XE_ACCOUNT_ID` and `XE_API_KEY`, which are now part of
the rail's credential set rather than optional extras.
- SSLCommerz, BD accounts only: `SSLCOMMERZ_STORE_ID` **and**
`SSLCOMMERZ_STORE_PASSWD`, **and additionally** the same two XE
variables.

Either BD rail additionally needs an account whose country resolves to
BD, since `AvailableRails` offers them to nobody else. Setting a value
with a trailing space is now safe: every variable is trimmed at the read
site.

## Buglog entry

```json
{"id":"bug-2026-08-29-unconfigured-rail-refused-last","date":"2026-08-29","title":"A checkout on a rail with no credentials built an intent and an FX snapshot before discovering it could not take the payment","error_message":"POST /api/v1/accounts/current/checkout/initiate answered 400 \"checkout failed\" on a box with no payment rail credentials, after inserting a payment intent that stayed in created forever; boot log said only `payments: 0 rail(s) active: []`","root_cause":"InitiateCheckout looked up s.rails[rail] at step 8, after the country read, the billing profile read, the FX snapshot and InsertPaymentIntent, and returned a plain fmt.Errorf that classifyInitiateError had no case for, so it fell through to the default 400. Separately, main.go registered each rail from a single variable, so a Stripe secret key with no webhook secret registered a rail that could take a payment and never settle it.","fix":"The rail lookup moved to the top of InitiateCheckout and returns the new ErrRailNotConfigured, whose message names the variables to set; classifyInitiateError maps it to 503 \"this payment method is unavailable right now and nothing was charged\", provider blind. payments.RailCredentialEnvs declares each rail's full credential set and main.go registers a rail only when the whole set is present, warning by name for each unset variable. .env.example gained the payment rail section it never had.","tags":["payments","checkout","control-plane","config","issue-1449"]}
```

Second entry, for the review round:

```json
{"id": "bug-2026-08-29-rail-constructor-accepted-empty-webhook-secret", "date": "2026-08-29", "title": "A payment rail could be constructed without the credential that verifies its settlement callbacks, and a failed provider call left the intent in created forever", "error_message": "stripe.NewRail accepted an empty webhook secret and stored it, so ProcessEvent HMACed with an empty key through webhook.ConstructEventWithOptions; separately, a configured rail whose Initiate call failed left a payment_intents row in status created with nothing to age it out", "root_cause": "The whole-credential-set gate lived only in the main() registration loop, which no test observed, so any second construction path reopened the forged-settlement hole and reverting the gate to a single-variable presence check kept the suite green. The BD credential sets also omitted XE_ACCOUNT_ID and XE_API_KEY, so a BD rail registered and advertised itself as enabled and then failed inside CreateSnapshot. And InitiateCheckout had no terminal transition on any post-insert failure.", "fix": "stripe.NewRail, bkash.NewRail and sslcommerz.NewRail return an error and refuse an incomplete credential set, so the registration gate is defence in depth. The registration decision moved into payments.RegisterRails, a pure function with tests that go red when the gate is relaxed. XE_ACCOUNT_ID and XE_API_KEY joined both BD credential sets. The configuration check now follows the country check so customer input is not answered as a deployment fault. Every post-insert failure drives the intent to failed via CompareAndSetStatus, so no created rows accumulate and no reaper is needed. All twelve payment variables are read through strings.TrimSpace.", "tags": ["payments", "checkout", "control-plane", "security", "webhook", "issue-1449"]}
```

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
sakibsadmanshajib added a commit that referenced this pull request Sep 2, 2026
## Summary

Reconciles the backlog created by the branch-append restriction in issue
#873: every fixed bug, error, failed test, or failed build must be
logged in `.wolf/buglog.jsonl`, but never appended directly on a feature
branch, since GitHub's server-side merge ignores the `merge=union`
driver and two branches that both appended land in hard conflict. The
route is to carry the entry in the fix PR's body and append it here
afterward, in a dedicated buglog-only PR.

This PR is that reconciliation, swept properly rather than trusting a
short known list:

- Searched all merged PRs whose body contains a "Buglog entry" heading
(287 PRs matched via GitHub code search).
- Extracted the JSON line following each heading (multiple headings per
PR body handled correctly, e.g. PR #814 and PR #1203 each carry two
matches, one a prose mention and one the real entry).
- Deduplicated against the 314 entries already on `main`, both by `id`
and by exact `error_message` text, plus deduplicated within this batch
itself.
- Result: **197 new entries from 167 source PRs**, spanning PR #787
through PR #1734.
- Validated every extracted line has the four required fields
(`error_message`, `root_cause`, `fix`, `tags`). All 297 raw extractions
had them; zero were rejected as incomplete.
- Five entries carried `tags` as a comma-separated string instead of an
array (inconsistent with the rest of the file's schema). Normalized to
an array by splitting on comma, content unchanged, nothing invented.
- The five false-positive "Buglog entry" mentions that were prose
references rather than real headings (PRs #1116, #1303 first match,
#1438 first match, #814 first match, #1203 first match) were correctly
skipped, either because no JSON followed or because the real entry was
found at a later heading in the same body.

## Diff scope

`.wolf/buglog.jsonl` only, 197 insertions, 0 deletions. No existing line
touched (verified byte-identical against the first 314 lines
pre-append).

## Test plan

- [x] Every one of the 511 resulting lines parses as valid single-line
JSON.
- [x] `git show --stat` on the pushed commit shows exactly one file
changed.
- [x] First 314 lines diffed identical to `origin/main`'s current file.
- [x] This is on the inert-path allowlist in `.github/workflows/ci.yml`,
so the six required checks should report green without running their
heavy steps.

Refs #873
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant