Skip to content

Commit 65bd8d7

Browse files
andyfellerandyfeller
authored
Merge commit from fork
Prevent `tokenForHost` from leaking `GITHUB_TOKEN` to non-GitHub host in Codespaces
2 parents 7177035 + 6240e99 commit 65bd8d7

File tree

2 files changed

+101
-69
lines changed

2 files changed

+101
-69
lines changed

‎pkg/auth/auth.go

Lines changed: 30 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"fmt"
77
"os"
88
"os/exec"
9-
"strconv"
109
"strings"
1110

1211
"github.com/cli/go-gh/v2/internal/set"
@@ -62,35 +61,42 @@ func TokenFromEnvOrConfig(host string) (string, string) {
6261
}
6362

6463
func tokenForHost(cfg *config.Config, host string) (string, string) {
65-
host = NormalizeHostname(host)
66-
if IsEnterprise(host) {
64+
normalizedHost := NormalizeHostname(host)
65+
// This code is currently the exact opposite of IsEnterprise. However, we have chosen
66+
// to write it separately, directly in line, because it is much clearer in the exact
67+
// scenarios that we expect to use GH_TOKEN and GITHUB_TOKEN.
68+
if normalizedHost == github || IsTenancy(normalizedHost) || normalizedHost == localhost {
69+
if token := os.Getenv(ghToken); token != "" {
70+
return token, ghToken
71+
}
72+
73+
if token := os.Getenv(githubToken); token != "" {
74+
return token, githubToken
75+
}
76+
} else {
6777
if token := os.Getenv(ghEnterpriseToken); token != "" {
6878
return token, ghEnterpriseToken
6979
}
80+
7081
if token := os.Getenv(githubEnterpriseToken); token != "" {
7182
return token, githubEnterpriseToken
7283
}
73-
if isCodespaces, _ := strconv.ParseBool(os.Getenv(codespaces)); isCodespaces {
74-
if token := os.Getenv(githubToken); token != "" {
75-
return token, githubToken
76-
}
77-
}
78-
if cfg != nil {
79-
token, _ := cfg.Get([]string{hostsKey, host, oauthToken})
80-
return token, oauthToken
81-
}
8284
}
83-
if token := os.Getenv(ghToken); token != "" {
84-
return token, ghToken
85-
}
86-
if token := os.Getenv(githubToken); token != "" {
87-
return token, githubToken
85+
86+
// If config is nil, something has failed much earlier and it's probably
87+
// more correct to panic because we don't expect to support anything
88+
// where the config isn't available, but that would be a breaking change,
89+
// so it's worth thinking about carefully, if we wanted to rework this.
90+
if cfg == nil {
91+
return "", defaultSource
8892
}
89-
if cfg != nil {
90-
token, _ := cfg.Get([]string{hostsKey, host, oauthToken})
91-
return token, oauthToken
93+
94+
token, err := cfg.Get([]string{hostsKey, normalizedHost, oauthToken})
95+
if err != nil {
96+
return "", defaultSource
9297
}
93-
return "", defaultSource
98+
99+
return token, oauthToken
94100
}
95101

96102
func tokenFromGh(path string, host string) (string, string) {
@@ -151,8 +157,10 @@ func defaultHost(cfg *config.Config) (string, string) {
151157
}
152158

153159
// IsEnterprise determines if a provided host is a GitHub Enterprise Server instance,
154-
// rather than GitHub.com or a tenancy GitHub instance.
160+
// rather than GitHub.com, a tenancy GitHub instance, or github.localhost.
155161
func IsEnterprise(host string) bool {
162+
// Note that if you are making changes here, you should also consider making the equivalent
163+
// in tokenForHost, which is the exact opposite of this function.
156164
normalizedHost := NormalizeHostname(host)
157165
return normalizedHost != github && normalizedHost != localhost && !IsTenancy(normalizedHost)
158166
}

‎pkg/auth/auth_test.go

Lines changed: 71 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import (
55

66
"github.com/cli/go-gh/v2/pkg/config"
77
"github.com/stretchr/testify/assert"
8+
"github.com/stretchr/testify/require"
89
)
910

1011
func TestTokenForHost(t *testing.T) {
@@ -18,95 +19,118 @@ func TestTokenForHost(t *testing.T) {
1819
config *config.Config
1920
wantToken string
2021
wantSource string
21-
wantNotFound bool
2222
}{
2323
{
24-
name: "token for github.com with no env tokens and no config token",
25-
host: "github.com",
26-
config: testNoHostsConfig(),
27-
wantToken: "",
28-
wantSource: "oauth_token",
29-
wantNotFound: true,
24+
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",
25+
host: "github.com",
26+
config: testNoHostsConfig(),
27+
wantToken: "",
28+
wantSource: defaultSource,
3029
},
3130
{
32-
name: "token for enterprise.com with no env tokens and no config token",
33-
host: "enterprise.com",
34-
config: testNoHostsConfig(),
35-
wantToken: "",
36-
wantSource: "oauth_token",
37-
wantNotFound: true,
31+
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",
32+
host: "enterprise.com",
33+
config: testNoHostsConfig(),
34+
wantToken: "",
35+
wantSource: defaultSource,
3836
},
3937
{
40-
name: "token for github.com with GH_TOKEN, GITHUB_TOKEN, and config token",
38+
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",
4139
host: "github.com",
4240
ghToken: "GH_TOKEN",
4341
githubToken: "GITHUB_TOKEN",
4442
config: testHostsConfig(),
4543
wantToken: "GH_TOKEN",
46-
wantSource: "GH_TOKEN",
44+
wantSource: ghToken,
4745
},
4846
{
49-
name: "token for github.com with GITHUB_TOKEN, and config token",
47+
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",
5048
host: "github.com",
5149
githubToken: "GITHUB_TOKEN",
5250
config: testHostsConfig(),
5351
wantToken: "GITHUB_TOKEN",
54-
wantSource: "GITHUB_TOKEN",
52+
wantSource: githubToken,
5553
},
5654
{
57-
name: "token for github.com with config token",
55+
name: "given a config token is set for github.com, when we get the token, then it returns that token and oauth_token source",
5856
host: "github.com",
5957
config: testHostsConfig(),
6058
wantToken: "xxxxxxxxxxxxxxxxxxxx",
61-
wantSource: "oauth_token",
59+
wantSource: oauthToken,
60+
},
61+
{
62+
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",
63+
host: "tenant.ghe.com",
64+
ghToken: "GH_TOKEN",
65+
githubToken: "GITHUB_TOKEN",
66+
config: testHostsConfig(),
67+
wantToken: "GH_TOKEN",
68+
wantSource: ghToken,
69+
},
70+
{
71+
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",
72+
host: "tenant.ghe.com",
73+
githubToken: "GITHUB_TOKEN",
74+
config: testHostsConfig(),
75+
wantToken: "GITHUB_TOKEN",
76+
wantSource: githubToken,
77+
},
78+
{
79+
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",
80+
host: "tenant.ghe.com",
81+
config: testHostsConfig(),
82+
wantToken: "zzzzzzzzzzzzzzzzzzzz",
83+
wantSource: oauthToken,
84+
},
85+
{
86+
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",
87+
host: "github.localhost",
88+
ghToken: "GH_TOKEN",
89+
githubToken: "GITHUB_TOKEN",
90+
config: testHostsConfig(),
91+
wantToken: "GH_TOKEN",
92+
wantSource: ghToken,
6293
},
6394
{
64-
name: "token for enterprise.com with GH_ENTERPRISE_TOKEN, GITHUB_ENTERPRISE_TOKEN, and config token",
95+
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",
96+
host: "github.localhost",
97+
githubToken: "GITHUB_TOKEN",
98+
config: testHostsConfig(),
99+
wantToken: "GITHUB_TOKEN",
100+
wantSource: githubToken,
101+
},
102+
{
103+
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",
65104
host: "enterprise.com",
66105
ghEnterpriseToken: "GH_ENTERPRISE_TOKEN",
67106
githubEnterpriseToken: "GITHUB_ENTERPRISE_TOKEN",
68107
config: testHostsConfig(),
69108
wantToken: "GH_ENTERPRISE_TOKEN",
70-
wantSource: "GH_ENTERPRISE_TOKEN",
109+
wantSource: ghEnterpriseToken,
71110
},
72111
{
73-
name: "token for enterprise.com with GITHUB_ENTERPRISE_TOKEN, and config token",
112+
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",
74113
host: "enterprise.com",
75114
githubEnterpriseToken: "GITHUB_ENTERPRISE_TOKEN",
76115
config: testHostsConfig(),
77116
wantToken: "GITHUB_ENTERPRISE_TOKEN",
78-
wantSource: "GITHUB_ENTERPRISE_TOKEN",
117+
wantSource: githubEnterpriseToken,
79118
},
80119
{
81-
name: "token for enterprise.com with config token",
120+
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",
82121
host: "enterprise.com",
83122
config: testHostsConfig(),
84123
wantToken: "yyyyyyyyyyyyyyyyyyyy",
85-
wantSource: "oauth_token",
124+
wantSource: oauthToken,
86125
},
87126
{
88-
name: "token for tenant with GH_TOKEN, GITHUB_TOKEN, and config token",
89-
host: "tenant.ghe.com",
127+
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",
128+
host: "unknown.com",
129+
config: testNoHostsConfig(),
90130
ghToken: "GH_TOKEN",
91131
githubToken: "GITHUB_TOKEN",
92-
config: testHostsConfig(),
93-
wantToken: "GH_TOKEN",
94-
wantSource: "GH_TOKEN",
95-
},
96-
{
97-
name: "token for tenant with GITHUB_TOKEN, and config token",
98-
host: "tenant.ghe.com",
99-
githubToken: "GITHUB_TOKEN",
100-
config: testHostsConfig(),
101-
wantToken: "GITHUB_TOKEN",
102-
wantSource: "GITHUB_TOKEN",
103-
},
104-
{
105-
name: "token for tenant with config token",
106-
host: "tenant.ghe.com",
107-
config: testHostsConfig(),
108-
wantToken: "zzzzzzzzzzzzzzzzzzzz",
109-
wantSource: "oauth_token",
132+
wantToken: "",
133+
wantSource: defaultSource,
110134
},
111135
}
112136

@@ -117,8 +141,8 @@ func TestTokenForHost(t *testing.T) {
117141
t.Setenv("GH_TOKEN", tt.ghToken)
118142
t.Setenv("GH_ENTERPRISE_TOKEN", tt.ghEnterpriseToken)
119143
token, source := tokenForHost(tt.config, tt.host)
120-
assert.Equal(t, tt.wantToken, token)
121-
assert.Equal(t, tt.wantSource, source)
144+
require.Equal(t, tt.wantToken, token, "Expected token for \"%s\" to be \"%s\", got \"%s\"", tt.host, tt.wantToken, token)
145+
require.Equal(t, tt.wantSource, source, "Expected source for \"%s\" to be \"%s\", got \"%s\"", tt.host, tt.wantSource, source)
122146
})
123147
}
124148
}

0 commit comments

Comments
 (0)