diff --git a/src/shared/Core.Tests/Authentication/OAuth2ClientTests.cs b/src/shared/Core.Tests/Authentication/OAuth2ClientTests.cs index 3dd87cc93..be660b99b 100644 --- a/src/shared/Core.Tests/Authentication/OAuth2ClientTests.cs +++ b/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 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() { diff --git a/src/shared/Core.Tests/Authentication/OAuth2SystemWebBrowserTests.cs b/src/shared/Core.Tests/Authentication/OAuth2SystemWebBrowserTests.cs new file mode 100644 index 000000000..2d80c94ab --- /dev/null +++ b/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(() => 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); + } +} diff --git a/src/shared/Core/Authentication/OAuth/OAuth2Client.cs b/src/shared/Core/Authentication/OAuth/OAuth2Client.cs index cb3686f8a..27834d2aa 100644 --- a/src/shared/Core/Authentication/OAuth/OAuth2Client.cs +++ b/src/shared/Core/Authentication/OAuth/OAuth2Client.cs @@ -138,7 +138,10 @@ public async Task 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 GetTokenByAuthorizationCodeAsync(OAuth2Auth if (authorizationCodeResult.RedirectUri != null) { - formData[OAuth2Constants.RedirectUriParameter] = authorizationCodeResult.RedirectUri.ToString(); + formData[OAuth2Constants.RedirectUriParameter] = authorizationCodeResult.RedirectUri.OriginalString; } if (authorizationCodeResult.CodeVerifier != null) diff --git a/src/shared/GitHub/GitHubConstants.cs b/src/shared/GitHub/GitHubConstants.cs index 82c98528c..0f6e3bd14 100644 --- a/src/shared/GitHub/GitHubConstants.cs +++ b/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); diff --git a/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs b/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs index 9ecdc52e9..d72ceb521 100644 --- a/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs +++ b/src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs @@ -71,9 +71,9 @@ private Task 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 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 { @@ -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); } } }