Skip to content

Commit aac3c5c

Browse files
tstrombergtstromberg
authored
Merge pull request #84 from codeGROOVE-dev/icons
Search for xdg-open in locations outside of PATH
2 parents 8496c12 + 6dd7fae commit aac3c5c

File tree

6 files changed

+1122
-128
lines changed

6 files changed

+1122
-128
lines changed

‎cmd/goose/cache.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"strings"
1313
"time"
1414

15+
"github.com/codeGROOVE-dev/goose/pkg/safebrowse"
1516
"github.com/codeGROOVE-dev/retry"
1617
"github.com/codeGROOVE-dev/turnclient/pkg/turn"
1718
)
@@ -97,7 +98,7 @@ func (app *App) checkCache(cacheFile, url string, updatedAt time.Time) (cachedDa
9798
func (app *App) turnData(ctx context.Context, url string, updatedAt time.Time) (*turn.CheckResponse, bool, error) {
9899
hasRunningTests := false
99100
// Validate URL before processing
100-
if err := validateURL(url); err != nil {
101+
if err := safebrowse.ValidateURL(url); err != nil {
101102
return nil, false, fmt.Errorf("invalid URL: %w", err)
102103
}
103104

‎cmd/goose/main.go‎

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -908,19 +908,16 @@ func (app *App) tryAutoOpenPR(ctx context.Context, pr *PR, autoBrowserEnabled bo
908908
"is_draft", pr.IsDraft,
909909
"age_since_creation", time.Since(pr.CreatedAt).Round(time.Second),
910910
"age_since_update", time.Since(pr.UpdatedAt).Round(time.Second))
911-
// Use strict validation for auto-opened URLs
912-
// Validate against strict GitHub PR URL pattern for auto-opening
913-
if err := validateGitHubPRURL(pr.URL); err != nil {
914-
slog.Warn("Auto-open strict validation failed", "url", sanitizeForLog(pr.URL), "error", err)
915-
return
916-
}
911+
// Use strict GitHub PR validation for auto-opening
917912
// Use ActionKind as the goose parameter value, or "next_action" if not set
918913
gooseParam := pr.ActionKind
919914
if gooseParam == "" {
920915
gooseParam = "next_action"
921916
}
917+
918+
// OpenWithParams will validate the URL and add the goose parameter
922919
if err := openURL(ctx, pr.URL, gooseParam); err != nil {
923-
slog.Error("[BROWSER] Failed to auto-open PR", "url", pr.URL, "error", err)
920+
slog.Error("[BROWSER] Failed to auto-open PR", "url", sanitizeForLog(pr.URL), "error", err)
924921
} else {
925922
app.browserRateLimiter.RecordOpen(pr.URL)
926923
slog.Info("[BROWSER] Successfully opened PR in browser",

‎cmd/goose/security_test.go‎

Lines changed: 196 additions & 57 deletions
Original file line numberDiff line numberDiff line change
@@ -1,132 +1,271 @@
11
package main
22

33
import (
4+
"strings"
45
"testing"
56
)
67

7-
func TestValidateGitHubPRURL(t *testing.T) {
8+
// URL validation tests are in pkg/safebrowse/safebrowse_test.go
9+
// This file only contains tests for goose-specific security functions
10+
11+
func TestValidateGitHubUsername(t *testing.T) {
812
tests := []struct {
9-
name string
10-
url string
11-
wantErr bool
13+
name string
14+
username string
15+
wantErr bool
1216
}{
13-
// Valid URLs
17+
// Valid usernames
1418
{
15-
name: "valid PR URL",
16-
url: "https://github.com/owner/repo/pull/123",
17-
wantErr: false,
19+
name: "simple username",
20+
username: "user",
21+
wantErr: false,
22+
},
23+
{
24+
name: "username with hyphen",
25+
username: "user-name",
26+
wantErr: false,
27+
},
28+
{
29+
name: "username with numbers",
30+
username: "user123",
31+
wantErr: false,
32+
},
33+
{
34+
name: "single character",
35+
username: "a",
36+
wantErr: false,
37+
},
38+
{
39+
name: "max length username",
40+
username: strings.Repeat("a", 39),
41+
wantErr: false,
42+
},
43+
44+
// Invalid usernames
45+
{
46+
name: "empty string",
47+
username: "",
48+
wantErr: true,
49+
},
50+
{
51+
name: "username starting with hyphen",
52+
username: "-user",
53+
wantErr: true,
54+
},
55+
{
56+
name: "username ending with hyphen",
57+
username: "user-",
58+
wantErr: true,
59+
},
60+
{
61+
name: "username with double hyphen",
62+
username: "user--name",
63+
wantErr: false, // GitHub allows this
64+
},
65+
{
66+
name: "username too long",
67+
username: strings.Repeat("a", 40),
68+
wantErr: true,
69+
},
70+
{
71+
name: "username with underscore",
72+
username: "user_name",
73+
wantErr: true,
74+
},
75+
{
76+
name: "username with dot",
77+
username: "user.name",
78+
wantErr: true,
1879
},
1980
{
20-
name: "valid PR URL with goose param",
21-
url: "https://github.com/owner/repo/pull/123?goose=1",
81+
name: "username with space",
82+
username: "user name",
83+
wantErr: true,
84+
},
85+
{
86+
name: "username with special chars",
87+
username: "user@name",
88+
wantErr: true,
89+
},
90+
}
91+
92+
for _, tt := range tests {
93+
t.Run(tt.name, func(t *testing.T) {
94+
err := validateGitHubUsername(tt.username)
95+
if (err != nil) != tt.wantErr {
96+
t.Errorf("validateGitHubUsername() error = %v, wantErr %v", err, tt.wantErr)
97+
}
98+
})
99+
}
100+
}
101+
102+
func TestValidateGitHubToken(t *testing.T) {
103+
tests := []struct {
104+
name string
105+
token string
106+
wantErr bool
107+
}{
108+
// Valid tokens
109+
{
110+
name: "classic token (40 hex chars)",
111+
token: "abcdef0123456789abcdef0123456789abcdef01",
22112
wantErr: false,
23113
},
24114
{
25-
name: "valid PR URL with goose=review param",
26-
url: "https://github.com/owner/repo/pull/123?goose=review",
115+
name: "personal access token (ghp_)",
116+
token: "ghp_" + strings.Repeat("a", 36),
27117
wantErr: false,
28118
},
29119
{
30-
name: "valid PR URL with goose=merge param",
31-
url: "https://github.com/owner/repo/pull/123?goose=merge",
120+
name: "server token (ghs_)",
121+
token: "ghs_" + strings.Repeat("A", 36),
32122
wantErr: false,
33123
},
34124
{
35-
name: "valid PR URL with goose=fix_tests param",
36-
url: "https://github.com/owner/repo/pull/123?goose=fix_tests",
125+
name: "refresh token (ghr_)",
126+
token: "ghr_" + strings.Repeat("1", 36),
37127
wantErr: false,
38128
},
39129
{
40-
name: "valid PR URL with goose=resolve_comments param",
41-
url: "https://github.com/owner/repo/pull/123?goose=resolve_comments",
130+
name: "OAuth token (gho_)",
131+
token: "gho_" + strings.Repeat("z", 36),
42132
wantErr: false,
43133
},
44134
{
45-
name: "valid PR URL with hyphen in owner",
46-
url: "https://github.com/owner-name/repo/pull/1",
135+
name: "user-to-server token (ghu_)",
136+
token: "ghu_" + strings.Repeat("Z", 36),
47137
wantErr: false,
48138
},
49139
{
50-
name: "valid PR URL with dots in repo",
51-
url: "https://github.com/owner/repo.name/pull/999",
140+
name: "fine-grained PAT",
141+
token: "github_pat_" + strings.Repeat("a", 82),
52142
wantErr: false,
53143
},
54144

55-
// Invalid URLs - security issues
145+
// Invalid tokens
56146
{
57-
name: "URL with credential injection",
58-
url: "https://evil@github.com/owner/repo/pull/123",
147+
name: "empty string",
148+
token: "",
59149
wantErr: true,
60150
},
61151
{
62-
name: "URL with encoded characters",
63-
url: "https://github.com/owner/repo/pull/123%2F../",
152+
name: "too short",
153+
token: "short",
64154
wantErr: true,
65155
},
66156
{
67-
name: "URL with double slashes",
68-
url: "https://github.com//owner/repo/pull/123",
157+
name: "too long",
158+
token: strings.Repeat("a", 300),
69159
wantErr: true,
70160
},
71161
{
72-
name: "URL with fragment",
73-
url: "https://github.com/owner/repo/pull/123#evil",
162+
name: "placeholder: your_token",
163+
token: "your_token_here_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
74164
wantErr: true,
75165
},
76166
{
77-
name: "URL with extra query params",
78-
url: "https://github.com/owner/repo/pull/123?foo=bar",
167+
name: "placeholder: xxx",
168+
token: "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx",
79169
wantErr: true,
80170
},
81171
{
82-
name: "URL with extra path segments",
83-
url: "https://github.com/owner/repo/pull/123/files",
172+
name: "placeholder with dots",
173+
token: "...aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa",
84174
wantErr: true,
85175
},
86-
87-
// Invalid URLs - wrong format
88176
{
89-
name: "URL with multiple query parameters",
90-
url: "https://github.com/owner/repo/pull/123?goose=review&other=param",
177+
name: "invalid format",
178+
token: "invalid_format_token_here_aaaaaaaaaaaaaaaa",
91179
wantErr: true,
92180
},
93181
{
94-
name: "not a PR URL",
95-
url: "https://github.com/owner/repo",
182+
name: "classic token too short",
183+
token: "abcdef0123456789abcdef0123456789abcdef0",
96184
wantErr: true,
97185
},
98186
{
99-
name: "issue URL instead of PR",
100-
url: "https://github.com/owner/repo/issues/123",
187+
name: "ghp_ token too short",
188+
token: "ghp_" + strings.Repeat("a", 35),
101189
wantErr: true,
102190
},
103191
{
104-
name: "HTTP instead of HTTPS",
105-
url: "http://github.com/owner/repo/pull/123",
192+
name: "fine-grained PAT too short",
193+
token: "github_pat_" + strings.Repeat("a", 81),
106194
wantErr: true,
107195
},
196+
}
197+
198+
for _, tt := range tests {
199+
t.Run(tt.name, func(t *testing.T) {
200+
err := validateGitHubToken(tt.token)
201+
if (err != nil) != tt.wantErr {
202+
t.Errorf("validateGitHubToken() error = %v, wantErr %v", err, tt.wantErr)
203+
}
204+
})
205+
}
206+
}
207+
208+
func TestSanitizeForLog(t *testing.T) {
209+
tests := []struct {
210+
name string
211+
input string
212+
wantHide bool // true if sensitive data should be redacted
213+
}{
108214
{
109-
name: "wrong domain",
110-
url: "https://gitlab.com/owner/repo/pull/123",
111-
wantErr: true,
215+
name: "classic token redacted",
216+
input: "token: abcdef0123456789abcdef0123456789abcdef01",
217+
wantHide: true,
112218
},
113219
{
114-
name: "PR number with leading zero",
115-
url: "https://github.com/owner/repo/pull/0123",
116-
wantErr: true,
220+
name: "ghp_ token redacted",
221+
input: "Authorization: ghp_" + strings.Repeat("a", 36),
222+
wantHide: true,
117223
},
118224
{
119-
name: "PR number zero",
120-
url: "https://github.com/owner/repo/pull/0",
121-
wantErr: true,
225+
name: "fine-grained PAT redacted",
226+
input: "token=github_pat_" + strings.Repeat("b", 82),
227+
wantHide: true,
228+
},
229+
{
230+
name: "bearer token redacted",
231+
input: "Bearer abc123xyz",
232+
wantHide: true,
233+
},
234+
{
235+
name: "authorization header redacted",
236+
input: "Authorization: Bearer token123",
237+
wantHide: true,
238+
},
239+
{
240+
name: "normal text not redacted",
241+
input: "This is just a normal log message",
242+
wantHide: false,
243+
},
244+
{
245+
name: "URL not redacted",
246+
input: "https://github.com/owner/repo/pull/123",
247+
wantHide: false,
122248
},
123249
}
124250

125251
for _, tt := range tests {
126252
t.Run(tt.name, func(t *testing.T) {
127-
err := validateGitHubPRURL(tt.url)
128-
if (err != nil) != tt.wantErr {
129-
t.Errorf("validateGitHubPRURL() error = %v, wantErr %v", err, tt.wantErr)
253+
result := sanitizeForLog(tt.input)
254+
255+
if tt.wantHide {
256+
// Should contain REDACTED marker
257+
if !strings.Contains(result, "[REDACTED") {
258+
t.Errorf("sanitizeForLog() = %v, should contain redaction marker", result)
259+
}
260+
// Should not contain original sensitive data patterns
261+
if strings.Contains(result, "ghp_") || strings.Contains(result, "github_pat_") {
262+
t.Errorf("sanitizeForLog() = %v, still contains sensitive pattern", result)
263+
}
264+
} else {
265+
// Should be unchanged
266+
if result != tt.input {
267+
t.Errorf("sanitizeForLog() = %v, want %v", result, tt.input)
268+
}
130269
}
131270
})
132271
}

0 commit comments

Comments
 (0)