Added length validation for RequestBodyStream #205
Conversation
| where | ||
| loop stream = do | ||
| loop n stream = do |
There was a problem hiding this comment.
This should probably be !n to avoid a space leak.
|
I actually think it's a perfectly elegant solution. I put in some inline comments. My only other thought: this deserves its own exception constructor instead of using the |
|
Yep, agree it should be its own exception constructor. WrongRequestBodyStreamSize Word64 Word64 perhaps (same layout as ResponseBodyTooShort). I have also applied all the changes that you suggested. Also, tested all of the changes including exception message (verified it prints correct values - caveat int64 to word64 conversion in case of negative length argument but this is expected). |
| if isChunked then connectionWrite "0\r\n\r\n" | ||
| -- If not chunked, then length argument is present | ||
| -- and should be validated | ||
| else unless (fromJust mlen == n) $ throwIO $ WrongRequestBodyStreamSize (fromIntegral . fromJust $ mlen) (fromIntegral n) |
There was a problem hiding this comment.
I don't feel comfortable including a partial function like fromJust here. Why not just pattern match on mlen instead of using isChunked?
There was a problem hiding this comment.
Yep, agree. Removed partial function in latest commit.
This is proposed fix for #204. Tested with my code to verify that the error doesn't happen any more in case of wrong length, and that when the length is correct, data upload goes through.
I repurposed
writeStreammachinery and addedlengthcheck for when chunking is false - not elegant solution :(Now, there is an additional branch - pseudo-code changes in bold below -
Before: if isChunked then return terminator else return ()
After: if isChunked then return terminator else (if length error return 400 status code else return ())
Not sure if Status code 400 is right one here. It seems to be appropriate for client-side error, to me.
If anything needs to be changed, please do let me know.