System.Net.Http.DiagnosticsHandler.SendAsync throws if Activity.Current.Baggage contains values that can't be parsed by NameValueHeaderValue after UrlEncoding

[alexperovich]

Description

This old issue #27106, was claimed to be fixed... but it is not fixed. In reality, the UrlEncoding used to "fix" the above issue breaks my workaround of quoting all the values.
This can be reproduced with:

var act = new Activity("test");
act.AddBaggage("test", "\"HelixAPI.Pages.Account.AccountController.SignIn (HelixAPI)\"");
act.Start();
try
{
	using (var c = new HttpClient())
	{
		await c.GetStringAsync("https://google.com");
	}
}
finally
{
	act.Stop();
}

This code throws the following exception

FormatException: The format of value '%22HelixAPI.Pages.Account.AccountController.SignIn+(HelixAPI)%22' is invalid.

Configuration

.NET Core 3.1, on windows 10
This isn't specific to any configuration

Regression?

This was a bug in older versions of .net core, it is still broken.

Other information

The bug is here: https://source.dot.net/#System.Net.Http/System/Net/Http/DiagnosticsHandler.cs,284

This should 100% not be using something that explodes when strange values are passed. This means that anywhere in the code of any program, if something completely benign is added to Activity.Current.Baggage, any HttpClient calls will fail.

If using NameValueHeaderValue is important for some reason.... at least it shouldn't crash the entire asp.net request because of a single baggage thingy.

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[karelz]

@lmolkova can you please take a look at this one?

[nnnvvvcv]

Hi,

Just wondering whether there has been any progress on this?

The work around I am using is to check the validity of the baggage strings before adding to the activity:

public static bool IsValidBaggageString(string value)
{
    return NameValueHeaderValue.TryParse(WebUtility.UrlEncode(value), out _)
}


. . .

    if (IsValidBaggageString(key) && IsValidBaggageString(value))
    {
        Activity.Current?.AddBaggage(key, value);
    }

. . .

It's not ideal because we end up losing data in our Baggage.

By the way - it seems in my case and the original poster's case that the offending characters are the braces (). Not sure if that helps at all...

Nick.

[MihaZupan]

We should really be using Uri.EscapeDataString instead of WebUtility.UrlEncode here.

[MihaZupan]

Fix for this issue would be to use Uri.EscapeDataString instead of WebUtility.UrlEncode here:

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/DistributedContextPropagator.cs

Line 120 in 5399ee5

baggageList.Append(WebUtility.UrlEncode(item.Key)).Append('=').Append(WebUtility.UrlEncode(item.Value)).Append(CommaWithSpace);

And Uri.UnescapeDataString instead of WebUtility.UrlDecode here:

runtime/src/libraries/System.Diagnostics.DiagnosticSource/src/System/Diagnostics/LegacyPropagator.cs

Lines 172 to 173 in 5399ee5

	WebUtility.UrlDecode(baggageString.Substring(keyStart, keyEnd - keyStart)).Trim(s_trimmingSpaceCharacters),
	WebUtility.UrlDecode(baggageString.Substring(valueStart, currentIndex - valueStart)).Trim(s_trimmingSpaceCharacters)));

Marking as up-for-grabs since it should be as simple as replacing the two calls and writing a test.

Moving to diagnostics area since all the changes belong in DistributedContextPropagator logic now.