Preserve exact original redirect URL in OAuth client #1281
Conversation
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.
| 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); |
There was a problem hiding this comment.
We don't seem to be testing any outcomes here. Are we just validating that this works with these redirect URLs?
There was a problem hiding this comment.
Look a little higher up at line 73:
server.AuthorizationEndpointInvoked += (_, request) =>
{
IDictionary<string, string> actualParams = request.RequestUri.GetQueryParameters();
Assert.True(actualParams.TryGetValue(OAuth2Constants.RedirectUriParameter, out string actualRedirectUri));
Assert.Equal(expectedRedirectUrl, actualRedirectUri);
};We are setting an event handler on the AuthorizationEndpointInvoked event, and performing the validation of the redirect_uri in there.
There was a problem hiding this comment.
Got it! Thanks for clarifying.
| { | ||
| return true; | ||
| } | ||
|
|
||
| // For localhost we ignore the port number | ||
| if (redirectUri.IsLoopback && uri.IsLoopback) | ||
| // For loopback URLs _only_ we ignore the port number |
There was a problem hiding this comment.
I'm curious - why do we ignore ports for loopback urls?
There was a problem hiding this comment.
This is to mimic real-world OAuth2 authorisation servers, which typically treat loopback addresses (localhost, 127.0.0.1, ::1) as special.
Redirect URLs are normally supposed to match exactly, including port number. This is the location where the authorisation server will redirect the user agent to, including the authorisation code in the query params for the client to consume.
For public client applications (ones that run on an end-user device), we need to get the authorisation code back to the client application from a user agent somehow. You can achieve this in two ways:
- Using an application-specific URL like (
myapp://oauth) that is registered with the operating system, or - Using a local loopback URL, and have the client application open a port listening for that HTTP request.
For option 2, you can either explicitly define a port number to use for the redirect, or leave it blank. Leaving it blank, the client will pick any random free port before it makes the initial authorisation request. Since the port may not be known ahead of time, the authorization server cannot restrict ahead of time.
Why not just pick an explicit port? Well.. which one should we use? 😜
Obviously one of the ephemeral ports (49152–65535), but there's of course a non-zero (although small) chance we pick a port that's in use by something else on the system...
| return cmp.Equals(redirectUri.Scheme, uri.Scheme) && | ||
| cmp.Equals(redirectUri.Authority, uri.Authority) && | ||
| cmp.Equals(redirectUri.Host, uri.Host) && |
There was a problem hiding this comment.
Was this incorrect before?
There was a problem hiding this comment.
Yes. I wanted to compare just the 'domain' part, not ports (as the comment said). But had Authority and Host backwards! 🤦
**Changes:** - Use in-proc methods for getting OS version number (#1240, #1264) - Update System.CommandLine (#1265) - Suppress GUI from command-line argument (#1267) - Add github (login|logout|list) commands (#1267) - cURL Cookie file support (#1251) - Update target framework on Mac/Linux to .NET 7 (#1274, #1282) - Replace JSON.NET with System.Text.Json (#1274) - Preserve exact redirect URI formatting in OAuth requests (#1281) - Use IP localhost redirect for GitHub (#1286) - Use WWW-Authenticate headers from Git for Azure Repos authority (#1288) - Better GitHub Enterprise Managed User (EMU) account support (#1190)
The OAuth 2.0 spec requires that redirect URLs be matched exactly if specified, including matching trailing slashes.
Since the .NET
Uritype'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.OriginalStringproperty instead.Shoring up this area in anticipation for changes to support multiple GitHub redirect URLs with #594