diff --git a/docs/guide/http/as-parameters.md b/docs/guide/http/as-parameters.md index a11ffb343..816acb9f1 100644 --- a/docs/guide/http/as-parameters.md +++ b/docs/guide/http/as-parameters.md @@ -253,6 +253,28 @@ member's default / initializer in both modes. The flag applies to query string b `[AsParameters]` members and for endpoint method arguments bound from the query string alike. Route argument binding is unaffected (an unparseable route value already returns `404`). +### Collections + +The flag covers **collection** query string parameters (`T[]`, `List`, `IList`, +`IReadOnlyList`, `IEnumerable`) too. With the flag off, an element that fails to parse is +silently dropped, which is especially dangerous for an optional filter: the endpoint sees `null` +(or a partial collection) and quietly returns an *unfiltered* `200` while the caller believes the +results were filtered. + +With the flag on, binding a collection is **all or nothing** — a single unparseable element +rejects the whole request with a `400` naming the parameter, rather than silently dropping the bad +element and keeping the good ones. For +`[FromQuery] public Colour[]? Colours { get; set; }` on `GET /widgets`: + +| Request | Flag off (default) | Flag on | +| --- | --- | --- | +| `GET /widgets?Colours=Red&Colours=Blue` | `200` — binds `[Red, Blue]` | `200` — binds `[Red, Blue]` | +| `GET /widgets` (missing) | `200` — keeps the initializer (`null`) | `200` — keeps the initializer (`null`) | +| `GET /widgets?Colours=Purple` | `200` — `Colours` is `null` | `400` — ProblemDetails naming `Colours` | +| `GET /widgets?Colours=Red&Colours=Purple` | `200` — binds only `[Red]` | `400` — ProblemDetails naming `Colours` | + +String collections are unaffected in both modes, as there is nothing to parse. + ::: warning `RejectUnparseableQueryValues` is opt-in (defaults to `false`) throughout Wolverine 6.x to preserve the previous lenient behavior, but the default flips to `true` (strict) in Wolverine 7.0. If you diff --git a/docs/guide/http/querystring.md b/docs/guide/http/querystring.md index bca081623..b195707eb 100644 --- a/docs/guide/http/querystring.md +++ b/docs/guide/http/querystring.md @@ -7,8 +7,9 @@ cannot be parsed -- the value passed to your method will be the `default` for wh type is. If you want a *present but unparseable* query string value to return a `400 Bad Request` instead (matching ASP.NET Core minimal APIs), opt into `WolverineHttpOptions.RejectUnparseableQueryValues` — see -[Strict Query String Binding](./as-parameters#strict-query-string-binding). The strict behavior -becomes the default in Wolverine 7.0. +[Strict Query String Binding](./as-parameters#strict-query-string-binding). That flag also covers +collection query string parameters, where an unparseable element otherwise gets silently dropped. +The strict behavior becomes the default in Wolverine 7.0. ::: Wolverine supports passing query string values to your HTTP method arguments for diff --git a/src/Http/Wolverine.Http.Tests/strict_query_string_binding.cs b/src/Http/Wolverine.Http.Tests/strict_query_string_binding.cs index 73d7dde17..b7881f304 100644 --- a/src/Http/Wolverine.Http.Tests/strict_query_string_binding.cs +++ b/src/Http/Wolverine.Http.Tests/strict_query_string_binding.cs @@ -247,6 +247,148 @@ public async Task missing_values_keep_initializers_when_flag_is_off() response.PageSize.ShouldBe(5); response.Token.ShouldBe(StrictQueryModel.DefaultToken); } + + // GH-3398: the collection case. An unparseable *element* of a multi-valued query string + // parameter was silently dropped, taking the whole collection to null/empty. That is worse + // than a plain 400: an optional filter predicate silently disappears and the endpoint answers + // 200 with an unfiltered result set. + + private Task expectCollection400(Action configure) + { + return _fixture.StrictHost.Scenario(x => + { + configure(x); + x.StatusCodeShouldBe(400); + x.ContentTypeShouldBe("application/problem+json"); + }); + } + + [Fact] + public async Task malformed_enum_array_element_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections").QueryString("Colours", "Purple")); + + var text = await result.ReadAsTextAsync(); + text.ShouldContain("Colours"); + text.ShouldContain("Purple"); + } + + [Fact] + public async Task one_bad_element_rejects_the_whole_collection() + { + // All-or-nothing: a valid element does not rescue the request. Silently keeping "Red" and + // dropping "Purple" would still be a silently wrong answer. + var result = await _fixture.StrictHost.Scenario(x => + { + x.Get.Url("/strict/collections?Colours=Red&Colours=Purple"); + x.StatusCodeShouldBe(400); + x.ContentTypeShouldBe("application/problem+json"); + }); + + var text = await result.ReadAsTextAsync(); + text.ShouldContain("Colours"); + text.ShouldContain("Purple"); + } + + [Fact] + public async Task malformed_int_array_element_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections").QueryString("Ids", "abc")); + + (await result.ReadAsTextAsync()).ShouldContain("Ids"); + } + + [Fact] + public async Task malformed_guid_array_element_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections").QueryString("Tokens", "not-a-guid")); + + (await result.ReadAsTextAsync()).ShouldContain("Tokens"); + } + + [Fact] + public async Task malformed_generic_list_element_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections").QueryString("Pages", "abc")); + + (await result.ReadAsTextAsync()).ShouldContain("Pages"); + } + + [Fact] + public async Task malformed_collection_element_on_direct_method_argument_returns_400() + { + var result = await expectCollection400(x => + x.Get.Url("/strict/collections/direct").QueryString("numbers", "abc")); + + (await result.ReadAsTextAsync()).ShouldContain("numbers"); + } + + [Fact] + public async Task valid_collection_values_bind_in_strict_mode() + { + var token = Guid.NewGuid(); + + var result = await _fixture.StrictHost.Scenario(x => + { + x.Get.Url($"/strict/collections?Colours=Red&Colours=Blue&Ids=1&Ids=2&Tokens={token}&Pages=3"); + x.StatusCodeShouldBe(200); + }); + + var response = await result.ReadAsJsonAsync(); + response.Colours.ShouldBe([StrictColour.Red, StrictColour.Blue]); + response.Ids.ShouldBe([1, 2]); + response.Tokens.ShouldBe([token]); + response.Pages.ShouldBe([3]); + } + + [Fact] + public async Task omitted_collections_keep_initializers_in_strict_mode() + { + var result = await _fixture.StrictHost.Scenario(x => + { + x.Get.Url("/strict/collections"); + x.StatusCodeShouldBe(200); + }); + + var response = await result.ReadAsJsonAsync(); + response.Colours.ShouldBeNull(); + response.Ids.ShouldBeNull(); + response.Tokens.ShouldBeNull(); + response.Pages.ShouldBeEmpty(); + } + + [Fact] + public async Task malformed_collection_values_are_lenient_when_flag_is_off() + { + // The pre-3398 lenient behavior is preserved by default in 6.x: bad elements are dropped + var result = await _fixture.LenientHost.Scenario(x => + { + x.Get.Url("/strict/collections?Colours=Purple&Ids=abc&Pages=abc"); + x.StatusCodeShouldBe(200); + }); + + var response = await result.ReadAsJsonAsync(); + response.Colours.ShouldBeNull(); + response.Ids.ShouldBeNull(); + response.Pages.ShouldBeEmpty(); + } + + [Fact] + public async Task partially_malformed_collection_values_are_lenient_when_flag_is_off() + { + var result = await _fixture.LenientHost.Scenario(x => + { + x.Get.Url("/strict/collections?Colours=Red&Colours=Purple"); + x.StatusCodeShouldBe(200); + }); + + var response = await result.ReadAsJsonAsync(); + response.Colours.ShouldBe([StrictColour.Red]); + } } public enum StrictSortOrder @@ -255,6 +397,28 @@ public enum StrictSortOrder Date = 2 } +public enum StrictColour +{ + Red, + Green, + Blue +} + +public class StrictCollectionQueryModel +{ + [FromQuery] + public StrictColour[]? Colours { get; set; } + + [FromQuery] + public int[]? Ids { get; set; } + + [FromQuery] + public Guid[]? Tokens { get; set; } + + [FromQuery] + public List Pages { get; set; } = []; +} + public class StrictQueryModel { public static readonly Guid DefaultToken = Guid.Parse("11111111-2222-3333-4444-555555555555"); @@ -291,4 +455,16 @@ public static string GetDirect(int number) { return number.ToString(); } + + [WolverineGet("/strict/collections")] + public static StrictCollectionQueryModel GetCollections([AsParameters] StrictCollectionQueryModel query) + { + return query; + } + + [WolverineGet("/strict/collections/direct")] + public static string GetCollectionDirect(int[] numbers) + { + return numbers.Length.ToString(); + } } diff --git a/src/Http/Wolverine.Http/CodeGen/ParsedArrayQueryStringValue.cs b/src/Http/Wolverine.Http/CodeGen/ParsedArrayQueryStringValue.cs index dc3ed912d..bb8f7cf67 100644 --- a/src/Http/Wolverine.Http/CodeGen/ParsedArrayQueryStringValue.cs +++ b/src/Http/Wolverine.Http/CodeGen/ParsedArrayQueryStringValue.cs @@ -8,10 +8,20 @@ namespace Wolverine.Http.CodeGen; internal class ParsedArrayQueryStringValue : SyncFrame, IReadHttpFrame { private string? _property; + private readonly bool _rejectUnparseableValue; - public ParsedArrayQueryStringValue(Type parameterType, string parameterName) + public ParsedArrayQueryStringValue(Type parameterType, string parameterName, bool rejectUnparseableValue = false) { Variable = new HttpElementVariable(parameterType, parameterName!, this); + + // GH-3398 strict query string binding for collections: only meaningful for parsed + // (non-string) elements. The 400 + ProblemDetails short circuit writes the response + // asynchronously, so the frame has to force the generated method into async mode. + _rejectUnparseableValue = rejectUnparseableValue && parameterType.GetElementType() != typeof(string); + if (_rejectUnparseableValue) + { + IsAsync = true; + } } public void AssignToProperty(string usage) @@ -63,8 +73,10 @@ public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) writer.Write($"{Variable.Usage}_List.Add({Variable.Usage}ValueParsed);"); writer.FinishBlock(); // parsing block + writeRejectionBlock(method, writer, $"{Variable.Usage}Value", elementAlias); + writer.FinishBlock(); // foreach blobck - + if (Mode == AssignMode.WriteToVariable) { writer.Write($"var {Variable.Usage} = {Variable.Usage}_List.ToArray();"); @@ -77,4 +89,15 @@ public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) Next?.GenerateCode(method, writer); } + + private void writeRejectionBlock(GeneratedMethod method, ISourceWriter writer, string valueUsage, + string elementAlias) + { + if (!_rejectUnparseableValue) + { + return; + } + + StrictQueryBinding.WriteRejectionBlock(method, writer, Variable.Name, valueUsage, elementAlias); + } } \ No newline at end of file diff --git a/src/Http/Wolverine.Http/CodeGen/QueryStringHandling.cs b/src/Http/Wolverine.Http/CodeGen/QueryStringHandling.cs index 72713b58e..505830f55 100644 --- a/src/Http/Wolverine.Http/CodeGen/QueryStringHandling.cs +++ b/src/Http/Wolverine.Http/CodeGen/QueryStringHandling.cs @@ -21,14 +21,48 @@ public interface IGeneratesCode void GenerateCode(GeneratedMethod method, ISourceWriter writer); } +/// +/// Shared emission for the GH-3372 / GH-3398 opt-in strict query string binding. A query string +/// value that is present but cannot be parsed short circuits the request with a 400 ProblemDetails +/// naming the offending parameter, matching ASP.NET Core minimal API binding. +/// +internal static class StrictQueryBinding +{ + /// + /// Emits the "else" arm of a collection element's TryParse: a present but unparseable element + /// rejects the entire binding. All-or-nothing is deliberate -- keeping the parseable elements + /// and dropping the bad one would still hand the endpoint a silently wrong filter (GH-3398). + /// + public static void WriteRejectionBlock(GeneratedMethod method, ISourceWriter writer, string parameterName, + string valueUsage, string elementAlias) + { + writer.Write($"BLOCK:else if (!string.IsNullOrEmpty({valueUsage}))"); + writer.Write( + $"await {nameof(HttpHandler.WriteQueryValueParsingProblem)}(httpContext, \"{parameterName}\", {valueUsage}, \"{elementAlias}\").ConfigureAwait(false);"); + writer.Write(method.ToExitStatement()); + writer.FinishBlock(); + } +} + internal class ParsedCollectionQueryStringValue : SyncFrame, IReadHttpFrame { private readonly Type _collectionElementType; + private readonly bool _rejectUnparseableValue; - public ParsedCollectionQueryStringValue(Type parameterType, string parameterName) + public ParsedCollectionQueryStringValue(Type parameterType, string parameterName, + bool rejectUnparseableValue = false) { Variable = new HttpElementVariable(parameterType, parameterName!, this); _collectionElementType = GetCollectionElementType(parameterType); + + // GH-3398 strict query string binding for collections: only meaningful for parsed + // (non-string) elements. The 400 + ProblemDetails short circuit writes the response + // asynchronously, so the frame has to force the generated method into async mode. + _rejectUnparseableValue = rejectUnparseableValue && _collectionElementType != typeof(string); + if (_rejectUnparseableValue) + { + IsAsync = true; + } } public HttpElementVariable Variable { get; } @@ -72,6 +106,12 @@ public override void GenerateCode(GeneratedMethod method, ISourceWriter writer) writer.Write($"{Variable.Usage}.Add({Variable.Name}ValueParsed);"); writer.FinishBlock(); // parsing block + if (_rejectUnparseableValue) + { + StrictQueryBinding.WriteRejectionBlock(method, writer, Variable.Name, $"{Variable.Name}Value", + elementAlias); + } + writer.FinishBlock(); // foreach blobck } diff --git a/src/Http/Wolverine.Http/HttpChain.cs b/src/Http/Wolverine.Http/HttpChain.cs index 9e46036ba..96f3ff1ff 100644 --- a/src/Http/Wolverine.Http/HttpChain.cs +++ b/src/Http/Wolverine.Http/HttpChain.cs @@ -712,14 +712,16 @@ public bool FindQuerystringVariable(Type variableType, string routeOrParameterNa if (parameterType.IsArray && RouteParameterStrategy.CanParse(parameterType.GetElementType()!)) { - variable = new ParsedArrayQueryStringValue(parameterType, key).Variable; + variable = new ParsedArrayQueryStringValue(parameterType, key, + rejectUnparseableValue: _parent.RejectUnparseableQueryValues).Variable; variable.Name = key; _querystringVariables.Add(variable); } if (ParsedCollectionQueryStringValue.CanParse(parameterType)) { - variable = new ParsedCollectionQueryStringValue(parameterType, key).Variable; + variable = new ParsedCollectionQueryStringValue(parameterType, key, + rejectUnparseableValue: _parent.RejectUnparseableQueryValues).Variable; variable.Name = key; _querystringVariables.Add(variable); } diff --git a/src/Http/Wolverine.Http/WolverineHttpOptions.cs b/src/Http/Wolverine.Http/WolverineHttpOptions.cs index bd9941fe1..7a4c33d23 100644 --- a/src/Http/Wolverine.Http/WolverineHttpOptions.cs +++ b/src/Http/Wolverine.Http/WolverineHttpOptions.cs @@ -206,9 +206,12 @@ public void UseApiVersioning(Action configure) /// This matches the behavior of ASP.NET Core minimal API parameter binding. A missing /// query string value still binds the parameter's default / property initializer value in /// either mode. Applies to query string binding for endpoint method arguments and for - /// [AsParameters] members bound from the query string. This is currently opt-in + /// [AsParameters] members bound from the query string, including collection parameters + /// (arrays, List<T>, IEnumerable<T>, etc.) where a single unparseable element rejects the + /// entire binding. This is currently opt-in /// (default false) to preserve the previous lenient behavior, but the default flips /// to true (strict) in Wolverine 7.0. See https://github.com/JasperFx/wolverine/issues/3372 + /// and https://github.com/JasperFx/wolverine/issues/3398 /// public bool RejectUnparseableQueryValues { get; set; }