fix(oracle): fail closed on a malformed set-price-cap body (GH#2509) - #2511
Merged
Conversation
`POST /api/oracle/set-price-cap` treats an EMPTY body as "apply to every
admin-oracle market". It implemented that by initialising `body = {}` and
swallowing every exception from `req.json()`:
let body: { slabAddress?: string; maxChangeE2bps?: number } = {};
try {
body = await req.json();
} catch {
// empty body is valid -- means "all admin-oracle markets"
}
So any parse failure left `body` as `{}` -- the exact state an empty body
produces. A truncated or malformed payload was therefore indistinguishable from
the deliberate all-market command: `body.slabAddress` is undefined, the route
takes the `else` branch at :155, selects every market whose `oracle_authority`
matches the crank pubkey (bounded only by MAX_SLAB_BATCH = 50), and submits one
signed transaction per validated market.
That is fail-OPEN scope expansion on an administrative write path -- the worse
the input, the broader the operation. It is reachable only after the admin
secret and crank-key checks pass, so this is not an auth bypass; it is an
authenticated operator's malformed request silently becoming a batch.
The fix reads the raw text once and branches on whether the caller actually
sent anything, so "no body" and "bad body" stop being the same state:
- empty / whitespace-only body -> unchanged, still the all-markets command
- non-empty body that fails JSON.parse -> 400
- non-empty body that parses to a NON-OBJECT -> 400
The third case matters and a try/catch alone does not cover it: `[]`, `"str"`,
`123` and `null` all parse successfully, and every field read on them yields
`undefined`, which reaches the same all-market branch. `typeof null === "object"`,
so null is excluded explicitly.
Tests exercise the real route through the existing mocked harness rather than
modelling the logic. One of them needs its evidence explained, and says so in a
comment: the harness cannot reach a response on the all-market branch, because
that path calls `new PublicKey(config.programId)` and the file's PublicKey mock
is an arrow function, so it throws "is not a constructor". That throw is raised
BELOW the parse gate, so reaching it proves the empty body was accepted and
dispatched -- rejection at the gate would return 400 and never touch PublicKey.
Mutation-tested, control either side:
control 8 passed
restore the swallowed parse 2 failed
drop the non-object guard only 1 failed
control 8 passed
Verified: 302 files, 3053 passed, 17 skipped; tsc --noEmit exit 0.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe POST handler now validates raw request bodies. Malformed and non-object JSON returns 400 without transaction signing. Empty or whitespace-only bodies retain the all-markets behavior. Regression tests cover omitted, empty, whitespace-only, malformed, and non-object payloads. ChangesSet-price-cap validation
Estimated code review effort: 3 (Moderate) | ~15–30 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@app/app/api/oracle/set-price-cap/route.ts`:
- Around line 114-115: The setPriceCap route must validate the raw request body
before invoking loadCrankKeypair(), so malformed authenticated requests return
400 even when CRANK_KEYPAIR is missing; update
app/app/api/oracle/set-price-cap/route.ts at lines 114-115 accordingly. Add a
malformed-body test without CRANK_KEYPAIR in
app/__tests__/api/oracle-set-price-cap.test.ts lines 117-128 and assert a 400
response.
- Line 140: Validate the parsed request object's allowed fields and types before
assigning it to body in the set-price-cap route; reject null, false, empty, or
otherwise invalid slabAddress values with HTTP 400 instead of selecting the
all-markets scope, and ensure invalid maxChangeE2bps or unknown fields are
handled per the existing contract. In app/app/api/oracle/set-price-cap/route.ts
lines 140-140, update the request validation; in
app/__tests__/api/oracle-set-price-cap.test.ts lines 130-140, add invalid
object-field cases asserting 400 responses and no transaction signing.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 702d597c-e2b4-4260-91e3-229f3a190cfa
📒 Files selected for processing (2)
app/__tests__/api/oracle-set-price-cap.test.tsapp/app/api/oracle/set-price-cap/route.ts
Comment on lines
+114
to
+115
| const rawBody = await req.text(); | ||
| if (rawBody.trim() !== "") { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate the body before loading operational configuration.
loadCrankKeypair() runs before Line 114. If CRANK_KEYPAIR is absent, an authenticated malformed request returns 503 instead of the required 400. Move keypair loading after request-body validation.
app/app/api/oracle/set-price-cap/route.ts#L114-L115: Complete raw-body validation before loading the crank keypair.app/__tests__/api/oracle-set-price-cap.test.ts#L117-L128: Add a malformed-body case with noCRANK_KEYPAIRand assert400.
📍 Affects 2 files
app/app/api/oracle/set-price-cap/route.ts#L114-L115(this comment)app/__tests__/api/oracle-set-price-cap.test.ts#L117-L128
🤖 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 `@app/app/api/oracle/set-price-cap/route.ts` around lines 114 - 115, The
setPriceCap route must validate the raw request body before invoking
loadCrankKeypair(), so malformed authenticated requests return 400 even when
CRANK_KEYPAIR is missing; update app/app/api/oracle/set-price-cap/route.ts at
lines 114-115 accordingly. Add a malformed-body test without CRANK_KEYPAIR in
app/__tests__/api/oracle-set-price-cap.test.ts lines 117-128 and assert a 400
response.
| { status: 400 }, | ||
| ); | ||
| } | ||
| body = parsed as { slabAddress?: string; maxChangeE2bps?: number | string }; |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate object fields before selecting operation scope.
Line 140 accepts any JSON object. Payloads such as {"slabAddress":null}, {"slabAddress":false}, or {"slabAddress":""} make body.slabAddress falsey and select the all-markets branch. This preserves the fail-open scope expansion for invalid object fields.
app/app/api/oracle/set-price-cap/route.ts#L140-L140: Validate allowed fields and field types before assigningbody. Reject invalid or falsey-presentslabAddressvalues with400.app/__tests__/api/oracle-set-price-cap.test.ts#L130-L140: Add invalid object-field cases and assert400with no transaction signing.
📍 Affects 2 files
app/app/api/oracle/set-price-cap/route.ts#L140-L140(this comment)app/__tests__/api/oracle-set-price-cap.test.ts#L130-L140
🤖 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 `@app/app/api/oracle/set-price-cap/route.ts` at line 140, Validate the parsed
request object's allowed fields and types before assigning it to body in the
set-price-cap route; reject null, false, empty, or otherwise invalid slabAddress
values with HTTP 400 instead of selecting the all-markets scope, and ensure
invalid maxChangeE2bps or unknown fields are handled per the existing contract.
In app/app/api/oracle/set-price-cap/route.ts lines 140-140, update the request
validation; in app/__tests__/api/oracle-set-price-cap.test.ts lines 130-140, add
invalid object-field cases asserting 400 responses and no transaction signing.
Owner
Author
Review: looks good, mergeThe bug is real and the fix is precisely scoped. The defect
The fixReads the body once via
Both handled, both commented with the reason. Verified rather than assumedI reverted the route to
That third one matters most — it guards against over-correcting and breaking the documented empty-body contract.
No changes requested. |
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.
Closes #2509.
The bug
POST /api/oracle/set-price-captreats an empty body as "apply to every admin-oracle market". It implemented that by initialisingbody = {}and swallowing every exception fromreq.json():Any parse failure leaves
bodyas{}— the exact state an empty body produces. So a truncated or malformed payload is indistinguishable from the deliberate all-market command:body.slabAddressis undefined, the route takes theelsebranch at:155, selects every market whoseoracle_authoritymatches the crank pubkey (bounded only byMAX_SLAB_BATCH = 50), and submits one signed transaction per validated market.Fail-open scope expansion on an administrative write path: the worse the input, the broader the operation.
Verified against
playground@41bb1304before writing anything — the swallow at:101-105and theif (body.slabAddress) … else { all markets }at:155are both exactly as #2509 describes.To be clear about severity, as the issue is: this is not an auth bypass. It is reachable only after the admin-secret and crank-key checks pass. It is an authenticated operator's malformed request silently becoming a batch.
The fix
Read the raw text once and branch on whether the caller actually sent anything, so "no body" and "bad body" stop being the same state:
JSON.parseThat third case is the one a try/catch alone does not cover, and it is why the fix is not simply "return 400 in the catch".
[],"a string",123andnullall parse successfully, and every field read on them yieldsundefined— reaching the same all-market branch by a different route.typeof null === "object", so null is excluded explicitly.Tests
They exercise the real route through the existing mocked harness in
oracle-set-price-cap.test.ts, rather than modelling the logic in the test.One of them needs its evidence explained, and the comment in the test says so: the harness cannot reach a response on the all-market branch, because that path calls
new PublicKey(config.programId)and this file'sPublicKeymock is an arrow function, so it throwsis not a constructor. That throw is raised below the parse gate — so reaching it proves the empty body was accepted and dispatched. Rejection at the gate would return 400 and never touchPublicKey. I would rather state that than write an assertion whose meaning is not obvious.Mutation-tested
The second row is the one worth noting: it isolates the non-object guard, so the test proves that half is load-bearing rather than decorative.
Verification
Full suite green on
playgroundwith no pre-existing failures.Summary by CodeRabbit
nullnow return a clear HTTP 400 response.