AspNetWebStack icon indicating copy to clipboard operation
AspNetWebStack copied to clipboard

FormUrlEncodedJson calls wrong overload of Error.Argument

Open KalleOlaviNiemitalo opened this issue 8 months ago • 1 comments

src/System.Net.Http.Formatting/Formatting/FormUrlEncodedJson.cs contains several Error.Argument calls that do not specify the parameter name. For example, in the FormUrlEncodedJson.AddToArray method:

https://github.com/aspnet/AspNetWebStack/blob/1231b77d79956152831b75ad7f094f844251b97f/src/System.Net.Http.Formatting/Formatting/FormUrlEncodedJson.cs#L400-L404

The arguments in this call are a format string from resources, and a string to use as a format argument. The call is intended to go to the internal static ArgumentException Argument(string messageFormat, params object[] messageArgs) method that is defined here:

https://github.com/aspnet/AspNetWebStack/blob/a55e96a451393aa854556ac9feb2d80e81988a9c/src/Common/Error.cs#L37-L40

Instead, it goes to the internal static ArgumentException Argument(string parameterName, string messageFormat, params object[] messageArgs) method that is defined here:

https://github.com/aspnet/AspNetWebStack/blob/a55e96a451393aa854556ac9feb2d80e81988a9c/src/Common/Error.cs#L49-L52

The format string that was read from Properties.Resources.FormUrlEncodedMismatchingTypes thus becomes misused as the parameter name.

I have not tried to reproduce this bug in practice, but the incorrect call is evident from the IL disassembly of lib/netstandard2.0/System.Net.Http.Formatting.dll in the Microsoft.AspNet.WebApi.Client 6.0.0 package:

  IL_001b:  call       string System.Net.Http.Properties.Resources::get_FormUrlEncodedMismatchingTypes()
  IL_0020:  ldarg.1
  IL_0021:  ldarg.1
  IL_0022:  ldlen
  IL_0023:  conv.i4
  IL_0024:  ldc.i4.1
  IL_0025:  sub
  IL_0026:  call       string System.Net.Http.Formatting.FormUrlEncodedJson::BuildPathString(string[],
                                                                                             int32)
  IL_002b:  call       !!0[] [netstandard]System.Array::Empty<object>()
  IL_0030:  call       class [netstandard]System.ArgumentException System.Web.Http.Error::Argument(string,
                                                                                                   string,
                                                                                                   object[])
  IL_0035:  throw

In the Microsoft.AspNet.WebApi.Client 4.0.20505 package (released on 31 May 2012), the FormUrlEncodedJson.AddToArray method did not have this bug yet. In the Microsoft.AspNet.WebApi.Client 4.0.20710 package (released on 11 August 2012), the method had the bug. I think the bug was introduced by commit f19f4683cc36ebb2666fa36f6dcd1825f4b214aa in May 2012.

Because the bug is so old and is not known to cause any problems in practice, I suspect you might decide not to fix it.

KalleOlaviNiemitalo avatar May 02 '25 10:05 KalleOlaviNiemitalo

For fixing this, I suggest deleting the internal static ArgumentException Argument(string messageFormat, params object[] messageArgs) method and changing FormUrlEncodedJson to pass null as string parameterName, like throw Error.Argument(null, Properties.Resources.FormUrlEncodedMismatchingTypes, BuildPathString(path, path.Length - 1)).

Because the method is internal, deleting it would not change the public API. As for whether other assemblies use the method via InternalsVisibleToAttribute such that deploying a mix of package versions could cause MissingMethodException at run time:

  • All InternalsVisibleToAttribute instances in the v3.3.0 source tree refer to test assemblies, except the internals of System.Web.WebPages are also visible to System.Web.Mvc and System.Web.Helpers.
  • src/System.Web.WebPages/System.Web.WebPages.csproj does not include ..\Common\Error.cs.
  • System.Web.Mvc has its own class Error at src/Microsoft.Web.Mvc/Error.cs.
  • src/System.Web.Helpers/ does not mention the Error identifier.

KalleOlaviNiemitalo avatar May 02 '25 10:05 KalleOlaviNiemitalo