Skip to content

Commit

Permalink
Fix token exposure for non-gh hosts in codespaces
Browse files Browse the repository at this point in the history 
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 <[email protected]>
  • Loading branch information
@williammartin @BagToad
williammartin and BagToad committed Oct 31, 2024
1 parent 7177035 commit 5d6079f
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 33 deletions.
27 changes: 18 additions & 9 deletions pkg/auth/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,32 +63,41 @@ 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
}
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
}
Expand Down
97 changes: 73 additions & 24 deletions pkg/auth/auth_test.go
View file
Original file line number Diff line number Diff line change
@@ -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) {
Expand All @@ -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",
Expand All @@ -43,22 +43,22 @@ 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",
host: "github.com",
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",
Expand All @@ -67,22 +67,22 @@ 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",
host: "enterprise.com",
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",
Expand All @@ -91,22 +91,70 @@ 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",
host: "tenant.ghe.com",
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,
},
}

Expand All @@ -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)
})
}
}
Expand Down

0 comments on commit 5d6079f

Please sign in to comment.