fix: oracle response-body bound, RLP length overflow guard, metrics port validation - #1033
Closed
curryxbo wants to merge 6 commits into
Closed
fix: oracle response-body bound, RLP length overflow guard, metrics port validation#1033curryxbo wants to merge 6 commits into
curryxbo wants to merge 6 commits into
Conversation
getJSONWithHeaders read price responses with an unbounded io.ReadAll, so a compromised or misbehaving CEX/Hermes endpoint could stream an arbitrarily large body and exhaust memory. Cap the read at 1 MiB via io.LimitReader (ticker and Hermes latest-price payloads are a few KB), rejecting anything larger. Covers the Binance, OKX and Pyth paths that share this helper. Co-authored-by: Cursor <cursoragent@cursor.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
Claude Code Review is paused for this repository. To reconnect it, an admin of this repository's GitHub organization (or the account owner, for personal repositories) who can also manage your Claude organization's Code Review settings needs to re-link GitHub in Code Review settings. This is a one-time step.
Tip: disable this comment in your organization's Code Review settings.
Contributor
📝 WalkthroughWalkthroughThe price client rejects HTTP bodies larger than 1 MiB. Tests cover oversized and boundary responses. Transaction-buffer length arithmetic now uses ChangesResponse-body limits
Transaction buffer sizing
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@token-price-oracle/client/cex_feed_test.go`:
- Around line 118-133: Update TestGetJSONAcceptsBodyAtLimit so the valid JSON
payload is padded with trailing whitespace using strings.Repeat until its length
is exactly maxResponseBodyBytes; add the strings import if needed, while
preserving the existing request and body-equality assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b211c62c-f57d-4427-a23d-9a1bf1c21370
📒 Files selected for processing (2)
token-price-oracle/client/cex_feed.gotoken-price-oracle/client/cex_feed_test.go
Comment on lines
+118
to
+133
| func TestGetJSONAcceptsBodyAtLimit(t *testing.T) { | ||
| payload := `{"symbol":"BTCUSDT","price":"64385.12"}` | ||
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Write([]byte(payload)) | ||
| })) | ||
| defer server.Close() | ||
|
|
||
| body, err := getJSON(context.Background(), server.Client(), server.URL) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if string(body) != payload { | ||
| t.Fatalf("body = %q, want %q", string(body), payload) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the boundary test send exactly maxResponseBodyBytes bytes.
The current payload is much smaller than the limit. This test does not verify the exact boundary and would pass even if boundary handling were incorrect. Pad the valid JSON with trailing whitespace to reach the configured byte limit.
Proposed fix
- payload := `{"symbol":"BTCUSDT","price":"64385.12"}`
+ payload := `{"symbol":"BTCUSDT","price":"64385.12"}`
+ payload += strings.Repeat(" ", maxResponseBodyBytes-len(payload))Add "strings" to the import block if needed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestGetJSONAcceptsBodyAtLimit(t *testing.T) { | |
| payload := `{"symbol":"BTCUSDT","price":"64385.12"}` | |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| w.Header().Set("Content-Type", "application/json") | |
| w.Write([]byte(payload)) | |
| })) | |
| defer server.Close() | |
| body, err := getJSON(context.Background(), server.Client(), server.URL) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if string(body) != payload { | |
| t.Fatalf("body = %q, want %q", string(body), payload) | |
| } | |
| } | |
| func TestGetJSONAcceptsBodyAtLimit(t *testing.T) { | |
| payload := `{"symbol":"BTCUSDT","price":"64385.12"}` | |
| payload += strings.Repeat(" ", maxResponseBodyBytes-len(payload)) | |
| server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| w.Header().Set("Content-Type", "application/json") | |
| w.Write([]byte(payload)) | |
| })) | |
| defer server.Close() | |
| body, err := getJSON(context.Background(), server.Client(), server.URL) | |
| if err != nil { | |
| t.Fatal(err) | |
| } | |
| if string(body) != payload { | |
| t.Fatalf("body = %q, want %q", string(body), payload) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@token-price-oracle/client/cex_feed_test.go` around lines 118 - 133, Update
TestGetJSONAcceptsBodyAtLimit so the valid JSON payload is padded with trailing
whitespace using strings.Repeat until its length is exactly
maxResponseBodyBytes; add the strings import if needed, while preserving the
existing request and body-equality assertions.
extractInnerTxFullBytes sized the full-tx buffer with 1+uint32(sizeByteLen)+size, which wraps when the declared RLP size is near MaxUint32 (e.g. a 0xffffffff length prefix), producing a buffer shorter than the slice copies below and panicking. The remaining-length guard added in #1028 already bounds size to the available input on the batch-decode path, so this only wraps for an out-of-band reader; computing the length in uint64 removes the panic unconditionally as defense in depth. Co-authored-by: Cursor <cursoragent@cursor.com>
curryxbo
changed the title
curryxbo
changed the title
The previous change widened the buffer arithmetic to uint64, which removes the overflow panic only because size happens to be a uint32; it leaves the real risk untouched. The declared size still drives make([]byte, size), and the remaining-length guard that should cap it was conditional -- skipped entirely when the reader does not expose Len(). Make the bound explicit and fail-closed instead: require a reader that reports its remaining length (every decode path funnels through DecodeTxsFromBytes with a *bytes.Reader) and reject a declared size that exceeds it. With size bounded by the actual remaining input, the allocation can no longer be attacker-controlled and the length arithmetic is a small in-range value by construction, independent of the width of size's type. Co-authored-by: Cursor <cursoragent@cursor.com>
End-to-end analysis shows the uint32 length wrap at blob.go is unreachable: the compressed batch input is hard-capped by the L1 blobs-per-tx limit, the #1028 size>remaining guard already bounds the declared length to the decompressed stream, and reaching a ~4.29 GiB stream would OOM inside zstd decompression before the RLP decoder runs. The #1028 guard on main is the sufficient defense; the extra hardening addressed a condition the real data flow cannot produce, so revert it and keep this PR to the oracle fix. Co-authored-by: Cursor <cursoragent@cursor.com>
curryxbo
changed the title
size is a uint32, so 1+sizeByteLen+size can exceed MaxUint32 and wrap to a tiny buffer length, leaving fullTxBytes shorter than the copies that follow and panicking. The #1028 remaining-bytes guard keeps this unreachable on the current decode path, but compute the length in uint64 and reject the overflow before allocating so the decoder stays safe if the size type or the upstream length bounds ever change. Co-authored-by: Cursor <cursoragent@cursor.com>
curryxbo
changed the title
MetricsPort is a uint64 with no upper bound, so a misconfigured value (e.g. >65535) produced an invalid listen address whose metrics ListenAndServe failed silently in the background, logging only. Reject a port outside 1..65535 in SetCliContext so the misconfig fails at startup. The default (26660, matching Tendermint's instrumentation port) is always in range, so valid configs are unaffected. Co-authored-by: Cursor <cursoragent@cursor.com>
curryxbo
changed the title
Contributor
Author
|
Superseded by #1034, which regroups these review-driven hardening fixes under a properly scoped branch (not oracle-specific) with a clean one-commit-per-fix history. |
Merged
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Small, independent hardening fixes surfaced during code review, grouped into one PR.
1. Bound HTTP price response body (token-price-oracle)
getJSONWithHeadersread price responses with an unboundedio.ReadAll, so a compromised or misbehaving CEX/Hermes endpoint could stream an arbitrarily large body and exhaust memory (the 10s client timeout limits time, not size). Cap the read at 1 MiB viaio.LimitReaderand reject anything larger. The helper is shared by the Binance, OKX and Pyth paths, so one change covers all three; Chainlink is unaffected (on-chain RPC).2. Reject RLP tx length that overflows uint32 (common/batch)
In
extractInnerTxFullBytes,sizeis auint32, so1+sizeByteLen+sizecan exceedMaxUint32and wrap to a tiny buffer length, leavingfullTxBytesshorter than the copies that follow and panicking. Thesize > remainingguard from #1028 keeps this unreachable on the current decode path (the declared length is bounded by the decompressed stream, itself bounded upstream), so this is defensive: compute the length in uint64 and reject the overflow before allocating, keeping the decoder safe if the size type or upstream bounds ever change. No behavior change on valid input.3. Validate layer1 metrics port range (node)
MetricsPort(uint64) had no upper-bound check, so a misconfigured value (e.g. >65535) produced an invalid listen address whose metricsListenAndServefailed silently in the background. Reject a port outside1..65535inSetCliContextso the misconfig fails at startup. The default (26660, matching Tendermint's instrumentation port so layer1 validators stay consistent with other node types) is always in range, so valid configs are unaffected.Test plan
go build ./...,go vet ./client/,go test ./client/(addedTestGetJSONRejectsOversizedBody,TestGetJSONAcceptsBodyAtLimit)CGO_ENABLED=1 go build/vet/test ./batch/(existingTestExtractInnerTxFullBytes*pass)go build/vet/test ./derivation/(addedTestMetricsPort_AcceptsDefaultInLayer1,TestMetricsPort_RejectsOutOfRange)