Conversation
Pull Request Test Coverage Report for Build 23367463396Details
💛 - Coveralls |
spacebear21
left a comment
There was a problem hiding this comment.
Since we are applying the 7168 uniformly across v1 and v2 requests, I don't see the value in extracting v1 routing into its own router? The current approach is also confusing because v1 routing is now seemingly happening in two places, both in v1_post_handler and in directory Service::serve_request
I noticed that applying the limiter globally breaked a lot of the integration test. Because the v1 sender never touches the ohttp relay, applying the limiter globally also breaks v2 that communicates via ohttp. my work around was extracting the v1 router (which only applies the limiter) and then route the request to the v1 handlers in the directory. if we want to still keep the manual size check rather than using tower_http limiter, i can rename v1_max_buffer to max_payload_size , add a size check in post_fallback_v1 and then update the branch name and commit mesage too |
This PR sets and rename v1_max_buffer_size. The buffer size for v1 payload is set to 7168 which is same for v2. The variable name is updated to better reflect what it is. The rationale behind the change is that it prevents v2 client fetching v1 request from leaking information about the nature of the request
nothingmuch
left a comment
There was a problem hiding this comment.
if all handled request bodies have a fixed size bound it might make sense to use an array type to pass them around as this could potentially simplify some conversions, perhaps an argument for fixing the size at 7168
| pub const BHTTP_REQ_BYTES: usize = | ||
| ENCAPSULATED_MESSAGE_BYTES - (CHACHA20_POLY1305_NONCE_LEN + POLY1305_TAG_SIZE); | ||
| const V1_MAX_BUFFER_SIZE: usize = 65536; | ||
| pub(crate) const MAX_PAYLOAD_SIZE: usize = 7168; |
There was a problem hiding this comment.
@DanGould has argued correctly that this can be closer to 8104 bytes, but there's a variable element of overhead in OHTTP requests relating to the request URI that makes this a bit tricky. i'm personally happy with this as the limit
This pr addresses #941, it implements a body size limit layer to reject request whose size is greater than 65536 bytes.
Pull Request Checklist
Please confirm the following before requesting review:
AI
in the body of this PR.