fix: hide server proxy upstream errors#5545
Conversation
github-actions
Bot
added
BCOS-L1
508704820
left a comment
508704820
left a comment
There was a problem hiding this comment.
Hide server proxy upstream errors. SECURITY: Good hardening — proxy upstream details can reveal internal infrastructure. Verify: all proxy error paths are covered, not just this one. — Xeophon (security review)
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Approved. I focused on the changed server-proxy exception path and its regression coverage.
Validation performed on head c5025b0319da91f05e0c52cec48e2bc022c22b57:
git diff --check origin/main...HEAD -- node/server_proxy.py tests/test_server_proxy_path.pypassed.python3 -B -m py_compile node/server_proxy.py tests/test_server_proxy_path.pypassed.PYTHONPATH=. uv run --no-project --with pytest --with flask --with requests python -B -m pytest -q -o addopts='' tests/test_server_proxy_path.pypassed with5 passed in 0.04s.- A focused Flask runtime probe monkeypatched
requests.get()to raise a fake upstream exception containing host/token/path sentinels;GET /api/minersreturned HTTP502with exactly{ "error": "Local server unavailable" }, and the sentinel host/token/path strings were not reflected in the response body.
The previous catch-all returned str(e) to the client, which could expose local hostnames, tokens, or filesystem paths from upstream request failures. This patch keeps the timeout behavior separate, logs the unexpected exception server-side, and returns a generic 502 for the public proxy response. The existing path-normalization tests plus the new redaction regression test cover the behavior I would expect for this narrow security fix.
|
Security Review ✅ CRITICAL FIX This fixes an information disclosure vulnerability. The previous code returned str(e) in the 500 response, which could leak:
The test case demonstrates this perfectly - a RuntimeError containing token and db path would have been sent directly to the client. Fix is correct:
Reviewed by Xeophon - Solana: Lt9nERv6VHsojw15LpFeiaabuphAggzfLF9sM9UXRrZ |
508704820
left a comment
508704820
left a comment
There was a problem hiding this comment.
Hide server proxy upstream errors. Good - server proxy errors can reveal internal routing, service URLs, and backend topology. Verify: (1) All proxy error paths are covered. (2) Error messages are generic for external consumers. (3) Internal logging still captures full error details for debugging. - Xeophon (security review)
LubuSeb
left a comment
LubuSeb
left a comment
There was a problem hiding this comment.
I found a remaining information-disclosure path in this proxy hardening change. The new handler hides Python exceptions raised by requests.get() / requests.post(), but once the local upstream returns a response object the proxy still reflects upstream error bodies verbatim.
Two cases still leak sensitive upstream detail to the public proxy client:
- non-JSON upstream errors go through
return response.text, response.status_code Content-Type: application/jsonresponses that fail JSON parsing also fall back toreturn response.text, response.status_code
That means a local server 500 text/html stack page, or a broken JSON error payload containing an internal token/path, is still exposed even though this PR is meant to hide server proxy upstream errors.
Validation on head c5025b0319da91f05e0c52cec48e2bc022c22b57:
git diff --check origin/main...HEAD -- node/server_proxy.py tests/test_server_proxy_path.pypassed.python -B -m py_compile node/server_proxy.py tests/test_server_proxy_path.pypassed in a Flask/requests review venv.PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -B -m pytest -q --noconftest -o addopts='' tests/test_server_proxy_path.pypassed with5 passed.- Focused probe monkeypatching
requests.get()to returnstatus_code = 500,Content-Type: text/html, and bodytrace token=super-secret path=/srv/rustchain/private.db host=127.0.0.1returned HTTP500to/api/minerswith all three sentinels still present in the client-visible body. - A second probe with
Content-Type: application/json, invalid JSON parsing, andtext = '{"error":"token=super-secret path=/srv/rustchain/private.db"}'returned the raw error body with the token/path still present.
For this security fix to hold, upstream 5xx responses and parse-failure paths should return a generic client error while logging the upstream detail server-side. If legitimate 2xx text endpoints must remain proxied, the sanitization can be limited to error statuses and invalid JSON/error content.
|
Thanks for catching the remaining upstream-body leak. I pushed 66128cc to redact upstream 5xx bodies and invalid JSON response bodies behind the same generic proxy error, while logging the upstream detail server-side. Validation run:
|
github-actions
Bot
added
size/M
Code ReviewPR: fix: hide server proxy upstream 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.
This fixes the exact exception-reflection case from #5544: unexpected upstream request failures now log server-side and return a generic JSON 502, with regression coverage for 127.0.0.1, token-like text, and private DB paths.
The second commit also improves the higher-risk case where the local node returns a 5xx body or malformed JSON; those bodies are now replaced with Local server unavailable, which is the right public-proxy behavior for upstream failures.
Potential non-blocking follow-up: non-JSON upstream responses with 4xx status are still proxied verbatim. That is probably acceptable for the narrow bug report, but if the local node ever returns diagnostic HTML for 400/404 responses, the proxy could still reflect internal text. I would consider redacting all non-JSON non-2xx responses in a separate hardening PR.
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 66128cc1859d486557b397c380c5e888202b7df8.
I approve this current revision. The first commit fixed the requests.get() / requests.post() exception-reflection path, and the current head also covers the follow-up upstream response cases: local 5xx bodies and invalid JSON bodies are replaced with a generic public 502 while the detail remains server-side logging.
Validation I ran:
git diff --check origin/main...HEAD -- node/server_proxy.py tests/test_server_proxy_path.py
python3 -B -m py_compile node/server_proxy.py tests/test_server_proxy_path.py
PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python3 -B -m pytest -q --noconftest -o addopts='' tests/test_server_proxy_path.py
# 7 passed
python3 tools/bcos_spdx_check.py --base-ref origin/main
# BCOS SPDX check: OK
I also ran a direct Flask test-client probe with mocked upstream calls and a sentinel string token=super-secret path=/srv/rustchain/private.db host=127.0.0.1:
exception -> status=502 body='{"error":"Local server unavailable"}' leaked=False
html_500 -> status=502 body='{"error":"Local server unavailable"}' leaked=False
invalid_json_200 -> status=502 body='{"error":"Local server unavailable"}' leaked=False
text_200 -> status=200 body='plain ok' leaked=False
json_200 -> status=200 body='{"ok":true,"source":"upstream"}' leaked=False
That covers the previous blocker I would have had on the earlier head: server-side upstream diagnostics are still logged, but exception text, local 5xx bodies, and invalid JSON error bodies are no longer reflected to proxy clients. Normal 2xx text and JSON proxy behavior remains intact. No blocking issue found in this scoped fix.
BossChaos
left a comment
BossChaos
left a comment
There was a problem hiding this comment.
PR #5545 Review - Proxy upstream error hiding (502/504)
Security Analysis
This PR fixes information leakage in the RustChain proxy.
Key improvements:
- _generic_upstream_error(): Server-side logging, client gets generic error
- Status 500+ handling: Upstream server errors now hidden
- JSON parse failure: ValueError now returns 502 instead of leaking error
- Exception handling: Generic exception no longer exposes details
Recommendation: Merge - proxy security hardening.
10 participants
Summary
app.logger.exception(...)for operators.Fixes #5544
Bounty: #305
Wallet:
RTC4e9b755109fc7b898eaebbd20ad665d9855449ebBCOS tier: BCOS-L1
Validation
/tmp/rustchain-bug-venv/bin/python -m pytest tests/test_server_proxy_path.py -q5 passed, 1 warning in 0.05s/tmp/rustchain-bug-venv/bin/python -m py_compile node/server_proxy.py tests/test_server_proxy_path.pygit diff --check -- node/server_proxy.py tests/test_server_proxy_path.py