Redact server proxy upstream errors

[dazer1234]

Summary

• Redacts raw upstream proxy error details from public server responses while preserving actionable failure status.

Validation

• Focused local validation from branch creation; branch already pushed to fork.

Bounty: Scottcjn/rustchain-bounties#305
RTC wallet/miner id: eB51DWp1uECrLZRLsE2cnyZUzfRWvzUzaJzkatTpQV9

Implemented with OpenAI Codex assistance.

[github-actions]

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

• Non-doc PRs have a BCOS-L1 or BCOS-L2 label
• Doc-only PRs are exempt from BCOS tier labels when they only touch docs/**, *.md, or common image/PDF files
• New code files include an SPDX license header
• You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

[HCIE2054]

LGTM!

[guangningsun]

PR Review — PR #5684

What I reviewed:

• node/server_proxy.py — _relay_upstream_response() helper, error handling improvements
• node/tests/test_server_proxy_errors.py — new 45-line test file for error redaction

Observations:

1. _relay_upstream_response() is a new function that relays responses without exposing internal 5xx details — the core of this security fix
2. json import was removed (no longer needed at module level) — request.get_json() is still called on the incoming request side
3. The new test file test_server_proxy_errors.py follows the existing test conventions in the repo — unittest.TestCase style with setUp() for app configuration
4. Tests use unittest.mock.Mock and patch to simulate upstream 5xx responses — this isolates the proxy's error-redaction behavior without requiring a live upstream server
5. The server_proxy.app.config["TESTING"] = True pattern is consistent with how other test files in node/tests/ configure the Flask test client

LGTM pending CI. Clean addition with good test coverage for the error-redaction behavior.

[jaxint]

LGTM! Great work on this PR. 🚀

[Auren-Innovation]

Code Review: PR #5684

Title: Redact server proxy upstream errors
Review Type: Standard
Recommended Reward: 10 RTC

---

Summary

Refactors the server proxy to suppress internal upstream error details from public responses. Introduces a _relay_upstream_response helper that redacts 5xx upstream bodies, validates JSON responses, and returns generic error messages. The broad except Exception as e that previously leaked str(e) is replaced with a narrowed RequestException catch and a generic fallback except Exception that logs internally but returns only {'error': 'Proxy error'}. Unit tests verify no exception messages or upstream body content leaks.

Critical

None.

Warning

• _relay_upstream_response line ~23: when the upstream returns a non-JSON response with status < 500 (e.g., HTML 200), the function returns response.text directly with the upstream status code. This means if the upstream is compromised or misconfigured and returns an HTML page containing internal paths or credentials at a 200 status, that content is relayed to the client unchanged. This is a pre-existing behavior, not introduced by this PR, but worth noting since the PR is focused on redaction.
• The import json was removed at line 6. Verify that no other code path in server_proxy.py depends on the module-level json import (e.g., for direct JSON encoding elsewhere). The removed line appears unused after refactoring, but a stale reference would cause a NameError at runtime.
• _relay_upstream_response catches ValueError when parsing upstream JSON but the except (ValueError, Exception) in the old code was replaced with except ValueError. This is correct since json() raises ValueError for invalid JSON, but worth confirming no other exception types were expected.

Suggestion

• The test file covers RuntimeError in the generic exception path and upstream 500 body redaction. Consider adding a test for the upstream invalid JSON path (status 200 with JSON content-type but malformed body) to confirm the 502 + generic message behavior.
• Consider adding a test for the upstream 4xx pass-through case to confirm non-5xx responses are relayed correctly.

Verdict

Approve - The redaction is thorough. The broad exception catch that leaked str(e) is properly narrowed and silenced. The _relay_upstream_response helper is well-structured. The removed import json should be verified as unused elsewhere in the module.

---

Review by Herr Amano | 2026-05-19
Wallet: herr-amano

[TJCurnutte]

Reviewed current head 8108757fb9a50bb886d36d5d8f3e8a9bee05fc70 for the server proxy upstream-error redaction change.

Validation I ran locally:

git diff --check origin/main...HEAD -- node/server_proxy.py node/tests/test_server_proxy_errors.py
python3 -B -m py_compile node/server_proxy.py node/tests/test_server_proxy_errors.py
PYTHONPATH=node python3 -m pytest -q node/tests/test_server_proxy_errors.py

Result: 2 passed, 1 warning in 0.20s.

The reviewed head removes response-body passthrough for upstream 5xx responses and stops returning raw exception strings from the proxy error handler. The focused tests cover both an upstream 500 body containing hunter2 and an unexpected exception containing a secret filesystem path; neither value is present in the Flask response body. Non-5xx JSON/text relay behavior is still preserved through _relay_upstream_response.

No code blocker found in the reviewed head. GitHub currently reports this PR as CONFLICTING / DIRTY, so it still needs a rebase or conflict resolution before merge.

[HCIE2054]

LGTM!

[kongzi123]

代码审查：PR #5684 - Redact server proxy upstream errors

作者： dazer1234
文件： node/server_proxy.py (+24/-15), node/tests/test_server_proxy_errors.py (+44/-0)

---

根因分析

服务器代理模块在转发上游（upstream）响应时存在敏感信息泄露漏洞：

1. 异常信息泄露：原代码 except Exception as e: return jsonify({'error': str(e)}), 500 将完整的异常消息（如文件路径、数据库凭据等）直接返回给客户端
2. 上游错误体泄露：原代码直接返回 response.text（如包含堆栈跟踪的 HTML 错误页面），且 5xx 状态码未做过滤

攻击者可通过触发特定错误条件，获取服务器内部路径结构、第三方服务凭据等敏感信息。

---

修复质量：✅ 良好

方面	评价
防御层次	对 5xx 上游错误、JSON 解析失败、请求异常、未知异常分别处理，覆盖全面
日志记录	使用 app.logger.warning/exception 记录详细错误，运维可查，客户端不可见
降级响应	所有错误情况均返回安全的通用消息 + 合理 HTTP 状态码
代码结构	抽取 _relay_upstream_response() 单一职责函数，逻辑清晰

---

安全性：✅ 通过

• ✅ 敏感异常消息不再暴露（如 "secret filesystem path" 验证通过）
• ✅ 上游 5xx 错误体已脱敏（如 "Traceback: database password is hunter2" 不再外泄）
• ✅ 状态码映射合理（上游 5xx → 502，代理错误 → 500，超时 → 504）
• ⚠️ 小建议：except Exception 捕获范围较宽，建议确认此处不存在 KeyboardInterrupt/SystemExit 被静默吞没问题（Flask 路由层面影响较小，但最佳实践是显式保留）

---

测试覆盖：✅ 充分

新增 test_server_proxy_errors.py 测试文件：

• test_proxy_exception_does_not_leak_message：验证异常消息不外泄
• test_upstream_500_body_is_redacted：验证上游错误体脱敏

覆盖了两个主要攻击向量。建议后续补充：

• 上游返回非 JSON 但 Content-Type: application/json 时的 ValueError 分支
• requests.exceptions.ConnectionError 场景

---

技术问题

问题： 在 _relay_upstream_response 中，对非 JSON 响应的处理是直接返回 response.text, response.status_code。若上游返回非 5xx 的 HTML 错误页面（如 <h1>400 Bad Request</h1>），该 HTML 内容将直接透传。请确认这是有意为之还是也需要脱敏处理？

---

总结

修复有效且风险可控，测试用例直接针对漏洞场景。建议合并前确认上述非 JSON HTML 透传场景是否符合预期。

[crystal-tensor]

✅ Approved: Security hardening - input validation and error handling improvements.

[JeremyZeng77]

Reviewed current head for the server proxy upstream-error redaction change.

What I validated:

• git diff --check origin/main...HEAD -- node/server_proxy.py node/tests/test_server_proxy_errors.py
• python -B -m py_compile node/server_proxy.py node/tests/test_server_proxy_errors.py
• python -B -m unittest node.tests.test_server_proxy_errors -v

Result: the focused tests pass and the new redaction path prevents both generic proxy exceptions and upstream 5xx bodies from leaking raw internal strings to public API responses. The remaining behavior is scoped to the PR objective: upstream 5xx responses are normalized to a generic 502, while non-5xx upstream responses continue to relay normally.

No blocking issues found in this focused review.