fix: reject malformed bridge initiate field types#5567
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! |
strongkeep-debug
left a comment
There was a problem hiding this comment.
I found two merge-gate issues in the new test coverage, not in the bridge validation logic itself.
On head baeba7d:
python tools/bcos_spdx_check.py --base-ref origin/main
# fails: node/test_bridge_initiate_type_validation.py is a new file without an SPDX header
python -m pytest -q node/test_bridge_initiate_type_validation.py tests/test_bridge_lock_ledger.py node/test_bridge_precision.py node/tests/test_bridge_api_limit_validation.py
# 2 failed, 47 passed, 9 subtests passed
Both pytest failures are from the new test module teardown on Windows:
node/test_bridge_initiate_type_validation.py:46
PermissionError: [WinError 32] ... os.unlink(self.db_path)
So the functional route checks are getting exercised, but the new temporary SQLite DB cleanup is not portable in the advertised validation command. Please add the required SPDX header and adjust the test cleanup so the DB handle/resources are fully released before unlinking on Windows.
kekehanshujun
left a comment
There was a problem hiding this comment.
I found one validation gap that remains in the changed route flow. validate_bridge_request lowercases source_chain/dest_chain only inside its returned details, but initiate_bridge continues to use the raw data object for validate_chain_address_format and for BridgeTransferRequest. That means a payload such as source_chain='Solana' can pass the normalized chain allow-list but then skip the Solana address-format branch because validate_chain_address_format receives 'Solana' instead of 'solana'. Please switch the route to use validation.details for the address-format checks and request object construction so the normalized values are actually enforced.
ZacharyZhang-NY
left a comment
There was a problem hiding this comment.
I found a blocking Windows test regression in the new regression test. The validation behavior reaches the expected 400 responses, but both new tests fail during tearDown because os.unlink(self.db_path) runs while the SQLite temp file is still locked on Windows.\n\nRepro on this PR head baeba7d:\n- python -B -m py_compile node/bridge_api.py node/test_bridge_initiate_type_validation.py -> passed\n- git diff --check origin/main...HEAD -- node/bridge_api.py node/test_bridge_initiate_type_validation.py -> passed\n- PYTHONPATH=node python -B -m pytest -q node/test_bridge_initiate_type_validation.py --noconftest -> 2 failed\n- PYTHONPATH=node python -B -m pytest -q node/test_bridge_initiate_type_validation.py tests/test_bridge_lock_ledger.py node/test_bridge_precision.py node/tests/test_bridge_api_limit_validation.py --noconftest -> 2 failed, 47 passed\n\nFailure:\nPermissionError [WinError 32] at node/test_bridge_initiate_type_validation.py:46 when deleting the temporary sqlite DB. Please close the setup cursor/connection resources in a Windows-safe way, or move the test DB into a temporary directory with cleanup that runs after all SQLite handles are released. The route hardening itself is directionally correct, but the added test file currently breaks local/CI validation on Windows.
|
Addressed the #5567 review blockers in commit Changes:
Validation run on the PR branch: python tools/bcos_spdx_check.py --base-ref upstream/main
PYTHONPATH=node python -B -m pytest -q node/test_bridge_initiate_type_validation.py tests/test_bridge_lock_ledger.py node/test_bridge_precision.py node/tests/test_bridge_api_limit_validation.py --noconftest
python -B -m py_compile node/bridge_api.py node/test_bridge_initiate_type_validation.py
git diff --check upstream/main...HEAD -- node/bridge_api.py node/test_bridge_initiate_type_validation.pyResult: SPDX check OK; |
TJCurnutte
left a comment
There was a problem hiding this comment.
Thanks for tightening the bridge-initiate type guards. The new object/string/non-finite checks are good, and the focused route tests pass, but one malformed scalar still reaches transfer creation: JSON boolean amount_rtc: true is accepted as 1.0 RTC.
Proof run at head 236472716fcd1069f9db9554b851a02e85ae40ec:
git diff --check origin/main...HEAD -- node/bridge_api.py node/test_bridge_initiate_type_validation.py
PYTHONPATH=node python3 -B -m py_compile node/bridge_api.py node/test_bridge_initiate_type_validation.py
PYTHONPATH=node uv run --no-project --with pytest --with flask --with flask-cors --with requests python -B -m pytest -q -o addopts='' --noconftest node/test_bridge_initiate_type_validation.py tests/test_bridge_lock_ledger.py node/test_bridge_precision.py node/tests/test_bridge_api_limit_validation.pyResult: 51 passed, 9 subtests passed in 2.09s.
Additional Flask/temp-SQLite probe against /api/bridge/initiate:
top_array -> 400 {'error': 'Request body must be a JSON object'}
amount_bool_true -> 200 {'ok': True, 'amount_rtc': 1.0, ...}
amount_bool_false -> 400 {'error': 'amount_rtc must be positive'}
amount_list -> 400 {'error': 'amount_rtc must be a number'}
amount_str_nan -> 400 {'error': 'amount_rtc must be finite'}
The issue is float(data.get("amount_rtc", 0)): in Python, bool is numeric enough for float(True) == 1.0, so a malformed JSON boolean becomes a real bridge withdrawal request. Since this PR is specifically closing malformed field-type behavior on a public bridge route, amount_rtc should reject booleans explicitly before numeric conversion and add a regression case for true/false.
|
Update after boolean amount review feedback:
Verification on head |
|
Security Review ✅ Bridge initiate field type validation + NaN/Infinity rejection. Same class as #5465 — non-finite amounts bypass min/max comparisons. Fix adds proper type checking before .lower() and length checks. Reviewed by Xeophon - Solana: Lt9nERv6VHsojw15LpFeiaabuphAggzfLF9sM9UXRrZ |
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
strongkeep-debug
left a comment
There was a problem hiding this comment.
Re-reviewed current head 2688165 after the bridge-initiate test and boolean-amount follow-ups. Approving: the blockers I previously reported are resolved on the current head, and the later boolean amount gap is covered too.
Validation run locally:
git diff --check origin/main...HEAD -- node/bridge_api.py node/test_bridge_initiate_type_validation.py
# passed
python -m py_compile node/bridge_api.py node/test_bridge_initiate_type_validation.py
# passed
python tools/bcos_spdx_check.py --base-ref origin/main
# BCOS SPDX check: OK
pytest node/test_bridge_initiate_type_validation.py -q
# 4 passed, 10 subtests passed
The focused test file now has the SPDX header, the SQLite test fixture closes the setup connection cleanly on Windows, and amount_rtc: true is rejected before transfer creation instead of being coerced as numeric 1.
Summary
/api/bridge/initiateJSON payloads and scalar fields to have the expected types before route validation calls.lower(), length checks, or membership checksamount_rtcvalues such asnanandinfwith a controlled 400 responseWhy
The public bridge initiation route accepted untrusted JSON and assumed fields like
source_chain,dest_chain,bridge_type, addresses, andmemowere already strings. Structured values could raise uncaught exceptions and produce 500 responses instead of client validation errors. Non-finite amount strings could also reach later integer conversion paths and fail as server errors.Validation
Observed locally:
49 passed, 9 subtests passed, py_compile passed, andgit diff --checkwas clean.Duplicate / safety checks
Before opening this PR I searched public issues and PRs for
bridge initiate type validation,malformed bridge initiate 500,reject malformed bridge initiate field types,source_chain list lower bridge_api, and related bridge-initiate type-confusion terms. I did not find an exact duplicate; the broad#500hit was an unrelated closed bounty title/number collision.No private keys, tokens, credentials, or account-internal data are included.