HttpCookie.Value should return the URL decoded cookie value

[afshinm]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

PR Title
HttpCookie.Value should return the URL decoded cookie value

PR Description

Create a new HttpCookie object in NET Standard / NET Core URL encodes the value of of cookie, whereas in NET Framework 4.7.2, this doesn't occur. This is probably the desired behavior but makes it difficult to share the same cookie between the two apps (NET Core and NET Framework).

To reproduce this bug:

var cookie = new HttpCookie(cookieName, "a|b") { Secure = true, SameSite = SameSiteMode.None };
Console.WriteLine(cookie.Value); // prints a%7Cb

Also, here is an example that HttpCookie should just return the cookie value without URL encoding it: https://dotnetfiddle.net/eusU6N (but Values should URL encode, which I added a unit test for it in this PR).

Addresses #393

[afshinm]

I manually tested these changes locally and confirmed that HttpCookie.Value is now correctly reflecting the URL decoded value, however, the Set-cookie header generated in the response gets URL encoded again.

I'm not quite sure why this is happening or how the Set-cookie header gets generated but I noticed that this file generates the Set-cookie header https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System.Web/HttpCookie.cs#L444. Maybe we should have the same method?

[twsouthwick]

I'm not quite sure why this is happening or how the Set-cookie header gets generated but I noticed that this file generates the Set-cookie header https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System.Web/HttpCookie.cs#L444. Maybe we should have the same method?

So it sounds like your fix gets us partially there, but there is still some URL encoding going on? If so, would you be up for integrating that method in? There's some e2e tests that set up a test host that should allow testing this scenario

[twsouthwick]

LGTM - let me know if you want to merge this in or do the more extensive set-cookie fix

[twsouthwick]

is this what system.web was doing?

[afshinm]

@twsouthwick no, System.web is doing an .IndexOf without the SystemComparison argument. Do you want me to change it to IndexOf?

[twsouthwick]

No this is fine -I was wondering if the rest of it was something that system.web was doing

[afshinm]

I'm not quite sure why this is happening or how the Set-cookie header gets generated but I noticed that this file generates the Set-cookie header https://github.com/microsoft/referencesource/blob/51cf7850defa8a17d815b4700b67116e3fa283c2/System.Web/HttpCookie.cs#L444. Maybe we should have the same method?

So it sounds like your fix gets us partially there, but there is still some URL encoding going on? If so, would you be up for integrating that method in? There's some e2e tests that set up a test host that should allow testing this scenario

@twsouthwick Thanks for the review.

I think I prefer to run some more e2e tests as this is not going to actually fix #393. Re e2e tests, are you referring to these Playwright tests here https://github.com/dotnet/systemweb-adapters/tree/main/test/Samples.MVCApp.Tests?

[twsouthwick]

I was referring to these tests: https://github.com/dotnet/systemweb-adapters/blob/main/test/Microsoft.AspNetCore.SystemWebAdapters.CoreServices.Tests/ResponseStreamTests.cs

[afshinm]

@twsouthwick Updated the PR and added the ToSetCookieHeaderValue method which correctly sets the Set-cookie headers. Also added some end to end tests. Let me know what you think.

[twsouthwick]

Can you add the usings?

[twsouthwick]

No this is fine -I was wondering if the rest of it was something that system.web was doing

[twsouthwick]

nit: httpresponsemessage should have a using; not sure why there's no analyzer catching that

[afshinm]

@twsouthwick good catch. I just updated the PR and added using. I can update the analyzer to catch this type of errors in a separate PR.

[twsouthwick]

Overall, looks good, just a few nits

[afshinm]

@twsouthwick Just updated the tests and I think this PR is now ready to be merged.