fix(#5550): quick fix#5555
Conversation
github-actions
Bot
added
BCOS-L1
|
Security Review ✅ Quick fix follow-up to #5550 (server proxy error masking). Minor correction. Reviewed by Xeophon - Solana: Lt9nERv6VHsojw15LpFeiaabuphAggzfLF9sM9UXRrZ |
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Thanks for the contribution.
kekehanshujun
left a comment
kekehanshujun
left a comment
There was a problem hiding this comment.
I reviewed this as part of the #73 code review bounty.
Findings:
- Scope mismatch: the PR says
Fixes #5550(server proxy redaction), but it also changesnode/beacon_x402.pyandotc-bridge/otc_bridge.py. Those hunks are unrelated to the server proxy issue and overlap with the #5549/#5554/#5556 changes, so this PR is likely to create avoidable conflicts and an unclear review surface. Please keep this PR tonode/server_proxy.pyor split the unrelated changes. - The server proxy redaction itself needs a focused regression test. A small test can monkeypatch
requests.getto raise an exception containing an internal URL/path and assert/proxy/...returnsLocal server unavailablewithout echoing the exception text. - The new logging call keeps useful diagnostics, but if upstream exceptions can include full URLs with query strings, logs may still capture secrets. Consider logging exception type plus route/path separately, or sanitizing query strings before writing the message.
I received RTC compensation for this review.
jaxint
left a comment
jaxint
left a comment
There was a problem hiding this comment.
LGTM! Great work on this PR. 🚀
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Thanks for tightening these follow-up paths. I can verify the proxy redaction, missing-contracts fallback, and OTC non-object guard, but I cannot approve this yet because the beacon relay wallet fallback still throws on sqlite3.Row.
Validation run on head 2a5af2101828ccc0fa105110f267ecea94e736d8:
git diff --check origin/main...HEAD -- node/beacon_x402.py node/server_proxy.py otc-bridge/otc_bridge.py/private/tmp/rustchain-review-5555-venv/bin/python -B -m py_compile node/beacon_x402.py node/server_proxy.py otc-bridge/otc_bridge.pyPYTHONPATH=. /private/tmp/rustchain-review-5555-venv/bin/python -B -m pytest -q -o addopts='' --noconftest tests/test_beacon_x402_wallet.py tests/test_otc_bridge_query_validation.py --tb=short→12 passed in 0.23s- Temp Flask/SQLite probes:
/api/premium/contracts/exportwith nocontractstable returned200and{'total': 0, 'contracts': []}.node.server_proxywithrequests.getraisingRuntimeError('SECRET_UPSTREAM_DETAIL_SHOULD_NOT_LEAK')returned502and{'error': 'Local server unavailable'}.POST /api/orderswith a top-level JSON array returned400and{'error': 'expected JSON object'}afterinit_db().GET /api/agents/relay1/walletwith arelay_agentsrow still returned500.
Blocking issue:
node/beacon_x402.py:257 still does relay.get("coinbase_address"), but the app opens SQLite with conn.row_factory = sqlite3.Row, and sqlite3.Row has no .get(). The runtime probe hit:
AttributeError: 'sqlite3.Row' object has no attribute 'get'
File "node/beacon_x402.py", line 257, in get_agent_wallet
if relay and relay.get("coinbase_address"):
Please convert the relay row before checking it (relay_dict = dict(relay)) or index it like the native-wallet path, and add a regression for /api/agents/<id>/wallet resolving a wallet from relay_agents. That fallback is still a 500 for a valid relay wallet row, so I am requesting changes.
|
Hi @TJCurnutte, could you take a look at this PR when you have a moment? Thanks! |
7 participants
Fixes #5550