fix(security): prevent Windows command injection and improve RFC 7009 compliance (#30)

* 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

* 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

* 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)

* 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

Author: appleboy

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
 }

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, "cmd", "/c", "start", 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)

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])
+				}
+			}
+		})
+	}
+}

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 retry HTTP client: %v\n", err)
+		os.Exit(1)
 	}
 
 	// Resolve timeout configuration.

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,6 +227,7 @@ func doRevoke(
 	cfg *AppConfig,
 	revokeURL string,
 	token string,
+	tokenTypeHint string,
 	timeout time.Duration,
 ) error {
 	ctx, cancel := context.WithTimeout(ctx, timeout)
@@ -222,6 +237,9 @@ func doRevoke(
 		"token":     {token},
 		"client_id": {cfg.ClientID},
 	}
+	if tokenTypeHint != "" {
+		data.Set("token_type_hint", tokenTypeHint)
+	}
 	if !cfg.IsPublicClient() {
 		data.Set("client_secret", cfg.ClientSecret)
 	}

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,16 +284,22 @@ 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 (
+			callCount        int
+			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 {
 					http.Error(w, "bad form", http.StatusBadRequest)
 					return
 				}
 				mu.Lock()
-				revokedTokens = append(revokedTokens, r.FormValue("token"))
+				callCount++
+				gotToken = r.FormValue("token")
+				gotTokenTypeHint = r.FormValue("token_type_hint")
 				mu.Unlock()
 				w.WriteHeader(http.StatusOK)
 			}),
@@ -319,11 +340,14 @@ 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 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")
 		}
-		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")
 		}
 	})
 }