Skip to content

sec(money): currency is dropped at every layer boundary, so USD-denominated caps are compared against non-USD amounts [platform part] #361

Closed

Description

@cristim

Split from LeanerCloud/cloud-commitments-cli#1637 (cross-component issue, monorepo split)

This is the internal/-scoped half of a cross-cutting class finding. Nothing in the original issue touches cmd/*.go or docs/cli/, so cloud-commitments-cli is not an affected repo and the original issue is closed with backlinks rather than kept open. The pkg/ and providers/ half of the same class went to cloud-commitments-go (see the shared root-cause section below, repeated in both issues since the fix direction only makes sense read as a whole).

Reviewed commit: be11bdcb5. Re-verified against this repo's current tree (2026-09-28); both new instances below still reproduce, line numbers re-checked.

What this issue is (and is not)

This is the class, not another copy of the instances. Four instances already have their own issues in cloud-commitments-cli (not yet split as of this writing, since they predate this monorepo split and are independently fixable regardless of which repo they end up scoped to):

  • #1558 fix(exchange): RI-exchange spend caps are compared against a quote whose CurrencyCode is never checked (go repo: pkg/exchange; ledger half in this repo: internal/config/store_postgres.go)
  • #1555 fix(api): Azure revoke refund-divergence guard is opt-in and currency-blind (this repo: internal/api/handler_purchases_revoke.go)
  • #1562 fix(azure): Savings Plan purchase hardcodes CurrencyCode USD (go repo: providers/azure/services/savingsplans/client.go)
  • fix(config): lock a deleted account's plans before disabling them #1087 AWS Savings Plans purchase path is USD-only (go repo: providers/aws/services/savingsplans/client.go)

Those stay open and stay independently fixable; do not double-fix them here or as part of this split. This issue (and its cloud-commitments-go sibling) exist because fixing those one at a time will not close the class: the same shape reappears in more places, and a per-site fix leaves the underlying type design intact so the next money field added reintroduces it.

The shared root cause

Two facts, together:

  1. Money crosses every layer boundary as a bare float64 (or a bare decimal string), with the currency dropped at the boundary. The currency is frequently read from the provider response and then either stored in a display-only field or discarded outright. common.Recommendation (cloud-commitments-go repo) carries no currency field at all, so there is nowhere for it to go even if a caller wanted to keep it.

  2. Every spend cap in the system is USD-denominated by naming convention only, with no currency field to compare against. internal/auth/types.go:65-66 (this repo):

    // MaxPurchaseAmount limits the maximum purchase amount
    MaxPurchaseAmount float64 `json:"max_purchase_amount,omitempty" dynamodbav:"MaxPurchaseAmount"`

    Same for pkg/exchange's MaxPaymentDueUSD *big.Rat and MaxPaymentPerExchangeUSD / MaxPaymentDailyUSD in the cloud-commitments-go repo. The USD is in the identifier, not in the type.

The result is that a guardrail comparison is between two numbers whose units are asserted by a variable name. On a non-USD deployment (aws-cn, an MCA subscription billed in EUR, a GBP-billed GCP billing account) that comparison is wrong in whichever direction the exchange rate happens to point: it silently blocks a legal purchase, or it silently authorises one an order of magnitude over the operator's stated ceiling.

The correct behaviour is to fail closed on a non-USD money path, not to compare across currencies. A guardrail that cannot establish the denomination of the number it is checking has not checked anything, and "assume USD" is exactly the fabricated-value pattern the project's no-silent-fallbacks rule exists to prevent. Where the currency genuinely cannot be resolved, the money path must error out and say so, rather than proceed on an assumption.


Instances in this repo (new in this sweep, not filed elsewhere)

  • max_payment_due_usd is never asserted to actually be USD

    • Where: internal/api/handler_ri_exchange.go:1749 (required-field check), :1781 (exchange.ParseDecimalRat(body.MaxPaymentDueUSD)), :1798 (maxPayment, _ := maxRat.Float64()), :1833 (MaxPaymentDueUSD: maxRat); request field declared at :1955. Cap type at internal/auth/types.go:65-66.
    • What: the request body field is named max_payment_due_usd and the handler's own error text calls it a "safety guardrail", but no code path establishes that the exchange being guarded is USD-denominated. The value becomes MaxPurchaseAmount, which has no currency field, and is also forwarded to pkg/exchange (cloud-commitments-go repo) as MaxPaymentDueUSD. The name is the only thing carrying the unit, across three layers.
    • Failure scenario: an aws-cn deployment. The operator sets max_payment_due_usd: "1000" meaning a thousand dollars. AWS quotes PaymentDue = "5000" in CNY (roughly USD 690). The comparison in pkg/exchange compares 5000 against 1000, refuses, and the operator sees a cap breach that is not one. The reverse case, a currency weaker per unit than the cap assumes, authorises an exchange over the real ceiling with no signal.
    • Secondary defect at the same site: maxPayment, _ := maxRat.Float64() at :1798 discards the exactness flag. Bounded (only inexact at absurd magnitudes) but it is the same "drop the metadata at the boundary" move. Keep the cap comparison in *big.Rat.
    • Fix direction: carry the currency of the amount being compared through to pkg/exchange's guardrail check (coordinate with the cloud-commitments-go sibling issue), and fail closed there when it is not USD or unknown.
  • firstNonEmptyCurrency labels reshape output "USD" by fiat when no RI reports a currency

    • Where: internal/api/handler_ri_exchange.go:1623-1636 (definition), consumed at :1594 (currencyCode := firstNonEmptyCurrency(instances)).
    • What: the helper walks the ConvertibleRI list and returns the first non-empty CurrencyCode; when none has one it returns the literal "USD". The unknown case is rendered as a definite answer.
    • Failure scenario: an aws-cn deployment whose RIs come back without CurrencyCode sees CNY-denominated savings figures rendered and compared as USD on the Reshape page. Bounded and display-side today because the field is populated in practice, which is exactly why it will survive until the day it is not.
    • Fix direction: return ("", false) and omit the currency from the response rather than defaulting.

Fix direction for the class

Fixing the individual sites without this will not hold. Suggested order:

  1. Carry the currency to the guardrail. The cloud-commitments-go sibling issue covers the read-side functions that must stop dropping it (amountValue, extractCostImpactFromRecommendation, the Advisor path), and common.Recommendation needs somewhere to put it. This is the load-bearing step: a comparison cannot fail closed on something it never received.
  2. Make every guardrail fail closed when the amount is not USD, or its currency is unknown. The pkg/exchange caps, the Azure revoke divergence guard, MaxPurchaseAmount evaluation. PR #1515 already settled the rule for one call site; this applies the same rule at the rest.
  3. Delete the "USD" defaults (firstNonEmptyCurrency above, the two Azure SP literals tracked on #1562) and replace them with an explicit unknown that step 2 then refuses.

Deliberately not in scope, unless and until CUDly actually transacts in a second currency:

  • A Money{Amount, Currency} type in pkg/common, or a currency field on MaxPurchaseAmount and the pkg/exchange cap fields. The caps are USD by construction (#1515), so giving them a field to hold "USD" adds a value that is never anything else, threaded through three layers. What the comparison is missing is the denomination of the incoming amount, which is step 1.
  • A currency column on ri_exchange_history.payment_due. With step 2 in place no non-USD amount can be written, so the daily-spend ledger query cannot sum across denominations. The column is needed on the day the product supports a second currency, and should be added with that work.

Regression tests must exercise a genuinely non-USD path (a quote with CurrencyCode: "JPY", an EUR-billed MCA subscription fixture), not a USD path with an extra assertion. A test that only ever feeds USD will pass with every one of these defects present.

Related

Acceptance criteria

  • Both new instances above are fixed to fail closed on a non-USD or unresolvable currency, with a regression test that exercises a genuinely non-USD input.
  • The fix is coordinated with the cloud-commitments-go sibling issue so the currency actually reaches this repo's guardrails (step 1 of the fix direction lives partly there).

Activity

  1. IAmJSD commented on Oct 23, 2026

    @IAmJSD
    Collaborator

    Claiming: working on this

  2. self-assigned this
    on Oct 23, 2026
  3. IAmJSD commented on Oct 26, 2026

    @IAmJSD
    Collaborator

    Decision (IAmJSD, acting owner, 2026-10-26): purchases under a spend cap fail closed on currency.

    Posted by IAmJSD's agent at IAmJSD's request (acting owner while the maintainers are away).

  4. IAmJSD commented on Oct 26, 2026

    @IAmJSD
    Collaborator

    Closing as fixed on main. Both acceptance-criteria instances are fixed on main. firstNonEmptyCurrency returns "" instead of USD (#514). max_payment_due_usd is now checked against the quote currency by the pinned go pkg's requireUSDPaymentDue/ErrQuoteNotUSD, mapped to 422 by #1212 with an EUR regression test. #1296 also made purchase spend caps fail closed on non-USD/unknown currency per the acting-owner decision comment. Caveat: the bounded 'maxRat.Float64() drops exactness' nit is still at handler_ri_exchange.go:1169/:2664. The plan cap max_step_commitment is tracked separately in open issue #1427, as that comment says.

    The fix PRs said Refs, so GitHub did not close this automatically. Checked against main on 2026-10-26 by IAmJSD's agent (IAmJSD is acting owner while the maintainers are away). Reopen if anything here is still wrong.

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

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions