diff --git a/CHANGELOG.md b/CHANGELOG.md index 25ec627..6193e0d 100644 --- a/CHANGELOG.md +++ b/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. diff --git a/docs/client-reference.md b/docs/client-reference.md index 55b94d7..e27a13f 100644 --- a/docs/client-reference.md +++ b/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`. diff --git a/internal/cli/parser_test.go b/internal/cli/parser_test.go new file mode 100644 index 0000000..6c5deaf --- /dev/null +++ b/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()) + } + } +} diff --git a/internal/cli/run.go b/internal/cli/run.go index 6f73706..9880d4a 100644 --- a/internal/cli/run.go +++ b/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 { diff --git a/internal/places/client.go b/internal/places/client.go index 05a434e..12a33ab 100644 --- a/internal/places/client.go +++ b/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() diff --git a/internal/places/client_security_test.go b/internal/places/client_security_test.go new file mode 100644 index 0000000..eb101be --- /dev/null +++ b/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) + } + } +} diff --git a/internal/places/errors.go b/internal/places/errors.go index 33ac76a..c6620cf 100644 --- a/internal/places/errors.go +++ b/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]") +} diff --git a/internal/places/redirect.go b/internal/places/redirect.go new file mode 100644 index 0000000..c16e61e --- /dev/null +++ b/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 "" + } +}