fix: redact tx handler internal errors#5546
Conversation
github-actions
Bot
added
BCOS-L1
|
CI note after opening PR: The blocking
Local targeted validation for this PR still passes:
|
LubuSeb
left a comment
LubuSeb
left a comment
There was a problem hiding this comment.
Reviewed PR #5546 at head 1b89b1c802af51933b3805925c7a9be194e11d34.
Validation performed:
git diff --check origin/main...HEAD -- node/rustchain_tx_handler.py tests/test_tx_handler_error_redaction.py-> passedpython -B -m py_compile node/rustchain_tx_handler.py tests/test_tx_handler_error_redaction.py-> passed- Windows focused redaction test in isolated venv:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=node:. python -B -m pytest -q -o addopts='' tests/test_tx_handler_error_redaction.py --tb=short-> 5 passed - WSL/Linux-style focused suite in isolated venv:
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=node:. python -B -m pytest -q -o addopts='' tests/test_tx_handler_error_redaction.py tests/test_tx_handler_limits.py tests/test_tx_submit_route.py --tb=short-> 22 passed - Checked the failing GitHub Actions
testjob: it fails during repository-wide collection becauseaiohttp,flask_cors, andmatplotlibare missing in CI, before reaching this PR's focused transaction-handler tests.
The patch removes raw str(e) responses from the transaction API catch-all paths and replaces them with a stable {"error": "internal_error"} response while preserving server-side exception logging. The new regression covers /tx/status, /tx/pending, /wallet/<address>/balance, /wallet/<address>/nonce, and /wallet/<address>/history with synthetic database path/table-name leakage. I also checked that the existing /tx/submit malformed/non-object JSON tests still pass in the focused WSL run. No blocker found in the scoped redaction change.
Reviewer wallet/miner ID for bounty #73: RTC4a92ce8abb537f827b8b0dac456242f8697c1ad6
508704820
left a comment
508704820
left a comment
There was a problem hiding this comment.
Redact tx handler internal errors. SECURITY: Transaction handlers process financial data — internal errors can leak wallet addresses, tx hashes, and internal state. Verify: all error paths in tx handling are covered. — Xeophon (security review)
508704820
left a comment
508704820
left a comment
There was a problem hiding this comment.
Redact tx handler internal errors. Good - transaction handler errors can expose internal state, wallet details, and protocol information. Verify: (1) All transaction types are covered. (2) Redaction happens before logging. (3) No error response reveals transaction amounts or wallet addresses. - Xeophon (security review)
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Reviewed head 1b89b1c802af51933b3805925c7a9be194e11d34.
I approve this one. The public API surface now returns a stable {"error":"internal_error"} for unexpected transaction-route failures instead of reflecting exception text, while still logging the traceback server-side for operators.
Validation I ran:
git diff --check origin/main...HEAD -- node/rustchain_tx_handler.py tests/test_tx_handler_error_redaction.py
python3 -B -m py_compile node/rustchain_tx_handler.py tests/test_tx_handler_error_redaction.py
PYTHONPATH=. uv run --no-project --with flask --with pytest python -B -m pytest -q -o addopts='' tests/test_tx_handler_error_redaction.pyResult: 5 passed in 0.04s.
I also ran a Flask test-client probe with the repo's tests.mock_crypto shim installed as rustchain_crypto, an exploding pool whose exception included sqlite password=secret at /var/prod/rustchain.db pending_transactions, and a monkeypatched sqlite connect failure for history. /tx/status/h, /tx/pending, /wallet/alice/balance, /wallet/alice/nonce, and /wallet/alice/history all returned HTTP 500 with exactly {"error":"internal_error"} and the probe reported leak_present=False for the secret/path/table markers.
No blocking issues found.
Code ReviewPR: fix: redact tx handler internal errors by @ZHOUEU-star
Wallet: Reviewing under Bounty #73 |
kekehanshujun
left a comment
kekehanshujun
left a comment
There was a problem hiding this comment.
Reviewing this as part of the RustChain code review bounty.
The change is focused and addresses the vulnerable pattern consistently across the transaction API routes that previously returned str(e) to callers. The new helper uses logger.exception(...), so operators keep stack context while clients get a stable {error:internal_error} response.
The regression tests cover the read/status-style routes plus wallet history, and they explicitly assert that SQLite table names and filesystem paths are not reflected in the response body. I do not see a blocking issue in this patch. One small follow-up worth considering is a targeted test for the /tx/submit exception path too, since that route was also converted, but the implementation there uses the same helper and the current diff is coherent.
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
iamdinhthuan
left a comment
iamdinhthuan
left a comment
There was a problem hiding this comment.
Reviewed head 1b89b1c802af51933b3805925c7a9be194e11d34.
I rechecked the tx-handler error redaction patch and approve it.
Validation performed:
git diff --check origin/main...HEAD -- node/rustchain_tx_handler.py tests/test_tx_handler_error_redaction.pypassed.PYTHONPATH=node:. python3 -B -m py_compile node/rustchain_tx_handler.py tests/test_tx_handler_error_redaction.pypassed.PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=node:. python3 -B -m pytest -q -o addopts='' tests/test_tx_handler_error_redaction.py --tb=shortpassed with5 passed.PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 PYTHONPATH=node:. python3 -B -m pytest -q -o addopts='' tests/test_tx_handler_error_redaction.py tests/test_tx_submit_route.py --tb=shortpassed with7 passed.python3 tools/bcos_spdx_check.py --base-ref origin/mainpassed withBCOS SPDX check: OK.- I also directly probed the converted
/tx/submitexception path by monkeypatchingSignedTransaction.from_dictto raise an injected internal string containingpending_transactionsand/srv/rustchain/prod.db. The route returned status500with{"error":"internal_error"}, and the response body checks reportedleak_in_body Falseandpath_in_body False.
The implementation is coherent: every converted handler now returns the same stable public internal_error response while logger.exception(...) keeps the traceback available server-side. The added regression tests cover the status, pending, wallet balance, wallet nonce, and wallet history paths; the direct submit probe confirms the same helper also protects the /tx/submit path that was changed in this PR.
No blocking issues found.
PR Review — PR #5546Title: fix: redact explorer upstream errors Author: ZHOUEU-star What I reviewed
Observations
LGTM pending CI. Good security fix with comprehensive test coverage. Proper error handling prevents information disclosure. Bounty: #2782 |
* 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>
11 participants
Summary
node/rustchain_tx_handler.py500 responses.logger.exception(...).Related
RTC4e9b755109fc7b898eaebbd20ad665d9855449eb/ZHOUEU-star.Validation
/tmp/rustchain-bug-venv/bin/python -m pytest tests/test_tx_handler_error_redaction.py tests/test_tx_handler_limits.py tests/test_tx_submit_route.py -q-> 22 passed, 1 urllib3 LibreSSL warning/tmp/rustchain-bug-venv/bin/python -m py_compile node/rustchain_tx_handler.py tests/test_tx_handler_error_redaction.py tests/test_tx_handler_limits.py tests/test_tx_submit_route.py-> passedgit diff --check-> passed