diff --git a/pkg/api/handler_balances_create_test.go b/pkg/api/handler_balances_create_test.go index 99a5588..9b68ed6 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", }, }, { @@ -39,6 +39,32 @@ 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, + }, + { + // 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", + }, + }, { 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..9d8cf9f 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" @@ -102,6 +103,85 @@ 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 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, + }, + { + // 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"), + }, + }, + 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", + request: wallet.CreditRequest{ + Amount: wallet.NewMonetary(big.NewInt(100), "USD"), + Sources: []wallet.Subject{ + wallet.NewWalletSubject("emitter1 @world", ""), + }, + }, + expectedStatusCode: http.StatusBadRequest, + expectedErrorCode: ErrorCodeValidation, + }, + { + // 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", + }, + 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", request: wallet.CreditRequest{ @@ -125,7 +205,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, @@ -158,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() @@ -173,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{ @@ -208,3 +291,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 4728941..a4b305a 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" @@ -182,6 +183,27 @@ var walletDebitTestCases = []testCase{ } }, }, + { + // 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"}, + }, + 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", request: wallet.DebitRequest{ @@ -318,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) } @@ -416,3 +447,86 @@ 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, + // 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: "injected\n@world", + }.LedgerMetadata(walletID)), + }, + }, nil + }), + 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) +} + +// 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) + }) + } +} diff --git a/pkg/balance.go b/pkg/balance.go index 715ff31..12db271 100644 --- a/pkg/balance.go +++ b/pkg/balance.go @@ -12,7 +12,16 @@ import ( "github.com/formancehq/go-libs/v5/pkg/types/time" ) -var balanceNameRegex = regexp.MustCompile("[0-9A-Za-z_-]+") +// balanceNameRegex constrains user-supplied balance names to a single, +// 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 d4ec6b0..56658f4 100644 --- a/pkg/chart.go +++ b/pkg/chart.go @@ -8,6 +8,32 @@ 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. +// +// 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. func (addr Address) String() string { s := strings.Join(addr, ":") s = strings.ReplaceAll(s, "-", "") diff --git a/pkg/credit.go b/pkg/credit.go index 45fb197..fa3bf93 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 } @@ -45,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/debit.go b/pkg/debit.go index 2406042..dc65c34 100644 --- a/pkg/debit.go +++ b/pkg/debit.go @@ -55,6 +55,9 @@ func (d Debit) getDestination() Subject { } func (d Debit) Validate() error { + if !accountSegmentRegexp.MatchString(d.WalletID) { + return newErrInvalidAccountName(d.WalletID) + } if d.Destination != nil { if err := d.Destination.Validate(); err != nil { return err @@ -66,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 c9566d6..5f9c342 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 0ff946a..c47ff17 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,6 +51,15 @@ func (s Subject) Validate() error { if s.Identifier == "" { return fmt.Errorf("wallet identifier cannot be empty") } + if !accountSegmentRegexp.MatchString(s.Identifier) { + return newErrInvalidAccountName(s.Identifier) + } + // 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) + } } return nil }