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

When MultipartBody is serialized the values are generated with CRLF #247

Open
MartinM85 opened this issue May 28, 2024 · 18 comments · Fixed by #248
Open

When MultipartBody is serialized the values are generated with CRLF #247

MartinM85 opened this issue May 28, 2024 · 18 comments · Fixed by #248
Assignees
Labels
area:serialization Focused on functional modules of the product type: bug A broken experience type:investigation Investigation work, output should be a document detailing findings or a prototype

Comments

@MartinM85
Copy link
Contributor

MartinM85 commented May 28, 2024

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

@andrueastman
Copy link
Member

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);

@andrueastman andrueastman added the status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close label May 29, 2024
@MartinM85
Copy link
Contributor Author

MartinM85 commented May 29, 2024

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);

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels May 29, 2024
@andrueastman
Copy link
Member

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.

https://github.com/microsoft/kiota-abstractions-dotnet/blob/0787139294b160818a89b22d24eb079084fc7ffd/src/MultipartBody.cs#L149

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?

@andrueastman andrueastman added type: bug A broken experience status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed Needs: Attention 👋 labels May 30, 2024
@MartinM85
Copy link
Contributor Author

Hi @andrueastman I will try to deliver a fix

@MartinM85
Copy link
Contributor Author

@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.

@andrueastman
Copy link
Member

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

@MartinM85
Copy link
Contributor Author

@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.

@baywet
Copy link
Member

baywet commented Jun 21, 2024

@MartinM85 FYI Andrew is out, we're not sure when he'll be back at this point.
What result are you getting today?

@MartinM85
Copy link
Contributor Author

@baywet In the controller, the properties values are null

@baywet
Copy link
Member

baywet commented Jun 21, 2024

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?

@MartinM85
Copy link
Contributor Author

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

@andrueastman
Copy link
Member

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--

@andrueastman andrueastman reopened this Jun 25, 2024
@MartinM85
Copy link
Contributor Author

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

@baywet
Copy link
Member

baywet commented Jun 25, 2024

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?

@andrueastman
Copy link
Member

andrueastman commented Jun 26, 2024

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...

@baywet
Copy link
Member

baywet commented Jun 26, 2024

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 ?

@andrueastman
Copy link
Member

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.

@andrueastman andrueastman added type:investigation Investigation work, output should be a document detailing findings or a prototype and removed Needs: Attention 👋 labels Jun 27, 2024
@baywet
Copy link
Member

baywet commented Jun 27, 2024

Sure, thanks for keeping the issues clean. Please go ahead.

@andrueastman andrueastman added the area:serialization Focused on functional modules of the product label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:serialization Focused on functional modules of the product type: bug A broken experience type:investigation Investigation work, output should be a document detailing findings or a prototype
Projects
Status: Todo 📃
Development

Successfully merging a pull request may close this issue.

3 participants