-
Notifications
You must be signed in to change notification settings - Fork 152
Log solve request data transfer #4082
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request refactors the /solve endpoint to measure the duration of request body data transfer, which helps in diagnosing reported delays. The implementation correctly changes the endpoint to accept a raw HTTP request and streams the body to hyper::body::Bytes while timing the operation. However, this change introduces a critical security vulnerability. By processing the raw request body without a size limit, the service is exposed to a Denial of Service attack via memory exhaustion from an overly large request payload. My review includes a comment with a suggested fix to mitigate this vulnerability by applying a size limit to the request body.
| futures = { workspace = true } | ||
| const-hex = { workspace = true } | ||
| hex-literal = { workspace = true } | ||
| http-body = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like you're not using this, at least directly. If I had to guess, you added this but then hyper had this built in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, exactly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't you remove this dep then? Or did we misunderstood each other?
Description
Some solvers reported that some requests come significantly delayed (judging by the auction deadline). Currently we have no way to distinguish between receiving the start of the
/solverequest and streaming the actual data.This PR makes this possible by making the
solvehandler take a raw http request and stream the body afterwards.Changes
Instead of:
String(including utf8 check)Stringinto anArcto make copying it cheapSolveRequestWe now do:
BytestypeSolveRequestSince handling the raw request seems to bypass axum's request size checks I did it manually for this endpoint.
How to test
Existing tests should suffice