From aeee301413f5f7a5688ee6cf185c584e8c4273d6 Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Thu, 2 Apr 2026 18:03:31 +0800 Subject: [PATCH 1/4] fix(security): prevent Windows command injection and improve RFC 7009 compliance - Use rundll32 instead of cmd /c start to prevent shell metacharacter injection in URLs on Windows - Replace panic with fmt.Fprintf + os.Exit(1) for consistent error handling in loadConfig - Write token save warning to stderr instead of stdout in refreshAccessToken - Add token_type_hint parameter to revocation requests per RFC 7009 --- auth.go | 3 ++- browser.go | 2 +- config.go | 3 ++- token_cmd.go | 24 ++++++++++++++++++++---- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/auth.go b/auth.go index 646f998..15aa829 100644 --- a/auth.go +++ b/auth.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/url" + "os" retry "github.com/appleboy/go-httpretry" "github.com/go-authgate/cli/tui" @@ -94,7 +95,7 @@ func refreshAccessToken( } if err := cfg.Store.Save(cfg.ClientID, *storage); err != nil { - fmt.Printf("Warning: Failed to save refreshed tokens: %v\n", err) + fmt.Fprintf(os.Stderr, "Warning: Failed to save refreshed tokens: %v\n", err) } return storage, nil } diff --git a/browser.go b/browser.go index fe72b4d..b8632c4 100644 --- a/browser.go +++ b/browser.go @@ -17,7 +17,7 @@ func openBrowser(ctx context.Context, url string) error { case "darwin": cmd = exec.CommandContext(ctx, "open", url) case "windows": - cmd = exec.CommandContext(ctx, "cmd", "/c", "start", url) + cmd = exec.CommandContext(ctx, "rundll32", "url.dll,FileProtocolHandler", url) default: cmd = exec.CommandContext(ctx, "xdg-open", url) } diff --git a/config.go b/config.go index 93d79c7..f37cfd1 100644 --- a/config.go +++ b/config.go @@ -236,7 +236,8 @@ func loadConfig() *AppConfig { var err error cfg.RetryClient, err = retry.NewBackgroundClient(retry.WithHTTPClient(baseHTTPClient)) if err != nil { - panic(fmt.Sprintf("failed to create retry client: %v", err)) + fmt.Fprintf(os.Stderr, "Error: failed to create HTTP client: %v\n", err) + os.Exit(1) } // Resolve timeout configuration. diff --git a/token_cmd.go b/token_cmd.go index 2d1af12..e84b8d5 100644 --- a/token_cmd.go +++ b/token_cmd.go @@ -172,7 +172,14 @@ func revokeTokenOnServer( if tok.RefreshToken != "" { wg.Go(func() { - if err := doRevoke(ctx, cfg, revokeURL, tok.RefreshToken, timeout); err != nil { + if err := doRevoke( + ctx, + cfg, + revokeURL, + tok.RefreshToken, + "refresh_token", + timeout, + ); err != nil { mu.Lock() refreshErr = err mu.Unlock() @@ -182,7 +189,14 @@ func revokeTokenOnServer( if tok.AccessToken != "" { wg.Go(func() { - if err := doRevoke(ctx, cfg, revokeURL, tok.AccessToken, timeout); err != nil { + if err := doRevoke( + ctx, + cfg, + revokeURL, + tok.AccessToken, + "access_token", + timeout, + ); err != nil { mu.Lock() accessErr = err mu.Unlock() @@ -213,14 +227,16 @@ func doRevoke( cfg *AppConfig, revokeURL string, token string, + tokenTypeHint string, timeout time.Duration, ) error { ctx, cancel := context.WithTimeout(ctx, timeout) defer cancel() data := url.Values{ - "token": {token}, - "client_id": {cfg.ClientID}, + "token": {token}, + "token_type_hint": {tokenTypeHint}, + "client_id": {cfg.ClientID}, } if !cfg.IsPublicClient() { data.Set("client_secret", cfg.ClientSecret) From 80416a30532cbf68aee1c67d6f8b437c015d03f1 Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Thu, 2 Apr 2026 18:42:14 +0800 Subject: [PATCH 2/4] fix(cli): address Copilot review feedback - Align error message wording to say "retry HTTP client" in loadConfig - Add token_type_hint assertions to revocation tests for RFC 7009 compliance --- config.go | 2 +- token_cmd_test.go | 51 ++++++++++++++++++++++++++++++++--------------- 2 files changed, 36 insertions(+), 17 deletions(-) diff --git a/config.go b/config.go index f37cfd1..7d5a7b6 100644 --- a/config.go +++ b/config.go @@ -236,7 +236,7 @@ func loadConfig() *AppConfig { var err error cfg.RetryClient, err = retry.NewBackgroundClient(retry.WithHTTPClient(baseHTTPClient)) if err != nil { - fmt.Fprintf(os.Stderr, "Error: failed to create HTTP client: %v\n", err) + fmt.Fprintf(os.Stderr, "Error: failed to create retry HTTP client: %v\n", err) os.Exit(1) } diff --git a/token_cmd_test.go b/token_cmd_test.go index 57fced5..09ef410 100644 --- a/token_cmd_test.go +++ b/token_cmd_test.go @@ -106,7 +106,11 @@ func TestRunTokenDelete(t *testing.T) { func TestRunTokenDelete_ServerRevocation(t *testing.T) { t.Run("successful revocation and local delete", func(t *testing.T) { - var revokedTokens []string + type revokeCall struct { + token string + tokenTypeHint string + } + var revokeCalls []revokeCall var mu sync.Mutex srv := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -115,7 +119,10 @@ func TestRunTokenDelete_ServerRevocation(t *testing.T) { return } mu.Lock() - revokedTokens = append(revokedTokens, r.FormValue("token")) + revokeCalls = append(revokeCalls, revokeCall{ + token: r.FormValue("token"), + tokenTypeHint: r.FormValue("token_type_hint"), + }) mu.Unlock() w.WriteHeader(http.StatusOK) }), @@ -163,16 +170,24 @@ func TestRunTokenDelete_ServerRevocation(t *testing.T) { mu.Lock() defer mu.Unlock() - if len(revokedTokens) != 2 { - t.Fatalf("expected 2 revoke calls, got %d", len(revokedTokens)) + if len(revokeCalls) != 2 { + t.Fatalf("expected 2 revoke calls, got %d", len(revokeCalls)) } // Revocations run concurrently, so order is non-deterministic. - got := map[string]bool{revokedTokens[0]: true, revokedTokens[1]: true} - if !got["refresh-456"] { - t.Errorf("expected refresh token to be revoked, got %v", revokedTokens) + // Build a map from token to its type hint for assertion. + hintByToken := make(map[string]string, len(revokeCalls)) + for _, c := range revokeCalls { + hintByToken[c.token] = c.tokenTypeHint + } + if hint, ok := hintByToken["refresh-456"]; !ok { + t.Errorf("expected refresh token to be revoked, got %v", revokeCalls) + } else if hint != "refresh_token" { + t.Errorf("refresh token_type_hint: got %q, want %q", hint, "refresh_token") } - if !got["access-123"] { - t.Errorf("expected access token to be revoked, got %v", revokedTokens) + if hint, ok := hintByToken["access-123"]; !ok { + t.Errorf("expected access token to be revoked, got %v", revokeCalls) + } else if hint != "access_token" { + t.Errorf("access token_type_hint: got %q, want %q", hint, "access_token") } }) @@ -269,8 +284,11 @@ func TestRunTokenDelete_ServerRevocation(t *testing.T) { }) t.Run("only access token no refresh token", func(t *testing.T) { - var revokedTokens []string - var mu sync.Mutex + var ( + gotToken string + gotTokenTypeHint string + mu sync.Mutex + ) srv := httptest.NewServer( http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if err := r.ParseForm(); err != nil { @@ -278,7 +296,8 @@ func TestRunTokenDelete_ServerRevocation(t *testing.T) { return } mu.Lock() - revokedTokens = append(revokedTokens, r.FormValue("token")) + gotToken = r.FormValue("token") + gotTokenTypeHint = r.FormValue("token_type_hint") mu.Unlock() w.WriteHeader(http.StatusOK) }), @@ -319,11 +338,11 @@ func TestRunTokenDelete_ServerRevocation(t *testing.T) { mu.Lock() defer mu.Unlock() - if len(revokedTokens) != 1 { - t.Fatalf("expected 1 revoke call (access only), got %d", len(revokedTokens)) + if gotToken != "access-only" { + t.Errorf("token: got %q, want %q", gotToken, "access-only") } - if revokedTokens[0] != "access-only" { - t.Errorf("expected access token, got %q", revokedTokens[0]) + if gotTokenTypeHint != "access_token" { + t.Errorf("token_type_hint: got %q, want %q", gotTokenTypeHint, "access_token") } }) } From 3db0245ff70baf0c0325335072d9a00ebf515b46 Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Thu, 2 Apr 2026 18:49:40 +0800 Subject: [PATCH 3/4] test(revocation): assert call count in access-only revocation test - Track revocation call count to ensure exactly one request is made when only an access token exists (no refresh token) --- token_cmd_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/token_cmd_test.go b/token_cmd_test.go index 09ef410..38501cc 100644 --- a/token_cmd_test.go +++ b/token_cmd_test.go @@ -285,6 +285,7 @@ func TestRunTokenDelete_ServerRevocation(t *testing.T) { t.Run("only access token no refresh token", func(t *testing.T) { var ( + callCount int gotToken string gotTokenTypeHint string mu sync.Mutex @@ -296,6 +297,7 @@ func TestRunTokenDelete_ServerRevocation(t *testing.T) { return } mu.Lock() + callCount++ gotToken = r.FormValue("token") gotTokenTypeHint = r.FormValue("token_type_hint") mu.Unlock() @@ -338,6 +340,9 @@ func TestRunTokenDelete_ServerRevocation(t *testing.T) { mu.Lock() defer mu.Unlock() + if callCount != 1 { + t.Fatalf("expected 1 revoke call (access only), got %d", callCount) + } if gotToken != "access-only" { t.Errorf("token: got %q, want %q", gotToken, "access-only") } From ade91f8f05a1378acce3d92676ce186c444aa93d Mon Sep 17 00:00:00 2001 From: Bo-Yi Wu Date: Thu, 2 Apr 2026 18:56:04 +0800 Subject: [PATCH 4/4] fix(cli): address Copilot review round 3 - Conditionally include token_type_hint only when non-empty for server compatibility - Extract browserCommand helper for testability and add unit tests covering darwin, windows, and linux command construction --- browser.go | 25 +++++++++++++--------- browser_test.go | 57 +++++++++++++++++++++++++++++++++++++++++++++++++ token_cmd.go | 8 ++++--- 3 files changed, 77 insertions(+), 13 deletions(-) create mode 100644 browser_test.go diff --git a/browser.go b/browser.go index b8632c4..509871e 100644 --- a/browser.go +++ b/browser.go @@ -7,20 +7,25 @@ import ( "runtime" ) -// openBrowser attempts to open url in the user's default browser. -// Returns an error if launching the browser fails, but callers should -// always print the URL as a fallback regardless of the error. -func openBrowser(ctx context.Context, url string) error { - var cmd *exec.Cmd - - switch runtime.GOOS { +// browserCommand returns the executable name and arguments for opening a URL +// on the given OS. This is extracted for testability. +func browserCommand(goos, url string) (name string, args []string) { + switch goos { case "darwin": - cmd = exec.CommandContext(ctx, "open", url) + return "open", []string{url} case "windows": - cmd = exec.CommandContext(ctx, "rundll32", "url.dll,FileProtocolHandler", url) + return "rundll32", []string{"url.dll,FileProtocolHandler", url} default: - cmd = exec.CommandContext(ctx, "xdg-open", url) + return "xdg-open", []string{url} } +} + +// openBrowser attempts to open url in the user's default browser. +// Returns an error if launching the browser fails, but callers should +// always print the URL as a fallback regardless of the error. +func openBrowser(ctx context.Context, url string) error { + name, args := browserCommand(runtime.GOOS, url) + cmd := exec.CommandContext(ctx, name, args...) if err := cmd.Start(); err != nil { return fmt.Errorf("failed to open browser: %w", err) diff --git a/browser_test.go b/browser_test.go new file mode 100644 index 0000000..00b9b6a --- /dev/null +++ b/browser_test.go @@ -0,0 +1,57 @@ +package main + +import "testing" + +func TestBrowserCommand(t *testing.T) { + tests := []struct { + goos string + url string + wantName string + wantArgs []string + }{ + { + goos: "darwin", + url: "https://example.com/auth", + wantName: "open", + wantArgs: []string{"https://example.com/auth"}, + }, + { + goos: "windows", + url: "https://example.com/auth", + wantName: "rundll32", + wantArgs: []string{"url.dll,FileProtocolHandler", "https://example.com/auth"}, + }, + { + goos: "linux", + url: "https://example.com/auth", + wantName: "xdg-open", + wantArgs: []string{"https://example.com/auth"}, + }, + { + goos: "windows", + url: "https://example.com/auth?foo=bar&baz=qux", + wantName: "rundll32", + wantArgs: []string{ + "url.dll,FileProtocolHandler", + "https://example.com/auth?foo=bar&baz=qux", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.goos, func(t *testing.T) { + name, args := browserCommand(tt.goos, tt.url) + if name != tt.wantName { + t.Errorf("name: got %q, want %q", name, tt.wantName) + } + if len(args) != len(tt.wantArgs) { + t.Fatalf("args length: got %d, want %d", len(args), len(tt.wantArgs)) + } + for i, arg := range args { + if arg != tt.wantArgs[i] { + t.Errorf("args[%d]: got %q, want %q", i, arg, tt.wantArgs[i]) + } + } + }) + } +} diff --git a/token_cmd.go b/token_cmd.go index e84b8d5..856a651 100644 --- a/token_cmd.go +++ b/token_cmd.go @@ -234,9 +234,11 @@ func doRevoke( defer cancel() data := url.Values{ - "token": {token}, - "token_type_hint": {tokenTypeHint}, - "client_id": {cfg.ClientID}, + "token": {token}, + "client_id": {cfg.ClientID}, + } + if tokenTypeHint != "" { + data.Set("token_type_hint", tokenTypeHint) } if !cfg.IsPublicClient() { data.Set("client_secret", cfg.ClientSecret)