diff --git a/NuGet.config b/NuGet.config index 1c2f27eb90ce..6d18aebaa59c 100644 --- a/NuGet.config +++ b/NuGet.config @@ -6,8 +6,11 @@ + + + @@ -28,8 +31,11 @@ + + + diff --git a/eng/Version.Details.xml b/eng/Version.Details.xml index 0211579c16c8..96242d41c625 100644 --- a/eng/Version.Details.xml +++ b/eng/Version.Details.xml @@ -9,37 +9,37 @@ --> - + https://dev.azure.com/dnceng/internal/_git/dotnet-efcore - f119d39bcc0bc6c50f0a5e1965a145878b2c13bd + 983cf3c21c0f9484cc0ed66e61b8c252d701a30b - + https://dev.azure.com/dnceng/internal/_git/dotnet-efcore - f119d39bcc0bc6c50f0a5e1965a145878b2c13bd + 983cf3c21c0f9484cc0ed66e61b8c252d701a30b - + https://dev.azure.com/dnceng/internal/_git/dotnet-efcore - f119d39bcc0bc6c50f0a5e1965a145878b2c13bd + 983cf3c21c0f9484cc0ed66e61b8c252d701a30b - + https://dev.azure.com/dnceng/internal/_git/dotnet-efcore - f119d39bcc0bc6c50f0a5e1965a145878b2c13bd + 983cf3c21c0f9484cc0ed66e61b8c252d701a30b - + https://dev.azure.com/dnceng/internal/_git/dotnet-efcore - f119d39bcc0bc6c50f0a5e1965a145878b2c13bd + 983cf3c21c0f9484cc0ed66e61b8c252d701a30b - + https://dev.azure.com/dnceng/internal/_git/dotnet-efcore - f119d39bcc0bc6c50f0a5e1965a145878b2c13bd + 983cf3c21c0f9484cc0ed66e61b8c252d701a30b - + https://dev.azure.com/dnceng/internal/_git/dotnet-efcore - f119d39bcc0bc6c50f0a5e1965a145878b2c13bd + 983cf3c21c0f9484cc0ed66e61b8c252d701a30b - + https://dev.azure.com/dnceng/internal/_git/dotnet-efcore - f119d39bcc0bc6c50f0a5e1965a145878b2c13bd + 983cf3c21c0f9484cc0ed66e61b8c252d701a30b https://dev.azure.com/dnceng/internal/_git/dotnet-runtime @@ -121,9 +121,9 @@ https://dev.azure.com/dnceng/internal/_git/dotnet-runtime 5535e31a712343a63f5d7d796cd874e563e5ac14 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 https://dev.azure.com/dnceng/internal/_git/dotnet-runtime @@ -185,9 +185,9 @@ https://dev.azure.com/dnceng/internal/_git/dotnet-runtime 5535e31a712343a63f5d7d796cd874e563e5ac14 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 https://github.com/dotnet/source-build-externals @@ -275,17 +275,17 @@ https://dev.azure.com/dnceng/internal/_git/dotnet-runtime 81cabf2857a01351e5ab578947c7403a5b128ad1 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 https://dev.azure.com/dnceng/internal/_git/dotnet-runtime @@ -316,22 +316,22 @@ Win-x64 is used here because we have picked an arbitrary runtime identifier to flow the version of the latest NETCore.App runtime. All Runtime.$rid packages should have the same version. --> - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 https://github.com/dotnet/xdt @@ -368,9 +368,9 @@ - + https://dev.azure.com/dnceng/internal/_git/dotnet-runtime - a6bde67c455f2ac219988c7a66171631090b6f65 + 46295af5828b062bbbf93a9cef50fd8cb9fbcb09 https://github.com/dotnet/winforms diff --git a/eng/Versions.props b/eng/Versions.props index 106e7560399f..4e98e2dc3715 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -66,12 +66,12 @@ 8.0.2 - 8.0.27 - 8.0.27 - 8.0.27 - 8.0.27 - 8.0.27 - 8.0.27-servicing.26229.22 + 8.0.28 + 8.0.28 + 8.0.28 + 8.0.28 + 8.0.28 + 8.0.28-servicing.26264.13 8.0.0 8.0.1 8.0.0 @@ -92,7 +92,7 @@ 8.0.0 8.0.0 8.0.0 - 8.0.27-servicing.26229.22 + 8.0.28-servicing.26264.13 8.0.1 8.0.1 8.0.1 @@ -108,7 +108,7 @@ 8.0.0 8.0.2 8.0.0 - 8.0.27-servicing.26229.22 + 8.0.28-servicing.26264.13 8.0.1 8.0.1 8.0.2 @@ -128,9 +128,9 @@ 8.0.0 8.0.0 8.0.0 - 8.0.27-servicing.26229.22 + 8.0.28-servicing.26264.13 - 8.0.27-servicing.26229.22 + 8.0.28-servicing.26264.13 8.0.0 8.0.1 @@ -142,14 +142,14 @@ 9.0.0-preview.9.24518.1 9.0.0-preview.9.24518.1 - 8.0.27 - 8.0.27 - 8.0.27 - 8.0.27 - 8.0.27 - 8.0.27 - 8.0.27 - 8.0.27 + 8.0.28 + 8.0.28 + 8.0.28 + 8.0.28 + 8.0.28 + 8.0.28 + 8.0.28 + 8.0.28 4.8.0-7.24574.2 4.8.0-7.24574.2 @@ -291,7 +291,7 @@ 2.57.0 2.57.0 2.57.0 - 2.5.187 + 2.5.302 3.2.0 3.2.0 3.2.0 diff --git a/global.json b/global.json index 0dbc21ab3076..6aaf098fe31f 100644 --- a/global.json +++ b/global.json @@ -1,9 +1,9 @@ { "sdk": { - "version": "8.0.126" + "version": "8.0.128" }, "tools": { - "dotnet": "8.0.126", + "dotnet": "8.0.128", "runtimes": { "dotnet/x86": [ "$(MicrosoftNETCoreBrowserDebugHostTransportVersion)" diff --git a/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/CallHandlers/CallHandlerDescriptorInfo.cs b/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/CallHandlers/CallHandlerDescriptorInfo.cs index b9c1b70c78c8..90a632ffed3c 100644 --- a/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/CallHandlers/CallHandlerDescriptorInfo.cs +++ b/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/CallHandlers/CallHandlerDescriptorInfo.cs @@ -24,7 +24,14 @@ public CallHandlerDescriptorInfo( BodyFieldDescriptor = bodyFieldDescriptor; RouteParameterDescriptors = routeParameterDescriptors; RouteAdapter = routeAdapter; - PathDescriptorsCache = new ConcurrentDictionary?>(); + PathDescriptorsCache = new ConcurrentDictionary>(); + + var jsonPaths = new HashSet(StringComparer.Ordinal); + foreach (var routeParameter in routeParameterDescriptors.Values) + { + jsonPaths.Add(routeParameter.JsonPath); + } + RouteParameterJsonPaths = jsonPaths; } public FieldDescriptor? ResponseBodyDescriptor { get; } @@ -34,5 +41,6 @@ public CallHandlerDescriptorInfo( public FieldDescriptor? BodyFieldDescriptor { get; } public Dictionary RouteParameterDescriptors { get; } public JsonTranscodingRouteAdapter RouteAdapter { get; } - public ConcurrentDictionary?> PathDescriptorsCache { get; } + public ConcurrentDictionary> PathDescriptorsCache { get; } + public HashSet RouteParameterJsonPaths { get; } } diff --git a/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/JsonRequestHelpers.cs b/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/JsonRequestHelpers.cs index ef0ec6d3f0cc..9e4ba1480ba4 100644 --- a/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/JsonRequestHelpers.cs +++ b/src/Grpc/JsonTranscoding/src/Microsoft.AspNetCore.Grpc.JsonTranscoding/Internal/JsonRequestHelpers.cs @@ -341,11 +341,20 @@ private static async ValueTask ReadDataAsync(JsonTranscodingServerCallCo private static List? GetPathDescriptors(JsonTranscodingServerCallContext serverCallContext, IMessage requestMessage, string path) { - return serverCallContext.DescriptorInfo.PathDescriptorsCache.GetOrAdd(path, p => + // Must not add null values for paths that don't resolve to a descriptor + var cache = serverCallContext.DescriptorInfo.PathDescriptorsCache; + if (cache.TryGetValue(path, out var pathDescriptors)) { - ServiceDescriptorHelpers.TryResolveDescriptors(requestMessage.Descriptor, p.Split('.'), allowJsonName: true, out var pathDescriptors); return pathDescriptors; - }); + } + + if (ServiceDescriptorHelpers.TryResolveDescriptors(requestMessage.Descriptor, path.Split('.'), allowJsonName: true, out pathDescriptors)) + { + cache.TryAdd(path, pathDescriptors); + return pathDescriptors; + } + + return null; } public static async ValueTask SendMessage(JsonTranscodingServerCallContext serverCallContext, JsonSerializerOptions serializerOptions, TResponse message, CancellationToken cancellationToken) where TResponse : class @@ -388,24 +397,25 @@ private static bool CanBindQueryStringVariable(JsonTranscodingServerCallContext { if (serverCallContext.DescriptorInfo.BodyDescriptor != null) { - var bodyFieldName = serverCallContext.DescriptorInfo.BodyFieldDescriptor?.Name; + var bodyFieldDescriptor = serverCallContext.DescriptorInfo.BodyFieldDescriptor; - // Null field name indicates "*" which means the entire message is bound to the body. - if (bodyFieldName == null) + // Null field descriptor indicates "*" which means the entire message is bound to the body. + if (bodyFieldDescriptor?.Name is null) { return false; } - // Exact match - if (variable == bodyFieldName) + var bodyFieldName = bodyFieldDescriptor.Name; + var bodyFieldJsonName = bodyFieldDescriptor.JsonName; + + // Exact match (proto name or JSON name) + if (variable == bodyFieldName || variable == bodyFieldJsonName) { return false; } - // Nested field of field name. - if (bodyFieldName.Length + 1 < variable.Length && - variable.StartsWith(bodyFieldName, StringComparison.Ordinal) && - variable[bodyFieldName.Length] == '.') + // Nested field of body field (proto name prefix or JSON name prefix). + if (IsNestedBodyField(variable, bodyFieldName) || IsNestedBodyField(variable, bodyFieldJsonName)) { return false; } @@ -416,6 +426,20 @@ private static bool CanBindQueryStringVariable(JsonTranscodingServerCallContext return false; } + // Also check JSON name aliases. Route parameter keys use proto names (e.g. "user_id"), + // but query parameters can use JSON names (e.g. "userId") which resolve to the same field. + if (serverCallContext.DescriptorInfo.RouteParameterJsonPaths.Contains(variable)) + { + return false; + } + return true; } + + private static bool IsNestedBodyField(string variable, string bodyFieldName) + { + return bodyFieldName.Length + 1 < variable.Length && + variable.StartsWith(bodyFieldName, StringComparison.Ordinal) && + variable[bodyFieldName.Length] == '.'; + } } diff --git a/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.IntegrationTests/RouteTests.cs b/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.IntegrationTests/RouteTests.cs index c3482d12264d..d3bef8922333 100644 --- a/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.IntegrationTests/RouteTests.cs +++ b/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.IntegrationTests/RouteTests.cs @@ -105,6 +105,35 @@ Task UnaryMethod(ComplextHelloRequest request, ServerCallContext con Assert.Equal("Hello complex_greeter/test2/b last_name!", result.RootElement.GetProperty("message").GetString()); } + [Fact] + public async Task ComplexParameter_NestedJsonNameQueryString_DoesNotOverwriteRouteValue() + { + // Arrange + Task UnaryMethod(ComplextHelloRequest request, ServerCallContext context) + { + return Task.FromResult(new HelloReply { Message = $"Hello {request.Name.FirstName} {request.Name.LastName}!" }); + } + var method = Fixture.DynamicGrpc.AddUnaryMethod( + UnaryMethod, + Greeter.Descriptor.FindMethodByName("SayHelloComplexCatchAll3")); + var client = new HttpClient(Fixture.Handler) { BaseAddress = new Uri("http://localhost") }; + + // route.binding sets + // request.Name.LastName = "last_name"; + // request.Name.FirstName = "complex_greeter/test2/b" + // ---- + // query binding tries to overwrite with + // name.firstName=query_first + // name.lastName=query_last + + // Act + var response = await client.GetAsync("/v1/last_name/complex_greeter/test2/b/c/d/two?name.firstName=query_first&name.lastName=query_last").DefaultTimeout(); + var responseStream = await response.Content.ReadAsStreamAsync(); + using var result = await JsonDocument.ParseAsync(responseStream); + // Assert + Assert.Equal("Hello complex_greeter/test2/b last_name!", result.RootElement.GetProperty("message").GetString()); + } + [Fact] public async Task SimpleCatchAllParameter_PrefixSuffixSlashes_MatchUrl_SuccessResult() { diff --git a/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/Proto/transcoding.proto b/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/Proto/transcoding.proto index e44535586bba..7b817554e737 100644 --- a/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/Proto/transcoding.proto +++ b/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/Proto/transcoding.proto @@ -218,6 +218,7 @@ message HelloRequest { string hiding_field_name = 24 [json_name="field_name"]; repeated SubMessage repeated_messages = 25; map map_keyint_valueint = 26; + SubMessage sub_data = 27; } message HelloReply { diff --git a/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/UnaryServerCallHandlerTests.cs b/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/UnaryServerCallHandlerTests.cs index 561a002bb8f7..23d7565502b6 100644 --- a/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/UnaryServerCallHandlerTests.cs +++ b/src/Grpc/JsonTranscoding/test/Microsoft.AspNetCore.Grpc.JsonTranscoding.Tests/UnaryServerCallHandlerTests.cs @@ -40,6 +40,12 @@ private static RouteParameter CreateRouteParameter(List descrip return new RouteParameter(descriptorPath, new HttpRouteVariable(), string.Empty); } + private static RouteParameter CreateRouteParameterWithJsonPath(List descriptorPath) + { + var jsonPath = string.Join(".", descriptorPath.Select(d => d.JsonName)); + return new RouteParameter(descriptorPath, new HttpRouteVariable(), jsonPath); + } + [Fact] public async Task HandleCallAsync_MatchingRouteValue_SetOnRequestMessage() { @@ -1345,6 +1351,46 @@ public async Task HandleCallAsync_MatchingRepeatedQueryStringValues_SetOnRequest Assert.Equal("TestSubfields2!", request!.Sub.Subfields[1]); } + [Fact] + public async Task HandleCallAsync_UnmatchedQueryStringValues_NotCached() + { + // Arrange + HelloRequest? request = null; + UnaryServerMethod invoker = (s, r, c) => + { + request = r; + return Task.FromResult(new HelloReply()); + }; + + var descriptorInfo = TestHelpers.CreateDescriptorInfo(); + var unaryServerCallHandler = CreateCallHandler(invoker, descriptorInfo); + var httpContext = TestHelpers.CreateHttpContext(); + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["age"] = "10", + ["sub.subfield"] = "TestSubfield!", + ["unknown"] = "value", + ["sub.unknown"] = "value", + ["name.unknown"] = "value" + }); + + // Act + await unaryServerCallHandler.HandleCallAsync(httpContext); + + // Assert + Assert.NotNull(request); + Assert.Equal(10, request!.Age); + Assert.Equal("TestSubfield!", request!.Sub.Subfield); + // matches query params + Assert.Equal(2, descriptorInfo.PathDescriptorsCache.Count); + Assert.True(descriptorInfo.PathDescriptorsCache.ContainsKey("age")); + Assert.True(descriptorInfo.PathDescriptorsCache.ContainsKey("sub.subfield")); + // not matched query params + Assert.False(descriptorInfo.PathDescriptorsCache.ContainsKey("unknown")); + Assert.False(descriptorInfo.PathDescriptorsCache.ContainsKey("sub.unknown")); + Assert.False(descriptorInfo.PathDescriptorsCache.ContainsKey("name.unknown")); + } + [Fact] public async Task HandleCallAsync_DataTypes_SetOnRequestMessage() { @@ -1644,6 +1690,186 @@ public async Task HandleCallAsync_MatchingQueryStringValues_KnownType_FieldSette Assert.Equal(fieldmask, request!.FieldMaskValue); } + [Fact] + public async Task HandleCallAsync_QueryStringJsonNameAlias_DoesNotOverwriteRouteValue() + { + // message HelloRequest { + // string name = 1; // proto name: "name", JSON name: "name" + // int32 age = 13; // proto name: "age", JSON name: "age" + // string field_name = 22 [json_name="json_customized_name"]; // proto name: "field_name", JSON name: "json_customized_name" + // ... other fields + // } + + // Arrange + // A query parameter using the JSON name should not overwrite a route-bound field. + HelloRequest? request = null; + UnaryServerMethod invoker = (s, r, c) => + { + request = r; + return Task.FromResult(new HelloReply()); + }; + + var fieldDescriptor = HelloRequest.Descriptor.FindFieldByName("field_name"); + var routeParameterDescriptors = new Dictionary + { + ["field_name"] = CreateRouteParameterWithJsonPath(new List(new[] { fieldDescriptor })) + }; + var descriptorInfo = TestHelpers.CreateDescriptorInfo(routeParameterDescriptors: routeParameterDescriptors); + var unaryServerCallHandler = CreateCallHandler(invoker, descriptorInfo: descriptorInfo); + var httpContext = TestHelpers.CreateHttpContext(); + httpContext.Request.RouteValues["field_name"] = "route_value"; + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["json_customized_name"] = "different_value" + }); + + // Act + await unaryServerCallHandler.HandleCallAsync(httpContext); + + // Assert + Assert.NotNull(request); + Assert.Equal("route_value", request!.FieldName); + } + + [Fact] + public async Task HandleCallAsync_QueryStringProtoName_DoesNotOverwriteRouteValue() + { + // Arrange + // A query parameter using the proto name is not overwritting the route-bound field. + HelloRequest? request = null; + UnaryServerMethod invoker = (s, r, c) => + { + request = r; + return Task.FromResult(new HelloReply()); + }; + + var routeParameterDescriptors = new Dictionary + { + ["name"] = CreateRouteParameterWithJsonPath(new List(new[] { HelloRequest.Descriptor.FindFieldByNumber(HelloRequest.NameFieldNumber) })) + }; + var descriptorInfo = TestHelpers.CreateDescriptorInfo(routeParameterDescriptors: routeParameterDescriptors); + var unaryServerCallHandler = CreateCallHandler(invoker, descriptorInfo: descriptorInfo); + var httpContext = TestHelpers.CreateHttpContext(); + httpContext.Request.RouteValues["name"] = "route_value"; + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["name"] = "different_value" + }); + + // Act + await unaryServerCallHandler.HandleCallAsync(httpContext); + + // Assert + Assert.NotNull(request); + Assert.Equal("route_value", request!.Name); + } + + [Fact] + public async Task HandleCallAsync_QueryStringNonRouteField_StillBindsNormally() + { + // Arrange + // Query parameters for fields NOT bound via route should work. + HelloRequest? request = null; + UnaryServerMethod invoker = (s, r, c) => + { + request = r; + return Task.FromResult(new HelloReply()); + }; + + var routeParameterDescriptors = new Dictionary + { + ["name"] = CreateRouteParameterWithJsonPath(new List(new[] { HelloRequest.Descriptor.FindFieldByNumber(HelloRequest.NameFieldNumber) })) + }; + var descriptorInfo = TestHelpers.CreateDescriptorInfo(routeParameterDescriptors: routeParameterDescriptors); + var unaryServerCallHandler = CreateCallHandler(invoker, descriptorInfo: descriptorInfo); + var httpContext = TestHelpers.CreateHttpContext(); + httpContext.Request.RouteValues["name"] = "route_value"; + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["age"] = "30" + }); + + // Act + await unaryServerCallHandler.HandleCallAsync(httpContext); + + // Assert + Assert.NotNull(request); + Assert.Equal("route_value", request!.Name); + Assert.Equal(30, request!.Age); + } + + [Fact] + public async Task HandleCallAsync_QueryStringBodyFieldJsonNameAlias_DoesNotOverwriteBodyValue() + { + // Arrange + // body: "sub" binds the sub field from JSON body. + // A query parameter using the JSON name of the body field should not overwrite it. + HelloRequest? request = null; + UnaryServerMethod invoker = (s, r, c) => + { + request = r; + return Task.FromResult(new HelloReply()); + }; + + var descriptorInfo = TestHelpers.CreateDescriptorInfo( + bodyDescriptor: HelloRequest.Types.SubMessage.Descriptor, + bodyFieldDescriptor: HelloRequest.Descriptor.FindFieldByName("sub")); + var unaryServerCallHandler = CreateCallHandler(invoker, descriptorInfo: descriptorInfo); + var httpContext = TestHelpers.CreateHttpContext(); + httpContext.Request.Body = new MemoryStream(Encoding.UTF8.GetBytes(JsonFormatter.Default.Format(new HelloRequest.Types.SubMessage + { + Subfield = "body_value" + }))); + httpContext.Request.ContentType = "application/json"; + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["sub.subfield"] = "different_value" + }); + + // Act + await unaryServerCallHandler.HandleCallAsync(httpContext); + + // Assert + Assert.NotNull(request); + Assert.Equal("body_value", request!.Sub.Subfield); + } + + [Fact] + public async Task HandleCallAsync_QueryStringBodyFieldJsonNamePrefix_DoesNotOverwriteBodyValue() + { + // Arrange + // body field: "sub_data" (proto name) has JSON name "subData". + // A query parameter using the JSON name prefix "subData.subfield" should be blocked. + HelloRequest? request = null; + UnaryServerMethod invoker = (s, r, c) => + { + request = r; + return Task.FromResult(new HelloReply()); + }; + + var descriptorInfo = TestHelpers.CreateDescriptorInfo( + bodyDescriptor: HelloRequest.Types.SubMessage.Descriptor, + bodyFieldDescriptor: HelloRequest.Descriptor.FindFieldByName("sub_data")); + var unaryServerCallHandler = CreateCallHandler(invoker, descriptorInfo: descriptorInfo); + var httpContext = TestHelpers.CreateHttpContext(); + httpContext.Request.Body = new MemoryStream(Encoding.UTF8.GetBytes(JsonFormatter.Default.Format(new HelloRequest.Types.SubMessage + { + Subfield = "body_value" + }))); + httpContext.Request.ContentType = "application/json"; + httpContext.Request.Query = new QueryCollection(new Dictionary + { + ["subData.subfield"] = "different_value" + }); + + // Act + await unaryServerCallHandler.HandleCallAsync(httpContext); + + // Assert + Assert.NotNull(request); + Assert.Equal("body_value", request!.SubData.Subfield); + } + private UnaryServerCallHandler CreateCallHandler( UnaryServerMethod invoker, CallHandlerDescriptorInfo? descriptorInfo = null, diff --git a/src/Middleware/OutputCaching/src/Policies/DefaultPolicy.cs b/src/Middleware/OutputCaching/src/Policies/DefaultPolicy.cs index cc9ea67ec2dd..1f633dbaff83 100644 --- a/src/Middleware/OutputCaching/src/Policies/DefaultPolicy.cs +++ b/src/Middleware/OutputCaching/src/Policies/DefaultPolicy.cs @@ -50,6 +50,12 @@ ValueTask IOutputCachePolicy.ServeResponseAsync(OutputCacheContext context, Canc return ValueTask.CompletedTask; } + if (context.HttpContext.User?.Identity?.IsAuthenticated == true) + { + context.AllowCacheStorage = false; + return ValueTask.CompletedTask; + } + // Check response code if (response.StatusCode != StatusCodes.Status200OK) { diff --git a/src/Middleware/OutputCaching/test/OutputCacheTests.cs b/src/Middleware/OutputCaching/test/OutputCacheTests.cs index 85de08aadc6d..dc50e960fcf7 100644 --- a/src/Middleware/OutputCaching/test/OutputCacheTests.cs +++ b/src/Middleware/OutputCaching/test/OutputCacheTests.cs @@ -1,10 +1,14 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -using System.Net; using System.Net.Http; +using System.Security.Claims; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; using Microsoft.Net.Http.Headers; namespace Microsoft.AspNetCore.OutputCaching.Tests; @@ -1030,6 +1034,190 @@ static async Task RunClient(TestServer server, int id) } } + [Theory] + [InlineData(true)] + [InlineData(false)] + public async Task AuthenticatedRequestsAreNotCached(bool authMiddlewareBeforeCache) + { + int finalEndpointHitCount = 0; + var builder = new HostBuilder() + .ConfigureWebHost(webHostBuilder => + { + webHostBuilder + .UseTestServer() + .ConfigureServices(services => + { + services.AddOutputCache(outputCachingOptions => + { + outputCachingOptions.BasePolicies = [new OutputCachePolicyBuilder().Build()]; + }); + }) + .Configure(app => + { + if (authMiddlewareBeforeCache) + { + AddAuth(app); + } + + app.UseOutputCache(); + + if (!authMiddlewareBeforeCache) + { + AddAuth(app); + } + + app.Run(async context => + { + finalEndpointHitCount++; + if (context.User.Identity?.IsAuthenticated == true) + { + await context.Response.WriteAsync(context.User.Identity.Name ?? "anonymous (authenticated)"); + } + else + { + await context.Response.WriteAsync(context.User.Identity?.Name ?? "anonymous"); + } + }); + }); + }); + + using var host = builder.Build(); + + await host.StartAsync(); + + using var server = host.GetTestServer(); + + var iterations = 10; + for (int i = 0; i < iterations; i++) + { + await RunClient(server, i); + } + + // RunClient sends two requests per iteration + Assert.Equal(iterations * 2, finalEndpointHitCount); + + // Unauthenticated request, check it still works + var client = server.CreateClient(); + var resp = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "")); + Assert.Equal("anonymous", await resp.Content.ReadAsStringAsync()); + + var resp2 = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "")); + Assert.Equal("anonymous", await resp2.Content.ReadAsStringAsync()); + // Smoke test that unauthenticated request can be served from cache. + Assert.True(resp2.Headers.Contains(HeaderNames.Age)); + + static async Task RunClient(TestServer server, int i) + { + var client = server.CreateClient(); + // Use headers for name since default vary-by uses query and we want the requests to look the same to the caching middleware + var resp = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "") { Headers = { { "name", $"{i}" } } }); + Assert.Equal($"{i}", await resp.Content.ReadAsStringAsync()); + + var resp2 = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "") { Headers = { { "name", $"{i}" } } }); + Assert.Equal($"{i}", await resp2.Content.ReadAsStringAsync()); + // Smoke test that authenticated request isn't served from cache. + Assert.False(resp2.Headers.Contains(HeaderNames.Age)); + } + + static void AddAuth(IApplicationBuilder app) + { + app.Use((c, n) => + { + if (c.Request.Headers.ContainsKey("name")) + { + c.User = new ClaimsPrincipal(new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, c.Request.Headers["name"]) }, authenticationType: "custom")); + Assert.True(c.User.Identity?.IsAuthenticated); + } + + return n(c); + }); + } + } + + // This is a negative test to show that if auth middleware is after caching, the auth middleware won't run if a cache entry exists + // which is why we recommend putting auth before caching so that you don't accidentally serve anonymous cached content to an authenticated user. + [Fact] + public async Task AuthMiddlewareAfterCachingNotRunIfCacheEntryExists() + { + int finalEndpointHitCount = 0; + var builder = new HostBuilder() + .ConfigureWebHost(webHostBuilder => + { + webHostBuilder + .UseTestServer() + .ConfigureServices(services => + { + services.AddOutputCache(outputCachingOptions => + { + outputCachingOptions.BasePolicies = [new OutputCachePolicyBuilder().Build()]; + }); + }) + .Configure(app => + { + app.UseOutputCache(); + // Auth middleware + app.Use((c, n) => + { + if (c.Request.Headers.ContainsKey("name")) + { + c.User = new ClaimsPrincipal(new ClaimsIdentity(new[] { new Claim(ClaimTypes.Name, c.Request.Headers["name"]) }, authenticationType: "custom")); + } + + return n(c); + }); + app.Run(async context => + { + finalEndpointHitCount++; + if (context.User.Identity?.IsAuthenticated == true) + { + await context.Response.WriteAsync(context.User.Identity.Name ?? "anonymous (authenticated)"); + } + else + { + await context.Response.WriteAsync(context.User.Identity?.Name ?? "anonymous"); + } + }); + }); + }); + + using var host = builder.Build(); + + await host.StartAsync(); + + using var server = host.GetTestServer(); + + // Make an unauthenticated request first to add a cache entry. + var client = server.CreateClient(); + var resp = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "")); + Assert.Equal("anonymous", await resp.Content.ReadAsStringAsync()); + + var resp2 = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "")); + Assert.Equal("anonymous", await resp2.Content.ReadAsStringAsync()); + // Smoke test that unauthenticated request is served from cache. + Assert.True(resp2.Headers.Contains(HeaderNames.Age)); + + var iterations = 10; + for (int i = 0; i < iterations; i++) + { + await RunClient(server, i); + } + + // Endpoint only hit once, for first request that doesn't have auth. + Assert.Equal(1, finalEndpointHitCount); + + static async Task RunClient(TestServer server, int i) + { + var client = server.CreateClient(); + // Use headers for name since default vary-by uses query and we want the requests to look the same to the caching middleware + var resp = await client.SendAsync(new HttpRequestMessage(HttpMethod.Get, "") { Headers = { { "name", $"{i}" } } }); + + // Because the output cache middleware is before auth, these requests will serve a cached response if it exists (and we made one exist for this test) + // While this isn't ideal, we recommend app developers put caching after the auth middleware where we will skip the cache layer. + Assert.Equal("anonymous", await resp.Content.ReadAsStringAsync()); + Assert.True(resp.Headers.Contains(HeaderNames.Age)); + } + } + private static void Assert304Headers(HttpResponseMessage initialResponse, HttpResponseMessage subsequentResponse) { // https://tools.ietf.org/html/rfc7232#section-4.1 diff --git a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs index 5ee7fc967785..86304743cfae 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http2/Http2Stream.cs @@ -430,8 +430,11 @@ private bool TryValidatePath(ReadOnlySpan pathSegment) for (var i = 0; i < pathSegment.Length; i++) { var ch = pathSegment[i]; - // The header parser should already be checking this - Debug.Assert(32 < ch && ch < 127); + if (ch > byte.MaxValue) + { + ResetAndAbort(new ConnectionAbortedException(CoreStrings.FormatHttp2StreamErrorPathInvalid(RawTarget)), Http2ErrorCode.PROTOCOL_ERROR); + return false; + } pathBuffer[i] = (byte)ch; } diff --git a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs index 17178ffbf133..cadc301b0c76 100644 --- a/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs +++ b/src/Servers/Kestrel/Core/src/Internal/Http3/Http3Stream.cs @@ -1176,8 +1176,11 @@ private bool TryValidatePath(ReadOnlySpan pathSegment) for (var i = 0; i < pathSegment.Length; i++) { var ch = pathSegment[i]; - // The header parser should already be checking this - Debug.Assert(32 < ch && ch < 127); + if (ch > byte.MaxValue) + { + Abort(new ConnectionAbortedException(CoreStrings.FormatHttp3StreamErrorPathInvalid(RawTarget)), Http3ErrorCode.ProtocolError); + return false; + } pathBuffer[i] = (byte)ch; } diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs index 4ec15cf429f2..0e1968c5677c 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http2/Http2StreamTests.cs @@ -798,6 +798,31 @@ public async Task HEADERS_Received_MaxRequestLineSize_Reset() await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); } + [Theory] + [InlineData("/\u0161")] + [InlineData("/a\u0161")] + [InlineData("/\u0161a")] + [InlineData("/a\u0161a")] + public async Task HEADERS_Received_CharacterLargerThanByte_Reset(string path) + { + var pathBytes = Encoding.UTF8.GetBytes(path); + var headerBlock = new byte[4 + pathBytes.Length]; + headerBlock[0] = 0x82; // Indexed: :method GET (HPACK static index 2) + headerBlock[1] = 0x86; // Indexed: :scheme http (HPACK static index 6) + headerBlock[2] = 0x44; // Literal incremental indexing, name index 4 (:path) + headerBlock[3] = (byte)pathBytes.Length; // Value length, no Huffman + pathBytes.CopyTo(headerBlock.AsSpan(4)); + + await InitializeConnectionAsync(_noopApplication); + + await StartStreamAsync(1, headerBlock, endStream: true); + + await WaitForStreamErrorAsync(expectedStreamId: 1, Http2ErrorCode.PROTOCOL_ERROR, + CoreStrings.FormatHttp2StreamErrorPathInvalid(path)); + + await StopConnectionAsync(expectedLastStreamId: 1, ignoreNonGoAwayFrames: false); + } + [Fact] public async Task HEADERS_Received_MaxRequestHeadersTotalSize_431() { diff --git a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs index c23e11328e43..a59a261e39fe 100644 --- a/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs +++ b/src/Servers/Kestrel/test/InMemory.FunctionalTests/Http3/Http3StreamTests.cs @@ -257,6 +257,36 @@ public async Task Path_DecodedAndNormalized(string input, string expected) Assert.Equal("0", responseHeaders["content-length"]); } + [Theory] + [InlineData("/\u0161dmin?x=1")] // U+0161 (353), truncates to 0x61 = 'a' → "/admin?x=1" + [InlineData("/\u0170ser")] // U+0170 (368), truncates to 0x70 = 'p' → "/pser" + [InlineData("/caf\u0165?q=1")] // U+0165 (357), truncates to 0x65 = 'e' → "/cafe?q=1" + public async Task NonAsciiPath_Reset(string path) + { + var pathBytes = Encoding.UTF8.GetBytes(path); + var qpackBlock = new byte[6 + pathBytes.Length]; + qpackBlock[0] = 0x00; // Required Insert Count = 0 + qpackBlock[1] = 0x00; // Delta Base = 0 + qpackBlock[2] = 0xD1; // Indexed: :method GET (QPACK static index 17) + qpackBlock[3] = 0xD6; // Indexed: :scheme http (QPACK static index 22) + qpackBlock[4] = 0x51; // Literal with static name ref, index 1 (:path) + qpackBlock[5] = (byte)pathBytes.Length; // Value length, no Huffman + pathBytes.CopyTo(qpackBlock.AsSpan(6)); + + await Http3Api.InitializeConnectionAsync(_noopApplication); + Http3Api.OutboundControlStream = await Http3Api.CreateControlStream(); + + var requestStream = await Http3Api.CreateRequestStream( + headers: (IEnumerable>)null, endStream: false); + + await requestStream.SendFrameAsync(Http3FrameType.Headers, qpackBlock, endStream: true); + + await requestStream.WaitForStreamErrorAsync( + Http3ErrorCode.ProtocolError, + AssertExpectedErrorMessages, + CoreStrings.FormatHttp3StreamErrorPathInvalid(path)); + } + [Theory] [InlineData(":path", "/")] [InlineData(":scheme", "http")] diff --git a/src/SignalR/common/SignalR.Common/test/Internal/Protocol/MessagePackHubProtocolTestBase.cs b/src/SignalR/common/SignalR.Common/test/Internal/Protocol/MessagePackHubProtocolTestBase.cs index 83c1f522b0b0..b748980724eb 100644 --- a/src/SignalR/common/SignalR.Common/test/Internal/Protocol/MessagePackHubProtocolTestBase.cs +++ b/src/SignalR/common/SignalR.Common/test/Internal/Protocol/MessagePackHubProtocolTestBase.cs @@ -258,6 +258,29 @@ public void ParseMessageWithExtraData() Assert.Equal(expectedMessage, message, TestHubMessageEqualityComparer.Instance); } + // Unknown invocation IDs are skipped over. We still need to parse the message + // to get to the next message, but we won't create any objects (except the outer hubmessage type) + [Fact] + public void SkipResult_WithDeeplyNestedStructure() + { + var expectedMessage = CompletionMessage.WithResult("xyz", null); + // Verify that the input binary string decodes to the expected MsgPack primitives + var bytes = new byte[] { ArrayBytes(5), + 3, // Completion message + 0x80, // Empty headers + StringBytes(3), (byte)'x', (byte)'y', (byte)'z', // invocation ID + 3, // result type; non-void + } + .Concat(Enumerable.Repeat((byte)0x91, 100_000).Append((byte)0xC0)) // 100,000 nested arrays with a null at the end + .ToArray(); + + bytes = Frame(bytes); + + var data = new ReadOnlySequence(bytes); + // Null types for TestBinder so parser will skip over the result (treats it as an unknown invocation ID) + Assert.True(HubProtocol.TryParseMessage(ref data, new TestBinder(), out var message)); + } + [Theory] [MemberData(nameof(BaseTestDataNames))] public void BaseWriteMessages(string testDataName) diff --git a/src/submodules/MessagePack-CSharp b/src/submodules/MessagePack-CSharp index 9aeb12b9bdb0..9614e6f396e9 160000 --- a/src/submodules/MessagePack-CSharp +++ b/src/submodules/MessagePack-CSharp @@ -1 +1 @@ -Subproject commit 9aeb12b9bdb024512ffe2e4bddfa2785dca6e39e +Subproject commit 9614e6f396e959ad67e4ba655d5ab6e1311bed23