Simplify v1 response processing function signatures #745
Conversation
spacebear21
force-pushed
the
u8-responses
branch
2 times, most recently
from
228c39b to
fd0e04a
Compare
|
Added one more commit which removes the now unused |
| impl UncheckedProposal { | ||
| pub fn from_request( | ||
| body: impl std::io::Read, | ||
| body: &[u8], |
There was a problem hiding this comment.
This will be helpful! I was having a really tough time figuring out how to get a Read across the ffi layer in the v2_to_v1 python integration test
Pull Request Test Coverage Report for Build 15543598153Details
💛 - Coveralls |
nothingmuch
left a comment
nothingmuch
left a comment
There was a problem hiding this comment.
cACK, utACK apart from one correctness suggestion (body.len() != content_length)
but i also have some suggestions for how to simplify a further which also apply to the sender side of things but i only commented on the receiver side, if you think they are appropriate then it's probably a bit cleaner to do it in this PR rather than a followup
| let mut buf = vec![0; content_length]; | ||
| body.read_exact(&mut buf).map_err(InternalRequestError::Io)?; | ||
| Ok(buf) | ||
| Ok(body.to_vec()) |
There was a problem hiding this comment.
what do you think about returning just Ok(body), with the following signature:
fn validate_body<'b>(headers: impl Headers, body: &'b [u8]) -> Result<&'b [u8]>, RequestError> {that would avoid allocation, and there's no real need to copy it just to return it back to the caller, since the caller already has a means of giving access to the body as a &[u8]
alternatively if we forego the content length consistency check, even simpler would be to rename this method validate_header, drop the body argument since it doesn't really do anything with the it, and return Result<usize, RequestError> where the usize is the expected length (or a newtype ExpectedContentLength(usize) but maybe that's overkill since this is all private)
| .parse::<usize>() | ||
| .map_err(InternalRequestError::InvalidContentLength)?; | ||
| if content_length > MAX_CONTENT_LENGTH { | ||
| if content_length > MAX_CONTENT_LENGTH || body.len() > MAX_CONTENT_LENGTH { |
There was a problem hiding this comment.
This should probably be:
| if content_length > MAX_CONTENT_LENGTH || body.len() > MAX_CONTENT_LENGTH { | |
| if content_length > MAX_CONTENT_LENGTH || body.len() != content_length { |
but a different error (which should still map to bad request error).
however, since body is already allocated, and presumably does not exceed the app's limits by the time this code is run, maybe it's better to just remove MAX_CONTENT_LENGTH altogether?
There was a problem hiding this comment.
Instead of introducing this check, this function now returns body[..content_length] which preserves the prior behavior of capping the body length without throwing an error.
There was a problem hiding this comment.
i think i prefer breaking compatibility here, because a truncated request should also error, but the error will be less intelligible (PSBT parsing presumably will fail)
| let validated_body = validate_body(headers, body).map_err(ReplyableError::V1)?; | ||
|
|
||
| let base64 = String::from_utf8(parsed_body).map_err(InternalPayloadError::Utf8)?; | ||
| let base64 = String::from_utf8(validated_body).map_err(InternalPayloadError::Utf8)?; |
There was a problem hiding this comment.
taken with the suggestion to have a validate_header method below, if a newtype ExpectedContentLength(usize), it could simply do
fn parse_body<'a>(&self, body: &'a [u8]) Result<&'a str, ReplyableError>
if body.len() != self.0 {
return Err(todo!());
}
str::from_utf8(body).map_err(InternalPayloadError::Utf8)
}it could handle this conversion from &[u8] using &str::from_utf8, to again avoid copying the data, since we assume we already have contiguous random access to it as a &[u8].
Simplify the function signature to take a bytes slice instead of a generic reader. `parse_body` is renamed to better reflect its role.
Simplify the function signature to take a bytes slice instead of a generic reader.
They are no longer constructed after the changes from the previous two commits.
|
I elected to keep the |
There is no real need to copy it just to return it back to the caller.
After payjoin#745 I realized we can go further and completely remove the FinalizeResponseError in favor of using FinalizedError which already has access to DirectoryResponse errors.
After #745 I realized we can go further and completely remove the `FinalizeResponseError` in favor of using `FinalizedError` which already has access to `DirectoryResponse` errors.
After payjoin#745 I realized we can go further and completely remove the FinalizeResponseError in favor of using FinalizedError which already has access to DirectoryResponse errors.
After payjoin#745 I realized we can go further and completely remove the FinalizeResponseError in favor of using FinalizedError which already has access to DirectoryResponse errors.
4 participants
This was prompted by the discussion in #710 (comment).
Using a reader still left callers susceptible to writing memory exhaustion vulnerable code, and just introduced extra complexity by having to buffer twice. We can recognize this and leave the buffering entirely up to the caller. Also, the V2 sender and receiver response processing functions already take &[u8], so this makes the function signatures more consistent across versions.