From dda8053a1b0c880a4f9a1a2d51a0e3d80bc998c7 Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Thu, 5 Oct 2023 09:21:42 -0700 Subject: [PATCH 1/3] Enhance path validation logic --- .../Microsoft.Gen.AutoClient/Emitter.cs | 30 +++++++- .../AutoClientUtilities.cs | 68 +++++++++++++++++ .../Generated/PathTests.cs | 74 ++++++++++++++++++- .../Generated/QueryTests.cs | 31 ++++++++ 4 files changed, 198 insertions(+), 5 deletions(-) create mode 100644 src/Libraries/Microsoft.Extensions.Http.AutoClient/AutoClientUtilities.cs diff --git a/src/Generators/Microsoft.Gen.AutoClient/Emitter.cs b/src/Generators/Microsoft.Gen.AutoClient/Emitter.cs index a4cfb3fc8f3..cac713912e7 100644 --- a/src/Generators/Microsoft.Gen.AutoClient/Emitter.cs +++ b/src/Generators/Microsoft.Gen.AutoClient/Emitter.cs @@ -43,6 +43,8 @@ internal sealed class Emitter : EmitterBase private const string AutoClientHttpError = "global::Microsoft.Extensions.Http.AutoClient.AutoClientHttpError"; private const string Invariant = "global::System.FormattableString.Invariant"; private const string UriKind = "global::System.UriKind"; + private const string ArgumentException = "global::System.ArgumentException"; + private const string AutoClientUtilities = "global::Microsoft.Extensions.Http.AutoClient.AutoClientUtilities"; public string EmitRestApis(IReadOnlyList restApiTypes, CancellationToken cancellationToken) { @@ -67,6 +69,8 @@ public string EmitRestApis(IReadOnlyList restApiTypes, Cancellation return Capture(); } + private static string GetEscapedParamName(string name) => $"{name}Escaped"; + private static string GetPathTemplate(RestApiMethod restApiMethod) { var pathTemplateSb = new StringBuilder(restApiMethod.Path); @@ -229,10 +233,26 @@ private void GenRestApiMethod(RestApiMethod restApiMethod, RestApiType restApiTy var pathSb = new StringBuilder(restApiMethod.Path); + // Validate path values + foreach (var param in restApiMethod.FormatParameters) + { + var escapedParamName = GetEscapedParamName(param); + + // Use interpolated string to handle any null values from any type like nullable value types or reference types. + OutLn($"var {escapedParamName} = $\"{{{param}}}\";"); + + // Values should not contain '/', '\', be empty, whitespace or made of dots (.) only + OutLn($"if (!{AutoClientUtilities}.IsPathParameterValid({escapedParamName})) throw new {ArgumentException}({staticsName}.ValidationExceptionMessage, nameof({param}));"); + OutLn(); + + _ = pathSb.Replace($"{{{param}}}", $"{{{escapedParamName}}}"); + } + + // Encode path values foreach (var param in restApiMethod.FormatParameters) { - var escapedParamName = $"{param}Escaped"; - OutLn($"var {escapedParamName} = {Uri}.EscapeDataString($\"{{{param}}}\");"); + var escapedParamName = GetEscapedParamName(param); + OutLn($"{escapedParamName} = {Uri}.EscapeDataString({escapedParamName});"); _ = pathSb.Replace($"{{{param}}}", $"{{{escapedParamName}}}"); } @@ -242,7 +262,7 @@ private void GenRestApiMethod(RestApiMethod restApiMethod, RestApiType restApiTy var firstQuery = true; foreach (var param in restApiMethod.AllParameters.Where(m => m.IsQuery)) { - var escapedParamName = $"{param.Name}Escaped"; + var escapedParamName = GetEscapedParamName(param.Name); // Use interpolated string to handle any null values from any type like nullable value types or reference types. OutLn($"var {escapedParamName} = {Uri}.EscapeDataString($\"{{{param.Name}}}\");"); @@ -357,6 +377,7 @@ private void GenRestApiMethod(RestApiMethod restApiMethod, RestApiType restApiTy OutCloseBrace(); OutCloseBrace(); + } private void EmitSendRequestMethod(string httpClientName, string optionsName) @@ -440,6 +461,9 @@ private void EmitStatics(RestApiType restApiType, string staticsName) OutLn($"private static class {staticsName}"); OutOpenBrace(); + OutLn("public const string ValidationExceptionMessage = \"The value can't contain '\\\\', '/', be empty, null or contain only dots (.).\";"); + OutLn(); + OutLn(@$"public static readonly {MediaTypeHeaderValue} ApplicationJsonHeader = new(""application/json"")"); OutOpenBrace(); OutLn($@"CharSet = {Encoding}.UTF8.WebName"); diff --git a/src/Libraries/Microsoft.Extensions.Http.AutoClient/AutoClientUtilities.cs b/src/Libraries/Microsoft.Extensions.Http.AutoClient/AutoClientUtilities.cs new file mode 100644 index 00000000000..d3f8f42b372 --- /dev/null +++ b/src/Libraries/Microsoft.Extensions.Http.AutoClient/AutoClientUtilities.cs @@ -0,0 +1,68 @@ +// 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.ComponentModel; + +namespace Microsoft.Extensions.Http.AutoClient; + +/// +/// Utilities for AutoClient feature. +/// +[EditorBrowsable(EditorBrowsableState.Never)] +public static class AutoClientUtilities +{ + private static readonly char[] _slashes = { '/', '\\' }; + private static readonly char[] _slashesOrDot = { '/', '\\', '.' }; + + /// + /// Returns whether a value is a valid path argument. + /// + /// The value to validate. + /// Whether the value is a valid path argument. + public static bool IsPathParameterValid(string value) + { + var trimmed = value.AsSpan().Trim(); + + if (trimmed.Length == 0) + { + return false; + } + + // Fast path for most common cases + var index = trimmed.IndexOfAny(_slashesOrDot); + if (index == -1) + { + return true; + } + + // Slashes can't be used + if (trimmed.Slice(index).IndexOfAny(_slashes) >= 0) + { + return false; + } + + // The trimmed string can't be made of dots only + +#if NET7_0_OR_GREATER + + if (trimmed.IndexOfAnyExcept('.') >= 0) + { + return true; + } + +#else + + for (var i = 0; i < trimmed.Length; i++) + { + if (trimmed[i] != '.') + { + return true; + } + } + +#endif + + return false; + } +} diff --git a/test/Generators/Microsoft.Gen.AutoClient/Generated/PathTests.cs b/test/Generators/Microsoft.Gen.AutoClient/Generated/PathTests.cs index 8e3b232118f..a076744f9ee 100644 --- a/test/Generators/Microsoft.Gen.AutoClient/Generated/PathTests.cs +++ b/test/Generators/Microsoft.Gen.AutoClient/Generated/PathTests.cs @@ -83,12 +83,14 @@ public async Task MultiplePathParameters() [Fact] public async Task EncodedPathParameters() { + // "+=&" aren't required to be escaped in path segments but there is no harm in doing so + _handlerMock .Protected() .Setup>("SendAsync", ItExpr.Is(message => message.Method == HttpMethod.Get && message.RequestUri != null && - message.RequestUri.PathAndQuery == "/api/users/some%2Fvalue/3"), + message.RequestUri.PathAndQuery == "/api/users/some%2B%3D%26value/3"), ItExpr.IsAny()) .ReturnsAsync(new HttpResponseMessage { @@ -96,8 +98,76 @@ public async Task EncodedPathParameters() Content = new StringContent("Success!") }); - var response = await _sut.GetUserFromTenant("some/value", 3); + var response = await _sut.GetUserFromTenant("some+=&value", 3); + + Assert.Equal("Success!", response); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + [InlineData(" ")] + [InlineData("a/b")] + [InlineData("/b")] + [InlineData("/b/")] + [InlineData("b/")] + [InlineData("//")] + [InlineData(".")] + [InlineData("..")] + [InlineData(" .")] + [InlineData(" ..")] + [InlineData(". ")] + [InlineData(".. ")] + [InlineData(" . ")] + [InlineData(" .. ")] + [InlineData("../")] + [InlineData("/..")] + [InlineData("./..")] + [InlineData("\\")] + [InlineData("a\\b")] + [InlineData("a\\")] + [InlineData("\\b")] + [InlineData("\\\\")] + public async Task EncodedPathInvalidParameters(string value) + { + var encodedValue = Uri.EscapeDataString($"{value}"); + + _handlerMock + .Protected() + .Setup>("SendAsync", ItExpr.IsAny(), + ItExpr.IsAny()); + + var ex = await Assert.ThrowsAsync("tenantId", () => _sut.GetUserFromTenant(value, 3)); + Assert.Contains("The value can't contain '\\', '/', be empty, null or contain only dots (.).", ex.Message); + } + + [Theory] + [InlineData("abc")] + [InlineData("a..b")] + [InlineData("..b")] + [InlineData("a..")] + [InlineData(" ..b")] + [InlineData(" a..")] + [InlineData("..b ")] + [InlineData("a.. ")] + [InlineData(" ..b ")] + [InlineData(" a.. ")] + public async Task EncodedPathValidParameters(string value) + { + var encodedValue = Uri.EscapeDataString($"{value}"); + + _handlerMock + .Protected() + .Setup>("SendAsync", ItExpr.IsAny(), + ItExpr.IsAny()) + .ReturnsAsync(new HttpResponseMessage + { + StatusCode = HttpStatusCode.OK, + Content = new StringContent("Success!") + }); + var response = await _sut.GetUserFromTenant(value, 3); Assert.Equal("Success!", response); } diff --git a/test/Generators/Microsoft.Gen.AutoClient/Generated/QueryTests.cs b/test/Generators/Microsoft.Gen.AutoClient/Generated/QueryTests.cs index 154444cdcd3..85bde7d4e33 100644 --- a/test/Generators/Microsoft.Gen.AutoClient/Generated/QueryTests.cs +++ b/test/Generators/Microsoft.Gen.AutoClient/Generated/QueryTests.cs @@ -80,6 +80,37 @@ public async Task QueryIsEscaped() Assert.Equal("Success!", response); } + [Theory] + [InlineData("?")] + [InlineData("=")] + [InlineData("&")] + [InlineData("%")] + [InlineData("+")] + [InlineData("#")] + [InlineData(" ")] + [InlineData("/")] // / isn't required to be escaped in query string values but there is no harm in doing so + public async Task SensitiveCharsAreEscaped(string value) + { + var encodedValue = Uri.EscapeDataString(value); + + _handlerMock + .Protected() + .Setup>("SendAsync", ItExpr.Is(message => + message.Method == HttpMethod.Get && + message.RequestUri != null && + message.RequestUri.PathAndQuery == $"/api/users?paramQuery={encodedValue}"), + ItExpr.IsAny()) + .ReturnsAsync(new HttpResponseMessage + { + StatusCode = HttpStatusCode.OK, + Content = new StringContent("Success!") + }); + + var response = await _sut.GetUsers(value); + + Assert.Equal("Success!", response); + } + [Fact] public async Task QueryFromParameterCustom() { From c42632a94aaaa2494990d423b7db169786bae4ba Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Thu, 5 Oct 2023 12:08:46 -0700 Subject: [PATCH 2/3] Update API manifest --- .../Microsoft.Extensions.Http.AutoClient.json | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/Libraries/Microsoft.Extensions.Http.AutoClient/Microsoft.Extensions.Http.AutoClient.json b/src/Libraries/Microsoft.Extensions.Http.AutoClient/Microsoft.Extensions.Http.AutoClient.json index 4c06dd4b296..4e14f29c44f 100644 --- a/src/Libraries/Microsoft.Extensions.Http.AutoClient/Microsoft.Extensions.Http.AutoClient.json +++ b/src/Libraries/Microsoft.Extensions.Http.AutoClient/Microsoft.Extensions.Http.AutoClient.json @@ -115,6 +115,16 @@ } ] }, + { + "Type": "static class Microsoft.Extensions.Http.AutoClient.AutoClientUtilities", + "Stage": "Stable", + "Methods": [ + { + "Member": "static bool IsPathParameterValid(string value);", + "Stage": "Stable" + } + ] + }, { "Type": "sealed class Microsoft.Extensions.Http.AutoClient.BodyAttribute : System.Attribute", "Stage": "Stable", From b213f84fccf7da3c048645abacb113b7ff2639cd Mon Sep 17 00:00:00 2001 From: Sebastien Ros Date: Thu, 5 Oct 2023 21:06:45 -0700 Subject: [PATCH 3/3] Fix API manifest --- .../Microsoft.Extensions.Http.AutoClient.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Libraries/Microsoft.Extensions.Http.AutoClient/Microsoft.Extensions.Http.AutoClient.json b/src/Libraries/Microsoft.Extensions.Http.AutoClient/Microsoft.Extensions.Http.AutoClient.json index 4e14f29c44f..5fd463b8e74 100644 --- a/src/Libraries/Microsoft.Extensions.Http.AutoClient/Microsoft.Extensions.Http.AutoClient.json +++ b/src/Libraries/Microsoft.Extensions.Http.AutoClient/Microsoft.Extensions.Http.AutoClient.json @@ -120,7 +120,7 @@ "Stage": "Stable", "Methods": [ { - "Member": "static bool IsPathParameterValid(string value);", + "Member": "static bool Microsoft.Extensions.Http.AutoClient.AutoClientUtilities.IsPathParameterValid(string value);", "Stage": "Stable" } ]