-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use localhost redirect URI for GHES instances #1330
Conversation
For github.com we've updated the redirect URI to 127.0.0.1, whilst also keeping the localhost variant around for backwards compatibility with older GCM clients. However, since GHES has not been updated with the new 127.0.0.1 redirect, and older GHES servers will be stuck with the old redirect we must continue to use the localhost redirect on the client for non-dotcom targets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commit message + comments explain the issue and implementation clearly. Everything looks good to me! 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, one other note - is GitHubOAuth2Client
unit tested? If not, could/should it be?
Unit testing would not have necessarily caught this sort of issue. The oversight here was that there are different "GitHub" environments (dotcom, AE, ES) and each of them may have different server-side configuration (and with GHES, different versions have different configuration over time). We forgot to do a real-world test against the latest GHES, and haven't yet had a new release of GHES that updates the redirect URI. End-to-end testing against a real GHES instance, latest plus N older versions, would be ideal and have caught this. |
**Changes since 2.2.1:** - Fix an issue where duplicate "Personal Access Token" GitHub account options are shown when Visual Studio has a GitHub account signed-in (#1325 #1328) - Fix an issue with Azure DevOps Server (TFS) and Windows Integrated Authentication (#1331 #1332) - Fix an issue with OAuth redirects GitHub Enterprise Server (#1329 #1330) - Correctly handle non-ASCII username/passwords with the WPF UI helpers (#1287 #1326)
For github.com we've updated the redirect URI to 127.0.0.1, whilst also keeping the localhost variant around for backwards compatibility with older GCM clients.
However, since GHES has not been updated with the new 127.0.0.1 redirect, and older GHES servers will be stuck with the old redirect we must continue to use the localhost redirect on the client for non-dotcom targets.
Fixes #1329