fix(security): validate account names to prevent Numscript injection (EN-1200)

[flemzord]

Problem (Critical + High — C1, H2)

The credit/debit Numscript templates interpolate account names directly into the script body (@{{ .source }}). But:

• type:"WALLET" subjects only checked Identifier != "" (neither the content nor Balance was validated);
• balanceNameRegex was unanchored ([0-9A-Za-z_-]+), so MatchString accepted any string containing at least one valid character (e.g. balance:injected, x\n@world).

An identifier/balance/balance-name containing a newline and Numscript tokens could close the source block and inject arbitrary statements executed with the service's ledger credentials (e.g. send [USD *] (source = @world destination = @attacker:main)).

Fix

• Anchor balanceNameRegex → ^[0-9A-Za-z_-]+$ (a name must be a single clean segment).
• Validate both the identifier and balance of WALLET subjects via accounts.ValidateAddress (the same guarantee the already-safe ACCOUNT branch relies on).
• Validate walletID before building debit/credit scripts.

Possible follow-up defense-in-depth: bind sources as account $sourceN variables instead of interpolating them — left out here to avoid destabilizing the exact-Numscript assertions in the existing tests.

Tests

• TestBalancesCreate: names with an account separator and with whitespace + Numscript tokens → 400.
• TestWalletsCredit: injection via the balance and via the identifier of a WALLET subject → 400.

From the in-depth repository review.

[coderabbitai]

Warning

Review limit reached

@flemzord, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 58 minutes and 40 seconds. Learn how PR review limits work.

Your organization has run out of usage credits. Purchase more credits in the billing tab to continue.

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1e7d0ab7-548a-4de5-923c-e1ba23555958

📥 Commits

Reviewing files that changed from the base of the PR and between 85e1d3f and 8800664.

📒 Files selected for processing (8)

• pkg/api/handler_balances_create_test.go
• pkg/api/handler_wallets_credit_test.go
• pkg/api/handler_wallets_debit_test.go
• pkg/balance.go
• pkg/credit.go
• pkg/debit.go
• pkg/manager.go
• pkg/subject.go

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/numscript-injection-validation

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[flemzord]

Revue inline: j’ai relevé deux trous de validation restants dans le durcissement des segments de compte.

[NumaryBot]

🛑 Changes requested — multi-model review

The PR correctly anchors the balance name regex and adds injection validation for wallet subjects and wallet IDs, closing the primary Numscript injection vectors. However, two major issues require attention before merging. First, removing '-' from valid balance names goes beyond the stated anchoring fix: the dash character is not itself a Numscript injection vector, and the change silently breaks debit operations for any pre-existing wallet whose balances have dashed names (since names are re-validated against the new regex when read back from the ledger). This is a backwards-incompatible behavioral change that needs either a rollback to permitting dashes or a migration/compatibility plan. Second, WalletID validation for the Credit path is performed inside the manager method rather than inside Credit.Validate(), creating an inconsistency with the Debit path and a potential bypass for non-manager callers. Additionally, there are no handler-level tests exercising malicious walletIDs supplied via URL parameters, and the stated rationale for dash exclusion (Address.String() aliasing) lacks a confirming test or code reference.

🟠 [major] Removing '-' from balance names breaks backwards compatibility with existing data

pkg/balance.go:18 — reported by claude, gpt

The regex change from [0-9A-Za-z_-]+ to ^[0-9A-Za-z_]+$ drops support for dashes in balance names. This is more than an anchoring fix: any pre-existing balance whose name contains '-' will now fail validation in Manager.Debit (where names are read back from the ledger via BalanceFromAccount and then checked against the new regex). This silently breaks debit operations for wallets that already have dash-containing balances — a regression on existing data. The dash character is not itself a Numscript injection vector; the actual injection vectors are whitespace, newlines, and ':' separators. If dash aliasing is a separate concern, it requires an explicit migration/compatibility plan.

Suggestion: Either keep '-' in the allowed character set while still anchoring the regex (i.e. ^[0-9A-Za-z_-]+$) to fix the actual injection vectors, or treat invalid stored names defensively (e.g. skip/warn rather than hard-fail) so a single legacy balance doesn't block all debits. If '-' exclusion is intentional, split it into a separate breaking change with release notes and a data migration plan.

🟠 [major] WalletID validation for Credit lives outside Validate(), making it bypassable

pkg/manager.go:290 — reported by claude

The WalletID validation in the Credit flow is performed directly inside Manager.Credit via accountSegmentRegexp rather than inside Credit.Validate(). This means any other caller of Credit.Validate() (e.g. tests or future handlers) will not have the WalletID checked. This is an inconsistency with how Debit.Validate() handles its WalletID, and represents a potential bypass vector if the manager method is not the sole entry point in the future.

Suggestion: Move the accountSegmentRegexp.MatchString(credit.WalletID) check into Credit.Validate() (mirroring the pattern in Debit.Validate()) so that WalletID validation is centralized and cannot be bypassed by callers that only invoke Validate().

🟡 [minor] No handler-level tests for malicious URL wallet IDs

pkg/api/handler_wallets_credit_test.go — reported by gpt

The new tests cover balance names and WALLET subject identifiers, but there are no handler tests that exercise a malicious walletID coming in via the URL path (e.g. wallet:injected or a newline-containing value). This security-relevant validation path could regress without any focused test failing.

Suggestion: Add credit and debit handler tests using URL walletIDs containing account separators or Numscript tokens (e.g. wallet:injected, wallet%0A@world). Assert an HTTP 400 response and that no ledger transaction is created.

🟡 [minor] Dash-exclusion rationale for balance names is unverified and should have a confirming test

pkg/subject.go:60 — reported by claude

Subject.Validate() enforces stricter balance-name rules (no '-') compared to wallet identifiers, with the justification being that Address.String() strips dashes and causes aliasing. However, there is no test or code reference that actually demonstrates this aliasing behavior. Without such evidence, the stricter rule appears arbitrary and the security rationale is unverifiable.

Suggestion: Add a test demonstrating that Chart().GetBalanceAccount with a dashed name aliases to the undashed account, or add a code comment citing the specific ledger Address.String() behavior that causes the aliasing concern.

⚪ [nit] Mock transaction creator returns nil,nil which could mask nil-deref panics

pkg/api/handler_wallets_debit_test.go:467 — reported by claude

In TestWalletsDebitRejectsInvalidBalanceMetadata, the WithCreateTransaction mock returns (nil, nil). If the validation check is ever accidentally removed and the code proceeds to use the nil transaction result, the test would panic rather than produce a clean assertion failure, potentially masking regressions.

Suggestion: Return a minimal non-nil &shared.V2Transaction{} from the mock to avoid a potential nil-dereference panic obscuring the intended assertion failure.

---

_{Reviewed in parallel by claude (anthropic/claude-opus-4-8) and gpt (openai/gpt-5.5), then consolidated. This comment is updated on each push.}