Skip to content

fix: reject malformed bridge initiate field types#5567

Merged
Scottcjn merged 3 commits into
Scottcjn:mainfrom
iamdinhthuan:fix-bridge-initiate-type-validation
May 18, 2026
Merged

fix: reject malformed bridge initiate field types#5567
Scottcjn merged 3 commits into
Scottcjn:mainfrom
iamdinhthuan:fix-bridge-initiate-type-validation

Conversation

@iamdinhthuan
Copy link
Copy Markdown
Contributor

Summary

  • require /api/bridge/initiate JSON payloads and scalar fields to have the expected types before route validation calls .lower(), length checks, or membership checks
  • reject non-finite amount_rtc values such as nan and inf with a controlled 400 response
  • add route-level regression coverage for malformed bridge initiation payloads

Why

The public bridge initiation route accepted untrusted JSON and assumed fields like source_chain, dest_chain, bridge_type, addresses, and memo were 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

PYTHONPATH=node python3 -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
python3 -m py_compile node/bridge_api.py node/test_bridge_initiate_type_validation.py
git diff --check

Observed locally: 49 passed, 9 subtests passed, py_compile passed, and git diff --check was 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 #500 hit was an unrelated closed bounty title/number collision.

No private keys, tokens, credentials, or account-internal data are included.

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related labels May 17, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Non-doc PRs have a BCOS-L1 or BCOS-L2 label
  • Doc-only PRs are exempt from BCOS tier labels when they only touch docs/**, *.md, or common image/PDF files
  • New code files include an SPDX license header
  • You've tested your changes against the live node

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!

Copy link
Copy Markdown

@strongkeep-debug strongkeep-debug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown
Contributor

@kekehanshujun kekehanshujun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown

@ZacharyZhang-NY ZacharyZhang-NY left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@iamdinhthuan
Copy link
Copy Markdown
Contributor Author

Addressed the #5567 review blockers in commit 2364727.

Changes:

  • added the SPDX header to node/test_bridge_initiate_type_validation.py
  • explicitly closes the setup SQLite connection before tests run, avoiding Windows file-lock cleanup failures
  • initiate_bridge now uses validation.details for address-format checks and BridgeTransferRequest construction, so normalized chain values are enforced and stored
  • added regressions for mixed-case chain input: invalid Base address now returns 400, and valid mixed-case chains are normalized in the success response

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.py

Result: SPDX check OK; 51 passed, 9 subtests passed; py_compile and diff check passed.

Copy link
Copy Markdown

@TJCurnutte TJCurnutte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.py

Result: 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.

@iamdinhthuan
Copy link
Copy Markdown
Contributor Author

Update after boolean amount review feedback:

  • Pushed commit 2688165.
  • Added a regression for amount_rtc: true on /api/bridge/initiate; it failed before the fix with HTTP 200 instead of 400.
  • validate_bridge_request() now rejects boolean amount_rtc before numeric conversion, so Python cannot coerce True to 1.0 RTC.

Verification on head 2688165:

PYTHONPATH=node python3 -B -m pytest -q node/test_bridge_initiate_type_validation.py --noconftest
# 4 passed, 10 subtests passed

PYTHONPATH=node python3 -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
# 51 passed, 10 subtests passed

python3 -B -m py_compile node/bridge_api.py node/test_bridge_initiate_type_validation.py
# passed

python3 tools/bcos_spdx_check.py --base-ref origin/main
# BCOS SPDX check: OK

git diff --check origin/main...HEAD -- node/bridge_api.py node/test_bridge_initiate_type_validation.py
# passed

@508704820
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Great work on this PR. 🚀

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown

@strongkeep-debug strongkeep-debug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Copy Markdown
Contributor

@HCIE2054 HCIE2054 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@Scottcjn Scottcjn merged commit 739f2e5 into Scottcjn:main May 18, 2026
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related size/M PR: 51-200 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants