Harden CLI parameter parsing and URL handling

Author: cjkenned

AGENTS.md

@@ -53,7 +53,7 @@ The CLI uses a **resource-registry** pattern:
 | --- | --- |
 | `types.go` | `Resource` interface, `Operation` struct, `OperationSchema`, `ParamSchema`, `OperationHooks` |
 | `registry.go` | Thread-safe global registry: `Register()`, `All()`, `Get()`, `Reset()` |
-| `builder.go` | Converts registered resources into Cobra commands with `--json`, `--id`, `--describe`, file input (`@filename`), parameter validation, and hook execution |
+| `builder.go` | Converts registered resources into Cobra commands with per-parameter flags, `--json`, `--describe`, file input (`@filename`), parameter validation, and hook execution |
 
 ### Resources (`internal/resources/`)
 
@@ -95,16 +95,16 @@ The CLI uses a **resource-registry** pattern:
 | `connectors` | `create` | Interactive credential flow | `workspace`, `template_name` or `template_id` |
 | `connectors` | `delete` | Delete a connector | `name`+`workspace` or `--id` |
 
-### Global Flags
+### Common Flags
 
 | Flag | Description | Default |
 | --- | --- | --- |
 | `--format` | Output format: `json` or `table` | `json` |
 | `--describe` | Print operation schema and exit (do not execute) | `false` |
 | `--output, -o` | Write output to file instead of stdout | -- |
 | `--verbose, -v` | Enable debug logging | `false` |
-| `--json` | Inline JSON parameters | -- |
-| `--id` | Convenience flag for resource ID | -- |
+| `--json` | Operation flag for inline JSON parameters; mutually exclusive with per-parameter flags | -- |
+| `--<param>` | Per-parameter operation flags generated from each scalar/array schema parameter, e.g. `--id`, `--workspace`, `--select-fields` | -- |
 
 ## Credential Security
 

README.md

@@ -98,7 +98,7 @@ Parameters can be supplied two ways: as a single JSON document via `--json`, or
 
 ### Two ways to pass parameters
 
-**1. Individual flags (recommended for humans)** — every parameter in the operation's schema is exposed as a `--<param>` flag, with snake_case keys converted to kebab-case (e.g. `select_fields` → `--select-fields`):
+**1. Individual flags (recommended for humans)** — scalar and array parameters in the operation's schema are exposed as `--<param>` flags, with snake_case keys converted to kebab-case (e.g. `select_fields` → `--select-fields`):
 
 ```bash
 airbyte connectors describe --workspace default --name hubspot
@@ -123,11 +123,11 @@ airbyte connectors execute --json '{
 
 Use `@filename` to load JSON from a file: `--json @params.json`. `--json` is the only way to pass nested objects (e.g. the `params` field on `connectors execute`).
 
-### Global flags
+### Common flags
 
 | Flag | Description | Default |
 | --- | --- | --- |
-| `--json` | Inline JSON parameters (or `@filename` to load from a file). Cannot be combined with per-parameter flags. | -- |
+| `--json` | Operation flag for inline JSON parameters (or `@filename` to load from a file). Cannot be combined with per-parameter flags. | -- |
 | `--format` | Output format: `json` or `table` | `json` |
 | `--describe` | Print the operation's parameter schema and exit | `false` |
 | `--output, -o` | Write output to a file instead of stdout | -- |

internal/client/client.go

@@ -4,12 +4,14 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"io"
 	"log"
 	"math"
 	"net/http"
 	"net/url"
+	"strings"
 	"time"
 
 	"github.com/airbytehq/airbyte-cli/internal/auth"
@@ -28,6 +30,7 @@ type Client struct {
 	tokenManager   *auth.TokenManager
 	httpClient     *http.Client
 	debug          bool
+	debugFunc      func() bool
 }
 
 type Option func(*Client)
@@ -38,6 +41,12 @@ func WithDebug(debug bool) Option {
 	}
 }
 
+func WithDebugFunc(debugFunc func() bool) Option {
+	return func(c *Client) {
+		c.debugFunc = debugFunc
+	}
+}
+
 func New(apiHost, organizationID, version string, tm *auth.TokenManager, opts ...Option) *Client {
 	c := &Client{
 		apiHost:        apiHost,
@@ -82,7 +91,11 @@ func (c *Client) Delete(ctx context.Context, path string) (json.RawMessage, erro
 }
 
 func (c *Client) GetURL(ctx context.Context, rawURL string) (json.RawMessage, error) {
-	return c.do(ctx, http.MethodGet, rawURL, nil)
+	safeURL, err := c.resolveAPIURL(rawURL)
+	if err != nil {
+		return nil, err
+	}
+	return c.do(ctx, http.MethodGet, safeURL, nil)
 }
 
 func (c *Client) doWithBody(ctx context.Context, method, path string, body any) (json.RawMessage, error) {
@@ -130,8 +143,8 @@ func (c *Client) do(ctx context.Context, method, rawURL string, body io.Reader)
 			return nil, err
 		}
 
-		if c.debug {
-			log.Printf("[DEBUG] request %s %s attempt %d failed: %v", method, rawURL, attempt+1, err)
+		if c.isDebug() {
+			log.Printf("[DEBUG] request %s %s attempt %d failed: %s", method, redactURL(rawURL), attempt+1, debugError(err))
 		}
 	}
 
@@ -157,8 +170,8 @@ func (c *Client) doOnce(ctx context.Context, method, rawURL string, body io.Read
 		req.Header.Set("X-Organization-Id", c.organizationID)
 	}
 
-	if c.debug {
-		log.Printf("[DEBUG] %s %s", method, rawURL)
+	if c.isDebug() {
+		log.Printf("[DEBUG] %s %s", method, redactURL(rawURL))
 	}
 
 	resp, err := c.httpClient.Do(req)
@@ -172,8 +185,8 @@ func (c *Client) doOnce(ctx context.Context, method, rawURL string, body io.Read
 		return nil, false, fmt.Errorf("reading response body: %w", err)
 	}
 
-	if c.debug {
-		log.Printf("[DEBUG] response %d: %s", resp.StatusCode, string(respBody))
+	if c.isDebug() {
+		log.Printf("[DEBUG] response status=%d bytes=%d", resp.StatusCode, len(respBody))
 	}
 
 	if resp.StatusCode >= 400 {
@@ -189,6 +202,78 @@ func (c *Client) doOnce(ctx context.Context, method, rawURL string, body io.Read
 	return json.RawMessage(respBody), false, nil
 }
 
+func (c *Client) isDebug() bool {
+	return c.debug || (c.debugFunc != nil && c.debugFunc())
+}
+
+func (c *Client) resolveAPIURL(rawURL string) (string, error) {
+	apiBase, err := url.Parse(c.apiHost)
+	if err != nil {
+		return "", fmt.Errorf("parsing API host: %w", err)
+	}
+	nextURL, err := url.Parse(rawURL)
+	if err != nil {
+		return "", fmt.Errorf("parsing URL: %w", err)
+	}
+	if !nextURL.IsAbs() {
+		return apiBase.ResolveReference(nextURL).String(), nil
+	}
+	if nextURL.Scheme != apiBase.Scheme || nextURL.Host != apiBase.Host {
+		return "", &APIError{
+			Type:       "validation_error",
+			Message:    "pagination URL points outside the configured API host",
+			StatusCode: 400,
+		}
+	}
+	return nextURL.String(), nil
+}
+
+func redactURL(rawURL string) string {
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		return rawURL
+	}
+	q := u.Query()
+	for key := range q {
+		if isSensitiveKey(key) {
+			q.Set(key, "[REDACTED]")
+		}
+	}
+	u.RawQuery = q.Encode()
+	return u.String()
+}
+
+func debugError(err error) string {
+	var apiErr *APIError
+	if errors.As(err, &apiErr) {
+		return fmt.Sprintf("%s status=%d retryable=%t", apiErr.Type, apiErr.StatusCode, apiErr.Retryable)
+	}
+	return redactText(err.Error())
+}
+
+func redactText(s string) string {
+	parts := strings.Fields(s)
+	for i, part := range parts {
+		if strings.Contains(part, "=") {
+			key, _, _ := strings.Cut(part, "=")
+			if isSensitiveKey(strings.Trim(key, `"'`)) {
+				parts[i] = key + "=[REDACTED]"
+			}
+		}
+	}
+	return strings.Join(parts, " ")
+}
+
+func isSensitiveKey(key string) bool {
+	key = strings.ToLower(key)
+	return strings.Contains(key, "token") ||
+		strings.Contains(key, "secret") ||
+		strings.Contains(key, "password") ||
+		strings.Contains(key, "credential") ||
+		strings.Contains(key, "api_key") ||
+		strings.Contains(key, "apikey")
+}
+
 func extractErrorMessage(body []byte, statusCode int) string {
 	var parsed struct {
 		Detail  string `json:"detail"`

internal/client/client_test.go

@@ -5,6 +5,7 @@ import (
 	"encoding/json"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"sync/atomic"
 	"testing"
 
@@ -357,3 +358,46 @@ func TestAPIError_JSONSerializable(t *testing.T) {
 		t.Errorf("StatusCode = %d, want %d", parsed.StatusCode, apiErr.StatusCode)
 	}
 }
+
+func TestClient_RejectsExternalPaginationURL(t *testing.T) {
+	creds := &auth.Credentials{ClientID: "id", ClientSecret: "secret"}
+	tm := auth.NewTokenManager("https://api.example.com", "", creds)
+	c := New("https://api.example.com", "", "test", tm)
+
+	_, err := c.GetURL(context.Background(), "https://evil.example.com/api/v1/workspaces?cursor=next")
+	if err == nil {
+		t.Fatal("expected error for external pagination URL")
+	}
+
+	apiErr, ok := err.(*APIError)
+	if !ok {
+		t.Fatalf("expected *APIError, got %T", err)
+	}
+	if apiErr.Type != "validation_error" {
+		t.Errorf("Type = %q, want validation_error", apiErr.Type)
+	}
+}
+
+func TestClient_DebugFuncIsEvaluatedAtRequestTime(t *testing.T) {
+	debug := false
+	c := New("https://api.example.com", "", "test", nil, WithDebugFunc(func() bool { return debug }))
+
+	if c.isDebug() {
+		t.Fatal("debug should initially be false")
+	}
+
+	debug = true
+	if !c.isDebug() {
+		t.Fatal("debug should reflect updated flag state")
+	}
+}
+
+func TestRedactURL(t *testing.T) {
+	got := redactURL("https://api.example.com/path?token=secret&client_secret=hidden&safe=value")
+	if strings.Contains(got, "token=secret") || strings.Contains(got, "client_secret=hidden") {
+		t.Fatalf("expected sensitive query values to be redacted, got %s", got)
+	}
+	if !strings.Contains(got, "safe=value") {
+		t.Fatalf("expected non-sensitive query value to remain, got %s", got)
+	}
+}