From 9f0bf7b7a83f2e78f5f5d64fae5121b18829b0e0 Mon Sep 17 00:00:00 2001 From: Maxence Maireaux Date: Thu, 11 Jun 2026 09:50:20 +0200 Subject: [PATCH 1/8] fix(security): validate account names to prevent Numscript injection The credit/debit Numscript templates interpolate account names directly into the script body (@{{ .source }}). WALLET subjects only checked that the identifier was non-empty, and the balance-name regex was unanchored (MatchString matches any substring), so a crafted identifier/balance/name containing newlines or Numscript tokens could break out of the source block and inject arbitrary statements executed with the service ledger credentials. - Anchor balanceNameRegex (^...$) so a name must be a single clean segment - Validate WALLET subject identifier and balance via accounts.ValidateAddress - Validate walletID before building debit/credit scripts Adds regression tests for separator/whitespace/token names and for injection via a WALLET source identifier and balance. --- pkg/api/handler_balances_create_test.go | 18 ++++++++++++++++++ pkg/api/handler_wallets_credit_test.go | 22 ++++++++++++++++++++++ pkg/balance.go | 2 +- pkg/debit.go | 4 ++++ pkg/manager.go | 5 +++++ pkg/subject.go | 6 ++++++ 6 files changed, 56 insertions(+), 1 deletion(-) diff --git a/pkg/api/handler_balances_create_test.go b/pkg/api/handler_balances_create_test.go index 99a5588..88ce8e8 100644 --- a/pkg/api/handler_balances_create_test.go +++ b/pkg/api/handler_balances_create_test.go @@ -39,6 +39,24 @@ var balanceCreateTestCases = []balanceCreateTestCase{ expectedStatusCode: http.StatusBadRequest, expectedErrorCode: ErrorCodeValidation, }, + { + // The name contains valid characters but also an account separator; + // an unanchored regex would have accepted it, allowing address/script injection. + name: "with name containing an account separator", + request: wallet.CreateBalance{ + Name: "balance:injected", + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: ErrorCodeValidation, + }, + { + name: "with name containing whitespace and numscript tokens", + request: wallet.CreateBalance{ + Name: "x\n@world", + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: ErrorCodeValidation, + }, { name: "with reserved name", request: wallet.CreateBalance{ diff --git a/pkg/api/handler_wallets_credit_test.go b/pkg/api/handler_wallets_credit_test.go index 06b8fdd..76356e1 100644 --- a/pkg/api/handler_wallets_credit_test.go +++ b/pkg/api/handler_wallets_credit_test.go @@ -102,6 +102,28 @@ func TestWalletsCredit(t *testing.T) { } }, }, + { + name: "with wallet source containing numscript injection in balance", + request: wallet.CreditRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Sources: []wallet.Subject{ + wallet.NewWalletSubject("emitter1", "secondary\n@world"), + }, + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: ErrorCodeValidation, + }, + { + name: "with wallet source containing numscript injection in identifier", + request: wallet.CreditRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Sources: []wallet.Subject{ + wallet.NewWalletSubject("emitter1 @world", ""), + }, + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: ErrorCodeValidation, + }, { name: "with secondary balance as destination", request: wallet.CreditRequest{ diff --git a/pkg/balance.go b/pkg/balance.go index 715ff31..6478256 100644 --- a/pkg/balance.go +++ b/pkg/balance.go @@ -12,7 +12,7 @@ import ( "github.com/formancehq/go-libs/v5/pkg/types/time" ) -var balanceNameRegex = regexp.MustCompile("[0-9A-Za-z_-]+") +var balanceNameRegex = regexp.MustCompile("^[0-9A-Za-z_-]+$") type CreateBalance struct { WalletID string `json:"walletID"` diff --git a/pkg/debit.go b/pkg/debit.go index 2406042..a90c51f 100644 --- a/pkg/debit.go +++ b/pkg/debit.go @@ -5,6 +5,7 @@ import ( "net/http" "github.com/formancehq/go-libs/v5/pkg/types/time" + "github.com/formancehq/ledger/pkg/accounts" "github.com/formancehq/ledger/pkg/assets" "github.com/formancehq/go-libs/v5/pkg/types/metadata" @@ -55,6 +56,9 @@ func (d Debit) getDestination() Subject { } func (d Debit) Validate() error { + if !accounts.ValidateAddress(d.WalletID) { + return newErrInvalidAccountName(d.WalletID) + } if d.Destination != nil { if err := d.Destination.Validate(); err != nil { return err diff --git a/pkg/manager.go b/pkg/manager.go index c9566d6..d52e187 100644 --- a/pkg/manager.go +++ b/pkg/manager.go @@ -14,6 +14,7 @@ import ( "github.com/formancehq/formance-sdk-go/v3/pkg/models/shared" "github.com/formancehq/go-libs/v5/pkg/types/metadata" + "github.com/formancehq/ledger/pkg/accounts" "github.com/pkg/errors" ) @@ -284,6 +285,10 @@ func (m *Manager) Credit(ctx context.Context, ik string, credit Credit) error { return err } + if !accounts.ValidateAddress(credit.WalletID) { + return newErrInvalidAccountName(credit.WalletID) + } + if credit.Balance != "" { if _, err := m.GetBalance(ctx, credit.WalletID, credit.Balance); err != nil { return err diff --git a/pkg/subject.go b/pkg/subject.go index 0ff946a..f6ba76b 100644 --- a/pkg/subject.go +++ b/pkg/subject.go @@ -43,6 +43,12 @@ func (s Subject) Validate() error { if s.Identifier == "" { return fmt.Errorf("wallet identifier cannot be empty") } + if !accounts.ValidateAddress(s.Identifier) { + return newErrInvalidAccountName(s.Identifier) + } + if s.Balance != "" && !accounts.ValidateAddress(s.Balance) { + return newErrInvalidAccountName(s.Balance) + } } return nil } From 653843f3433c65029318aa5d5bad2eb520459ffe Mon Sep 17 00:00:00 2001 From: Maxence Maireaux Date: Thu, 11 Jun 2026 12:40:33 +0200 Subject: [PATCH 2/8] fix(security): use a single-segment validator for wallet IDs and balances Address review feedback: accounts.ValidateAddress accepts a full multi-segment address, so a WALLET subject with identifier/balance like "foo:bar" passed and resolved to a nested account outside the balance model. Validate wallet IDs and balance names against an anchored single-segment regex instead, keeping ValidateAddress only for ACCOUNT subjects. Documents the residual dash-stripping collision in Address.String() (kept as a separate, migration-bearing change rather than forbidding dashes, which would break legitimate dashed/UUID balance names). --- pkg/api/handler_wallets_credit_test.go | 11 +++++++++++ pkg/balance.go | 11 +++++++++++ pkg/debit.go | 3 +-- pkg/manager.go | 3 +-- pkg/subject.go | 14 ++++++++++++-- 5 files changed, 36 insertions(+), 6 deletions(-) diff --git a/pkg/api/handler_wallets_credit_test.go b/pkg/api/handler_wallets_credit_test.go index 76356e1..332306e 100644 --- a/pkg/api/handler_wallets_credit_test.go +++ b/pkg/api/handler_wallets_credit_test.go @@ -113,6 +113,17 @@ func TestWalletsCredit(t *testing.T) { expectedStatusCode: http.StatusBadRequest, expectedErrorCode: ErrorCodeValidation, }, + { + name: "with wallet source spanning multiple account segments", + request: wallet.CreditRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Sources: []wallet.Subject{ + wallet.NewWalletSubject("emitter1", "balance:injected"), + }, + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: ErrorCodeValidation, + }, { name: "with wallet source containing numscript injection in identifier", request: wallet.CreditRequest{ diff --git a/pkg/balance.go b/pkg/balance.go index 6478256..5191e00 100644 --- a/pkg/balance.go +++ b/pkg/balance.go @@ -12,6 +12,17 @@ import ( "github.com/formancehq/go-libs/v5/pkg/types/time" ) +// balanceNameRegex constrains a user-supplied balance name to a single, +// anchored ledger segment (no ':' separator), which closes the Numscript +// injection vector while still allowing names clients rely on, including +// dashed names and UUIDs. +// +// NOTE: Address.String() currently strips '-', so "foo-bar" and "foobar" +// resolve to the same ledger account. Forbidding '-' here would break +// legitimate dashed/UUID names, and removing the global strip would change +// the address of every already-created wallet/balance; resolving that +// collision cleanly needs a coordinated change to Address.String() plus a +// data migration, tracked separately. var balanceNameRegex = regexp.MustCompile("^[0-9A-Za-z_-]+$") type CreateBalance struct { diff --git a/pkg/debit.go b/pkg/debit.go index a90c51f..d523181 100644 --- a/pkg/debit.go +++ b/pkg/debit.go @@ -5,7 +5,6 @@ import ( "net/http" "github.com/formancehq/go-libs/v5/pkg/types/time" - "github.com/formancehq/ledger/pkg/accounts" "github.com/formancehq/ledger/pkg/assets" "github.com/formancehq/go-libs/v5/pkg/types/metadata" @@ -56,7 +55,7 @@ func (d Debit) getDestination() Subject { } func (d Debit) Validate() error { - if !accounts.ValidateAddress(d.WalletID) { + if !accountSegmentRegexp.MatchString(d.WalletID) { return newErrInvalidAccountName(d.WalletID) } if d.Destination != nil { diff --git a/pkg/manager.go b/pkg/manager.go index d52e187..efe3d29 100644 --- a/pkg/manager.go +++ b/pkg/manager.go @@ -14,7 +14,6 @@ import ( "github.com/formancehq/formance-sdk-go/v3/pkg/models/shared" "github.com/formancehq/go-libs/v5/pkg/types/metadata" - "github.com/formancehq/ledger/pkg/accounts" "github.com/pkg/errors" ) @@ -285,7 +284,7 @@ func (m *Manager) Credit(ctx context.Context, ik string, credit Credit) error { return err } - if !accounts.ValidateAddress(credit.WalletID) { + if !accountSegmentRegexp.MatchString(credit.WalletID) { return newErrInvalidAccountName(credit.WalletID) } diff --git a/pkg/subject.go b/pkg/subject.go index f6ba76b..01a441e 100644 --- a/pkg/subject.go +++ b/pkg/subject.go @@ -2,6 +2,7 @@ package wallet import ( "fmt" + "regexp" "github.com/formancehq/ledger/pkg/accounts" ) @@ -11,6 +12,13 @@ const ( SubjectTypeWallet string = "WALLET" ) +// accountSegmentRegexp matches a single ledger account segment (no ':' +// separator), anchored. WALLET identifiers and balance names are used as +// individual chart segments, so — unlike a full ledger address — they must +// not contain ':' (which would resolve to a nested account outside the +// expected balance model and is the Numscript-injection vector). +var accountSegmentRegexp = regexp.MustCompile("^" + accounts.SegmentRegex + "$") + type Subject struct { Type string `json:"type"` Identifier string `json:"identifier"` @@ -43,10 +51,12 @@ func (s Subject) Validate() error { if s.Identifier == "" { return fmt.Errorf("wallet identifier cannot be empty") } - if !accounts.ValidateAddress(s.Identifier) { + if !accountSegmentRegexp.MatchString(s.Identifier) { return newErrInvalidAccountName(s.Identifier) } - if s.Balance != "" && !accounts.ValidateAddress(s.Balance) { + // The balance is a single chart segment too, so apply the same + // single-segment rule (rejecting ':' and any Numscript metacharacter). + if s.Balance != "" && !accountSegmentRegexp.MatchString(s.Balance) { return newErrInvalidAccountName(s.Balance) } } From 2d477ae8609d71a021ce92bd7a77ac2426df24e4 Mon Sep 17 00:00:00 2001 From: Maxence Maireaux Date: Fri, 12 Jun 2026 10:08:21 +0200 Subject: [PATCH 3/8] fix(security): close balance validation gaps --- pkg/api/handler_balances_create_test.go | 10 ++++- pkg/api/handler_wallets_credit_test.go | 22 ++++++++++- pkg/api/handler_wallets_debit_test.go | 49 +++++++++++++++++++++++++ pkg/balance.go | 17 +++------ pkg/credit.go | 3 ++ pkg/debit.go | 5 +++ pkg/manager.go | 3 ++ pkg/subject.go | 6 +-- 8 files changed, 98 insertions(+), 17 deletions(-) diff --git a/pkg/api/handler_balances_create_test.go b/pkg/api/handler_balances_create_test.go index 88ce8e8..37b9388 100644 --- a/pkg/api/handler_balances_create_test.go +++ b/pkg/api/handler_balances_create_test.go @@ -28,7 +28,7 @@ var balanceCreateTestCases = []balanceCreateTestCase{ { name: "nominal", request: wallet.CreateBalance{ - Name: uuid.NewString(), + Name: "balance1", }, }, { @@ -57,6 +57,14 @@ var balanceCreateTestCases = []balanceCreateTestCase{ expectedStatusCode: http.StatusBadRequest, expectedErrorCode: ErrorCodeValidation, }, + { + name: "with name containing a dash", + request: wallet.CreateBalance{ + Name: "foo-bar", + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: ErrorCodeValidation, + }, { name: "with reserved name", request: wallet.CreateBalance{ diff --git a/pkg/api/handler_wallets_credit_test.go b/pkg/api/handler_wallets_credit_test.go index 332306e..b82aff0 100644 --- a/pkg/api/handler_wallets_credit_test.go +++ b/pkg/api/handler_wallets_credit_test.go @@ -124,6 +124,17 @@ func TestWalletsCredit(t *testing.T) { expectedStatusCode: http.StatusBadRequest, expectedErrorCode: ErrorCodeValidation, }, + { + name: "with wallet source containing dash in balance", + request: wallet.CreditRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Sources: []wallet.Subject{ + wallet.NewWalletSubject("emitter1", "foo-bar"), + }, + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: ErrorCodeValidation, + }, { name: "with wallet source containing numscript injection in identifier", request: wallet.CreditRequest{ @@ -135,6 +146,15 @@ func TestWalletsCredit(t *testing.T) { expectedStatusCode: http.StatusBadRequest, expectedErrorCode: ErrorCodeValidation, }, + { + name: "with dash in destination balance", + request: wallet.CreditRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Balance: "foo-bar", + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: ErrorCodeValidation, + }, { name: "with secondary balance as destination", request: wallet.CreditRequest{ @@ -158,7 +178,7 @@ func TestWalletsCredit(t *testing.T) { name: "with not existing secondary balance as destination", request: wallet.CreditRequest{ Amount: wallet.NewMonetary(big.NewInt(100), "USD"), - Balance: "not-existing", + Balance: "not_existing", }, expectedStatusCode: http.StatusBadRequest, expectedErrorCode: ErrorCodeValidation, diff --git a/pkg/api/handler_wallets_debit_test.go b/pkg/api/handler_wallets_debit_test.go index 4728941..ac99ebd 100644 --- a/pkg/api/handler_wallets_debit_test.go +++ b/pkg/api/handler_wallets_debit_test.go @@ -182,6 +182,15 @@ var walletDebitTestCases = []testCase{ } }, }, + { + name: "with dash in balance source", + request: wallet.DebitRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Balances: []string{"foo-bar"}, + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: string(sdkerrors.SchemasErrorCodeValidation), + }, { name: "with wildcard balance as source", request: wallet.DebitRequest{ @@ -416,3 +425,43 @@ func TestWalletsDebit(t *testing.T) { }) } } + +func TestWalletsDebitRejectsInvalidBalanceMetadata(t *testing.T) { + t.Parallel() + + walletID := uuid.NewString() + req := newRequest(t, http.MethodPost, "/wallets/"+walletID+"/debit", wallet.DebitRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Balances: []string{"legacy"}, + }) + rec := httptest.NewRecorder() + + var ( + createdTransaction bool + testEnv *testEnv + ) + testEnv = newTestEnv( + WithGetAccount(func(ctx context.Context, ledger, account string) (*wallet.AccountWithVolumesAndBalances, error) { + require.Equal(t, testEnv.Chart().GetBalanceAccount(walletID, "legacy"), account) + return &wallet.AccountWithVolumesAndBalances{ + Account: wallet.Account{ + Address: account, + Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ + Name: "foo-bar", + }.LedgerMetadata(walletID)), + }, + }, nil + }), + WithCreateTransaction(func(ctx context.Context, ledger, ik string, p wallet.PostTransaction) (*shared.V2Transaction, error) { + createdTransaction = true + return nil, nil + }), + ) + + testEnv.Router().ServeHTTP(rec, req) + + require.Equal(t, http.StatusBadRequest, rec.Result().StatusCode) + errorResponse := readErrorResponse(t, rec) + require.Equal(t, ErrorCodeValidation, errorResponse.ErrorCode) + require.False(t, createdTransaction) +} diff --git a/pkg/balance.go b/pkg/balance.go index 5191e00..4dff3a5 100644 --- a/pkg/balance.go +++ b/pkg/balance.go @@ -12,18 +12,11 @@ import ( "github.com/formancehq/go-libs/v5/pkg/types/time" ) -// balanceNameRegex constrains a user-supplied balance name to a single, -// anchored ledger segment (no ':' separator), which closes the Numscript -// injection vector while still allowing names clients rely on, including -// dashed names and UUIDs. -// -// NOTE: Address.String() currently strips '-', so "foo-bar" and "foobar" -// resolve to the same ledger account. Forbidding '-' here would break -// legitimate dashed/UUID names, and removing the global strip would change -// the address of every already-created wallet/balance; resolving that -// collision cleanly needs a coordinated change to Address.String() plus a -// data migration, tracked separately. -var balanceNameRegex = regexp.MustCompile("^[0-9A-Za-z_-]+$") +// balanceNameRegex constrains user-supplied balance names to a single, +// anchored ledger segment (no ':' separator) and excludes '-' because +// Address.String() strips dashes globally, making "foo-bar" and "foobar" +// resolve to the same ledger account. +var balanceNameRegex = regexp.MustCompile("^[0-9A-Za-z_]+$") type CreateBalance struct { WalletID string `json:"walletID"` diff --git a/pkg/credit.go b/pkg/credit.go index 45fb197..ff2b118 100644 --- a/pkg/credit.go +++ b/pkg/credit.go @@ -36,6 +36,9 @@ func (c CreditRequest) Validate() error { if !assets.IsValid(c.Amount.Asset) { return newErrInvalidAsset(c.Amount.Asset) } + if c.Balance != "" && !balanceNameRegex.MatchString(c.Balance) { + return newErrInvalidAccountName(c.Balance) + } return nil } diff --git a/pkg/debit.go b/pkg/debit.go index d523181..dc65c34 100644 --- a/pkg/debit.go +++ b/pkg/debit.go @@ -69,5 +69,10 @@ func (d Debit) Validate() error { if !assets.IsValid(d.Amount.Asset) { return newErrInvalidAsset(d.Amount.Asset) } + for _, balance := range d.Balances { + if balance != "*" && !balanceNameRegex.MatchString(balance) { + return newErrInvalidAccountName(balance) + } + } return nil } diff --git a/pkg/manager.go b/pkg/manager.go index efe3d29..c4643d7 100644 --- a/pkg/manager.go +++ b/pkg/manager.go @@ -169,6 +169,9 @@ func (m *Manager) Debit(ctx context.Context, ik string, debit Debit) (*DebitHold if balance.ExpiresAt != nil && !balance.ExpiresAt.IsZero() && balance.ExpiresAt.Before(time.Now()) { continue } + if !balanceNameRegex.MatchString(balance.Name) { + return nil, newErrInvalidAccountName(balance.Name) + } sources = append(sources, m.chart.GetBalanceAccount(debit.WalletID, balance.Name)) } diff --git a/pkg/subject.go b/pkg/subject.go index 01a441e..ae58527 100644 --- a/pkg/subject.go +++ b/pkg/subject.go @@ -54,9 +54,9 @@ func (s Subject) Validate() error { if !accountSegmentRegexp.MatchString(s.Identifier) { return newErrInvalidAccountName(s.Identifier) } - // The balance is a single chart segment too, so apply the same - // single-segment rule (rejecting ':' and any Numscript metacharacter). - if s.Balance != "" && !accountSegmentRegexp.MatchString(s.Balance) { + // Balance names are stricter than wallet identifiers because + // Address.String() strips dashes globally, creating aliases. + if s.Balance != "" && !balanceNameRegex.MatchString(s.Balance) { return newErrInvalidAccountName(s.Balance) } } From 65d25e34c11eaa4ff39a16abc3416fd79b6b2f43 Mon Sep 17 00:00:00 2001 From: fabrice guery Date: Tue, 23 Jun 2026 18:20:12 +0200 Subject: [PATCH 4/8] docs(chart): document dash-aliasing hazard in Address.String() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address.String() strips all '-' after joining segments, so inputs differing only by dashes collapse to the same ledger account ("foo-bar" == "foobar"). This affects wallet IDs, balance names and hold IDs alike and can silently make two distinct entities share one account on create/get/debit/credit. Constraint: the strip cannot be removed without changing the address of every already-created wallet/balance/hold (needs a data migration) Directive: do not remove the strip or relax segment validation without a migration plan — the proper fix (stop stripping + migrate) is a separate ticket Confidence: high Scope-risk: narrow --- pkg/chart.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/pkg/chart.go b/pkg/chart.go index d4ec6b0..57bc71e 100644 --- a/pkg/chart.go +++ b/pkg/chart.go @@ -8,6 +8,24 @@ const MainBalance = "main" type Address []string +// String renders the address as a ledger account path by joining the +// segments with ':'. +// +// WARNING — known aliasing hazard: every '-' is stripped from the joined +// result. This means any two segments that differ only by dashes collapse +// to the same ledger account: "foo-bar" and "foobar" both render to +// "foobar", as do wallet IDs / balance names / hold IDs differing only in +// dash placement. Because wallet and hold IDs are UUIDs (which contain +// dashes), distinct inputs can therefore resolve to the SAME underlying +// account, causing silent collisions on create/get/debit/credit — two +// wallets, balances, or holds sharing one ledger account. +// +// The strip cannot simply be removed: all existing accounts were created +// with dashes already stripped, so dropping it would change the address of +// every wallet/balance/hold already in the ledger and requires a data +// migration. Fixing this properly (stop stripping + migrate) should be a +// dedicated ticket. Until then, callers validate user-supplied segments to +// limit — but do not fully eliminate — the collision surface. func (addr Address) String() string { s := strings.Join(addr, ":") s = strings.ReplaceAll(s, "-", "") From 2600de7fd3f7d90165de31e5eea8cd24b2ad1201 Mon Sep 17 00:00:00 2001 From: fabrice guery Date: Tue, 23 Jun 2026 18:20:20 +0200 Subject: [PATCH 5/8] fix(security): centralize Credit WalletID validation in Validate() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The WalletID was validated ad-hoc inside Manager.Credit, so any caller invoking Credit.Validate() directly (tests, future handlers) skipped the check. Move it into Credit.Validate() — mirroring Debit.Validate() — so the single-segment guard cannot be bypassed, and drop the duplicate check from the manager. Constraint: WalletID is interpolated as a chart segment, so it must be a single anchored segment with no ':' or Numscript metacharacters Rejected: keep the check only in the manager | bypassable by non-manager callers Confidence: high Scope-risk: narrow --- pkg/credit.go | 14 ++++++++++++++ pkg/manager.go | 4 ---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/pkg/credit.go b/pkg/credit.go index ff2b118..fa3bf93 100644 --- a/pkg/credit.go +++ b/pkg/credit.go @@ -48,6 +48,20 @@ type Credit struct { WalletID string `json:"walletID"` } +// Validate centralizes all credit validation, including the WalletID, so it +// cannot be bypassed by callers that only invoke Validate() (mirrors +// Debit.Validate). The WalletID is used as a chart segment, so it must be a +// single anchored segment with no ':' separator or Numscript metacharacters. +func (c Credit) Validate() error { + if err := c.CreditRequest.Validate(); err != nil { + return err + } + if !accountSegmentRegexp.MatchString(c.WalletID) { + return newErrInvalidAccountName(c.WalletID) + } + return nil +} + func (c Credit) destinationAccount(chart *Chart) string { if c.Balance == "" { return chart.GetMainBalanceAccount(c.WalletID) diff --git a/pkg/manager.go b/pkg/manager.go index c4643d7..5f9c342 100644 --- a/pkg/manager.go +++ b/pkg/manager.go @@ -287,10 +287,6 @@ func (m *Manager) Credit(ctx context.Context, ik string, credit Credit) error { return err } - if !accountSegmentRegexp.MatchString(credit.WalletID) { - return newErrInvalidAccountName(credit.WalletID) - } - if credit.Balance != "" { if _, err := m.GetBalance(ctx, credit.WalletID, credit.Balance); err != nil { return err From 60fae3285a7d60d44e9948f1f085971dc3db603f Mon Sep 17 00:00:00 2001 From: fabrice guery Date: Tue, 23 Jun 2026 18:20:32 +0200 Subject: [PATCH 6/8] test(api): cover malicious wallet IDs from URL and harden debit mock Add credit and debit handler tests that send a WalletID via the URL path spanning multiple account segments ("wallet:injected") or carrying Numscript tokens ("wallet\n@world"), asserting a 400 and that no ledger transaction is created. Also return a non-nil transaction from the WithCreateTransaction mock in TestWalletsDebitRejectsInvalidBalanceMetadata so a regression surfaces as an assertion failure rather than a nil-deref panic. Confidence: high Scope-risk: narrow --- pkg/api/handler_wallets_credit_test.go | 40 ++++++++++++++++++++++++ pkg/api/handler_wallets_debit_test.go | 42 +++++++++++++++++++++++++- 2 files changed, 81 insertions(+), 1 deletion(-) diff --git a/pkg/api/handler_wallets_credit_test.go b/pkg/api/handler_wallets_credit_test.go index b82aff0..e64ade4 100644 --- a/pkg/api/handler_wallets_credit_test.go +++ b/pkg/api/handler_wallets_credit_test.go @@ -5,6 +5,7 @@ import ( "math/big" "net/http" "net/http/httptest" + "net/url" "testing" "github.com/formancehq/go-libs/v5/pkg/types/time" @@ -261,3 +262,42 @@ func TestWalletsCredit(t *testing.T) { }) } } + +// TestWalletsCreditRejectsInvalidWalletID guards the WalletID supplied via the +// URL path: a value spanning multiple account segments or carrying Numscript +// tokens must be rejected before any ledger transaction is created. +func TestWalletsCreditRejectsInvalidWalletID(t *testing.T) { + t.Parallel() + + for _, walletID := range []string{ + "wallet:injected", + "wallet\n@world", + } { + walletID := walletID + t.Run(walletID, func(t *testing.T) { + t.Parallel() + + req := newRequest(t, http.MethodPost, "/wallets/"+url.PathEscape(walletID)+"/credit", wallet.CreditRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + }) + rec := httptest.NewRecorder() + + var ( + testEnv *testEnv + createdTransaction bool + ) + testEnv = newTestEnv( + WithCreateTransaction(func(ctx context.Context, ledger, ik string, p wallet.PostTransaction) (*shared.V2Transaction, error) { + createdTransaction = true + return &shared.V2Transaction{}, nil + }), + ) + testEnv.Router().ServeHTTP(rec, req) + + require.Equal(t, http.StatusBadRequest, rec.Result().StatusCode) + errorResponse := readErrorResponse(t, rec) + require.Equal(t, ErrorCodeValidation, errorResponse.ErrorCode) + require.False(t, createdTransaction) + }) + } +} diff --git a/pkg/api/handler_wallets_debit_test.go b/pkg/api/handler_wallets_debit_test.go index ac99ebd..a5f8f25 100644 --- a/pkg/api/handler_wallets_debit_test.go +++ b/pkg/api/handler_wallets_debit_test.go @@ -7,6 +7,7 @@ import ( "math/big" "net/http" "net/http/httptest" + "net/url" "testing" "github.com/formancehq/go-libs/v5/pkg/types/time" @@ -454,7 +455,7 @@ func TestWalletsDebitRejectsInvalidBalanceMetadata(t *testing.T) { }), WithCreateTransaction(func(ctx context.Context, ledger, ik string, p wallet.PostTransaction) (*shared.V2Transaction, error) { createdTransaction = true - return nil, nil + return &shared.V2Transaction{}, nil }), ) @@ -465,3 +466,42 @@ func TestWalletsDebitRejectsInvalidBalanceMetadata(t *testing.T) { require.Equal(t, ErrorCodeValidation, errorResponse.ErrorCode) require.False(t, createdTransaction) } + +// TestWalletsDebitRejectsInvalidWalletID guards the WalletID supplied via the +// URL path: a value spanning multiple account segments or carrying Numscript +// tokens must be rejected before any ledger transaction is created. +func TestWalletsDebitRejectsInvalidWalletID(t *testing.T) { + t.Parallel() + + for _, walletID := range []string{ + "wallet:injected", + "wallet\n@world", + } { + walletID := walletID + t.Run(walletID, func(t *testing.T) { + t.Parallel() + + req := newRequest(t, http.MethodPost, "/wallets/"+url.PathEscape(walletID)+"/debit", wallet.DebitRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + }) + rec := httptest.NewRecorder() + + var ( + testEnv *testEnv + createdTransaction bool + ) + testEnv = newTestEnv( + WithCreateTransaction(func(ctx context.Context, ledger, ik string, p wallet.PostTransaction) (*shared.V2Transaction, error) { + createdTransaction = true + return &shared.V2Transaction{}, nil + }), + ) + testEnv.Router().ServeHTTP(rec, req) + + require.Equal(t, http.StatusBadRequest, rec.Result().StatusCode) + errorResponse := readErrorResponse(t, rec) + require.Equal(t, ErrorCodeValidation, errorResponse.ErrorCode) + require.False(t, createdTransaction) + }) + } +} From 072421d9f8ba72dd74ebd3a5beb572c0ae0b53e0 Mon Sep 17 00:00:00 2001 From: fabrice guery Date: Tue, 23 Jun 2026 18:53:34 +0200 Subject: [PATCH 7/8] fix(security): allow dashes in balance names, document residual aliasing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Re-allow '-' in balance names (^[0-9A-Za-z_-]+$), matching the charset already used for wallet IDs. The anchoring remains the real injection fix (no ':' separator, whitespace or Numscript tokens); dashes are not an injection vector, and forbidding them regressed legitimate dashed/UUID balance names — notably read-back validation in Manager.Debit would reject pre-existing balances. Dashes still alias under Address.String() (it strips '-', so "foo-bar" and "foobar" collapse to one ledger account). This affects every Chart-routed segment — wallet IDs, balance names, hold IDs — but not SubjectTypeLedgerAccount identifiers, which bypass the strip. The comments on balance.go/subject.go/chart.go now document that the collision is live, not theoretical. Constraint: dashes must stay valid — wallet/hold IDs are UUIDs and dashed balance names already exist in production data Rejected: forbid '-' to dodge aliasing | regresses existing data; aliasing also affects wallet/hold IDs, so a regex tweak is not a real fix Directive: the aliasing root cause (Address.String stripping '-') needs a separate ticket (stop stripping + migrate) — do not "fix" it via validation Confidence: high Scope-risk: moderate --- pkg/balance.go | 13 +++++++++---- pkg/chart.go | 12 ++++++++++-- pkg/subject.go | 5 +++-- 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/pkg/balance.go b/pkg/balance.go index 4dff3a5..12db271 100644 --- a/pkg/balance.go +++ b/pkg/balance.go @@ -13,10 +13,15 @@ import ( ) // balanceNameRegex constrains user-supplied balance names to a single, -// anchored ledger segment (no ':' separator) and excludes '-' because -// Address.String() strips dashes globally, making "foo-bar" and "foobar" -// resolve to the same ledger account. -var balanceNameRegex = regexp.MustCompile("^[0-9A-Za-z_]+$") +// anchored ledger segment: it permits [0-9A-Za-z_-] but no ':' separator, +// whitespace or Numscript metacharacters (the actual injection vectors). +// +// Dashes are allowed so legitimate dashed/UUID balance names keep working. +// This does NOT make names collision-free: per the warning on +// Address.String(), "foo-bar" still resolves to the same ledger account as +// "foobar". Allowing dashes only prevents validation from rejecting existing +// data — the aliasing itself is tracked as a separate ticket. +var balanceNameRegex = regexp.MustCompile("^[0-9A-Za-z_-]+$") type CreateBalance struct { WalletID string `json:"walletID"` diff --git a/pkg/chart.go b/pkg/chart.go index 57bc71e..56658f4 100644 --- a/pkg/chart.go +++ b/pkg/chart.go @@ -20,12 +20,20 @@ type Address []string // account, causing silent collisions on create/get/debit/credit — two // wallets, balances, or holds sharing one ledger account. // +// Scope: this only affects segments routed through the Chart — wallet IDs, +// balance names and hold IDs. SubjectTypeLedgerAccount identifiers are used +// verbatim (see Subject.getAccount) and bypass this strip, so their dashes +// are preserved. +// +// Note segment validation deliberately ALLOWS '-' (so dashed/UUID names keep +// working), which means this collision is live, not theoretical: a name like +// "foo-bar" passes validation yet still aliases to "foobar" here. +// // The strip cannot simply be removed: all existing accounts were created // with dashes already stripped, so dropping it would change the address of // every wallet/balance/hold already in the ledger and requires a data // migration. Fixing this properly (stop stripping + migrate) should be a -// dedicated ticket. Until then, callers validate user-supplied segments to -// limit — but do not fully eliminate — the collision surface. +// dedicated ticket. func (addr Address) String() string { s := strings.Join(addr, ":") s = strings.ReplaceAll(s, "-", "") diff --git a/pkg/subject.go b/pkg/subject.go index ae58527..c47ff17 100644 --- a/pkg/subject.go +++ b/pkg/subject.go @@ -54,8 +54,9 @@ func (s Subject) Validate() error { if !accountSegmentRegexp.MatchString(s.Identifier) { return newErrInvalidAccountName(s.Identifier) } - // Balance names are stricter than wallet identifiers because - // Address.String() strips dashes globally, creating aliases. + // Balance names use the same single-segment charset as wallet + // identifiers (dashes allowed). Both remain subject to the + // dash-aliasing collision documented on Address.String(). if s.Balance != "" && !balanceNameRegex.MatchString(s.Balance) { return newErrInvalidAccountName(s.Balance) } From 9c0eb063f106211efb88f801111fbbc905748b5c Mon Sep 17 00:00:00 2001 From: fabrice guery Date: Tue, 23 Jun 2026 18:59:36 +0200 Subject: [PATCH 8/8] test(api): dashes are valid balance names; assert success paths Flip the dash cases to success now that '-' is allowed: balance creation, a dashed balance in a credit wallet source, a dashed credit destination, and a dashed debit balance source all resolve and post a transaction. TestWalletsDebitRejectsInvalidBalanceMetadata now stores a genuine injection name ("injected\n@world") so it still proves read-back validation rejects Numscript tokens rather than a now-valid dash. Confidence: high Scope-risk: narrow --- pkg/api/handler_balances_create_test.go | 4 +- pkg/api/handler_wallets_credit_test.go | 55 +++++++++++++++++++------ pkg/api/handler_wallets_debit_test.go | 33 +++++++++++++-- 3 files changed, 73 insertions(+), 19 deletions(-) diff --git a/pkg/api/handler_balances_create_test.go b/pkg/api/handler_balances_create_test.go index 37b9388..9b68ed6 100644 --- a/pkg/api/handler_balances_create_test.go +++ b/pkg/api/handler_balances_create_test.go @@ -58,12 +58,12 @@ var balanceCreateTestCases = []balanceCreateTestCase{ expectedErrorCode: ErrorCodeValidation, }, { + // Dashes are allowed: dashed/UUID balance names must keep working. + // (They still alias under Address.String(); see chart.go.) name: "with name containing a dash", request: wallet.CreateBalance{ Name: "foo-bar", }, - expectedStatusCode: http.StatusBadRequest, - expectedErrorCode: ErrorCodeValidation, }, { name: "with reserved name", diff --git a/pkg/api/handler_wallets_credit_test.go b/pkg/api/handler_wallets_credit_test.go index e64ade4..9d8cf9f 100644 --- a/pkg/api/handler_wallets_credit_test.go +++ b/pkg/api/handler_wallets_credit_test.go @@ -126,15 +126,29 @@ func TestWalletsCredit(t *testing.T) { expectedErrorCode: ErrorCodeValidation, }, { - name: "with wallet source containing dash in balance", + // Dashes are allowed in balance names (they still alias under + // Address.String(); see chart.go), so a dashed wallet source resolves. + name: "with dashed balance in wallet source", request: wallet.CreditRequest{ Amount: wallet.NewMonetary(big.NewInt(100), "USD"), Sources: []wallet.Subject{ wallet.NewWalletSubject("emitter1", "foo-bar"), }, }, - expectedStatusCode: http.StatusBadRequest, - expectedErrorCode: ErrorCodeValidation, + expectedPostTransaction: func(testEnv *testEnv, walletID string) wallet.PostTransaction { + return wallet.PostTransaction{ + Script: &shared.V2PostTransactionScript{ + Plain: pointer.For(wallet.BuildCreditWalletScript( + testEnv.Chart().GetBalanceAccount("emitter1", "foo-bar"), + )), + Vars: map[string]string{ + "destination": testEnv.Chart().GetMainBalanceAccount(walletID), + "amount": "USD 100", + }, + }, + Metadata: wallet.TransactionMetadata(nil), + } + }, }, { name: "with wallet source containing numscript injection in identifier", @@ -148,13 +162,25 @@ func TestWalletsCredit(t *testing.T) { expectedErrorCode: ErrorCodeValidation, }, { - name: "with dash in destination balance", + // Dashes are allowed in balance names (still alias under + // Address.String(); see chart.go), so a dashed destination resolves. + name: "with dashed destination balance", request: wallet.CreditRequest{ Amount: wallet.NewMonetary(big.NewInt(100), "USD"), Balance: "foo-bar", }, - expectedStatusCode: http.StatusBadRequest, - expectedErrorCode: ErrorCodeValidation, + expectedPostTransaction: func(testEnv *testEnv, walletID string) wallet.PostTransaction { + return wallet.PostTransaction{ + Script: &shared.V2PostTransactionScript{ + Plain: pointer.For(wallet.BuildCreditWalletScript("world")), + Vars: map[string]string{ + "destination": testEnv.Chart().GetBalanceAccount(walletID, "foo-bar"), + "amount": "USD 100", + }, + }, + Metadata: wallet.TransactionMetadata(nil), + } + }, }, { name: "with secondary balance as destination", @@ -212,6 +238,7 @@ func TestWalletsCredit(t *testing.T) { t.Parallel() walletID := uuid.NewString() secondaryBalance := wallet.NewBalance("secondary", nil) + dashedBalance := wallet.NewBalance("foo-bar", nil) req := newRequest(t, http.MethodPost, "/wallets/"+walletID+"/credit", testCase.request) rec := httptest.NewRecorder() @@ -227,13 +254,15 @@ func TestWalletsCredit(t *testing.T) { return &testCase.postTransactionResult, nil }), WithGetAccount(func(ctx context.Context, ledger, account string) (*wallet.AccountWithVolumesAndBalances, error) { - if testEnv.Chart().GetBalanceAccount(walletID, secondaryBalance.Name) == account { - return &wallet.AccountWithVolumesAndBalances{ - Account: wallet.Account{ - Address: account, - Metadata: metadataWithExpectingTypesAfterUnmarshalling(secondaryBalance.LedgerMetadata(walletID)), - }, - }, nil + for _, b := range []wallet.Balance{secondaryBalance, dashedBalance} { + if testEnv.Chart().GetBalanceAccount(walletID, b.Name) == account { + return &wallet.AccountWithVolumesAndBalances{ + Account: wallet.Account{ + Address: account, + Metadata: metadataWithExpectingTypesAfterUnmarshalling(b.LedgerMetadata(walletID)), + }, + }, nil + } } return &wallet.AccountWithVolumesAndBalances{ Account: wallet.Account{ diff --git a/pkg/api/handler_wallets_debit_test.go b/pkg/api/handler_wallets_debit_test.go index a5f8f25..a4b305a 100644 --- a/pkg/api/handler_wallets_debit_test.go +++ b/pkg/api/handler_wallets_debit_test.go @@ -184,13 +184,25 @@ var walletDebitTestCases = []testCase{ }, }, { - name: "with dash in balance source", + // Dashes are allowed in balance names (still alias under + // Address.String(); see chart.go), so a dashed balance source resolves. + name: "with dashed balance source", request: wallet.DebitRequest{ Amount: wallet.NewMonetary(big.NewInt(100), "USD"), Balances: []string{"foo-bar"}, }, - expectedStatusCode: http.StatusBadRequest, - expectedErrorCode: string(sdkerrors.SchemasErrorCodeValidation), + expectedPostTransaction: func(testEnv *testEnv, walletID string, h *wallet.DebitHold) wallet.PostTransaction { + return wallet.PostTransaction{ + Script: &shared.V2PostTransactionScript{ + Plain: pointer.For(wallet.BuildDebitWalletScript(map[string]map[string]string{}, testEnv.Chart().GetBalanceAccount(walletID, "foo-bar"))), + Vars: map[string]string{ + "destination": "world", + "amount": "USD 100", + }, + }, + Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.TransactionMetadata(nil)), + } + }, }, { name: "with wildcard balance as source", @@ -328,6 +340,15 @@ func TestWalletsDebit(t *testing.T) { }.LedgerMetadata(walletID)), }, }, nil + case testEnv.Chart().GetBalanceAccount(walletID, "foo-bar"): + return &wallet.AccountWithVolumesAndBalances{ + Account: wallet.Account{ + Address: testEnv.Chart().GetBalanceAccount(walletID, "foo-bar"), + Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ + Name: "foo-bar", + }.LedgerMetadata(walletID)), + }, + }, nil default: return nil, errors.New("unexpected account: " + account) } @@ -447,8 +468,12 @@ func TestWalletsDebitRejectsInvalidBalanceMetadata(t *testing.T) { return &wallet.AccountWithVolumesAndBalances{ Account: wallet.Account{ Address: account, + // A stored balance name carrying Numscript tokens (e.g. tampered + // ledger metadata) must be rejected on read-back before any + // transaction is built. Dashes alone are valid, so use a real + // injection vector here. Metadata: metadataWithExpectingTypesAfterUnmarshalling(wallet.Balance{ - Name: "foo-bar", + Name: "injected\n@world", }.LedgerMetadata(walletID)), }, }, nil