fix(#5551): quick fix#5556
Conversation
github-actions
Bot
added
BCOS-L1
|
Security Review ✅ Quick fix follow-up to #5551 (OTC order validation). 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.
TJCurnutte
left a comment
TJCurnutte
left a comment
There was a problem hiding this comment.
Thanks for the quick follow-up. I checked this against head 2a5af2101828ccc0fa105110f267ecea94e736d8.
Validation run:
git diff --check origin/main...HEAD -- node/beacon_x402.py node/server_proxy.py otc-bridge/otc_bridge.pypython3 -B -m py_compile node/beacon_x402.py node/server_proxy.py otc-bridge/otc_bridge.py- Flask test-client probe for
POST /api/orderswith a tempOTC_DB_PATHafterinit_db():- JSON array body now returns
400 {'error': 'expected JSON object'}. ttl_seconds='abc'still returns a server500fromValueError: invalid literal for int() with base 10: 'abc'atttl = int(data.get("ttl_seconds", ORDER_TTL_DEFAULT)).ttl_seconds=1.5is accepted with201and writes an order.ttl_seconds=Trueis accepted with201and writes an order.
- JSON array body now returns
So the non-object body guard is good, but this does not fully close the ttl_seconds validation issue from #5551. The conversion still happens before typed validation, and int(...) both throws on bad strings and silently coerces bools/floats into a valid TTL. For this endpoint I would expect ttl_seconds to be absent or an actual integer value, with strings/floats/bools rejected as 400 before any order is created.
Also worth splitting if this PR is meant only to fix #5551: the same head includes Beacon x402 and server-proxy changes too, which makes the review/merge boundary less clear.
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 #5551(OTC order validation), but 2 of the 3 changed files are unrelated Beacon x402 / server proxy changes. Those hunks appear to duplicate the #5549/#5554 work and will add merge noise for an OTC-only fix. Please narrow this PR tootc-bridge/otc_bridge.py, or retitle and re-scope it so reviewers know it is intentionally touching three different issues. - The OTC change adds a non-object body guard, but there is no regression test for the reported failure mode (
[], scalar JSON, invalid/hugettl_seconds). Without that test coverage, the PR can still miss the #5551 acceptance path even if the one-line guard is correct. - CI is currently red on the global pytest collection because required test dependencies are missing (
aiohttp,flask_cors,matplotlib). That looks repository-wide rather than caused by this patch, but the PR should either document the unrelated failure or re-run once the dependency fix lands.
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. 🚀
|
Hi @TJCurnutte, could you take a look at this PR when you have a moment? Thanks! |
7 participants
Fixes #5551