"crow::multipart::message throws when the boundary is empty or malformed

[Iuliean]

This is related to issue #916 where the multipart message causes a segmentation fault when a multipart request with an empty boundary is received.

I added two throws to indicate whenever the boundary is empty or it could not be found/differs from the boundary inside the body.

I hope these changes are enough to prevent the code from being unable to parse the headers thus leaving to the segmentation faults.

I also added two new tests that aim to check that these exceptions are thrown whenever the boundary is either missing or not matching

Edit: Something that occured to me as an afterthought is that maybe the line that finds the "name" element should also be checked and emmit some exception or something to be 100% that this does not segfault in the future

[The-EDev]

Quick question about this, I understand that raising an exception is a valid solution, but if this exception goes unhandled then we end up with a 500 response to the client, rather than the appropriate 400 response.

Would it be appropriate to just respond with a 400 rather than raise an exception and have the handler deal with it? What are your thoughts?

[Iuliean]

I have thought about automatically responding with an error code when this is detected but i mostly didn't know where to look to see how that can be achieved.

If you point me in the right direction i'll try to make the necessary changes.

One counter argument i can give to what you said is. If the user does not handle the error in his handler returning 500 seems completely fine to me since an internal error did happen but the user did not check for it so it is unknown (to me that seems completely reasonable). Any other unhandled error would have the same result i assume.

But if you wish to automatically respond with this. The easiest way would probably be to have a BadRequestException that can be caught in the method that handles each handler. But let me know if there is a better way

[The-EDev]

One counter argument i can give to what you said is. If the user does not handle the error in his handler returning 500 seems completely fine to me since an internal error did happen but the user did not check for it so it is unknown (to me that seems completely reasonable). Any other unhandled error would have the same result i assume.

since the exception is the result of an error within the request itself, and since the only fix is to actually change the request, then it should be a 400, 500 should be used if the error is the result of a flaw in the server itself, which doesn't match this case.

The easiest way would probably be to have a BadRequestException that can be caught in the method that handles each handler. But let me know if there is a better way

I agree, I think this would be ideal, Crow handles exceptions here. I think modifying the default exception handler to recognize a bad request exception is a valid solution.

[Iuliean]

Yes i agree, i'll make the required changes as soon as i can

[Iuliean]

One question i have is. Looking at the code for the request class, it seems that it just a holder of all the information needed to then build a multipart message.

What if request itself could handle the multipart stuff, then when the request itself is being built the error would be caught way before it enters the user's handler. Wouldn't that be preferred ? I mean you lose the "lazy loading" part of it where if you don't parse you have 0 performace loss but all errors are caught before they become an issue. Also ,maybe this would be a very rare case, it could happen that you respond to invalid requests sometimes. Say you check for some url values first and based on that you deduce there is no need to parse it, thus an otherwise invalid request is treated as normal.

[The-EDev]

What if request itself could handle the multipart stuff, then when the request itself is being built the error would be caught way before it enters the user's handler. Wouldn't that be preferred ?

well it's a design decision, as far as I can tell, the design is that request and response are only meant to hold data, with any logic being separated from them.

Say you check for some url values first and based on that you deduce there is no need to parse it, thus an otherwise invalid request is treated as normal.

wouldn't putting this kind of logic in the handler be a good solution?