fix: reject non-finite bridge lock amounts#5465
Conversation
|
Welcome to RustChain! Thanks for your first pull request. Before we review, please make sure:
Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150) A maintainer will review your PR soon. Thanks for contributing! |
github-actions
Bot
added
BCOS-L1
|
CI note: the Focused validation for this PR remains:
|
kekehanshujun
left a comment
kekehanshujun
left a comment
There was a problem hiding this comment.
LGTM. The fix rejects NaN, Infinity, and -Infinity immediately after parsing and before any range checks or base-unit conversion, which matches #5361's expected behavior and keeps the response consistent with other invalid amounts.
Validation run on the PR diff:
python -m py_compile bridge/bridge_api.py bridge/test_bridge_api.pypython -m pytest bridge/test_bridge_api.py::TestLockEndpoint::test_lock_rejects_non_finite_amount -q --tb=short-> 3 passedpython -m pytest bridge/test_bridge_api.py -q --tb=short-> 44 passedgit diff --check -- bridge/bridge_api.py bridge/test_bridge_api.py
galanime
left a comment
galanime
left a comment
There was a problem hiding this comment.
Reviewed the non-finite bridge amount guard. The patch is small and matches #5361's failure mode: NaN/Infinity are rejected immediately after float(...) parsing, before min/max comparison and before _amount_to_base(...) can raise.
Validation run on head ce7d1e80e55efb0f8b7e13afcdd6d9dc5ab4a5cc:
python -m pytest bridge\test_bridge_api.py::TestLockEndpoint::test_lock_rejects_non_finite_amount -q -o addopts=''-> 3 passed after creating the local Windows\tmpdirectory required by the existing test fixture.python -m py_compile bridge\bridge_api.py bridge\test_bridge_api.py-> passed.git diff --check origin/main...HEAD-> passed.
One environment note: the full bridge\test_bridge_api.py file is still not reliably Windows-portable because it hard-codes /tmp/bridge_test_727.db and can hit sqlite3.OperationalError or a Windows file-lock PermissionError during collection. That appears to be pre-existing test harness behavior rather than a regression from this PR, and the focused non-finite amount regression passes once the fixture path exists.
ZacharyZhang-NY
left a comment
ZacharyZhang-NY
left a comment
There was a problem hiding this comment.
Validation performed on Windows Python 3.12:
python -m py_compile bridge/bridge_api.py bridge/test_bridge_api.pypassed.- Created
C:\tmpbecause the test module uses/tmp/bridge_test_727.dband Windows maps that path toC:\tmp\.... python -m pytest bridge/test_bridge_api.py::TestLockEndpoint::test_lock_rejects_non_finite_amount -qpassed with3 passed in 0.15s.python -m pytest bridge/test_bridge_api.py -qpassed with44 passed in 0.29s.
The fix is placed immediately after float parsing and before sender/min/max/base-unit handling, so NaN, Infinity, and -Infinity share the existing invalid amount 400 response and avoid the previous conversion path. I did not find a blocking issue.
ZacharyZhang-NY
left a comment
ZacharyZhang-NY
left a comment
There was a problem hiding this comment.
Reviewed PR #5465 at head ce7d1e8.
Validation performed:
- Checked issue #5361. The scoped bug is
/bridge/lockaccepting non-finite parsed amounts such asNaNthrough range checks and reaching base-unit conversion, which raises instead of returning a clean client error. - git fetch origin pull/5465/head:review-pr-5465 --force
- git diff --check origin/main...review-pr-5465 -- bridge/bridge_api.py bridge/test_bridge_api.py -> passed.
- python -m py_compile bridge/bridge_api.py bridge/test_bridge_api.py on an extracted PR-head tree -> passed.
- python -m pytest bridge/test_bridge_api.py::TestLockEndpoint::test_lock_rejects_non_finite_amount -q --confcutdir= -> 3 passed.
- python -m pytest bridge/test_bridge_api.py -q --confcutdir= -> 44 passed.
- Ran Flask test-client probes for
NaN,Infinity, and-Infinity; each returned 400 with{"error":"invalid amount"}.
The bridge lock endpoint now rejects non-finite amounts immediately after parsing and before range checks or base-unit conversion. Approving.
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Approved — focused bridge lock validation pass.
Validation:
git diff --check origin/main...HEAD -- bridge/bridge_api.py bridge/test_bridge_api.pypython3 -B -m py_compile bridge/bridge_api.py bridge/test_bridge_api.pyuv run --no-project --with pytest --with flask python -B -m pytest -q bridge/test_bridge_api.py --noconftest→44 passed in 0.34s- Flask test-client probe with
BRIDGE_REQUIRE_PROOF=falseverified/bridge/lockreturns HTTP 400{"error":"invalid amount"}for"NaN","Infinity", and"-Infinity", while finite"1.25"still returns 201 with staterequested.
Reasoning:
float() accepts these non-finite strings, so the added math.isfinite(amount_float) gate is in the right place before min/max checks and DB persistence. The regression test covers all three non-finite spellings, and the full bridge API test file still passes.
Code Review — Bounty #73PR: fix: reject non-finite bridge lock amounts by @ramimbo
SummaryThis is a bug fix PR. Changes appear consistent with project patterns. Wallet: Reviewing under Bounty #73 — Code Review Bounty Program |
508704820
left a comment
508704820
left a comment
There was a problem hiding this comment.
Reject non-finite bridge lock amounts. Verify: (1) NaN, Infinity, -Infinity are all rejected. (2) Rejection happens at the API boundary, not deep in bridge logic. (3) Zero and negative amounts also rejected for lock operations. - Xeophon (security review)
|
Security Review ✅ IMPORTANT Reject NaN/Infinity/-Infinity bridge lock amounts. This is a classic float exploitation: float('NaN') bypasses all min/max comparisons (NaN < X is always False, NaN > X is always False) and then crashes at int(round(...)). Attacker could use this to bypass amount validation or cause 500 errors. Reviewed by Xeophon - Solana: Lt9nERv6VHsojw15LpFeiaabuphAggzfLF9sM9UXRrZ |
9 participants
What Changed
/bridge/lockamounts (NaN,Infinity,-Infinity) immediately after parsing.400with the existinginvalid amountresponse instead of reaching base-unit conversion or min/max checks.Why
float("NaN")bypasses normal min/max comparisons and then raises duringint(round(...)), producing an unhandled error path. Non-finite values should be treated the same as other invalid amounts before any lock state or proof handling runs.Fixes #5361
Testing / Evidence
python -m pytest bridge/test_bridge_api.py::TestLockEndpoint::test_lock_rejects_non_finite_amount -q-> 3 passedpython -m pytest bridge/test_bridge_api.py -q-> 44 passedpython -m py_compile bridge/bridge_api.py bridge/test_bridge_api.py-> passedgit diff --check -- bridge/bridge_api.py bridge/test_bridge_api.py-> passedBCOS Checklist
BCOS-L1orBCOS-L2(this bridge validation change should beBCOS-L2)RTC payout address for any accepted bounty:
RTC877021895fd29d034f35c87e1b37af8534703792