Skip to content

Commit 4e8674a

Browse files
mjcheethammjcheetham
authored
Preserve exact original redirect URL in OAuth client (#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 #594
2 parents 6109dc3 + af429fe commit 4e8674a

File tree

5 files changed

+142
-22
lines changed

5 files changed

+142
-22
lines changed

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

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,55 @@ public async Task OAuth2Client_GetAuthorizationCodeAsync()
4444
Assert.Equal(expectedAuthCode, result.Code);
4545
}
4646

47+
[Theory]
48+
[InlineData("http://localhost")]
49+
[InlineData("http://localhost/")]
50+
[InlineData("http://localhost/oauth-callback")]
51+
[InlineData("http://localhost/oauth-callback/")]
52+
[InlineData("http://127.0.0.1")]
53+
[InlineData("http://127.0.0.1/")]
54+
[InlineData("http://127.0.0.1/oauth-callback")]
55+
[InlineData("http://127.0.0.1/oauth-callback/")]
56+
public async Task OAuth2Client_GetAuthorizationCodeAsync_RedirectUrlOriginalStringPreserved(string expectedRedirectUrl)
57+
{
58+
var baseUri = new Uri("https://example.com");
59+
OAuth2ServerEndpoints endpoints = CreateEndpoints(baseUri);
60+
61+
var httpHandler = new TestHttpMessageHandler {ThrowOnUnexpectedRequest = true};
62+
63+
OAuth2Application app = new OAuth2Application(TestClientId)
64+
{
65+
Secret = TestClientSecret,
66+
RedirectUris = new[] {new Uri(expectedRedirectUrl)}
67+
};
68+
69+
var server = new TestOAuth2Server(endpoints);
70+
server.RegisterApplication(app);
71+
server.Bind(httpHandler);
72+
server.TokenGenerator.AuthCodes.Add("unused");
73+
server.AuthorizationEndpointInvoked += (_, request) =>
74+
{
75+
IDictionary<string, string> actualParams = request.RequestUri.GetQueryParameters();
76+
Assert.True(actualParams.TryGetValue(OAuth2Constants.RedirectUriParameter, out string actualRedirectUri));
77+
Assert.Equal(expectedRedirectUrl, actualRedirectUri);
78+
};
79+
80+
IOAuth2WebBrowser browser = new TestOAuth2WebBrowser(httpHandler);
81+
82+
var redirectUri = new Uri(expectedRedirectUrl);
83+
84+
var trace2 = new NullTrace2();
85+
OAuth2Client client = new OAuth2Client(
86+
new HttpClient(httpHandler),
87+
endpoints,
88+
TestClientId,
89+
trace2,
90+
redirectUri,
91+
TestClientSecret);
92+
93+
await client.GetAuthorizationCodeAsync(new[] { "unused" }, browser, null, CancellationToken.None);
94+
}
95+
4796
[Fact]
4897
public async Task OAuth2Client_GetAuthorizationCodeAsync_ExtraQueryParams()
4998
{

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

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
using System;
2+
using GitCredentialManager.Authentication.OAuth;
3+
using GitCredentialManager.Tests.Objects;
4+
using Xunit;
5+
6+
namespace GitCredentialManager.Tests.Authentication;
7+
8+
public class OAuth2SystemWebBrowserTests
9+
{
10+
[Fact]
11+
public void OAuth2SystemWebBrowser_UpdateRedirectUri_NonLoopback_ThrowsError()
12+
{
13+
var env = new TestEnvironment();
14+
var options = new OAuth2WebBrowserOptions();
15+
var browser = new OAuth2SystemWebBrowser(env, options);
16+
17+
Assert.Throws<ArgumentException>(() => browser.UpdateRedirectUri(new Uri("http://example.com")));
18+
}
19+
20+
[Theory]
21+
[InlineData("http://localhost:1234", "http://localhost:1234")]
22+
[InlineData("http://localhost:1234/", "http://localhost:1234/")]
23+
[InlineData("http://localhost:1234/oauth-callback", "http://localhost:1234/oauth-callback")]
24+
[InlineData("http://localhost:1234/oauth-callback/", "http://localhost:1234/oauth-callback/")]
25+
[InlineData("http://127.0.0.7:1234", "http://127.0.0.7:1234")]
26+
[InlineData("http://127.0.0.7:1234/", "http://127.0.0.7:1234/")]
27+
[InlineData("http://127.0.0.7:1234/oauth-callback", "http://127.0.0.7:1234/oauth-callback")]
28+
[InlineData("http://127.0.0.7:1234/oauth-callback/", "http://127.0.0.7:1234/oauth-callback/")]
29+
public void OAuth2SystemWebBrowser_UpdateRedirectUri_SpecificPort(string input, string expected)
30+
{
31+
var env = new TestEnvironment();
32+
var options = new OAuth2WebBrowserOptions();
33+
var browser = new OAuth2SystemWebBrowser(env, options);
34+
35+
Uri actualUri = browser.UpdateRedirectUri(new Uri(input));
36+
37+
Assert.Equal(expected, actualUri.OriginalString);
38+
}
39+
40+
[Theory]
41+
[InlineData("http://localhost")]
42+
[InlineData("http://localhost/")]
43+
[InlineData("http://localhost/oauth-callback")]
44+
[InlineData("http://localhost/oauth-callback/")]
45+
[InlineData("http://127.0.0.7")]
46+
[InlineData("http://127.0.0.7/")]
47+
[InlineData("http://127.0.0.7/oauth-callback")]
48+
[InlineData("http://127.0.0.7/oauth-callback/")]
49+
public void OAuth2SystemWebBrowser_UpdateRedirectUri_AnyPort(string input)
50+
{
51+
var env = new TestEnvironment();
52+
var options = new OAuth2WebBrowserOptions();
53+
var browser = new OAuth2SystemWebBrowser(env, options);
54+
55+
var inputUri = new Uri(input);
56+
Uri actualUri = browser.UpdateRedirectUri(inputUri);
57+
58+
Assert.Equal(inputUri.Scheme, actualUri.Scheme);
59+
Assert.Equal(inputUri.Host, actualUri.Host);
60+
Assert.Equal(
61+
inputUri.GetComponents(UriComponents.Path, UriFormat.Unescaped),
62+
actualUri.GetComponents(UriComponents.Path, UriFormat.Unescaped)
63+
);
64+
Assert.False(actualUri.IsDefaultPort);
65+
}
66+
}

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,10 @@ public async Task<OAuth2AuthorizationCodeResult> GetAuthorizationCodeAsync(IEnum
138138
if (_redirectUri != null)
139139
{
140140
redirectUri = browser.UpdateRedirectUri(_redirectUri);
141-
queryParams[OAuth2Constants.RedirectUriParameter] = redirectUri.ToString();
141+
142+
// We must use the .OriginalString property here over .ToString() because OAuth requires the redirect
143+
// URLs to be compared exactly, respecting missing/present trailing slashes, byte-for-byte.
144+
queryParams[OAuth2Constants.RedirectUriParameter] = redirectUri.OriginalString;
142145
}
143146

144147
string scopesStr = string.Join(" ", scopes);
@@ -235,7 +238,7 @@ public async Task<OAuth2TokenResult> GetTokenByAuthorizationCodeAsync(OAuth2Auth
235238

236239
if (authorizationCodeResult.RedirectUri != null)
237240
{
238-
formData[OAuth2Constants.RedirectUriParameter] = authorizationCodeResult.RedirectUri.ToString();
241+
formData[OAuth2Constants.RedirectUriParameter] = authorizationCodeResult.RedirectUri.OriginalString;
239242
}
240243

241244
if (authorizationCodeResult.CodeVerifier != null)

‎src/shared/GitHub/GitHubConstants.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ public static class GitHubConstants
1414

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

‎src/shared/TestInfrastructure/Objects/TestOAuth2Server.cs‎

Lines changed: 21 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,9 @@ private Task<HttpResponseMessage> OnAuthorizationEndpointAsync(HttpRequestMessag
7171
throw new Exception($"Unknown OAuth application '{clientId}'");
7272
}
7373

74-
// Redirect is optional, but if it is specified it must match a registered URI
75-
reqQuery.TryGetValue(OAuth2Constants.RedirectUriParameter, out string redirectUriStr);
76-
Uri redirectUri = app.ValidateRedirect(redirectUriStr);
74+
// Redirect is optional, but if it is specified it must match a registered URL
75+
reqQuery.TryGetValue(OAuth2Constants.RedirectUriParameter, out string redirectUrlStr);
76+
Uri redirectUri = app.ValidateRedirect(redirectUrlStr);
7777

7878
// Scope is optional
7979
reqQuery.TryGetValue(OAuth2Constants.ScopeParameter, out string scopeStr);
@@ -104,7 +104,7 @@ private Task<HttpResponseMessage> OnAuthorizationEndpointAsync(HttpRequestMessag
104104

105105
// Create the auth code grant
106106
OAuth2Application.AuthCodeGrant grant = app.CreateAuthorizationCodeGrant(
107-
TokenGenerator, scopes, redirectUriStr, codeChallenge, codeChallengeMethod);
107+
TokenGenerator, scopes, redirectUrlStr, codeChallenge, codeChallengeMethod);
108108

109109
var respQuery = new Dictionary<string, string>
110110
{
@@ -527,23 +527,25 @@ public TokenEndpointResponseJson CreateTokenByDeviceCodeGrant(TestOAuth2ServerTo
527527
};
528528
}
529529

530-
private bool IsValidRedirect(Uri uri)
530+
private bool IsValidRedirect(string url)
531531
{
532532
foreach (Uri redirectUri in RedirectUris)
533533
{
534-
if (redirectUri == uri)
534+
// We only accept exact matches, including trailing slashes and case sensitivity
535+
if (StringComparer.Ordinal.Equals(redirectUri.OriginalString, url))
535536
{
536537
return true;
537538
}
538539

539-
// For localhost we ignore the port number
540-
if (redirectUri.IsLoopback && uri.IsLoopback)
540+
// For loopback URLs _only_ we ignore the port number
541+
if (Uri.TryCreate(url, UriKind.Absolute, out Uri uri) && uri.IsLoopback && redirectUri.IsLoopback)
541542
{
542-
var cmp = StringComparer.OrdinalIgnoreCase;
543+
// *Case-sensitive* comparison of scheme, host and path
544+
var cmp = StringComparer.Ordinal;
543545

544-
// Uri::Authority does not include port, whereas Uri::Host does
546+
// Uri::Authority includes port, whereas Uri::Host does not
545547
return cmp.Equals(redirectUri.Scheme, uri.Scheme) &&
546-
cmp.Equals(redirectUri.Authority, uri.Authority) &&
548+
cmp.Equals(redirectUri.Host, uri.Host) &&
547549
cmp.Equals(redirectUri.GetComponents(UriComponents.Path, UriFormat.UriEscaped),
548550
uri.GetComponents(UriComponents.Path, UriFormat.UriEscaped));
549551
}
@@ -552,26 +554,26 @@ private bool IsValidRedirect(Uri uri)
552554
return false;
553555
}
554556

555-
internal Uri ValidateRedirect(string redirectStr)
557+
internal Uri ValidateRedirect(string redirectUrl)
556558
{
557559
// Use default redirect URI if one has not been specified for this grant
558-
if (redirectStr == null)
560+
if (redirectUrl == null)
559561
{
560562
return RedirectUris.First();
561563
}
562564

563-
if (!Uri.TryCreate(redirectStr, UriKind.Absolute, out Uri redirectUri))
565+
if (!Uri.TryCreate(redirectUrl, UriKind.Absolute, out _))
564566
{
565-
throw new Exception($"Redirect '{redirectStr}' is not a valid URI");
567+
throw new Exception($"Redirect '{redirectUrl}' is not a valid URL");
566568
}
567569

568-
if (!IsValidRedirect(redirectUri))
570+
if (!IsValidRedirect(redirectUrl))
569571
{
570-
// If a redirect URI has been specified, it must match one of those that has been previously registered
571-
throw new Exception($"Redirect URI '{redirectUri}' does not match any registered values.");
572+
// If a redirect URL has been specified, it must match one of those that has been previously registered
573+
throw new Exception($"Redirect URL '{redirectUrl}' does not match any registered values.");
572574
}
573575

574-
return redirectUri;
576+
return new Uri(redirectUrl);
575577
}
576578
}
577579
}

0 commit comments

Comments
 (0)