Skip to content
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

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

mjcheetham
Copy link
Collaborator

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

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.
Copy link
Contributor

@vdye vdye left a 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! 🚢

Copy link
Contributor

@vdye vdye left a 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?

@mjcheetham
Copy link
Collaborator Author

mjcheetham commented Jul 12, 2023

is GitHubOAuth2Client unit tested? If not, could/should it be?

GitHubOAuth2Client is not unit tested, but the base class OAuth2Client is heavily unit tested. The GitHub* subclass adds only configuration, which is server/instance specific.

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.

@mjcheetham mjcheetham merged commit 7ff639d into git-ecosystem:main Jul 12, 2023
@mjcheetham mjcheetham deleted the ghes-redirect-fix branch July 12, 2023 16:28
@mjcheetham mjcheetham mentioned this pull request Jul 12, 2023
mjcheetham added a commit that referenced this pull request Jul 12, 2023
**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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GitHub Enterprise redirects to localhost:80 instead of 127.0.0.1:60167
3 participants