When MultipartBody is serialized the values are generated with CRLF
MartinM85 opened this issue · 18 comments
I've a controller
[HttpPost("login")]
[ProducesResponseType(typeof(TokenResponse), StatusCodes.Status200OK)]
[ProducesResponseType(typeof(ErrorResponse), StatusCodes.Status401Unauthorized)]
[ProducesResponseType(typeof(ErrorResponse), StatusCodes.Status500InternalServerError)]
[Produces(MediaTypeNames.Application.Json)]
[AllowAnonymous]
public IActionResult Login([FromForm] Login login)
{
}
Login class has properties Username and Password
Part of swagger.json from which a client is generated
"/api/auth/login": {
"post": {
"tags": [
"Auth"
],
"summary": "Generates the token from the request data",
"requestBody": {
"content": {
"multipart/form-data": {
"schema": {
"type": "object",
"properties": {
"username": {
"type": "string",
"description": "The user's name"
},
"password": {
"type": "string",
"description": "The user's password"
}
}
},
"encoding": {
"username": {
"style": "form"
},
"password": {
"style": "form"
}
}
}
}
},
Calling the login method from the client
var client = new ApiClient();
var multipartBody = new MultipartBody();
multipartBody.AddOrReplacePart("username", "application/x-www-form-urlencoded", "test");
multipartBody.AddOrReplacePart("password", "application/x-www-form-urlencoded", "abc");
var tokenResponse = await client.Api.Auth.Login.PostAsync(multipartBody);
When debugging the login method, I see that the value in Username is "test\r\n" and in Password is "abc\r\n". At the first sight it seems to me that CRLF are added during serialization.
Same for content type text/plain
Thanks for raising this @MartinM85
Any chance you can confirm that the seriliazed payload is as expected when you run the following?
var stringValue = KiotaSerializer.SerializeAsString("multipart/form-data", multipartBody);Hi @andrueastman, Text Visualizer in VS shows me this
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
string value:
"--605a817187c144bbae08f23dda69523a\r\nContent-Type: application/x-www-form-urlencoded\r\nContent-Disposition: form-data; name=\"username\"\r\n\r\ntestn\r\n\r\n--605a817187c144bbae08f23dda69523a\r\nContent-Type: application/x-www-form-urlencoded\r\nContent-Disposition: form-data; name=\"password\"\r\n\r\nabc\r\n\r\n--605a817187c144bbae08f23dda69523a--\r\n"
In MultipartBody.Serialize, I see calling of AddNewLine(writer);
It looks like since the value is a string value, the writer isn't using the serializer based on the content type the same way it would for a parsable but using the multipart writer instead hence the new line character.
We would need to update the if clause to be similar to the parasable clause above it to pull the right serializationWriter.
Any chance this is this something you would be willing to submit a PR for @MartinM85?
Hi @andrueastman I will try to deliver a fix
@andrueastman Fix delivered, but I'm worried about unit tests. Unit tests for MultipartBody are part of Abstraction library, but in this case, we need to set up concrete serialization writer. Not sure if mocking of json serializer is a good idea.
Seems like only
https://github.com/microsoft/kiota-abstractions-dotnet/blob/0787139294b160818a89b22d24eb079084fc7ffd/src/MultipartBody.cs#L149
is covered by the current unit tests.
It would be nice to have some integration tests to ensure that everything will work.
What I'd suggest for now is adding a test and referencing the project in the multipart library. Similar to this.
https://github.com/microsoft/kiota-serialization-multipart-dotnet/blob/e0b280eb3304d009592e79cf074a2867d984c604/Microsoft.Kiota.Serialization.Multipart.Tests/MultipartSerializationWriterTests.cs#L98. The build will fail for now but pass once released and capture this going forward.
This should all get easier to do later on once we move the projects to one repo to make the end to end testing easier for such via #238
@andrueastman Seems like my change didn't work, but it's quite challenge to test it properly. I've tried to add unit test to https://github.com/microsoft/kiota-serialization-multipart-dotnet, but I need to reference local Microsoft.Kiota.Abstractions and Microsoft.Kiota.Serialization.Json. It's time consuming and what's important, I don't know what should be the expected result.
Based on this: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Disposition#examples the correct serialized content should be:
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
Maybe the fix should be removing AddNewLine: https://github.com/microsoft/kiota-abstractions-dotnet/blob/0787139294b160818a89b22d24eb079084fc7ffd/src/MultipartBody.cs#L179, but removing this line can break another funcionality.
@MartinM85 FYI Andrew is out, we're not sure when he'll be back at this point.
What result are you getting today?
let me be more specific with my question: when you send the payload from the client today, how it is formatted? and how does it differ from what would be required?
Behavior for version 1.9.3:
var client = new ApiClient();
var multipartBody = new MultipartBody();
multipartBody.AddOrReplacePart("username", "application/x-www-form-urlencoded", "test");
multipartBody.AddOrReplacePart("password", "application/x-www-form-urlencoded", "abc");
var tokenResponse = await client.Api.Auth.Login.PostAsync(multipartBody);
Serialized payload when calling:
var stringValue = KiotaSerializer.SerializeAsString("multipart/form-data", multipartBody);
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
Server:
Login.Username has value "test\r\n"
Login.Password has value "abc\r\n"
Behavior for version >=1.9.4:
var stringValue = await KiotaSerializer.SerializeAsStringAsync("multipart/form-data", multipartBody); throws System.InvalidOperationException: 'SerializationWriterFactory'
StackTrace:
at Microsoft.Kiota.Abstractions.MultipartBody.Serialize(ISerializationWriter writer)
at Microsoft.Kiota.Serialization.Multipart.MultipartSerializationWriter.WriteObjectValue[T](String key, T value, IParsable[] additionalValuesToMerge)
at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.SerializeAsStream[T](String contentType, T value)
at Microsoft.Kiota.Abstractions.Serialization.KiotaSerializer.SerializeAsStringAsync[T](String contentType, T value, CancellationToken cancellationToken)
Server:
Login.Username has value null
Login.Password has value null
Re-opening this one for now....
The error occurs as the property multipartBody.RequestAdapter is not set (this is done automagically by the request adapter when sending the request.) So you will need to add a line like multipartBody.RequestAdapter = graphClient.RequestAdapter; before calling the serializer.
It also looks like the writer for x-www-form-urlencoded will not write values if a key is not specified as the multipart writer will pass the use the key as the part name. See https://github.com/microsoft/kiota-serialization-form-dotnet/blob/50d7a29ac768c2f3c0047d8909f12c2df623a30f/src/FormSerializationWriter.cs#L203
Using the code below will yield the expected result albeit with the wrong content type(i.e. "text/plain").
var multipartBody = new MultipartBody();
multipartBody.AddOrReplacePart("username", "text/plain", "test");
multipartBody.AddOrReplacePart("password", "text/plain", "abc");
multipartBody.RequestAdapter = graphClient.RequestAdapter;
var stringValue = await KiotaSerializer.SerializeAsStringAsync("multipart/form-data", multipartBody);Open question here is shouldn't the body for url-form-encoded be with key value pairs as below?
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
username=test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
password=abc
--605a817187c144bbae08f23dda69523a--Trying the requests from Postman. API is running on ASPNET Core 8
1.
POST {app_url}/login
Content-Type: multipart/form-data;boundary=605a817187c144bbae08f23dda69523a
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="username"
username=test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="password"
password=abc
--605a817187c144bbae08f23dda69523a--
Username is set to username=test
Password is set to password=abc
2.
POST {app_url}/login
Content-Type: multipart/form-data;boundary=605a817187c144bbae08f23dda69523a
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
Username is set to test
Password is set to abc
3.
POST {app_url}/login
Content-Type: multipart/form-data;boundary=605a817187c144bbae08f23dda69523a
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="username"
test
--605a817187c144bbae08f23dda69523a
Content-Type: application/x-www-form-urlencoded //or text/plain or Content-Type is missing
Content-Disposition: form-data; name="password"
abc
--605a817187c144bbae08f23dda69523a--
Username is set to test\r\n
Password is set to abc\r\n
looking at both RFC1867 and RFC7578 it is clear there shouldn't be a line return between the part and the next boundary (see different between previous message 2 and 3 examples). This is something we should fix.
Then I think there's a misunderstanding there. I believe that since the multipart format is already handling the "segmentation of multiple properties" is doesn't need to be repeated in the different parts. I.E test not username=test. And since the application is not doing a sub-segmentation (i.e. username is a string, not an object with properties), form encoded or text plain are functionally equivalent. I'd still recommend using text plain to avoid getting strange encoding behaviours with special characters (likely to happen for passwords).
Does that make sense?
looking at both RFC1867 and RFC7578 it is clear there shouldn't be a line return between the part and the next boundary (see different between previous message 2 and 3 examples). This is something we should fix.
Thanks @baywet, I believe this is already fixed in the latest abstractions with @MartinM85 PR.
I was just highlighting that using the code below
var multipartBody = new MultipartBody();
multipartBody.AddOrReplacePart("username", "text/plain", "test");
multipartBody.AddOrReplacePart("password", "text/plain", "abc");
multipartBody.RequestAdapter = graphClient.RequestAdapter;
var stringValue = await KiotaSerializer.SerializeAsStringAsync("multipart/form-data", multipartBody);will result in the correct and expected result as below (no spaces but content type as text plain)
--2d3a3832fb72430abed317363391ca7d
Content-Type: text/plain
Content-Disposition: form-data; name="username"
test
--2d3a3832fb72430abed317363391ca7d
Content-Type: text/plain
Content-Disposition: form-data; name="password"
abc
--2d3a3832fb72430abed317363391ca7d--
However, changing the content type of the parts to application/x-www-form-urlencoded results in empty values as below. (Hence the null values observed by @MartinM85.)
--45b491730a624687a4559322c1491f72
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="username"
--45b491730a624687a4559322c1491f72
Content-Type: application/x-www-form-urlencoded
Content-Disposition: form-data; name="password"
--45b491730a624687a4559322c1491f72--
This is because the form writer does not accept writing values when the key is not passed/present/empty at https://github.com/microsoft/kiota-serialization-form-dotnet/blob/50d7a29ac768c2f3c0047d8909f12c2df623a30f/src/FormSerializationWriter.cs#L203.
I believe the question really is that should we update the form writer to accept empty keys? Or does setting the content type as application/x-www-form-urlencoded result in the expectation that key value pairs are expected?
In my head, the only valid case for using application/x-www-form-urlencoded would be for an object where the partname would be object name and then we would write key value pairs for its properties in the payload.
So, a scenario like this should probably not have the part content type as application/x-www-form-urlencoded since we only want to write the values. With this clarified we can probably close this issue now...
Before we make that last change in the form serialization, we should double check in the specs (I believe that form encoded is a split between RFC and WHATWG) if this is valid. Would you look into that @andrueastman ?
No worries. Will take a look into this to confirm.
Way forward will either be
- create new issue in form repo to fix and close this.
- close this as nothing is to be done.
Sure, thanks for keeping the issues clean. Please go ahead.