Assume Content-Type is json for endpoints that require json #4575
Conversation
|
EDIT: nevermind! |
|
@eserilev I don't think any of our GETs fail due to lack of |
## Issue Addressed Closes #3404 (mostly) ## Proposed Changes - Remove all uses of Warp's `and_then` (which backtracks) in favour of `then` (which doesn't). - Bump the priority of the `POST` method for `v2/blocks` to `P0`. Publishing a block needs to happen quickly. - Run the new SSZ POST endpoints on the beacon processor. I think this was missed in between merging #4462 and #4504/#4479. - Fix a minor issue in the validator registrations endpoint whereby an error from spawning the task on the beacon processor would be dropped. ## Additional Info I've tested this manually and can confirm that we no longer get the dreaded `Unsupported endpoint version` errors for queries like: ``` $ curl -X POST -H "Content-Type: application/json" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: WeakSubjectivityConflict", "stacktraces": [] } ``` ``` $ curl -X POST -H "Content-Type: application/octet-stream" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: invalid SSZ: OffsetOutOfBounds(572530811)", "stacktraces": [] } ``` ``` $ curl "http://localhost:5052/eth/v2/validator/blocks/7067595" {"code":400,"message":"BAD_REQUEST: invalid query: Invalid query string","stacktraces":[]} ``` However, I can still trigger it by leaving off the `Content-Type`. We can re-test this aspect with #4575.
## Issue Addressed Closes #3404 (mostly) ## Proposed Changes - Remove all uses of Warp's `and_then` (which backtracks) in favour of `then` (which doesn't). - Bump the priority of the `POST` method for `v2/blocks` to `P0`. Publishing a block needs to happen quickly. - Run the new SSZ POST endpoints on the beacon processor. I think this was missed in between merging #4462 and #4504/#4479. - Fix a minor issue in the validator registrations endpoint whereby an error from spawning the task on the beacon processor would be dropped. ## Additional Info I've tested this manually and can confirm that we no longer get the dreaded `Unsupported endpoint version` errors for queries like: ``` $ curl -X POST -H "Content-Type: application/json" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: WeakSubjectivityConflict", "stacktraces": [] } ``` ``` $ curl -X POST -H "Content-Type: application/octet-stream" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: invalid SSZ: OffsetOutOfBounds(572530811)", "stacktraces": [] } ``` ``` $ curl "http://localhost:5052/eth/v2/validator/blocks/7067595" {"code":400,"message":"BAD_REQUEST: invalid query: Invalid query string","stacktraces":[]} ``` However, I can still trigger it by leaving off the `Content-Type`. We can re-test this aspect with #4575.
|
@michaelsproul I did some digging in the curl manual, apparently curl defaults to
https://curl.se/docs/manpage.html As i mentioned above, the default I believe my changes here should effectively ignore the content-type header and resolve issues users have when making curl requests to relevant endpoints. |
|
By the way, per the HTTP spec, I think ignoring/overriding the content-type header in this situation is ok
https://www.rfc-editor.org/rfc/rfc9110.html#name-content-type |
|
I have tested this and for queries without Details: With curl -X 'POST' 'http://localhost:5052/eth/v1/beacon/rewards/sync_committee/head' -H 'accept: application/json' -d '["502264"]' | jqreturns: {
"execution_optimistic": true,
"finalized": false,
"data": []
}With {
"code": 400,
"message": "BAD_REQUEST: Unsupported endpoint version: v1",
"stacktraces": []
}If this is good we can merge this PR? |
|
Did a testing on passing an invalid data. With Lighthouse curl -X 'POST' 'http://localhost:5052/eth/v1/beacon/rewards/sync_committee/head' -H 'accept: application/json' -d '[“a"]' | jqreturns: {
"code": 400,
"message": "BAD_REQUEST: Error(\"a cannot be parsed as a slot: invalid digit found in string\", line: 3, column: 1)",
"stacktraces": []
}With {
"code": 400,
"message": "BAD_REQUEST: Unsupported endpoint version: v1",
"stacktraces": []
}The updated code is able to show the correct error message when an invalid data is passed, which is good. |
Closes sigp#3404 (mostly) - Remove all uses of Warp's `and_then` (which backtracks) in favour of `then` (which doesn't). - Bump the priority of the `POST` method for `v2/blocks` to `P0`. Publishing a block needs to happen quickly. - Run the new SSZ POST endpoints on the beacon processor. I think this was missed in between merging sigp#4462 and sigp#4504/sigp#4479. - Fix a minor issue in the validator registrations endpoint whereby an error from spawning the task on the beacon processor would be dropped. I've tested this manually and can confirm that we no longer get the dreaded `Unsupported endpoint version` errors for queries like: ``` $ curl -X POST -H "Content-Type: application/json" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: WeakSubjectivityConflict", "stacktraces": [] } ``` ``` $ curl -X POST -H "Content-Type: application/octet-stream" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: invalid SSZ: OffsetOutOfBounds(572530811)", "stacktraces": [] } ``` ``` $ curl "http://localhost:5052/eth/v2/validator/blocks/7067595" {"code":400,"message":"BAD_REQUEST: invalid query: Invalid query string","stacktraces":[]} ``` However, I can still trigger it by leaving off the `Content-Type`. We can re-test this aspect with sigp#4575.
Closes sigp#3404 (mostly) - Remove all uses of Warp's `and_then` (which backtracks) in favour of `then` (which doesn't). - Bump the priority of the `POST` method for `v2/blocks` to `P0`. Publishing a block needs to happen quickly. - Run the new SSZ POST endpoints on the beacon processor. I think this was missed in between merging sigp#4462 and sigp#4504/sigp#4479. - Fix a minor issue in the validator registrations endpoint whereby an error from spawning the task on the beacon processor would be dropped. I've tested this manually and can confirm that we no longer get the dreaded `Unsupported endpoint version` errors for queries like: ``` $ curl -X POST -H "Content-Type: application/json" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: WeakSubjectivityConflict", "stacktraces": [] } ``` ``` $ curl -X POST -H "Content-Type: application/octet-stream" --data @block.json "http://localhost:5052/eth/v2/beacon/blocks" | jq { "code": 400, "message": "BAD_REQUEST: invalid SSZ: OffsetOutOfBounds(572530811)", "stacktraces": [] } ``` ``` $ curl "http://localhost:5052/eth/v2/validator/blocks/7067595" {"code":400,"message":"BAD_REQUEST: invalid query: Invalid query string","stacktraces":[]} ``` However, I can still trigger it by leaving off the `Content-Type`. We can re-test this aspect with sigp#4575.
| warp::body::bytes().and_then(|bytes: Bytes| async move { | ||
| Json::decode(bytes).map_err(|err| reject::custom_bad_request(format!("{:?}", err))) | ||
| }) | ||
| } |
There was a problem hiding this comment.
Diff from warp::filters::body::json
pub fn json<T: DeserializeOwned + Send>() -> impl Filter<Extract = (T,), Error = Rejection> + Copy {
- is_content_type::<Json>()
.and(bytes())
.and_then(|buf| async move {
- Json::decode(buf).map_err(|err| {
- tracing::debug!("request json body error: {}", err);
- reject::known(BodyDeserializeError { cause: err })
- })
+ Json::decode(bytes).map_err(|err| reject::custom_bad_request(format!("{:?}", err)))
})
}impl Decode for Json {
- fn decode<B: Buf, T: DeserializeOwned>(mut buf: B) -> Result<T, BoxError> {
+ fn decode<T: DeserializeOwned>(bytes: Bytes) -> Result<T, BoxError> {
- serde_json::from_slice(&buf.copy_to_bytes(buf.remaining())).map_err(Into::into)
+ serde_json::from_slice(&bytes).map_err(Into::into)
}
}There was a problem hiding this comment.
👍 Equivalent code with the suggestion to remove the copy from #4575 (comment)
There was a problem hiding this comment.
@eserilev consider appending some extra message context like
lighthouse/common/warp_utils/src/reject.rs
Lines 164 to 165 in 1be5253
common/warp_utils/src/reject.rs
Outdated
| pub struct CustomDeserializeError(pub String); | ||
|
|
||
| impl Reject for CustomBadRequest {} | ||
|
|
||
| pub fn custom_deserialize_error(msg: String) -> warp::reject::Rejection { | ||
| warp::reject::custom(CustomDeserializeError(msg)) | ||
| } |
There was a problem hiding this comment.
added some extra message context
common/warp_utils/src/reject.rs
Outdated
| } else if let Some(e) = err.find::<crate::reject::CustomDeserializeError>() { | ||
| message = format!("BAD_REQUEST: body deserialize error: {}", e); | ||
| code = StatusCode::BAD_REQUEST; |
There was a problem hiding this comment.
added some extra message context
|
conflicts resolved and added additional error message context
From what I understand #5104 is caused by nested futures. Since the json filter still makes an async call, I have a feeling it doesnt make any improvements. Tbh I dont know enough to give a definite answer though |
michaelsproul
added
ready-for-merge
|
@Mergifyio queue |
🛑 The pull request has been removed from the queue
|
|
@Mergifyio requeue |
✅ This pull request will be re-embarked automaticallyThe followup |
✅ The pull request has been merged automaticallyThe pull request has been merged automatically at 1d87edb |
* added default content type filter * Merge branch 'unstable' of https://github.com/sigp/lighthouse into unstable * create custom warp json filter that ignores content type header * cargo fmt and linting * updated test * updated test * merge unstable * merge conflicts * workspace=true * use Bytes instead of Buf * resolve merge conflict * resolve merge conflicts * add extra error message context * merge conflicts * lint
Issue Addressed
#4568
Proposed Changes
Create a custom filter that parses json in the request body by default without checking the content-type header. Apply the filter to endpoints that expect JSON in the request body.
Additional Info
The built in warp json filter
warp::body::jsondefaults toContent-Type application/jsonif a Content-Type isn't provided. If aContent-Typeis provided and it is notapplication/jsonthe request will fail.I ran several tests on my own, and was unable to recreate a failed endpoint request when
Content-Typewas omitted. I was however able to recreate a failed endpoint request whenContent-Typewas included but wasn'tapplication/json.In out test suite note that
builder.json()appendsContent-Type application/jsonto the header by default.I created a new function
post_generic_json_without_content_type_headerwhich creates a post request with serialized json in the body without includingContent-Type application/jsonin the header.Locally I replaced all post requests in our test suite to NOT include the
Content-Typeheader and all the tests passed. In this PR I only updated a single test to send a POST request without theContent-Typeheader.I plan on also spinning up the client locally and making some curl requests myself to make sure I haven't missed anything.