Skip to content

fix: validate bridge base address hex#5435

Merged
Scottcjn merged 1 commit into
Scottcjn:mainfrom
hungle123-dev:codex/bridge-base-address-hex
May 18, 2026
Merged

fix: validate bridge base address hex#5435
Scottcjn merged 1 commit into
Scottcjn:mainfrom
hungle123-dev:codex/bridge-base-address-hex

Conversation

@hungle123-dev
Copy link
Copy Markdown
Contributor

/claim #5434
/claim #305

Summary

  • Reject non-hex Base addresses in node/bridge_api.py::validate_chain_address_format.
  • Add regression coverage for a length-valid 0xZZ... Base address.
  • Preserve existing valid Base address and existing prefix/length validation behavior.

Root cause

The Base address validator only checked address.startswith(0x) and len(address) == 42. That allowed strings that are the right length but not valid hexadecimal addresses.

Validation

RED first:

  • python -m pytest tests/test_bridge_lock_ledger.py::TestAddressValidation::test_invalid_base_address_non_hex -q failed with assert True is False.

GREEN after fix:

  • python -m pytest tests/test_bridge_lock_ledger.py::TestAddressValidation -q -> 8 passed.
  • python -m py_compile node\bridge_api.py tests\test_bridge_lock_ledger.py -> passed.
  • git diff --check -- node/bridge_api.py tests/test_bridge_lock_ledger.py -> passed.

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) node Node server related tests Test suite changes size/XS PR: 1-10 lines labels May 16, 2026
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.

Reviewed head e0e089c6a7519ec2c5e626a04aa3c0015ba6bbbf for the node/bridge_api.py Base address format validation.

Validation run from a minimal PR-head extraction:

  • $env:PYTHONPATH=.tmp-pr5435-files\node; python -m pytest ".tmp-pr5435-files\tests\test_bridge_lock_ledger.py::TestAddressValidation" -q --confcutdir=.tmp-pr5435-files -> 8 passed
  • python -m py_compile .tmp-pr5435-files\node\bridge_api.py .tmp-pr5435-files\node\lock_ledger.py .tmp-pr5435-files\tests\test_bridge_lock_ledger.py -> passed

The validator now rejects 0x + length-valid strings containing non-hex characters while preserving the existing prefix and length checks. I did not find a blocking issue in this change.

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! Thanks for contributing!

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! Thanks for contributing!

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.

Approved after a focused bridge address-validation pass.

What I checked:

cd /tmp/rustchain-review-pr5435-76484
git diff --check origin/main...HEAD -- node/bridge_api.py tests/test_bridge_lock_ledger.py
python3 -B -m py_compile node/bridge_api.py tests/test_bridge_lock_ledger.py
uv run --no-project --with pytest --with flask python -B -m pytest -q tests/test_bridge_lock_ledger.py --noconftest
PYTHONPATH=node python3 -B - <<'PY'
from bridge_api import validate_chain_address_format
cases = {
    'non_hex': ('base', '0xZZ2d35Cc6634C0532925a3b844Bc9e7595f0bEb0'),
    'short': ('base', '0x1'),
    'valid_mixed': ('base', '0x742d35Cc6634C0532925a3b844Bc9e7595f0bEb0'),
}
for name,args in cases.items():
    print(name, validate_chain_address_format(*args))
PY

Validation result: 42 passed in 2.43s; the runtime probe returned non_hex (False, 'Invalid Base address hex'), short (False, 'Invalid Base address length'), and valid_mixed (True, '').

The change is narrow and lands in the right order: Base addresses still fail fast on missing 0x and bad length, and 42-character values now also reject non-hex payloads before the bridge accepts them. The added regression test covers the non-hex case directly. I do not see a blocker in this PR-scoped review.

@508704820
Copy link
Copy Markdown
Contributor

Security Review:

Validating bridge base address hex format prevents malformed addresses from being stored in the bridge contract.

Security considerations:

  1. Address collision risk - Hex validation alone doesn't prevent address collisions. Two different input formats could normalize to the same address. Ensure canonical form is enforced before storage.
  2. Bridge replay attacks - Can a previously valid bridge address be replayed after the real address is updated? Bridge address changes should invalidate pending transactions.
  3. Cross-chain address format - Is the hex format specific to RTC addresses, or does it need to support external chain addresses (Ethereum, Solana)? Different chains have different address lengths and formats.
  4. Address whitelisting - For critical bridge operations, consider maintaining an allowlist of approved bridge addresses rather than just format validation.
  • Xeophon (wallet: RTC9d7caca3039130d3b26d41f7343d8f4ef4592360)

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.

Reviewed PR #5435 at head e0e089c.

Validation performed:

  • Checked issue #5434. The scoped bug is that validate_chain_address_format("base", address) accepts 42-character 0x addresses containing non-hex characters.
  • git fetch origin pull/5435/head:review-pr-5435 --force
  • git diff --check origin/main...review-pr-5435 -- node/bridge_api.py tests/test_bridge_lock_ledger.py -> passed.
  • python -m py_compile node/bridge_api.py tests/test_bridge_lock_ledger.py on an extracted PR-head tree -> passed.
  • PYTHONPATH=/node python -m pytest tests/test_bridge_lock_ledger.py::TestAddressValidation -q --confcutdir= -> 8 passed.
  • Ran a focused runtime probe for Base address validation: a mixed-case valid hex address returned (True, ""), 0xZZ... returned (False, "Invalid Base address hex"), a missing 0x prefix returned the existing prefix error, and a short address returned the existing length error.

The patch keeps the existing Base prefix and length checks intact, adds the missing hex-character validation, and adds direct regression coverage for the issue. Approving.

@BossChaos
Copy link
Copy Markdown
Contributor

Code Review — Bounty #73

PR: fix: validate bridge base address hex by @hungle123-dev
Files changed: 2 (+9/-0)

  • ✅ Bug fix or input validation
  • ✅ Input validation present

Summary

This is a bug fix PR. Changes appear consistent with project patterns.

Wallet: 0xdaE5d307339074A24F579dB48e7c639359D94904

Reviewing under Bounty #73 — Code Review Bounty Program

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

@508704820 508704820 left a comment

Choose a reason for hiding this comment

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

Validate bridge base address hex. Verify: (1) Both 0x-prefixed and raw hex are handled correctly. (2) Invalid addresses are rejected with 400, not 500. (3) Validation consistent with #5437 and #5471. - Xeophon (security review, bridge)

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 2f29110 into Scottcjn:main May 18, 2026
3 checks passed
Scottcjn added a commit that referenced this pull request Jun 2, 2026
* test(ci): fix 21 stale tests + add httpx dep — match merged hardening

main's pytest job was red (31 failed). These are tests lagging behind
intentionally-merged hardening PRs, not product regressions:
- tx-redaction (5): /tx/*,/wallet/* GETs admin-gated (#6295) — send X-Admin-Key
- gpu (3): /gpu/nodes admin-gated (#6557) — send X-Admin-Key
- bridge (8): RTC addr now RTC+40hex (#5435) stale fixtures; lock GETs admin-gated — auth
- signed_transfer (2): httpx test dep missing; /pending/confirm redacts to internal_error (#3202/#5546)
- proxy (1): proxy forwards headers= now — mock accepts it
- monitor (2): removed a bad-merge duplicate normalize_miners_payload; print_miners guards non-list

Verified: 130 passed across the 6 touched test files.
Remaining (separate follow-up, also stale-test not regression): governance (4, #6719
admin-gated vote + propose wallet-format) and otc confirm (1, mock serialization).

* test(ci): fix remaining 11 stale tests (governance, otc, beacon) — full suite green

Completes the CI repair (all 31 main pytest failures now fixed):
- governance (4): /governance/vote @admin_required (#6719) -> auth; propose now
  requires signed proposer auth before balance check -> sign + patch verify
- otc confirm (1): test mock's .json() returned a MagicMock -> serializable dict
- beacon_atlas (6): /api/contracts,/api/reputation,... admin-gated (#6322-era)
  -> set RC_ADMIN_KEY + inject X-Admin-Key via a test-client wrapper

All intended hardening with lagging tests, not regressions. 166 passed across
the 9 previously-failing files.

* test(ci): scope RC_ADMIN_KEY via monkeypatch — fix env leak into wallet_review

The redaction tests set os.environ["RC_ADMIN_KEY"] directly (no restore),
leaking into later test files (test_wallet_review_holds ran after and saw a
configured admin key -> 2 spurious failures). Use an autouse monkeypatch
fixture (auto-restored) instead. No behavior change to the redaction tests.

---------

Co-authored-by: Scott Boudreaux <scottbphone12@gmail.com>
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/XS PR: 1-10 lines tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants