From 5d6079f8ad16f553cdaea1d56fedcb4a3a1db082 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 31 Oct 2024 14:07:48 +0100 Subject: [PATCH 1/3] Fix token exposure for non-gh hosts in codespaces This commit introduces a fix for `GITHUB_TOKEN` being exposed to non-github hosts while in a codespace. We no longer return the `GITHUB_TOKEN` for any host except github.com and github.localhost while in a codespace (while the env var `CODESPACES` is `true`). This commit also changes how tokens are returned when no oAuth token is found in a config. Previously, an empty string and the `oauthToken` source was returned. Now, we return an empty string and the `defaultSource` source. The intention behind this change is to make more logical sense by not returning an `oauthToken` source when we didn't get any token. It's also worth mentioning that this change also improves our test coverage - all lines in `tokenForHost` are now covered by tests, and we don't have unreachable code. Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com> --- pkg/auth/auth.go | 27 ++++++++---- pkg/auth/auth_test.go | 97 ++++++++++++++++++++++++++++++++----------- 2 files changed, 91 insertions(+), 33 deletions(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index a903736..4378e75 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -63,6 +63,15 @@ func TokenFromEnvOrConfig(host string) (string, string) { func tokenForHost(cfg *config.Config, host string) (string, string) { host = NormalizeHostname(host) + + if isCodespaces, _ := strconv.ParseBool(os.Getenv(codespaces)); isCodespaces { + if host == github || host == localhost { + if token := os.Getenv(githubToken); token != "" { + return token, githubToken + } + } + } + if IsEnterprise(host) { if token := os.Getenv(ghEnterpriseToken); token != "" { return token, ghEnterpriseToken @@ -70,25 +79,25 @@ func tokenForHost(cfg *config.Config, host string) (string, string) { if token := os.Getenv(githubEnterpriseToken); token != "" { return token, githubEnterpriseToken } - if isCodespaces, _ := strconv.ParseBool(os.Getenv(codespaces)); isCodespaces { - if token := os.Getenv(githubToken); token != "" { - return token, githubToken - } - } if cfg != nil { - token, _ := cfg.Get([]string{hostsKey, host, oauthToken}) - return token, oauthToken + if token, _ := cfg.Get([]string{hostsKey, host, oauthToken}); token != "" { + return token, oauthToken + } } + return "", defaultSource } + if token := os.Getenv(ghToken); token != "" { return token, ghToken } if token := os.Getenv(githubToken); token != "" { return token, githubToken } + if cfg != nil { - token, _ := cfg.Get([]string{hostsKey, host, oauthToken}) - return token, oauthToken + if token, _ := cfg.Get([]string{hostsKey, host, oauthToken}); token != "" { + return token, oauthToken + } } return "", defaultSource } diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index e6b7b9a..93ed35d 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -1,10 +1,12 @@ package auth import ( + "strconv" "testing" "github.com/cli/go-gh/v2/pkg/config" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestTokenForHost(t *testing.T) { @@ -15,26 +17,24 @@ func TestTokenForHost(t *testing.T) { githubEnterpriseToken string ghToken string ghEnterpriseToken string + codespaces bool config *config.Config wantToken string wantSource string - wantNotFound bool }{ { - name: "token for github.com with no env tokens and no config token", - host: "github.com", - config: testNoHostsConfig(), - wantToken: "", - wantSource: "oauth_token", - wantNotFound: true, + name: "token for github.com with no env tokens and no config token", + host: "github.com", + config: testNoHostsConfig(), + wantToken: "", + wantSource: defaultSource, }, { - name: "token for enterprise.com with no env tokens and no config token", - host: "enterprise.com", - config: testNoHostsConfig(), - wantToken: "", - wantSource: "oauth_token", - wantNotFound: true, + name: "token for enterprise.com with no env tokens and no config token", + host: "enterprise.com", + config: testNoHostsConfig(), + wantToken: "", + wantSource: defaultSource, }, { name: "token for github.com with GH_TOKEN, GITHUB_TOKEN, and config token", @@ -43,7 +43,7 @@ func TestTokenForHost(t *testing.T) { githubToken: "GITHUB_TOKEN", config: testHostsConfig(), wantToken: "GH_TOKEN", - wantSource: "GH_TOKEN", + wantSource: ghToken, }, { name: "token for github.com with GITHUB_TOKEN, and config token", @@ -51,14 +51,14 @@ func TestTokenForHost(t *testing.T) { githubToken: "GITHUB_TOKEN", config: testHostsConfig(), wantToken: "GITHUB_TOKEN", - wantSource: "GITHUB_TOKEN", + wantSource: githubToken, }, { name: "token for github.com with config token", host: "github.com", config: testHostsConfig(), wantToken: "xxxxxxxxxxxxxxxxxxxx", - wantSource: "oauth_token", + wantSource: oauthToken, }, { name: "token for enterprise.com with GH_ENTERPRISE_TOKEN, GITHUB_ENTERPRISE_TOKEN, and config token", @@ -67,7 +67,7 @@ func TestTokenForHost(t *testing.T) { githubEnterpriseToken: "GITHUB_ENTERPRISE_TOKEN", config: testHostsConfig(), wantToken: "GH_ENTERPRISE_TOKEN", - wantSource: "GH_ENTERPRISE_TOKEN", + wantSource: ghEnterpriseToken, }, { name: "token for enterprise.com with GITHUB_ENTERPRISE_TOKEN, and config token", @@ -75,14 +75,14 @@ func TestTokenForHost(t *testing.T) { githubEnterpriseToken: "GITHUB_ENTERPRISE_TOKEN", config: testHostsConfig(), wantToken: "GITHUB_ENTERPRISE_TOKEN", - wantSource: "GITHUB_ENTERPRISE_TOKEN", + wantSource: githubEnterpriseToken, }, { name: "token for enterprise.com with config token", host: "enterprise.com", config: testHostsConfig(), wantToken: "yyyyyyyyyyyyyyyyyyyy", - wantSource: "oauth_token", + wantSource: oauthToken, }, { name: "token for tenant with GH_TOKEN, GITHUB_TOKEN, and config token", @@ -91,7 +91,7 @@ func TestTokenForHost(t *testing.T) { githubToken: "GITHUB_TOKEN", config: testHostsConfig(), wantToken: "GH_TOKEN", - wantSource: "GH_TOKEN", + wantSource: ghToken, }, { name: "token for tenant with GITHUB_TOKEN, and config token", @@ -99,14 +99,62 @@ func TestTokenForHost(t *testing.T) { githubToken: "GITHUB_TOKEN", config: testHostsConfig(), wantToken: "GITHUB_TOKEN", - wantSource: "GITHUB_TOKEN", + wantSource: githubToken, }, { name: "token for tenant with config token", host: "tenant.ghe.com", config: testHostsConfig(), wantToken: "zzzzzzzzzzzzzzzzzzzz", - wantSource: "oauth_token", + wantSource: oauthToken, + }, + { + name: "Token for non-github host in a codespace", + host: "doesnotmatter.com", + config: testNoHostsConfig(), + githubToken: "GITHUB_TOKEN", + codespaces: true, + wantToken: "", + wantSource: defaultSource, + }, + { + name: "Token for github.com in a codespace", + host: "github.com", + config: testNoHostsConfig(), + githubToken: "GITHUB_TOKEN", + codespaces: true, + wantToken: "GITHUB_TOKEN", + wantSource: githubToken, + }, + { + // We are in a codespace (dotcom), and we have set our own GITHUB_TOKEN, not using the codespace one + // and we are targeting tenant.ghe.com + name: "Token for tenant.ghe.com in a codespace", + host: "tenant.ghe.com", + config: testNoHostsConfig(), + githubToken: "GITHUB_TOKEN", + codespaces: true, + wantToken: "GITHUB_TOKEN", + wantSource: githubToken, + }, + { + name: "Token for github.localhost in a codespace", + host: "github.localhost", + config: testNoHostsConfig(), + githubToken: "GITHUB_TOKEN", + codespaces: true, + wantToken: "GITHUB_TOKEN", + wantSource: githubToken, + }, + { + // We are in codespace (dotcom), and we have set a GITHUB_ENTERPRISE_TOKEN, and we are targeting GHES + name: "Enterprise Token for GHES in a codespace", + host: "enterprise.com", + config: testNoHostsConfig(), + githubEnterpriseToken: "GITHUB_ENTERPRISE_TOKEN", + codespaces: true, + wantToken: "GITHUB_ENTERPRISE_TOKEN", + wantSource: githubEnterpriseToken, }, } @@ -116,9 +164,10 @@ func TestTokenForHost(t *testing.T) { t.Setenv("GITHUB_ENTERPRISE_TOKEN", tt.githubEnterpriseToken) t.Setenv("GH_TOKEN", tt.ghToken) t.Setenv("GH_ENTERPRISE_TOKEN", tt.ghEnterpriseToken) + t.Setenv("CODESPACES", strconv.FormatBool(tt.codespaces)) token, source := tokenForHost(tt.config, tt.host) - assert.Equal(t, tt.wantToken, token) - assert.Equal(t, tt.wantSource, source) + require.Equal(t, tt.wantToken, token, "Expected token for \"%s\" to be \"%s\", got \"%s\"", tt.host, tt.wantToken, token) + require.Equal(t, tt.wantSource, source, "Expected source for \"%s\" to be \"%s\", got \"%s\"", tt.host, tt.wantSource, source) }) } } From 5f282a5481c2bf906ef8470daf173dff58a44179 Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 31 Oct 2024 15:47:45 +0100 Subject: [PATCH 2/3] Refactor tokenForHost for readability Co-authored-by: Kynan Ware <47394200+BagToad@users.noreply.github.com> --- pkg/auth/auth.go | 53 ++++++++++---------- pkg/auth/auth_test.go | 109 ++++++++++++++++-------------------------- 2 files changed, 68 insertions(+), 94 deletions(-) diff --git a/pkg/auth/auth.go b/pkg/auth/auth.go index 4378e75..4c54642 100644 --- a/pkg/auth/auth.go +++ b/pkg/auth/auth.go @@ -6,7 +6,6 @@ import ( "fmt" "os" "os/exec" - "strconv" "strings" "github.com/cli/go-gh/v2/internal/set" @@ -62,44 +61,42 @@ func TokenFromEnvOrConfig(host string) (string, string) { } func tokenForHost(cfg *config.Config, host string) (string, string) { - host = NormalizeHostname(host) - - if isCodespaces, _ := strconv.ParseBool(os.Getenv(codespaces)); isCodespaces { - if host == github || host == localhost { - if token := os.Getenv(githubToken); token != "" { - return token, githubToken - } + normalizedHost := NormalizeHostname(host) + // This code is currently the exact opposite of IsEnterprise. However, we have chosen + // to write it separately, directly in line, because it is much clearer in the exact + // scenarios that we expect to use GH_TOKEN and GITHUB_TOKEN. + if normalizedHost == github || IsTenancy(normalizedHost) || normalizedHost == localhost { + if token := os.Getenv(ghToken); token != "" { + return token, ghToken } - } - if IsEnterprise(host) { + if token := os.Getenv(githubToken); token != "" { + return token, githubToken + } + } else { if token := os.Getenv(ghEnterpriseToken); token != "" { return token, ghEnterpriseToken } + if token := os.Getenv(githubEnterpriseToken); token != "" { return token, githubEnterpriseToken } - if cfg != nil { - if token, _ := cfg.Get([]string{hostsKey, host, oauthToken}); token != "" { - return token, oauthToken - } - } - return "", defaultSource } - if token := os.Getenv(ghToken); token != "" { - return token, ghToken - } - if token := os.Getenv(githubToken); token != "" { - return token, githubToken + // If config is nil, something has failed much earlier and it's probably + // more correct to panic because we don't expect to support anything + // where the config isn't available, but that would be a breaking change, + // so it's worth thinking about carefully, if we wanted to rework this. + if cfg == nil { + return "", defaultSource } - if cfg != nil { - if token, _ := cfg.Get([]string{hostsKey, host, oauthToken}); token != "" { - return token, oauthToken - } + token, err := cfg.Get([]string{hostsKey, normalizedHost, oauthToken}) + if err != nil { + return "", defaultSource } - return "", defaultSource + + return token, oauthToken } func tokenFromGh(path string, host string) (string, string) { @@ -160,8 +157,10 @@ func defaultHost(cfg *config.Config) (string, string) { } // IsEnterprise determines if a provided host is a GitHub Enterprise Server instance, -// rather than GitHub.com or a tenancy GitHub instance. +// rather than GitHub.com, a tenancy GitHub instance, or github.localhost. func IsEnterprise(host string) bool { + // Note that if you are making changes here, you should also consider making the equivalent + // in tokenForHost, which is the exact opposite of this function. normalizedHost := NormalizeHostname(host) return normalizedHost != github && normalizedHost != localhost && !IsTenancy(normalizedHost) } diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index 93ed35d..f4379a4 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -1,7 +1,6 @@ package auth import ( - "strconv" "testing" "github.com/cli/go-gh/v2/pkg/config" @@ -17,27 +16,26 @@ func TestTokenForHost(t *testing.T) { githubEnterpriseToken string ghToken string ghEnterpriseToken string - codespaces bool config *config.Config wantToken string wantSource string }{ { - name: "token for github.com with no env tokens and no config token", + name: "given there is no env token and no config token, when we get the token for github.com, then it returns the empty string and default source", host: "github.com", config: testNoHostsConfig(), wantToken: "", wantSource: defaultSource, }, { - name: "token for enterprise.com with no env tokens and no config token", + name: "given there is no env token and no config token, when we get the token for an enterprise server host, then it returns the empty string and default source", host: "enterprise.com", config: testNoHostsConfig(), wantToken: "", wantSource: defaultSource, }, { - name: "token for github.com with GH_TOKEN, GITHUB_TOKEN, and config token", + name: "given GH_TOKEN and GITHUB_TOKEN and a config token are set, when we get the token for github.com, then it returns GH_TOKEN as the priority", host: "github.com", ghToken: "GH_TOKEN", githubToken: "GITHUB_TOKEN", @@ -46,7 +44,7 @@ func TestTokenForHost(t *testing.T) { wantSource: ghToken, }, { - name: "token for github.com with GITHUB_TOKEN, and config token", + name: "given GITHUB_TOKEN and a config token are set, when we get the token for github.com, then it returns GITHUB_TOKEN as the priority", host: "github.com", githubToken: "GITHUB_TOKEN", config: testHostsConfig(), @@ -54,38 +52,14 @@ func TestTokenForHost(t *testing.T) { wantSource: githubToken, }, { - name: "token for github.com with config token", + name: "given a config token is set for github.com, when we get the token, then it returns that token and oauth_token source", host: "github.com", config: testHostsConfig(), wantToken: "xxxxxxxxxxxxxxxxxxxx", wantSource: oauthToken, }, { - name: "token for enterprise.com with GH_ENTERPRISE_TOKEN, GITHUB_ENTERPRISE_TOKEN, and config token", - host: "enterprise.com", - ghEnterpriseToken: "GH_ENTERPRISE_TOKEN", - githubEnterpriseToken: "GITHUB_ENTERPRISE_TOKEN", - config: testHostsConfig(), - wantToken: "GH_ENTERPRISE_TOKEN", - wantSource: ghEnterpriseToken, - }, - { - name: "token for enterprise.com with GITHUB_ENTERPRISE_TOKEN, and config token", - host: "enterprise.com", - githubEnterpriseToken: "GITHUB_ENTERPRISE_TOKEN", - config: testHostsConfig(), - wantToken: "GITHUB_ENTERPRISE_TOKEN", - wantSource: githubEnterpriseToken, - }, - { - name: "token for enterprise.com with config token", - host: "enterprise.com", - config: testHostsConfig(), - wantToken: "yyyyyyyyyyyyyyyyyyyy", - wantSource: oauthToken, - }, - { - name: "token for tenant with GH_TOKEN, GITHUB_TOKEN, and config token", + name: "given GH_TOKEN and GITHUB_TOKEN and a config token are set, when we get the token for any subdomain of ghe.com, then it returns GH_TOKEN as the priority", host: "tenant.ghe.com", ghToken: "GH_TOKEN", githubToken: "GITHUB_TOKEN", @@ -94,7 +68,7 @@ func TestTokenForHost(t *testing.T) { wantSource: ghToken, }, { - name: "token for tenant with GITHUB_TOKEN, and config token", + name: "given GITHUB_TOKEN and a config token are set, when we get the token for any subdomain of ghe.com, then it returns GITHUB_TOKEN as the priority", host: "tenant.ghe.com", githubToken: "GITHUB_TOKEN", config: testHostsConfig(), @@ -102,60 +76,62 @@ func TestTokenForHost(t *testing.T) { wantSource: githubToken, }, { - name: "token for tenant with config token", + name: "given a config token is set for any subdomain of ghe.com, when we get the token, then it returns that token and oauth_token source", host: "tenant.ghe.com", config: testHostsConfig(), wantToken: "zzzzzzzzzzzzzzzzzzzz", wantSource: oauthToken, }, { - name: "Token for non-github host in a codespace", - host: "doesnotmatter.com", - config: testNoHostsConfig(), - githubToken: "GITHUB_TOKEN", - codespaces: true, - wantToken: "", - wantSource: defaultSource, - }, - { - name: "Token for github.com in a codespace", - host: "github.com", - config: testNoHostsConfig(), + name: "given GH_TOKEN and GITHUB_TOKEN and a config token are set, when we get the token for github.localhost, then it returns GH_TOKEN as the priority", + host: "github.localhost", + ghToken: "GH_TOKEN", githubToken: "GITHUB_TOKEN", - codespaces: true, - wantToken: "GITHUB_TOKEN", - wantSource: githubToken, + config: testHostsConfig(), + wantToken: "GH_TOKEN", + wantSource: ghToken, }, { - // We are in a codespace (dotcom), and we have set our own GITHUB_TOKEN, not using the codespace one - // and we are targeting tenant.ghe.com - name: "Token for tenant.ghe.com in a codespace", - host: "tenant.ghe.com", - config: testNoHostsConfig(), + name: "given GITHUB_TOKEN and a config token are set, when we get the token for any subdomain of github.localhost, then it returns GITHUB_TOKEN as the priority", + host: "github.localhost", githubToken: "GITHUB_TOKEN", - codespaces: true, + config: testHostsConfig(), wantToken: "GITHUB_TOKEN", wantSource: githubToken, }, { - name: "Token for github.localhost in a codespace", - host: "github.localhost", - config: testNoHostsConfig(), - githubToken: "GITHUB_TOKEN", - codespaces: true, - wantToken: "GITHUB_TOKEN", - wantSource: githubToken, + name: "given GH_ENTERPRISE_TOKEN and GITHUB_ENTERPRISE_TOKEN and a config token are set, when we get the token for an enterprise server host, then it returns GH_ENTERPRISE_TOKEN as the priority", + host: "enterprise.com", + ghEnterpriseToken: "GH_ENTERPRISE_TOKEN", + githubEnterpriseToken: "GITHUB_ENTERPRISE_TOKEN", + config: testHostsConfig(), + wantToken: "GH_ENTERPRISE_TOKEN", + wantSource: ghEnterpriseToken, }, { - // We are in codespace (dotcom), and we have set a GITHUB_ENTERPRISE_TOKEN, and we are targeting GHES - name: "Enterprise Token for GHES in a codespace", + name: "given GITHUB_ENTERPRISE_TOKEN and a config token are set, when we get the token for an enterprise server host, then it returns GITHUB_ENTERPRISE_TOKEN as the priority", host: "enterprise.com", - config: testNoHostsConfig(), githubEnterpriseToken: "GITHUB_ENTERPRISE_TOKEN", - codespaces: true, + config: testHostsConfig(), wantToken: "GITHUB_ENTERPRISE_TOKEN", wantSource: githubEnterpriseToken, }, + { + name: "given a config token is set for an enterprise server host, when we get the token for that host, then it returns that token and oauth_token source", + host: "enterprise.com", + config: testHostsConfig(), + wantToken: "yyyyyyyyyyyyyyyyyyyy", + wantSource: oauthToken, + }, + { + name: "given GH_TOKEN or GITHUB_TOKEN are set, when I get the token for any host not owned by GitHub, we do not get those tokens", + host: "unknown.com", + config: testNoHostsConfig(), + ghToken: "GH_TOKEN", + githubToken: "GITHUB_TOKEN", + wantToken: "", + wantSource: defaultSource, + }, } for _, tt := range tests { @@ -164,7 +140,6 @@ func TestTokenForHost(t *testing.T) { t.Setenv("GITHUB_ENTERPRISE_TOKEN", tt.githubEnterpriseToken) t.Setenv("GH_TOKEN", tt.ghToken) t.Setenv("GH_ENTERPRISE_TOKEN", tt.ghEnterpriseToken) - t.Setenv("CODESPACES", strconv.FormatBool(tt.codespaces)) token, source := tokenForHost(tt.config, tt.host) require.Equal(t, tt.wantToken, token, "Expected token for \"%s\" to be \"%s\", got \"%s\"", tt.host, tt.wantToken, token) require.Equal(t, tt.wantSource, source, "Expected source for \"%s\" to be \"%s\", got \"%s\"", tt.host, tt.wantSource, source) From 6240e99f60bed3b9895b1f004aa3114fdd9f604b Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 31 Oct 2024 18:35:21 +0100 Subject: [PATCH 3/3] Adjust ghe subdomain test behavioural description --- pkg/auth/auth_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/auth/auth_test.go b/pkg/auth/auth_test.go index f4379a4..f149c36 100644 --- a/pkg/auth/auth_test.go +++ b/pkg/auth/auth_test.go @@ -76,7 +76,7 @@ func TestTokenForHost(t *testing.T) { wantSource: githubToken, }, { - name: "given a config token is set for any subdomain of ghe.com, when we get the token, then it returns that token and oauth_token source", + name: "given a config token is set for a subdomain of ghe.com, when we get the token for that subdomain, then it returns that token and oauth_token source", host: "tenant.ghe.com", config: testHostsConfig(), wantToken: "zzzzzzzzzzzzzzzzzzzz",