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

Allow GitHub OAuth params to be overridden at runtime #103

Merged
merged 1 commit into from
Apr 28, 2020

Conversation

mjcheetham
Copy link
Collaborator

Allow the OAuth client ID, secret, and redirect URI to be overridden at runtime using environment variables or config.
This allows for easier testing for developers. These configuration options are not officially supported and are therefore not documented. They are for development only.

@mjcheetham mjcheetham added the host:github Specific to the GitHub host provider label Apr 24, 2020
@mjcheetham mjcheetham closed this Apr 24, 2020
@mjcheetham mjcheetham reopened this Apr 24, 2020
return GitHubConstants.OAuthClientId;
}

private static Uri GetRedirectUri(ISettings settings)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would let me route the redirect to something besides localhost. I can't express
whether this bothers me or whether I'm just being paranoid. I tried to walk thru the
MITM diagrams from the other day, but I'm in over my head I fear. It may not be a
problem, but I don't know that.

I'm wondering what you would use this env var for in testing -- if as I understand it,
the responses need to come back into the builtin GCM server to advance the state
machine. It seems like this would route the redirect somewhere else and you wouldn't
get responses back in the GCM. But again, I don't completely understand the flow.

Alternatively, would it be safer to have this env var be under a #if DEBUG ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redirect URIs in OAuth2 are tied to the client ID. That is to say each registered application must define at least one redirect URI, and during the various endpoint calls the client must send a redirect URI that is associated with that app.

For example GCM just uses http://localhost/ as the only redirect URI (loopback hosts are special cased in that the port is lazily matched; otherwise port must === too).

Other applications such as GitHub CLI use http://127.0.0.1/callback as the redirect URI (it includes the path /callback).

Also note that 127.0.0.1 != localhost textually, so these are considered different redirect URIs.

This would let me route the redirect to something besides localhost.

In theory yes, in practice no since the OAuth2SystemWebBrowser component that we use in GCM (the component that launches the browser, and starts the HTTP listener for the redirect) mandates that the redirect URI Uri::IsLoopback here and here.

So we won't end up gifting http://evil-server.com with a authorisation code because:

  1. We won't even start the auth flow via the browser unless we have a local redirect URI,
  2. The server would need to agree to send the code to the redirect URI.

The 'problem' that might occur however is redirecting to another localhost application listening on with a different path prefix. This is 'fixed' with PKCE as mentioned and implemented by GCM previously, but that requires server support.

Copy link

@jeffhostetler jeffhostetler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern is the long comment on the redirect URL.
Everything else looked fine.

Allow the OAuth client ID, secret, and redirect URI to be overridden at
runtime using environment variables or config.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
host:github Specific to the GitHub host provider
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants