Skip to content

Commit 72e9747

Browse files
mislavmislav
authored
Merge pull request cli#1626 from cli/ghe-auth-tweaks
Make GitHub remote parsing and authentication stricter
2 parents d13e6b3 + ece17c4 commit 72e9747

File tree

19 files changed

+555
-278
lines changed

19 files changed

+555
-278
lines changed

‎cmd/gh/main.go‎

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -92,23 +92,17 @@ func main() {
9292
}
9393
}
9494

95-
authCheckEnabled := cmdutil.IsAuthCheckEnabled(cmd)
96-
97-
// TODO support other names
98-
ghtoken := os.Getenv("GITHUB_TOKEN")
99-
if ghtoken != "" {
100-
authCheckEnabled = false
101-
}
102-
95+
authCheckEnabled := os.Getenv("GITHUB_TOKEN") == "" &&
96+
os.Getenv("GITHUB_ENTERPRISE_TOKEN") == "" &&
97+
cmdutil.IsAuthCheckEnabled(cmd)
10398
if authCheckEnabled {
104-
hasAuth := false
105-
10699
cfg, err := cmdFactory.Config()
107-
if err == nil {
108-
hasAuth = cmdutil.CheckAuth(cfg)
100+
if err != nil {
101+
fmt.Fprintf(stderr, "failed to read configuration: %s\n", err)
102+
os.Exit(2)
109103
}
110104

111-
if !hasAuth {
105+
if !cmdutil.CheckAuth(cfg) {
112106
fmt.Fprintln(stderr, utils.Bold("Welcome to GitHub CLI!"))
113107
fmt.Fprintln(stderr)
114108
fmt.Fprintln(stderr, "To authenticate, please run `gh auth login`.")

‎git/url.go‎

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,24 +2,35 @@ package git
22

33
import (
44
"net/url"
5-
"regexp"
65
"strings"
76
)
87

9-
var (
10-
protocolRe = regexp.MustCompile("^[a-zA-Z_+-]+://")
11-
)
12-
138
func IsURL(u string) bool {
14-
return strings.HasPrefix(u, "git@") || protocolRe.MatchString(u)
9+
return strings.HasPrefix(u, "git@") || isSupportedProtocol(u)
10+
}
11+
12+
func isSupportedProtocol(u string) bool {
13+
return strings.HasPrefix(u, "ssh:") ||
14+
strings.HasPrefix(u, "git+ssh:") ||
15+
strings.HasPrefix(u, "git:") ||
16+
strings.HasPrefix(u, "http:") ||
17+
strings.HasPrefix(u, "https:")
18+
}
19+
20+
func isPossibleProtocol(u string) bool {
21+
return isSupportedProtocol(u) ||
22+
strings.HasPrefix(u, "ftp:") ||
23+
strings.HasPrefix(u, "ftps:") ||
24+
strings.HasPrefix(u, "file:")
1525
}
1626

1727
// ParseURL normalizes git remote urls
1828
func ParseURL(rawURL string) (u *url.URL, err error) {
19-
if !protocolRe.MatchString(rawURL) &&
20-
strings.Contains(rawURL, ":") &&
29+
if !isPossibleProtocol(rawURL) &&
30+
strings.ContainsRune(rawURL, ':') &&
2131
// not a Windows path
22-
!strings.Contains(rawURL, "\\") {
32+
!strings.ContainsRune(rawURL, '\\') {
33+
// support scp-like syntax for ssh protocol
2334
rawURL = "ssh://" + strings.Replace(rawURL, ":", "/", 1)
2435
}
2536

‎git/url_test.go‎

Lines changed: 195 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,195 @@
1+
package git
2+
3+
import "testing"
4+
5+
func TestIsURL(t *testing.T) {
6+
tests := []struct {
7+
name string
8+
url string
9+
want bool
10+
}{
11+
{
12+
name: "scp-like",
13+
url: "git@example.com:owner/repo",
14+
want: true,
15+
},
16+
{
17+
name: "scp-like with no user",
18+
url: "example.com:owner/repo",
19+
want: false,
20+
},
21+
{
22+
name: "ssh",
23+
url: "ssh://git@example.com/owner/repo",
24+
want: true,
25+
},
26+
{
27+
name: "git",
28+
url: "git://example.com/owner/repo",
29+
want: true,
30+
},
31+
{
32+
name: "https",
33+
url: "https://example.com/owner/repo.git",
34+
want: true,
35+
},
36+
{
37+
name: "no protocol",
38+
url: "example.com/owner/repo",
39+
want: false,
40+
},
41+
}
42+
for _, tt := range tests {
43+
t.Run(tt.name, func(t *testing.T) {
44+
if got := IsURL(tt.url); got != tt.want {
45+
t.Errorf("IsURL() = %v, want %v", got, tt.want)
46+
}
47+
})
48+
}
49+
}
50+
51+
func TestParseURL(t *testing.T) {
52+
type url struct {
53+
Scheme string
54+
User string
55+
Host string
56+
Path string
57+
}
58+
tests := []struct {
59+
name string
60+
url string
61+
want url
62+
wantErr bool
63+
}{
64+
{
65+
name: "HTTPS",
66+
url: "https://example.com/owner/repo.git",
67+
want: url{
68+
Scheme: "https",
69+
User: "",
70+
Host: "example.com",
71+
Path: "/owner/repo.git",
72+
},
73+
},
74+
{
75+
name: "HTTP",
76+
url: "http://example.com/owner/repo.git",
77+
want: url{
78+
Scheme: "http",
79+
User: "",
80+
Host: "example.com",
81+
Path: "/owner/repo.git",
82+
},
83+
},
84+
{
85+
name: "git",
86+
url: "git://example.com/owner/repo.git",
87+
want: url{
88+
Scheme: "git",
89+
User: "",
90+
Host: "example.com",
91+
Path: "/owner/repo.git",
92+
},
93+
},
94+
{
95+
name: "ssh",
96+
url: "ssh://git@example.com/owner/repo.git",
97+
want: url{
98+
Scheme: "ssh",
99+
User: "git",
100+
Host: "example.com",
101+
Path: "/owner/repo.git",
102+
},
103+
},
104+
{
105+
name: "ssh with port",
106+
url: "ssh://git@example.com:443/owner/repo.git",
107+
want: url{
108+
Scheme: "ssh",
109+
User: "git",
110+
Host: "example.com",
111+
Path: "/owner/repo.git",
112+
},
113+
},
114+
{
115+
name: "git+ssh",
116+
url: "git+ssh://example.com/owner/repo.git",
117+
want: url{
118+
Scheme: "ssh",
119+
User: "",
120+
Host: "example.com",
121+
Path: "/owner/repo.git",
122+
},
123+
},
124+
{
125+
name: "scp-like",
126+
url: "git@example.com:owner/repo.git",
127+
want: url{
128+
Scheme: "ssh",
129+
User: "git",
130+
Host: "example.com",
131+
Path: "/owner/repo.git",
132+
},
133+
},
134+
{
135+
name: "scp-like, leading slash",
136+
url: "git@example.com:/owner/repo.git",
137+
want: url{
138+
Scheme: "ssh",
139+
User: "git",
140+
Host: "example.com",
141+
Path: "/owner/repo.git",
142+
},
143+
},
144+
{
145+
name: "file protocol",
146+
url: "file:///example.com/owner/repo.git",
147+
want: url{
148+
Scheme: "file",
149+
User: "",
150+
Host: "",
151+
Path: "/example.com/owner/repo.git",
152+
},
153+
},
154+
{
155+
name: "file path",
156+
url: "/example.com/owner/repo.git",
157+
want: url{
158+
Scheme: "",
159+
User: "",
160+
Host: "",
161+
Path: "/example.com/owner/repo.git",
162+
},
163+
},
164+
{
165+
name: "Windows file path",
166+
url: "C:\\example.com\\owner\\repo.git",
167+
want: url{
168+
Scheme: "c",
169+
User: "",
170+
Host: "",
171+
Path: "",
172+
},
173+
},
174+
}
175+
for _, tt := range tests {
176+
t.Run(tt.name, func(t *testing.T) {
177+
u, err := ParseURL(tt.url)
178+
if (err != nil) != tt.wantErr {
179+
t.Fatalf("got error: %v", err)
180+
}
181+
if u.Scheme != tt.want.Scheme {
182+
t.Errorf("expected scheme %q, got %q", tt.want.Scheme, u.Scheme)
183+
}
184+
if u.User.Username() != tt.want.User {
185+
t.Errorf("expected user %q, got %q", tt.want.User, u.User.Username())
186+
}
187+
if u.Host != tt.want.Host {
188+
t.Errorf("expected host %q, got %q", tt.want.Host, u.Host)
189+
}
190+
if u.Path != tt.want.Path {
191+
t.Errorf("expected path %q, got %q", tt.want.Path, u.Path)
192+
}
193+
})
194+
}
195+
}

‎internal/config/config_type.go‎

Lines changed: 23 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,12 @@ const defaultGitProtocol = "https"
1515
// This interface describes interacting with some persistent configuration for gh.
1616
type Config interface {
1717
Get(string, string) (string, error)
18+
GetWithSource(string, string) (string, string, error)
1819
Set(string, string, string) error
1920
UnsetHost(string)
2021
Hosts() ([]string, error)
2122
Aliases() (*AliasConfig, error)
23+
CheckWriteable(string, string) error
2224
Write() error
2325
}
2426

@@ -200,42 +202,51 @@ func (c *fileConfig) Root() *yaml.Node {
200202
}
201203

202204
func (c *fileConfig) Get(hostname, key string) (string, error) {
205+
val, _, err := c.GetWithSource(hostname, key)
206+
return val, err
207+
}
208+
209+
func (c *fileConfig) GetWithSource(hostname, key string) (string, string, error) {
203210
if hostname != "" {
204211
var notFound *NotFoundError
205212

206213
hostCfg, err := c.configForHost(hostname)
207214
if err != nil && !errors.As(err, &notFound) {
208-
return "", err
215+
return "", "", err
209216
}
210217

211218
var hostValue string
212219
if hostCfg != nil {
213220
hostValue, err = hostCfg.GetStringValue(key)
214221
if err != nil && !errors.As(err, &notFound) {
215-
return "", err
222+
return "", "", err
216223
}
217224
}
218225

219226
if hostValue != "" {
220-
return hostValue, nil
227+
// TODO: avoid hardcoding this
228+
return hostValue, "~/.config/gh/hosts.yml", nil
221229
}
222230
}
223231

232+
// TODO: avoid hardcoding this
233+
defaultSource := "~/.config/gh/config.yml"
234+
224235
value, err := c.GetStringValue(key)
225236

226237
var notFound *NotFoundError
227238

228239
if err != nil && errors.As(err, &notFound) {
229-
return defaultFor(key), nil
240+
return defaultFor(key), defaultSource, nil
230241
} else if err != nil {
231-
return "", err
242+
return "", defaultSource, err
232243
}
233244

234245
if value == "" {
235-
return defaultFor(key), nil
246+
return defaultFor(key), defaultSource, nil
236247
}
237248

238-
return value, nil
249+
return value, defaultSource, nil
239250
}
240251

241252
func (c *fileConfig) Set(hostname, key, value string) error {
@@ -281,6 +292,11 @@ func (c *fileConfig) configForHost(hostname string) (*HostConfig, error) {
281292
return nil, &NotFoundError{fmt.Errorf("could not find config entry for %q", hostname)}
282293
}
283294

295+
func (c *fileConfig) CheckWriteable(hostname, key string) error {
296+
// TODO: check filesystem permissions
297+
return nil
298+
}
299+
284300
func (c *fileConfig) Write() error {
285301
mainData := yaml.Node{Kind: yaml.MappingNode}
286302
hostsData := yaml.Node{Kind: yaml.MappingNode}

0 commit comments

Comments
 (0)