URL redirect and ValidateIssuer CodeQL bugs (#9398)

* added Url redirect codeql suppressions

* codeQL bug = suppression :)

* moved suppression comment closer to the relevant line of code

* We are restricting the reCAPTCHA request to a specific host. Changed the suppression comment to reflect this as the mitigation for the vulnerability.

* Only allow relative Urls as a return Url. Add/adapt tests.

* removed unnecessary usings

* modified suppression comment for recaptcha redirection

Author: advay26

src/NuGetGallery.Services/Authentication/Providers/AzureActiveDirectoryV2/AzureActiveDirectoryV2Authenticator.cs

@@ -102,6 +102,7 @@ protected override void AttachToOwinApp(IGalleryConfigurationService config, IAp
                 PostLogoutRedirectUri = siteRoot,
                 Scope = OpenIdConnectScope.OpenIdProfile + " email",
                 ResponseType = OpenIdConnectResponseType.IdToken,
+                // CodeQL [SM03926] We do not restrict issuers to a limited set of tenants for our multi-tenant app
                 TokenValidationParameters = new Microsoft.IdentityModel.Tokens.TokenValidationParameters() { ValidateIssuer = false },
                 Notifications = new OpenIdConnectAuthenticationNotifications
                 {

src/NuGetGallery/Controllers/AuthenticationController.cs

@@ -445,6 +445,12 @@ public virtual ActionResult AuthenticateExternal(string returnUrl)
                     .SecurityPolicies
                     .Any(policy => policy.Name == nameof(RequireOrganizationTenantPolicy)));
 
+            // Validate that the returnUrl is a relative URL to prevent untrusted URL redirection
+            if (!Url.IsLocalUrl(returnUrl))
+            {
+                returnUrl = "/";
+            }
+
             if (userOrganizationsWithTenantPolicy != null && userOrganizationsWithTenantPolicy.Any())
             {
                 var aadCredential = user?.Credentials.GetAzureActiveDirectoryCredential();

src/NuGetGallery/Filters/ValidateRecaptchaResponseAttribute.cs

@@ -49,6 +49,7 @@ private async Task<bool> RecaptchaIsValid(string privateKey, string response)
 
             try
             {
+                // CodeQL [SM03781] The validation Url is restricted to a specific host, which mitigates the risk of unwanted redirection
                 var reply = await Client.Value.GetStringAsync(validationUrl);
                 var state = JsonConvert.DeserializeObject<RecaptchaState>(reply);
                 return state.Success;

tests/NuGetGallery.Facts/Controllers/AuthenticationControllerFacts.cs

@@ -1576,23 +1576,27 @@ public async Task GivenEnableMultiFactorAuthenticationInUserIsFalseAndLoginUsedM
 
         public class TheAuthenticateExternalAction : TestContainer
         {
-            [Fact]
-            public void ForMissingExternalProvider_ErrorIsReturned()
+            [Theory]
+            [InlineData("/theReturnUrl")]
+            [InlineData("/")]
+            public void ForMissingExternalProvider_ErrorIsReturned(string returnUrl)
             {
                 // Arrange
                 var controller = GetController<AuthenticationController>();
                 controller.SetCurrentUser(TestUtility.FakeUser);
 
                 // Act
-                var result = controller.AuthenticateExternal("theReturnUrl");
+                var result = controller.AuthenticateExternal(returnUrl);
 
                 // Assert
-                ResultAssert.IsRedirectTo(result, "theReturnUrl");
+                ResultAssert.IsRedirectTo(result, returnUrl);
                 Assert.Equal(Strings.ChangeCredential_ProviderNotFound, controller.TempData["Message"]);
             }
 
-            [Fact]
-            public void ForAADLinkedAccount_ErrorIsReturnedDueToOrgPolicy()
+            [Theory]
+            [InlineData("/theReturnUrl")]
+            [InlineData("/")]
+            public void ForAADLinkedAccount_ErrorIsReturnedDueToOrgPolicy(string returnUrl)
             {
                 // Arrange
                 var fakes = Get<Fakes>();
@@ -1612,15 +1616,17 @@ public void ForAADLinkedAccount_ErrorIsReturnedDueToOrgPolicy()
                 controller.SetCurrentUser(user);
 
                 // Act
-                var result = controller.AuthenticateExternal("theReturnUrl");
+                var result = controller.AuthenticateExternal(returnUrl);
 
                 // Assert
-                ResultAssert.IsRedirectTo(result, "theReturnUrl");
+                ResultAssert.IsRedirectTo(result, returnUrl);
                 Assert.NotNull(controller.TempData["WarningMessage"]);
             }
 
-            [Fact]
-            public void ForNonAADLinkedAccount_WithOrgPolicyCompletesSuccessfully()
+            [Theory]
+            [InlineData("/theReturnUrl")]
+            [InlineData("/")]
+            public void ForNonAADLinkedAccount_WithOrgPolicyCompletesSuccessfully(string returnUrl)
             {
                 // Arrange
                 var fakes = Get<Fakes>();
@@ -1640,11 +1646,11 @@ public void ForNonAADLinkedAccount_WithOrgPolicyCompletesSuccessfully()
                 controller.SetCurrentUser(user);
 
                 // Act
-                var result = controller.AuthenticateExternal("theReturnUrl");
+                var result = controller.AuthenticateExternal(returnUrl);
 
                 // Assert
                 Assert.Null(controller.TempData["WarningMessage"]);
-                ResultAssert.IsChallengeResult(result, "AzureActiveDirectoryV2", controller.Url.LinkOrChangeExternalCredential("theReturnUrl"));
+                ResultAssert.IsChallengeResult(result, "AzureActiveDirectoryV2", controller.Url.LinkOrChangeExternalCredential(returnUrl));
             }
 
             [Theory]
@@ -1685,6 +1691,37 @@ public void WillCallChallengeAuthenticationForAADv2Provider()
                 // Assert
                 ResultAssert.IsChallengeResult(result, "AzureActiveDirectoryV2", controller.Url.LinkOrChangeExternalCredential(returnUrl));
             }
+
+            [Fact]
+            public void WillRedirectToHomePageOnFailureForAbsoluteUrls()
+            {
+                // Arrange
+                var controller = GetController<AuthenticationController>();
+                controller.SetCurrentUser(TestUtility.FakeUser);
+
+                // Act
+                var result = controller.AuthenticateExternal("theReturnUrl"); // not a relative URL
+
+                // Assert
+                ResultAssert.IsRedirectTo(result, "/");
+                Assert.Equal(Strings.ChangeCredential_ProviderNotFound, controller.TempData["Message"]);
+            }
+
+            [Fact]
+            public void WillRedirectToHomePageOnSuccessForAbsoluteUrls()
+            {
+                // Arrange
+                const string returnUrl = "theReturnUrl"; // not a relative URL
+                EnableAllAuthenticators(Get<AuthenticationService>());
+                var controller = GetController<AuthenticationController>();
+                controller.SetCurrentUser(TestUtility.FakeUser);
+
+                // Act
+                var result = controller.AuthenticateExternal(returnUrl);
+
+                // Assert
+                ResultAssert.IsChallengeResult(result, "AzureActiveDirectoryV2", controller.Url.LinkOrChangeExternalCredential("/"));
+            }
         }
 
         public class TheLinkExternalAccountAction : TestContainer