Skip to content

fix: harden bridge confirmation thresholds#6524

Merged
Scottcjn merged 1 commit into
Scottcjn:mainfrom
sirakinb:codex/harden-bridge-confirmation-thresholds
May 28, 2026
Merged

fix: harden bridge confirmation thresholds#6524
Scottcjn merged 1 commit into
Scottcjn:mainfrom
sirakinb:codex/harden-bridge-confirmation-thresholds

Conversation

@sirakinb
Copy link
Copy Markdown
Contributor

Summary

Fixes #6521 by moving bridge confirmation threshold enforcement into update_external_confirmation() instead of relying only on the HTTP parser.

Changes

  • Adds a configurable BRIDGE_MAX_CONFIRMATIONS ceiling.
  • Rejects non-integer, negative, and over-ceiling confirmation counts in the core helper.
  • Rejects callback attempts to lower an existing required_confirmations threshold.
  • Adds regression tests for lowered thresholds and direct helper bypass of route parsing.

Verification

  • ./.venv/bin/python -m pytest -q tests/test_bridge_lock_ledger.py::TestIntegration::test_full_deposit_flow tests/test_bridge_lock_ledger.py::TestIntegration::test_external_confirmation_rejects_lowered_required_threshold tests/test_bridge_lock_ledger.py::TestIntegration::test_external_confirmation_helper_rejects_unbounded_counts -> 3 passed
  • PYTHONPYCACHEPREFIX=/private/tmp/rustchain-pycache python3 -m py_compile node/bridge_api.py tests/test_bridge_lock_ledger.py
  • git diff --check

Note: the full tests/test_bridge_lock_ledger.py suite currently has unrelated failures on current main around existing address/auth expectations; the targeted bridge confirmation regressions pass.

@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/M PR: 51-200 lines labels May 28, 2026
Copy link
Copy Markdown
Contributor

@eliasx45 eliasx45 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 this path. I like the direction of moving threshold checks into update_external_confirmation() because it protects direct helper callers, not just Flask requests, and the new helper-level regression for bypassing route parsing passes locally.

I found one blocker before this is ready, though: the HTTP callback route still clamps over-ceiling values before calling the hardened helper, so over-ceiling route input is not actually rejected. In register_bridge_routes(), both confirmations and required_confirmations are parsed with _parse_non_negative_int_arg(..., max_value=1000), and that parser uses min(parsed, max_value). That means a callback body with confirmations = BRIDGE_MAX_CONFIRMATIONS + 1 is normalized to 1000; the helper never sees the over-ceiling input and can complete the transfer.

Local reproduction on current head c5b23975b6ffa71527c976720c66ae8a74418c2e:

  • Created a deposit bridge transfer using the existing tests/test_bridge_lock_ledger.py fixture schema.
  • Posted to /api/bridge/update-external with valid X-API-Key, the transfer tx_hash, external_tx_hash, and confirmations = bridge_api.BRIDGE_MAX_CONFIRMATIONS + 1.
  • Actual result: HTTP 200 with {'external_confirmations': 1000, 'ok': True, 'required_confirmations': 12, 'status': 'completed', ...}.
  • Stored transfer result: status = completed, external_confirmations = 1000.

I expected the route to return 400 and leave the transfer pending, matching the new helper-level rejection. A route-level regression for oversized confirmations and oversized required_confirmations would catch this; alternatively the route parser could reject rather than clamp for this endpoint, or call the helper with the raw parsed integer so the new core guard remains authoritative.

Other validation performed:

  • C:\Users\home\Downloads\0-Elias\coprinter\Rustchain\.venv\Scripts\python.exe -m pytest -q tests/test_bridge_lock_ledger.py::TestIntegration::test_full_deposit_flow tests/test_bridge_lock_ledger.py::TestIntegration::test_external_confirmation_rejects_lowered_required_threshold tests/test_bridge_lock_ledger.py::TestIntegration::test_external_confirmation_helper_rejects_unbounded_counts -> 3 passed.
  • C:\Users\home\Downloads\0-Elias\coprinter\Rustchain\.venv\Scripts\python.exe -m py_compile node/bridge_api.py tests/test_bridge_lock_ledger.py -> passed.
  • git diff --check origin/main...HEAD -> clean.
  • gh pr checks 6524 -R Scottcjn/Rustchain -> test is failing; other listed checks pass.

Requesting changes so the public callback path enforces the same over-ceiling rejection as the helper path.

@Scottcjn Scottcjn merged commit 4cefe53 into Scottcjn:main May 28, 2026
11 of 12 checks passed
@Scottcjn
Copy link
Copy Markdown
Owner

Merged + paid 25 RTC (Bug Bounty Medium #2867). tx: f98d7da10150502c99115a6dedd7c4a8 — 24h void window applies.

Bridge confirmation hardening is timely — coincides with federation RFC #6522 and upstream proposal. Bridge custody primitives in node/bridge_api.py are stronger now.

Thanks @sirakinb.

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 tests Test suite changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bridge external confirmations can lower completion threshold

4 participants