Skip to content
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

Content-Type multipart/form-data is not seen as byte[] anymore #248

Closed
bgiot opened this issue Jan 10, 2019 · 18 comments
Closed

Content-Type multipart/form-data is not seen as byte[] anymore #248

bgiot opened this issue Jan 10, 2019 · 18 comments

Comments

@bgiot
Copy link

bgiot commented Jan 10, 2019

Since version 1.0.4.18, if the request body is of content-type "multipart/form-data", it is seen as string body instead of byte body. Bug seems to be in the BodyParser class where the content-type is not check anymore before trying reading the content as string. Before version 1.0.4.18, a check was done to see if the content is of "text" type before trying to convert body to string. Our MultipartMatcher implementation cannot work anymore because of this.

@StefH
Copy link
Collaborator

StefH commented Jan 10, 2019

Which version are you using now ? 1.0.6 ?

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

It only works with version 1.0.4.17. I tested all the other versions from 1.0.4.18 upto 1.0.6 and I have the issue

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

In version 1.0.4.17, you can see in the BodyParser class that a test is done on the ContentType header before trying to read the body as string (from line 50):

           if (contentTypeHeaderValue != null && TextContentTypes.Any(text => contentTypeHeaderValue.StartsWith(text, StringComparison.OrdinalIgnoreCase)))
            {
                try
                {
                    var stringData = await ReadStringAsync(stream);
                    data.BodyAsString = stringData.Item1;
                    data.Encoding = stringData.Item2;
                }
                catch
                {
                    // Reading as string failed, just get the ByteArray.
                    data.BodyAsBytes = await ReadBytesAsync(stream);
                }
            }

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

from version 1.0.4.18, the code doesn't check the content type anymore before trying reading body as string:


           var data = new BodyData
            {
                BodyAsBytes = await ReadBytesAsync(stream),
                DetectedBodyType = BodyType.Bytes,
                DetectedBodyTypeFromContentType = DetectBodyTypeFromContentType(contentType)
            };

            // Try to get the body as String
            try
            {
                data.BodyAsString = DefaultEncoding.GetString(data.BodyAsBytes);
                data.Encoding = DefaultEncoding;
                data.DetectedBodyType = BodyType.String;

@StefH
Copy link
Collaborator

StefH commented Jan 10, 2019

Yes I see.
Currently changing the code to add MultiPart.

@StefH
Copy link
Collaborator

StefH commented Jan 10, 2019

See PR
#249
and comment if this is ok.

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

I don't understand what you are going to do by saying "add multipart". Putting back the tests? In fact multipart MUST be seen as binary and NOT string... Otherwise any byte "zero" in the body content will stop the reading of the body. The body must be read as byte[] and not string....

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

I checked your fix. Indeed it's a shortcut. And it should work. But putting back the tests as before is not more backward compatible ?

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

I'm ready to validate this new version when available. I'll keep you posted...

@StefH
Copy link
Collaborator

StefH commented Jan 10, 2019

"add multipart" --> I mean just fixing this code to support your test scenario.

@StefH
Copy link
Collaborator

StefH commented Jan 10, 2019

But putting back the tests as before is not more backward compatible ? what you mean by this?

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

The content-type is the key element to decide how to read the body. Trying reading the body as string without checking that the content type is of "text" type can be an issue sometime... no ? I think the way it was done in version 1.4.0.17 is safe. Any special case that needed to remove those checks ? Also in the content-type info you can have the encoding to properly read the body...

@StefH
Copy link
Collaborator

StefH commented Jan 10, 2019

By that time I changed the code for parsing from the body.
And with that try to read as string, it's easy for users to use the body as string, even if the content-type may be missing or invalid.

For your specific case, it looks like a work-around yes.
I'll just wait for more cases to pop-up and maybe then I need to do a (breaking) change.

@StefH StefH closed this as completed Jan 10, 2019
@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

In fact the property DetectedBodyTypeFromContentType is not used afterward (but only is your fix right now for the multipart). I think a test must be added based on that value just before the "try" statement for string/json.

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

notion of "breaking change" ;-) In fact since 1.0.4.18 it was a breaking change ;-) Let's wait indeed for more usecases to see if more changes are needed. Any info about release time for this version 1.0.6.1?
Thanks a lot for your speedy reaction!

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

really appreciate your reactivity! Thanks a lot again!

@StefH
Copy link
Collaborator

StefH commented Jan 10, 2019

In a few minutes, the 1.0.6.1`NuGet will be visible.

@bgiot
Copy link
Author

bgiot commented Jan 10, 2019

I did some tests and all seems to work fine! Good job! Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants