fix(#5442): hide internal exception details in dashboard health endpoint#5470
fix(#5442): hide internal exception details in dashboard health endpoint#5470weilixiong wants to merge 1 commit into
Conversation
github-actions
Bot
added
the
BCOS-L1
|
@Scottcjn New error leak fix ready for review. Same pattern as the other PRs. |
kekehanshujun
left a comment
kekehanshujun
left a comment
There was a problem hiding this comment.
LGTM. I verified the health endpoint no longer returns raw exception text for the database, Solana RPC, or mint-account checks, while preserving the existing health response shape and component status behavior.
Validation run on the PR diff:
python -m py_compile bridge/dashboard_api.pypython -m pytest bridge/test_dashboard_api.py -q --tb=short-> 22 passed
I also confirmed the three previous str(e) detail paths now return generic unavailable messages.
ZacharyZhang-NY
left a comment
ZacharyZhang-NY
left a comment
There was a problem hiding this comment.
Requesting changes on head 788e3719867cc24d59eb5185a7c0fdc055613ad1.
Validation performed:
git diff --check origin/main...review-pr-5470 -- bridge/dashboard_api.pypython -B -m py_compile bridge/dashboard_api.py bridge/test_dashboard_api.pypython -B -m pytest -q bridge/test_dashboard_api.py --confcutdir=<temp>-> 22 passed- source check for
logger.exception,logging.getLogger, and generic health detail strings
The client-facing redaction part is moving in the right direction, but the linked #5442 report explicitly calls for regression tests for the three leak paths and server-side logging of the original exceptions. This PR only changes the response strings in bridge/dashboard_api.py; it adds no coverage for the database/RPC/mint leak cases and leaves no server-side logger.exception(...) path for operators.
Please add focused tests that force failures for the database check, Solana RPC check, and mint check, assert that secret-bearing exception text is absent from the response, and log the caught exceptions server-side while returning generic details to clients.
Code Review — Bounty #73PR: fix(#5442): hide internal exception details in dashboard health endpoint by @weilixiong
SummaryThis is a bug fix PR. Changes appear consistent with project patterns. Wallet: Reviewing under Bounty #73 — Code Review Bounty Program |
|
Thanks for the review @BossChaos! Will address any suggestions. |
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Approved. I reviewed the dashboard health exception handling in bridge/dashboard_api.py.
Validation proof:
git diff --check origin/main...HEAD -- bridge/dashboard_api.py
PYTHONPYCACHEPREFIX=/tmp/rustchain-pycache-5470 python3 -B -m py_compile bridge/dashboard_api.py
PYTHONPATH=. uv run --no-project --with pytest --with flask --with flask-cors --with requests python -B -m pytest -q -o addopts='' bridge/test_dashboard_api.py --noconftest -p no:cacheproviderResults: git diff --check passed, py_compile passed, and the focused dashboard API suite passed with 22 passed in 1.75s.
I also ran a Flask test-client probe for /bridge/dashboard/health with get_db() forced to raise a private SQLite/path/password string and urllib.request.urlopen() forced to raise a private Solana RPC/token/keypair string, with WRTC_MINT_ADDRESS configured so the mint-check branch executed too. The response stayed HTTP 200/degraded and returned only the generic detail values:
{
"rustchain": "Database error: unavailable",
"solana_rpc": "RPC error: unavailable",
"wrtc_mint": "Mint check error: unavailable"
}The response body did not reflect the injected super-secret, private paths, keypair filename, or RPC URL. That covers all three changed exception paths while preserving the health endpoint contract.
508704820
left a comment
508704820
left a comment
There was a problem hiding this comment.
Hide internal exception details in dashboard. Good - dashboard errors often leak DB queries, internal IPs, and service versions. Verify all dashboard error handlers follow this pattern. - Xeophon (security review)
|
Closing as duplicate of #5443 by @hungle123-dev (first-posted 4h earlier on the same issue). Per first-poster rule, only the original submitter gets bounty credit on a given issue. The fix in #5443 addresses the same root cause and is already under review. Appreciate the effort — for next time, please check open PRs against the same issue before filing. Thanks for the contribution. |
8 participants
Fix #5442\n\nReplaced
str(e)with generic "unavailable" messages in dashboard health endpoint.\n\nFiles: bridge/dashboard_api.py (3 occurrences)