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

HttpCookie.Value should return the URL decoded cookie value #394

Merged
merged 12 commits into from
Sep 7, 2023
76 changes: 43 additions & 33 deletions src/Microsoft.AspNetCore.SystemWebAdapters/HttpCookie.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,15 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System.Collections.Specialized;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.SystemWebAdapters.Internal;
using Microsoft.Net.Http.Headers;

namespace System.Web;

public sealed class HttpCookie
{
// If the .Values collection hasn't been accessed, this will remain a string. However, once .Values is accessed,
// this will become a HttpValueCollection. If .ToString is called on it, it will reconsistute the full string
private object? _holder;
private string? _stringValue;
private HttpValueCollection? _multiValue;

public HttpCookie(string name)
{
Expand All @@ -21,7 +20,7 @@ public HttpCookie(string name)
public HttpCookie(string name, string? value)
{
Name = name;
_holder = value;
_stringValue = value;
}

/// <summary>
Expand All @@ -42,21 +41,30 @@ public string? this[string key]

/// <summary>
/// Gets or sets an individual cookie value.
/// </summary>
public string? Value
{
get => _holder?.ToString();
get
{
if (_multiValue != null)
return _multiValue.ToString(false);
else
return _stringValue;
}

set
{
if (_holder is HttpValueCollection values)
if (_multiValue != null)
{
// reset multivalue collection to contain single keyless value
values.Clear();
values.Add(null, value);
// reset multivalue collection to contain
// single keyless value
_multiValue.Clear();
_multiValue.Add(null, value);
}
else
{
_holder = value;
// remember as string
_stringValue = value;
}
}
}
Expand All @@ -68,37 +76,38 @@ public NameValueCollection Values
{
get
{
if (_holder is HttpValueCollection values)
if (_multiValue == null)
{
return values;
// create collection on demand
_multiValue = new HttpValueCollection();

// convert existing string value into multivalue
if (_stringValue != null)
{
if (_stringValue.Contains('&', StringComparison.InvariantCulture) || _stringValue.Contains('=', StringComparison.InvariantCulture))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this what system.web was doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_multiValue.FillFromString(_stringValue);
else
_multiValue.Add(null, _stringValue);

_stringValue = null;
}
}

// create collection on demand
var collection = new HttpValueCollection();

// convert existing string value into multivalue
if (_holder is string str)
{
collection.FillFromString(str);
}

_holder = collection;

return collection;
return _multiValue;
}
}

internal void CopyTo(HttpValueCollection other)
{
if (_holder is string s)
if (_stringValue != null)
{
other.Add(null, s);
other.Add(null, _stringValue);
}
else if (_holder is HttpValueCollection collection)
else if (_multiValue != null)
{
for (var i = 0; i < collection.Count; i++)
for (var i = 0; i < _multiValue.Count; i++)
{
other.Add(collection.GetKey(i), collection[i]);
other.Add(_multiValue.GetKey(i), _multiValue[i]);
}
}
}
Expand All @@ -115,6 +124,7 @@ internal void CopyTo(HttpValueCollection other)

/// <summary>
/// Gets or sets the virtual path to transmit with the current cookie.
/// </summary>
public string Path { get; set; } = "/";

/// <summary>
Expand All @@ -135,21 +145,21 @@ internal void CopyTo(HttpValueCollection other)
/// <summary>
/// Gets a value indicating whether this cookie is allowed to participate in output caching.
/// </summary>
/// <remarks>
public bool Shareable { get; set; }

/// <summary>
/// Gets or sets the domain to associate the cookie with.
/// </summary>
public string? Domain { get; set; }

internal CookieOptions ToCookieOptions() => new()
internal SetCookieHeaderValue ToSetCookieHeaderValue() => new(Name)
{
Value = Value ?? string.Empty,
Domain = Domain,
Expires = (Expires == DateTime.MinValue) ? null : new DateTimeOffset(Expires),
HttpOnly = HttpOnly,
Path = Path,
SameSite = (Microsoft.AspNetCore.Http.SameSiteMode)SameSite,
SameSite = (Microsoft.Net.Http.Headers.SameSiteMode)SameSite,
Secure = Secure,
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Threading.Tasks;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.SystemWebAdapters;
using Microsoft.Extensions.Primitives;
using Microsoft.Net.Http.Headers;

namespace System.Web;
Expand All @@ -31,14 +32,14 @@ internal HttpCookieCollection(HttpResponse response)
response.UnwrapAdapter().OnStarting(static state =>
{
var response = (HttpResponse)state;
var cookies = response.UnwrapAdapter().Cookies;
var headers = response.UnwrapAdapter().Headers;
var isShareable = false;

for (var i = 0; i < response.Cookies.Count; i++)
{
if (response.Cookies[i] is { } cookie)
{
cookies.Append(cookie.Name, cookie.Value ?? string.Empty, cookie.ToCookieOptions());
headers.SetCookie = StringValues.Concat(headers.SetCookie, cookie.ToSetCookieHeaderValue().ToString());

isShareable |= cookie.Shareable;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ internal void FillFromString(string s, bool urlencoded = false, Encoding? encodi

public override string ToString() => ToString(true);

private string ToString(bool urlencoded)
internal string ToString(bool urlencoded)
{
int count = Count;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Linq;
using System.Net.Http;
using System.Threading.Tasks;
using System.Web;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.TestHost;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Hosting;
using Microsoft.Net.Http.Headers;
using Xunit;
using SameSiteMode = System.Web.SameSiteMode;

namespace Microsoft.AspNetCore.SystemWebAdapters.CoreServices.Tests;

[Collection(nameof(SelfHostedTests))]
public class ResponseHeaderTests
{
private readonly string ContentValue = Guid.NewGuid().ToString();

[Fact]
public async Task SetCookie()
{
using var result = await RunAsync(context =>
{
var cookie = new HttpCookie("test", ContentValue);
context.Response.Cookies.Add(cookie);
});

var cookies = result.Headers.GetValues(HeaderNames.SetCookie).ToList();
Assert.Single(cookies);
Assert.Equal($"test={ContentValue}; path=/; samesite=lax", cookies.First());
}

[Fact]
public async Task SetMultipleCookie()
{
// Arrange
var cookie1 = new HttpCookie("test1", Guid.NewGuid().ToString());
var cookie2 = new HttpCookie("test2", Guid.NewGuid().ToString());

// Act
using var result = await RunAsync(context =>
{
context.Response.Cookies.Add(cookie1);
context.Response.Cookies.Add(cookie2);
});

// Assert
var cookies = result.Headers.GetValues(HeaderNames.SetCookie).ToList();
Assert.Equal(2, cookies.Count);
Assert.Equal($"test1={cookie1.Value}; path=/; samesite=lax", cookies.First());
Assert.Equal($"test2={cookie2.Value}; path=/; samesite=lax", cookies.Last());
}

[Fact]
public async Task SetCookieWithPath()
{
using var result = await RunAsync(context =>
{
var cookie = new HttpCookie("test", ContentValue) { Path = "/abc" };
context.Response.Cookies.Add(cookie);
});

Assert.Equal($"test={ContentValue}; path=/abc; samesite=lax", result.Headers.GetValues(HeaderNames.SetCookie).First());
}

[Fact]
public async Task SetCookieSameSite()
{
using var result = await RunAsync(context =>
{
var cookie = new HttpCookie("test", ContentValue) { SameSite = SameSiteMode.None };
context.Response.Cookies.Add(cookie);
});

Assert.Equal($"test={ContentValue}; path=/; samesite=none", result.Headers.GetValues(HeaderNames.SetCookie).First());
}

[Fact]
public async Task SetCookieUrlencodedValue()
{
// Arrange
const string cookieValue = "hello|world";
var cookie = new HttpCookie("test", cookieValue) { SameSite = SameSiteMode.None };

// Act
using var result = await RunAsync(context =>
{
context.Response.Cookies.Add(cookie);
});

// Assert
Assert.Equal($"test={cookieValue}; path=/; samesite=none", result.Headers.GetValues(HeaderNames.SetCookie).First());
}

private static Task<HttpResponseMessage> RunAsync(Action<HttpContext> action, Action<IEndpointConventionBuilder>? builder = null)
=> RunAsync(ctx =>
{
action(ctx);
return Task.CompletedTask;
}, builder);

private static async Task<HttpResponseMessage> RunAsync(Func<HttpContext, Task> action, Action<IEndpointConventionBuilder>? builder = null)
{
builder ??= _ => { };

using var host = await new HostBuilder()
.ConfigureWebHost(webBuilder =>
{
webBuilder
.UseTestServer(options =>
{
options.AllowSynchronousIO = true;
})
.ConfigureServices(services =>
{
services.AddRouting();
services.AddSystemWebAdapters();
})
.Configure(app =>
{
app.UseRouting();
app.UseSystemWebAdapters();
app.UseEndpoints(endpoints =>
{
builder(endpoints.Map("/", (context) => action(context)));
});
});
})
.StartAsync();

var uri = new Uri("/", UriKind.Relative);

try
{
return await host.GetTestClient().GetAsync(uri).ConfigureAwait(false);
}
finally
{
await host.StopAsync();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -196,4 +196,49 @@ public void HasKeysAddSingleValue()
// Assert
Assert.True(result);
}

[Fact]
public void GetValueUrlencodedValue()
{
// Arrange
var value = $"{_fixture.Create<string>()}|${_fixture.Create<string>()}";

// Act
var cookie = new HttpCookie(_fixture.Create<string>(), value);

// Assert
Assert.Equal(value, cookie.Value);
}

[Fact]
public void GetValueUrlencodedValues()
{
// Arrange
var value1 = $"{_fixture.Create<string>()}|${_fixture.Create<string>()}";
var value2 = $"{_fixture.Create<string>()}|${_fixture.Create<string>()}";

// Act
var cookie = new HttpCookie(_fixture.Create<string>());
cookie.Values.Add(null, value1);
cookie.Values.Add(null, value2);

// Assert
Assert.Equal($"{value1}&{value2}", cookie.Value);
}

[Fact]
public void ValuesToStringUrlencodedValues()
{
// Arrange
var value1 = $"{_fixture.Create<string>()}|${_fixture.Create<string>()}";
var value2 = $"{_fixture.Create<string>()}|${_fixture.Create<string>()}";

// Act
var cookie = new HttpCookie(_fixture.Create<string>());
cookie.Values.Add(null, value1);
cookie.Values.Add(null, value2);

// Assert
Assert.Equal(HttpUtility.UrlEncode(value1) + "&" + HttpUtility.UrlEncode(value2), cookie.Values.ToString());
}
}