fix(security): scope API redirects and sanitize diagnostics

CHANGELOG.md

@@ -2,6 +2,8 @@
 
 ## Unreleased
 
+- Security: reject cross-origin API redirects and redact API keys echoed in errors while preserving typed error causes.
+- CLI: sanitize parser diagnostics so invalid arguments cannot emit terminal control sequences.
 - Build: require Go 1.26.8 for upstream compiler and runtime fixes; update GoReleaser to 2.18.1, Node to 26.8.2, deadcode to 0.50.0, and govulncheck to 1.8.0 with matching release-verifier pins.
 - CI: pin Actions by commit, test Windows with Go 1.27.1 and macOS with the race detector, verify documentation metadata, reject formatting drift, and avoid duplicate branch-push audits.
 

docs/client-reference.md

@@ -13,3 +13,5 @@ These request details are easy to miss when moving between the CLI and library:
 - CLI directions route modifiers apply only to a driving primary or comparison route.
 
 Field masks are defined with each request implementation so calls ask Google only for the fields represented by that workflow.
+
+Redirects may stay within the original origin (scheme, hostname, and port); redirects to another origin are rejected to keep the API key scoped to the configured endpoint. Configure an endpoint override directly when using a different host. A custom `HTTPClient.CheckRedirect` can still stop redirects. API keys echoed in upstream or transport diagnostics are redacted; error causes remain available through `errors.Is` and `errors.As`.

internal/cli/parser_test.go

@@ -0,0 +1,24 @@
+package cli
+
+import (
+	"bytes"
+	"strings"
+	"testing"
+)
+
+func TestRunParserErrorsSanitizeTerminalControls(t *testing.T) {
+	for _, args := range [][]string{{"search", "coffee", "--bad\x1b[31m"}, {"bad\u202ecommand"}} {
+		var stdout, stderr bytes.Buffer
+		if code := Run(args, &stdout, &stderr); code == 0 {
+			t.Fatal("expected parse error")
+		}
+		for _, output := range []string{stdout.String(), stderr.String()} {
+			if strings.ContainsAny(output, "\x1b\u202e") {
+				t.Fatalf("unsafe diagnostic: %q", output)
+			}
+		}
+		if !strings.Contains(stderr.String(), "bad") {
+			t.Fatalf("missing diagnostic: %q", stderr.String())
+		}
+	}
+}

internal/cli/run.go

@@ -48,7 +48,7 @@ func Run(args []string, stdout, stderr io.Writer) int {
 		kong.Vars{"version": currentVersion()},
 	)
 	if err != nil {
-		_, _ = fmt.Fprintln(stderr, err)
+		writeError(stderr, err.Error())
 		return 1
 	}
 
@@ -59,10 +59,10 @@ func Run(args []string, stdout, stderr io.Writer) int {
 	if err != nil {
 		if parseErr, ok := err.(*kong.ParseError); ok {
 			_ = parseErr.Context.PrintUsage(true)
-			_, _ = fmt.Fprintln(stderr, parseErr.Error())
+			writeError(stderr, parseErr.Error())
 			return parseErr.ExitCode()
 		}
-		_, _ = fmt.Fprintln(stderr, err)
+		writeError(stderr, err.Error())
 		return 2
 	}
 	if root.Global.JSON {

internal/places/client.go

@@ -6,7 +6,6 @@ import (
 	"context"
 	"encoding/json"
 	"errors"
-	"fmt"
 	"io"
 	"net/http"
 	"net/url"
@@ -84,14 +83,14 @@ func (c *Client) doRequest(
 	if body != nil {
 		payload, err := json.Marshal(body)
 		if err != nil {
-			return nil, fmt.Errorf("goplaces: encode request: %w", err)
+			return nil, c.requestError("encode request", err)
 		}
 		reader = bytes.NewReader(payload)
 	}
 
 	request, err := http.NewRequestWithContext(ctx, method, endpoint, reader)
 	if err != nil {
-		return nil, fmt.Errorf("goplaces: build request: %w", err)
+		return nil, c.requestError("build request", err)
 	}
 
 	request.Header.Set("Content-Type", "application/json")
@@ -101,9 +100,12 @@ func (c *Client) doRequest(
 		request.Header.Set("X-Goog-FieldMask", fieldMask)
 	}
 
-	response, err := c.httpClient.Do(request)
+	// Keep the caller's shared client and redirect policy unchanged.
+	httpClient := *c.httpClient
+	httpClient.CheckRedirect = restrictRedirects(httpClient.CheckRedirect)
+	response, err := httpClient.Do(request)
 	if err != nil {
-		return nil, fmt.Errorf("goplaces: request failed: %w", err)
+		return nil, c.requestError("request failed", err)
 	}
 	defer func() {
 		_ = response.Body.Close()
@@ -112,11 +114,11 @@ func (c *Client) doRequest(
 	// Hard-cap payload size to avoid runaway error bodies.
 	payload, err := io.ReadAll(io.LimitReader(response.Body, 1<<20))
 	if err != nil {
-		return nil, fmt.Errorf("goplaces: read response: %w", err)
+		return nil, c.requestError("read response", err)
 	}
 
 	if response.StatusCode >= http.StatusBadRequest {
-		apiErr := &APIError{StatusCode: response.StatusCode, Body: strings.TrimSpace(string(payload))}
+		apiErr := &APIError{StatusCode: response.StatusCode, Body: c.redactAPIKey(strings.TrimSpace(string(payload)))}
 		return nil, apiErr
 	}
 
@@ -135,7 +137,7 @@ func (c *Client) buildURL(path string, query map[string]string) (string, error)
 
 	parsed, err := url.Parse(endpoint)
 	if err != nil {
-		return "", fmt.Errorf("goplaces: invalid url: %w", err)
+		return "", c.requestError("invalid url", err)
 	}
 
 	values := parsed.Query()

internal/places/client_security_test.go

@@ -0,0 +1,169 @@
+package places
+
+import (
+	"context"
+	"errors"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"net/url"
+	"strings"
+	"sync/atomic"
+	"testing"
+)
+
+const securityTestKey = "synthetic-api-key"
+
+func TestClientRejectsCrossOriginRedirect(t *testing.T) {
+	var received atomic.Int32
+	destination := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		received.Add(1)
+		_, _ = fmt.Fprint(w, `{"id":"unexpected"}`)
+	}))
+	defer destination.Close()
+	source := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, destination.URL+"/places/fixture?key="+securityTestKey, http.StatusFound)
+	}))
+	defer source.Close()
+	client := NewClient(Options{APIKey: securityTestKey, BaseURL: source.URL})
+	_, err := client.Details(t.Context(), "fixture")
+	if err == nil {
+		t.Fatal("cross-origin redirect was followed")
+	}
+	if received.Load() != 0 {
+		t.Fatal("redirect destination received a request")
+	}
+	if strings.Contains(err.Error(), securityTestKey) {
+		t.Fatal("redirect error exposed the API key")
+	}
+	var urlErr *url.Error
+	if !errors.As(err, &urlErr) {
+		t.Fatalf("lost typed URL error: %T", err)
+	}
+}
+
+func TestClientSameOriginRedirect(t *testing.T) {
+	var calls atomic.Int32
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		calls.Add(1)
+		if r.Header.Get("X-Goog-Api-Key") != securityTestKey {
+			t.Error("API key missing on same-origin request")
+		}
+		if r.URL.Path != "/final" {
+			http.Redirect(w, r, "/final", http.StatusFound)
+			return
+		}
+		_, _ = fmt.Fprint(w, `{"id":"fixture"}`)
+	}))
+	defer server.Close()
+	client := NewClient(Options{APIKey: securityTestKey, BaseURL: server.URL})
+	place, err := client.Details(t.Context(), "fixture")
+	if err != nil || place.PlaceID != "fixture" || calls.Load() != 2 {
+		t.Fatalf("place=%+v calls=%d err=%v", place, calls.Load(), err)
+	}
+}
+
+func TestClientPreservesCustomRedirectPolicy(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		w.Header().Set("Location", "https://example.invalid/redirect")
+		w.WriteHeader(http.StatusFound)
+		_, _ = fmt.Fprint(w, `{"id":"stopped"}`)
+	}))
+	defer server.Close()
+	var calls int
+	httpClient := server.Client()
+	httpClient.CheckRedirect = func(_ *http.Request, _ []*http.Request) error { calls++; return http.ErrUseLastResponse }
+	client := NewClient(Options{APIKey: securityTestKey, BaseURL: server.URL, HTTPClient: httpClient})
+	for range 2 {
+		place, err := client.Details(t.Context(), "fixture")
+		if err != nil || place.PlaceID != "stopped" {
+			t.Fatalf("place=%+v err=%v", place, err)
+		}
+	}
+	if calls != 2 {
+		t.Fatalf("custom policy calls=%d", calls)
+	}
+}
+
+func TestClientPreservesRedirectLimit(t *testing.T) {
+	var calls atomic.Int32
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		calls.Add(1)
+		http.Redirect(w, r, "/loop", http.StatusFound)
+	}))
+	defer server.Close()
+	client := NewClient(Options{APIKey: securityTestKey, BaseURL: server.URL})
+	_, err := client.Details(t.Context(), "fixture")
+	if err == nil || !strings.Contains(err.Error(), "stopped after 10 redirects") || calls.Load() != 10 {
+		t.Fatalf("calls=%d err=%v", calls.Load(), err)
+	}
+}
+
+func TestClientRedactsAPIError(t *testing.T) {
+	server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		http.Error(w, "rejected "+securityTestKey, http.StatusForbidden)
+	}))
+	defer server.Close()
+	client := NewClient(Options{APIKey: securityTestKey, BaseURL: server.URL})
+	_, err := client.Details(t.Context(), "fixture")
+	var apiErr *APIError
+	if !errors.As(err, &apiErr) {
+		t.Fatalf("expected APIError, got %v", err)
+	}
+	if strings.Contains(apiErr.Body, securityTestKey) || strings.Contains(err.Error(), securityTestKey) {
+		t.Fatal("API error exposed the key")
+	}
+	if apiErr.StatusCode != http.StatusForbidden || apiErr.Body != "rejected [REDACTED]" {
+		t.Fatalf("unexpected API error: %+v", apiErr)
+	}
+}
+
+type securityTransport func(*http.Request) (*http.Response, error)
+
+func (f securityTransport) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) }
+
+func TestClientRedactsTransportErrorAndPreservesCause(t *testing.T) {
+	cause := fmt.Errorf("rejected %s: %w", securityTestKey, context.Canceled)
+	client := NewClient(Options{APIKey: securityTestKey, HTTPClient: &http.Client{Transport: securityTransport(func(*http.Request) (*http.Response, error) { return nil, cause })}})
+	_, err := client.Details(t.Context(), "fixture")
+	if err == nil || strings.Contains(err.Error(), securityTestKey) {
+		t.Fatal("transport error exposed the key")
+	}
+	if !errors.Is(err, context.Canceled) || !errors.Is(err, cause) {
+		t.Fatalf("lost error cause: %v", err)
+	}
+}
+
+func TestClientRedactsInvalidURL(t *testing.T) {
+	client := NewClient(Options{APIKey: securityTestKey, BaseURL: "http://%" + securityTestKey})
+	_, err := client.Details(t.Context(), "fixture")
+	if err == nil || strings.Contains(err.Error(), securityTestKey) {
+		t.Fatal("invalid URL error exposed the key")
+	}
+}
+
+func TestSameOrigin(t *testing.T) {
+	for _, tt := range []struct {
+		a, b string
+		want bool
+	}{
+		{"https://example.com/a", "https://EXAMPLE.com:443/b", true},
+		{"http://example.com/a", "http://example.com:80/b", true},
+		{"https://example.com", "http://example.com", false},
+		{"http://example.com:8000", "http://example.com:8001", false},
+		{"https://example.com", "https://sub.example.com", false},
+		{"custom://example.com", "custom://example.com", true},
+	} {
+		a, err := url.Parse(tt.a)
+		if err != nil {
+			t.Fatal(err)
+		}
+		b, err := url.Parse(tt.b)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if sameOrigin(a, b) != tt.want {
+			t.Errorf("sameOrigin(%s,%s) want %v", tt.a, tt.b, tt.want)
+		}
+	}
+}

internal/places/errors.go

@@ -3,6 +3,7 @@ package places
 import (
 	"errors"
 	"fmt"
+	"strings"
 )
 
 // ErrMissingAPIKey indicates a missing API key.
@@ -30,3 +31,25 @@ func (e *APIError) Error() string {
 	}
 	return fmt.Sprintf("goplaces: api error (%d): %s", e.StatusCode, e.Body)
 }
+
+type clientError struct {
+	message string
+	cause   error
+}
+
+func (e *clientError) Error() string { return e.message }
+func (e *clientError) Unwrap() error { return e.cause }
+
+func (c *Client) requestError(action string, err error) error {
+	return &clientError{
+		message: c.redactAPIKey(fmt.Sprintf("goplaces: %s: %v", action, err)),
+		cause:   err,
+	}
+}
+
+func (c *Client) redactAPIKey(message string) string {
+	if c.apiKey == "" {
+		return message
+	}
+	return strings.ReplaceAll(message, c.apiKey, "[REDACTED]")
+}

internal/places/redirect.go

@@ -0,0 +1,45 @@
+package places
+
+import (
+	"errors"
+	"net/http"
+	"net/url"
+	"strings"
+)
+
+func restrictRedirects(check func(*http.Request, []*http.Request) error) func(*http.Request, []*http.Request) error {
+	return func(request *http.Request, via []*http.Request) error {
+		if check != nil {
+			if err := check(request, via); err != nil {
+				return err
+			}
+		} else if len(via) >= 10 {
+			return errors.New("stopped after 10 redirects")
+		}
+		// net/http forwards custom API-key headers even across origins.
+		if !sameOrigin(request.URL, via[0].URL) {
+			return errors.New("goplaces: refusing cross-origin redirect")
+		}
+		return nil
+	}
+}
+
+func sameOrigin(a, b *url.URL) bool {
+	return strings.EqualFold(a.Scheme, b.Scheme) &&
+		strings.EqualFold(a.Hostname(), b.Hostname()) &&
+		originPort(a) == originPort(b)
+}
+
+func originPort(endpoint *url.URL) string {
+	if port := endpoint.Port(); port != "" {
+		return port
+	}
+	switch strings.ToLower(endpoint.Scheme) {
+	case "https":
+		return "443"
+	case "http":
+		return "80"
+	default:
+		return ""
+	}
+}