From c2143b57c88ac41640b9cd1d9dcca0c3c014d4a9 Mon Sep 17 00:00:00 2001 From: martincostello Date: Wed, 8 Nov 2023 12:56:00 +0000 Subject: [PATCH 1/2] Fix duplicated query string parameters Fix query string parameters being duplicated if `AuthorizationEndpoint` contains any user-added query string parameters. --- .../VisualStudioAuthenticationHandler.cs | 9 +++++++-- .../VisualStudio/VisualStudioTests.cs | 17 ++++++++++++++--- 2 files changed, 21 insertions(+), 5 deletions(-) diff --git a/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs b/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs index 3c81d00a5..afefe9854 100644 --- a/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs +++ b/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs @@ -91,12 +91,17 @@ protected override string BuildChallengeUrl([NotNull] AuthenticationProperties p var challengeUrl = base.BuildChallengeUrl(properties, redirectUri); // Visual Studio Online/Azure DevOps uses "Assertion" instead of "code" - var challengeUri = new Uri(challengeUrl, UriKind.Absolute); + var challengeUri = new UriBuilder(challengeUrl); var query = QueryHelpers.ParseQuery(challengeUri.Query); query["response_type"] = "Assertion"; - return QueryHelpers.AddQueryString(Options.AuthorizationEndpoint, query); + // Remove the query and re-add with the edit so that the parameters are not duplicated. + // See https://github.com/dotnet/aspnetcore/issues/47054 for more context. + challengeUri.Query = string.Empty; + challengeUrl = challengeUri.Uri.ToString(); + + return QueryHelpers.AddQueryString(challengeUrl, query); } private static partial class Log diff --git a/test/AspNet.Security.OAuth.Providers.Tests/VisualStudio/VisualStudioTests.cs b/test/AspNet.Security.OAuth.Providers.Tests/VisualStudio/VisualStudioTests.cs index 98465e9ac..6ce6adf13 100644 --- a/test/AspNet.Security.OAuth.Providers.Tests/VisualStudio/VisualStudioTests.cs +++ b/test/AspNet.Security.OAuth.Providers.Tests/VisualStudio/VisualStudioTests.cs @@ -26,9 +26,13 @@ public async Task Can_Sign_In_Using_Visual_Studio(string claimType, string claim => await AuthenticateUserAndAssertClaimValue(claimType, claimValue); [Theory] - [InlineData(false)] - [InlineData(true)] - public async Task BuildChallengeUrl_Generates_Correct_Url(bool usePkce) + [InlineData(false, "")] + [InlineData(true, "")] + [InlineData(false, "?foo=bar")] + [InlineData(true, "?foo=bar")] + public async Task BuildChallengeUrl_Generates_Correct_Url( + bool usePkce, + string authorizationEndpointSuffix) { // Arrange var options = new VisualStudioAuthenticationOptions() @@ -36,6 +40,8 @@ public async Task BuildChallengeUrl_Generates_Correct_Url(bool usePkce) UsePkce = usePkce, }; + options.AuthorizationEndpoint += authorizationEndpointSuffix; + var redirectUrl = "https://my-site.local/signin-visualstudio"; // Act @@ -66,5 +72,10 @@ public async Task BuildChallengeUrl_Generates_Correct_Url(bool usePkce) query.ShouldNotContainKey(OAuthConstants.CodeChallengeKey); query.ShouldNotContainKey(OAuthConstants.CodeChallengeMethodKey); } + + foreach (var parameter in query) + { + parameter.Value.Count.ShouldBe(1, $"Query string parameter {parameter.Key} appears more than once: {parameter.Value}."); + } } } From 7869cbca1d3fe2805cca8d52a8defa5de859f778 Mon Sep 17 00:00:00 2001 From: martincostello Date: Wed, 8 Nov 2023 17:26:00 +0000 Subject: [PATCH 2/2] Refactor fix Apply code review suggestions. --- .../VisualStudioAuthenticationHandler.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs b/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs index afefe9854..cb589ee08 100644 --- a/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs +++ b/src/AspNet.Security.OAuth.VisualStudio/VisualStudioAuthenticationHandler.cs @@ -9,6 +9,7 @@ using System.Security.Claims; using System.Text.Encodings.Web; using System.Text.Json; +using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.WebUtilities; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; @@ -96,12 +97,11 @@ protected override string BuildChallengeUrl([NotNull] AuthenticationProperties p query["response_type"] = "Assertion"; - // Remove the query and re-add with the edit so that the parameters are not duplicated. + // Replace the query with the edit so that the parameters are not duplicated. // See https://github.com/dotnet/aspnetcore/issues/47054 for more context. - challengeUri.Query = string.Empty; - challengeUrl = challengeUri.Uri.ToString(); + challengeUri.Query = QueryString.Create(query).Value; - return QueryHelpers.AddQueryString(challengeUrl, query); + return challengeUri.Uri.AbsoluteUri; } private static partial class Log