Skip to content

fix(account): tighten payment input validation#322

Open
Kubudak90 wants to merge 1 commit into
base:masterfrom
Kubudak90:fix/validation-strict-amount-and-period
Open

fix(account): tighten payment input validation#322
Kubudak90 wants to merge 1 commit into
base:masterfrom
Kubudak90:fix/validation-strict-amount-and-period

Conversation

@Kubudak90

Copy link
Copy Markdown

Summary

Two bugs in @base-org/account payment validation, fixed together because they share validation.ts:

  • Bug: Payment amount validation accepts malformed numeric strings #313validateStringAmount used parseFloat, which silently accepts inputs like "1abc" or "1.2.3" by reading only the numeric prefix. The function now requires a strict decimal literal ^\d+(\.\d+)?$, so malformed amounts fail immediately with a clear error rather than reaching downstream payment encoding with a truncated value.
  • subscribe() accepts zero, negative, or invalid subscription periods #314subscribe() did not bound periodInDays or overridePeriodInSecondsForTestnet, so 0, negative, fractional, NaN, Infinity, or values above uint48 could reach wallet_sign. A new validatePositiveSafeInteger(value, fieldName, max?) helper enforces a positive safe integer (optionally bounded by UINT48_MAX = 281474976710655), called at the same point as the existing input validation in subscribe().

pay() benefits from the stricter validateStringAmount automatically, since it calls the same validator.

Changes

File Change
validation.ts STRICT_DECIMAL_RE rejects malformed/scientific/whitespace; validatePositiveSafeInteger helper; UINT48_MAX constant exported
subscribe.ts Imports the new helper; validates the active period parameter before any wallet interaction
validation.test.ts +12 assertions: malformed prefixes, multiple dots, scientific notation, signs, whitespace, leading/trailing dot, empty; full coverage of validatePositiveSafeInteger (positive/zero/negative/non-integer/non-number/max-bound/unsafe-integer)
subscribe.test.ts +11 assertions: it.each over both period parameters for zero/negative/non-integer/NaN/Infinity; uint48 upper-bound rejection

Behavior changes worth noting

  • validateStringAmount("-10", 6) previously threw "Invalid amount: must be greater than 0"; now throws "Invalid amount: must be a valid number". Still rejects negatives — the message is just more accurate (negative-sign also fails the strict-decimal format check first). Existing test updated.
  • No other public-API surface changes; new exports (validatePositiveSafeInteger, UINT48_MAX) are additive.

Test Plan

  • yarn vitest run src/interface/payment/utils/validation.test.ts src/interface/payment/subscribe.test.ts42 / 42 passing.
  • Wider payment suite still passes the 177 tests that were green on master; two test files (pay.test.ts, base.test.ts) fail at the Vite transform stage with Failed to resolve import "./Dialog-css.js" — verified to fail identically on the unmodified master baseline (pre-existing).
  • biome check --write applied to the four touched files; no new diagnostics from my diff.

Closes #313
Closes #314

validateStringAmount now requires a strict decimal literal (`^\d+(\.\d+)?$`)
so malformed inputs like '1abc' or '1.2.3' can't slip past parseFloat —
which previously read only the numeric prefix and silently truncated the
rest. Closes base#313.

subscribe() now validates periodInDays and overridePeriodInSecondsForTestnet
as positive safe integers within the uint48 range used by spend-permission
typed data, called at the same point as the existing input validation.
Zero, negative, fractional, NaN, Infinity, and out-of-range values now fail
fast with a clear error instead of reaching wallet_sign. Closes base#314.

Also exports a reusable validatePositiveSafeInteger helper and the
UINT48_MAX constant.

Tests:
- 12 new assertions in validation.test.ts covering the strict format and
  the positive-safe-integer helper.
- 11 new assertions in subscribe.test.ts covering both period parameters,
  including the uint48 upper bound.
- Existing '-10' assertion message updated (still rejects; the new message
  is more accurate).
@cb-heimdall

Copy link
Copy Markdown
Collaborator

🟡 Heimdall Review Status

Requirement Status More Info
Reviews 🟡 0/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 1
Sum 2

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.

subscribe() accepts zero, negative, or invalid subscription periods Bug: Payment amount validation accepts malformed numeric strings

2 participants