Preserve exact original redirect URL in OAuth client (git-ecosystem#1281

)

The OAuth 2.0 spec requires that redirect URLs be matched _exactly_ if
specified, including matching trailing slashes.

Since the .NET `Uri` type's `.ToString()` method will append a trailing
slash to the end of path-less URLs (e.g., "http://foo" => "http://foo/")
we need to use the `.OriginalString` property instead.

Shoring up this area in anticipation for changes to support multiple
GitHub redirect URLs with git-ecosystem#594

src/shared/Core.Tests/Authentication/OAuth2ClientTests.cs

@@ -44,6 +44,55 @@ public async Task OAuth2Client_GetAuthorizationCodeAsync()
             Assert.Equal(expectedAuthCode, result.Code);
         }
 
+        [Theory]
+        [InlineData("http://localhost")]
+        [InlineData("http://localhost/")]
+        [InlineData("http://localhost/oauth-callback")]
+        [InlineData("http://localhost/oauth-callback/")]
+        [InlineData("http://127.0.0.1")]
+        [InlineData("http://127.0.0.1/")]
+        [InlineData("http://127.0.0.1/oauth-callback")]
+        [InlineData("http://127.0.0.1/oauth-callback/")]
+        public async Task OAuth2Client_GetAuthorizationCodeAsync_RedirectUrlOriginalStringPreserved(string expectedRedirectUrl)
+        {
+            var baseUri = new Uri("https://example.com");
+            OAuth2ServerEndpoints endpoints = CreateEndpoints(baseUri);
+
+            var httpHandler = new TestHttpMessageHandler {ThrowOnUnexpectedRequest = true};
+
+            OAuth2Application app = new OAuth2Application(TestClientId)
+            {
+                Secret = TestClientSecret,
+                RedirectUris = new[] {new Uri(expectedRedirectUrl)}
+            };
+
+            var server = new TestOAuth2Server(endpoints);
+            server.RegisterApplication(app);
+            server.Bind(httpHandler);
+            server.TokenGenerator.AuthCodes.Add("unused");
+            server.AuthorizationEndpointInvoked += (_, request) =>
+            {
+                IDictionary<string, string> actualParams = request.RequestUri.GetQueryParameters();
+                Assert.True(actualParams.TryGetValue(OAuth2Constants.RedirectUriParameter, out string actualRedirectUri));
+                Assert.Equal(expectedRedirectUrl, actualRedirectUri);
+            };
+
+            IOAuth2WebBrowser browser = new TestOAuth2WebBrowser(httpHandler);
+
+            var redirectUri = new Uri(expectedRedirectUrl);
+
+            var trace2 = new NullTrace2();
+            OAuth2Client client =  new OAuth2Client(
+                new HttpClient(httpHandler),
+                endpoints,
+                TestClientId,
+                trace2,
+                redirectUri,
+                TestClientSecret);
+
+            await client.GetAuthorizationCodeAsync(new[] { "unused" }, browser, null, CancellationToken.None);
+        }
+
         [Fact]
         public async Task OAuth2Client_GetAuthorizationCodeAsync_ExtraQueryParams()
         {

src/shared/Core.Tests/Authentication/OAuth2SystemWebBrowserTests.cs

@@ -0,0 +1,66 @@
+using System;
+using GitCredentialManager.Authentication.OAuth;
+using GitCredentialManager.Tests.Objects;
+using Xunit;
+
+namespace GitCredentialManager.Tests.Authentication;
+
+public class OAuth2SystemWebBrowserTests
+{
+    [Fact]
+    public void OAuth2SystemWebBrowser_UpdateRedirectUri_NonLoopback_ThrowsError()
+    {
+        var env = new TestEnvironment();
+        var options = new OAuth2WebBrowserOptions();
+        var browser = new OAuth2SystemWebBrowser(env, options);
+
+        Assert.Throws<ArgumentException>(() => browser.UpdateRedirectUri(new Uri("http://example.com")));
+    }
+
+    [Theory]
+    [InlineData("http://localhost:1234", "http://localhost:1234")]
+    [InlineData("http://localhost:1234/", "http://localhost:1234/")]
+    [InlineData("http://localhost:1234/oauth-callback", "http://localhost:1234/oauth-callback")]
+    [InlineData("http://localhost:1234/oauth-callback/", "http://localhost:1234/oauth-callback/")]
+    [InlineData("http://127.0.0.7:1234", "http://127.0.0.7:1234")]
+    [InlineData("http://127.0.0.7:1234/", "http://127.0.0.7:1234/")]
+    [InlineData("http://127.0.0.7:1234/oauth-callback", "http://127.0.0.7:1234/oauth-callback")]
+    [InlineData("http://127.0.0.7:1234/oauth-callback/", "http://127.0.0.7:1234/oauth-callback/")]
+    public void OAuth2SystemWebBrowser_UpdateRedirectUri_SpecificPort(string input, string expected)
+    {
+        var env = new TestEnvironment();
+        var options = new OAuth2WebBrowserOptions();
+        var browser = new OAuth2SystemWebBrowser(env, options);
+
+        Uri actualUri = browser.UpdateRedirectUri(new Uri(input));
+
+        Assert.Equal(expected, actualUri.OriginalString);
+    }
+
+    [Theory]
+    [InlineData("http://localhost")]
+    [InlineData("http://localhost/")]
+    [InlineData("http://localhost/oauth-callback")]
+    [InlineData("http://localhost/oauth-callback/")]
+    [InlineData("http://127.0.0.7")]
+    [InlineData("http://127.0.0.7/")]
+    [InlineData("http://127.0.0.7/oauth-callback")]
+    [InlineData("http://127.0.0.7/oauth-callback/")]
+    public void OAuth2SystemWebBrowser_UpdateRedirectUri_AnyPort(string input)
+    {
+        var env = new TestEnvironment();
+        var options = new OAuth2WebBrowserOptions();
+        var browser = new OAuth2SystemWebBrowser(env, options);
+
+        var inputUri = new Uri(input);
+        Uri actualUri = browser.UpdateRedirectUri(inputUri);
+
+        Assert.Equal(inputUri.Scheme, actualUri.Scheme);
+        Assert.Equal(inputUri.Host, actualUri.Host);
+        Assert.Equal(
+            inputUri.GetComponents(UriComponents.Path, UriFormat.Unescaped),
+            actualUri.GetComponents(UriComponents.Path, UriFormat.Unescaped)
+        );
+        Assert.False(actualUri.IsDefaultPort);
+    }
+}

src/shared/Core/Authentication/OAuth/OAuth2Client.cs

@@ -138,7 +138,10 @@ public async Task<OAuth2AuthorizationCodeResult> GetAuthorizationCodeAsync(IEnum
             if (_redirectUri != null)
             {
                 redirectUri = browser.UpdateRedirectUri(_redirectUri);
-                queryParams[OAuth2Constants.RedirectUriParameter] = redirectUri.ToString();
+
+                // We must use the .OriginalString property here over .ToString() because OAuth requires the redirect
+                // URLs to be compared exactly, respecting missing/present trailing slashes, byte-for-byte.
+                queryParams[OAuth2Constants.RedirectUriParameter] = redirectUri.OriginalString;
             }
 
             string scopesStr = string.Join(" ", scopes);
@@ -235,7 +238,7 @@ public async Task<OAuth2TokenResult> GetTokenByAuthorizationCodeAsync(OAuth2Auth
 
             if (authorizationCodeResult.RedirectUri != null)
             {
-                formData[OAuth2Constants.RedirectUriParameter] = authorizationCodeResult.RedirectUri.ToString();
+                formData[OAuth2Constants.RedirectUriParameter] = authorizationCodeResult.RedirectUri.OriginalString;
             }
 
             if (authorizationCodeResult.CodeVerifier != null)

src/shared/GitHub/GitHubConstants.cs

@@ -14,7 +14,7 @@ public static class GitHubConstants
 
         // [SuppressMessage("Microsoft.Security", "CS002:SecretInNextLine", Justification="OAuth2 public client application 'secrets' are required and permitted to be public")]
         public const string OAuthClientSecret = "18867509d956965542b521a529a79bb883344c90";
-        public static readonly Uri OAuthRedirectUri = new Uri("http://localhost/");
+        public static readonly Uri OAuthRedirectUri = new Uri("http://localhost/"); // Note that the trailing slash is important!
         public static readonly Uri OAuthAuthorizationEndpointRelativeUri = new Uri("/login/oauth/authorize", UriKind.Relative);
         public static readonly Uri OAuthTokenEndpointRelativeUri = new Uri("/login/oauth/access_token", UriKind.Relative);
         public static readonly Uri OAuthDeviceEndpointRelativeUri = new Uri("/login/device/code", UriKind.Relative);

src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs

@@ -71,9 +71,9 @@ private Task<HttpResponseMessage> OnAuthorizationEndpointAsync(HttpRequestMessag
                 throw new Exception($"Unknown OAuth application '{clientId}'");
             }
 
-            // Redirect is optional, but if it is specified it must match a registered URI
-            reqQuery.TryGetValue(OAuth2Constants.RedirectUriParameter, out string redirectUriStr);
-            Uri redirectUri = app.ValidateRedirect(redirectUriStr);
+            // Redirect is optional, but if it is specified it must match a registered URL
+            reqQuery.TryGetValue(OAuth2Constants.RedirectUriParameter, out string redirectUrlStr);
+            Uri redirectUri = app.ValidateRedirect(redirectUrlStr);
 
             // Scope is optional
             reqQuery.TryGetValue(OAuth2Constants.ScopeParameter, out string scopeStr);
@@ -104,7 +104,7 @@ private Task<HttpResponseMessage> OnAuthorizationEndpointAsync(HttpRequestMessag
 
             // Create the auth code grant
             OAuth2Application.AuthCodeGrant grant = app.CreateAuthorizationCodeGrant(
-                TokenGenerator, scopes, redirectUriStr, codeChallenge, codeChallengeMethod);
+                TokenGenerator, scopes, redirectUrlStr, codeChallenge, codeChallengeMethod);
 
             var respQuery = new Dictionary<string, string>
             {
@@ -527,23 +527,25 @@ public TokenEndpointResponseJson CreateTokenByDeviceCodeGrant(TestOAuth2ServerTo
             };
         }
 
-        private bool IsValidRedirect(Uri uri)
+        private bool IsValidRedirect(string url)
         {
             foreach (Uri redirectUri in RedirectUris)
             {
-                if (redirectUri == uri)
+                // We only accept exact matches, including trailing slashes and case sensitivity
+                if (StringComparer.Ordinal.Equals(redirectUri.OriginalString, url))
                 {
                     return true;
                 }
 
-                // For localhost we ignore the port number
-                if (redirectUri.IsLoopback && uri.IsLoopback)
+                // For loopback URLs _only_ we ignore the port number
+                if (Uri.TryCreate(url, UriKind.Absolute, out Uri uri) && uri.IsLoopback && redirectUri.IsLoopback)
                 {
-                    var cmp = StringComparer.OrdinalIgnoreCase;
+                    // *Case-sensitive* comparison of scheme, host and path
+                    var cmp = StringComparer.Ordinal;
 
-                    // Uri::Authority does not include port, whereas Uri::Host does
+                    // Uri::Authority includes port, whereas Uri::Host does not
                     return cmp.Equals(redirectUri.Scheme, uri.Scheme) &&
-                           cmp.Equals(redirectUri.Authority, uri.Authority) &&
+                           cmp.Equals(redirectUri.Host, uri.Host) &&
                            cmp.Equals(redirectUri.GetComponents(UriComponents.Path, UriFormat.UriEscaped),
                                uri.GetComponents(UriComponents.Path, UriFormat.UriEscaped));
                 }
@@ -552,26 +554,26 @@ private bool IsValidRedirect(Uri uri)
             return false;
         }
 
-        internal Uri ValidateRedirect(string redirectStr)
+        internal Uri ValidateRedirect(string redirectUrl)
         {
             // Use default redirect URI if one has not been specified for this grant
-            if (redirectStr == null)
+            if (redirectUrl == null)
             {
                 return RedirectUris.First();
             }
 
-            if (!Uri.TryCreate(redirectStr, UriKind.Absolute, out Uri redirectUri))
+            if (!Uri.TryCreate(redirectUrl, UriKind.Absolute, out _))
             {
-                throw new Exception($"Redirect '{redirectStr}' is not a valid URI");
+                throw new Exception($"Redirect '{redirectUrl}' is not a valid URL");
             }
 
-            if (!IsValidRedirect(redirectUri))
+            if (!IsValidRedirect(redirectUrl))
             {
-                // If a redirect URI has been specified, it must match one of those that has been previously registered
-                throw new Exception($"Redirect URI '{redirectUri}' does not match any registered values.");
+                // If a redirect URL has been specified, it must match one of those that has been previously registered
+                throw new Exception($"Redirect URL '{redirectUrl}' does not match any registered values.");
             }
 
-            return redirectUri;
+            return new Uri(redirectUrl);
         }
     }
 }