From f9d70863141317fc9929eefdc0e458990231e8e3 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 26 Aug 2026 14:39:41 -0700 Subject: [PATCH 01/16] Implement lfx api command for raw authenticated API calls Add flags for method, headers, request body (--input, --field, --raw-field, or piped stdin), gjson-based response filtering (--query/-q), and --hostname to override the resolved API base URL. Refactor the token refresh logic in newAuthTokenCommand into a shared resolveAccessToken helper (also returning the login's audience as the default API base URL), used by both `auth token` and `api`. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- README.md | 8 +- go.mod | 3 + go.sum | 6 + internal/commands/api.go | 236 +++++++++++++++++++++++++++++++++++++- internal/commands/auth.go | 120 ++++++++++--------- 5 files changed, 308 insertions(+), 65 deletions(-) diff --git a/README.md b/README.md index 45cda85..483ffa6 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,8 @@ lfx auth token lfx auth logout # Make an authenticated call to an LFX platform API endpoint. -lfx api +lfx api +lfx api -X POST /projects --field name=example ``` Credentials (refresh token, cached access token) are stored in your @@ -63,11 +64,6 @@ lfx auth login --backend=keychain Run `lfx --help` or `lfx --help` for full details on any command. -> **Note:** This project is under active development. `lfx auth` is fully -> implemented; `lfx api` is currently a stub. See the -> [LFXV2-2509 epic](https://linuxfoundation.atlassian.net/browse/LFXV2-2509) -> for status. - ## Development ```bash diff --git a/go.mod b/go.mod index e66af03..79ad9f7 100644 --- a/go.mod +++ b/go.mod @@ -6,6 +6,7 @@ go 1.25.14 require ( github.com/99designs/keyring v1.2.2 + github.com/tidwall/gjson v1.19.0 github.com/urfave/cli-docs/v3 v3.1.0 github.com/urfave/cli/v3 v3.11.0 golang.org/x/oauth2 v0.36.0 @@ -20,6 +21,8 @@ require ( github.com/gsterjov/go-libsecret v0.0.0-20161001094733-a6f4afe4910c // indirect github.com/mtibben/percent v0.2.1 // indirect github.com/russross/blackfriday/v2 v2.1.0 // indirect + github.com/tidwall/match v1.1.1 // indirect + github.com/tidwall/pretty v1.2.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/term v0.45.0 // indirect ) diff --git a/go.sum b/go.sum index 44c705b..c0e834f 100644 --- a/go.sum +++ b/go.sum @@ -29,6 +29,12 @@ github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY= github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA= github.com/stretchr/testify v1.11.1 h1:7s2iGBzp5EwR7/aIZr8ao5+dra3wiQyKjjFuvgVKu7U= github.com/stretchr/testify v1.11.1/go.mod h1:wZwfW3scLgRK+23gO65QZefKpKQRnfz6sD981Nm4B6U= +github.com/tidwall/gjson v1.19.0 h1:xwxm7n691Uf3u5OFjzngavjGTh55KX5q/9w9xHW88JU= +github.com/tidwall/gjson v1.19.0/go.mod h1:V37/opeE/JbLUOfH0QTXiNez2l0RUjYUhpT4szFQAfc= +github.com/tidwall/match v1.1.1 h1:+Ho715JplO36QYgwN9PGYNhgZvoUSc9X2c80KVTi+GA= +github.com/tidwall/match v1.1.1/go.mod h1:eRSPERbgtNPcGhD8UCthc6PmLEQXEWd3PRB5JTxsfmM= +github.com/tidwall/pretty v1.2.0 h1:RWIZEg2iJ8/g6fDDYzMpobmaoGh5OLl4AXtGUGPcqCs= +github.com/tidwall/pretty v1.2.0/go.mod h1:ITEVvHYasfjBbM0u2Pg8T2nJnzm8xPwvNhhsoaGGjNU= github.com/urfave/cli-docs/v3 v3.1.0 h1:Sa5xm19IpE5gpm6tZzXdfjdFxn67PnEsE4dpXF7vsKw= github.com/urfave/cli-docs/v3 v3.1.0/go.mod h1:59d+5Hz1h6GSGJ10cvcEkbIe3j233t4XDqI72UIx7to= github.com/urfave/cli/v3 v3.11.0 h1:P/euJp99kb9p0tlVY+iYTLYYTAQlfl0hR2gUO1Img1Q= diff --git a/internal/commands/api.go b/internal/commands/api.go index cea6f7f..a5af6e9 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -5,24 +5,248 @@ package commands import ( + "bytes" "context" + "encoding/json" + "errors" "fmt" + "io" + "net/http" + "os" + "strconv" + "strings" + "github.com/tidwall/gjson" "github.com/urfave/cli/v3" ) +// Flag names for the `api` command. +const ( + apiMethodFlagName = "method" + apiHeaderFlagName = "header" + apiInputFlagName = "input" + apiFieldFlagName = "field" + apiRawFieldFlagName = "raw-field" + apiQueryFlagName = "query" + apiHostnameFlagName = "hostname" +) + +// apiAllowedMethods enumerates the HTTP methods `lfx api` accepts via +// --method. PATCH is deliberately excluded per the command's plan. +var apiAllowedMethods = map[string]bool{ + http.MethodGet: true, + http.MethodPost: true, + http.MethodPut: true, + http.MethodDelete: true, +} + // NewAPICommand builds the `lfx api` command for making raw authenticated // calls against LFX platform APIs. -// -// This is currently a stub; the real implementation lands in LFXV2-2517. func NewAPICommand() *cli.Command { return &cli.Command{ Name: "api", Usage: "Make an authenticated call to an LFX platform API endpoint", - ArgsUsage: " ", - Action: func(_ context.Context, _ *cli.Command) error { - fmt.Println("lfx api: not yet implemented (see LFXV2-2517)") - return nil + ArgsUsage: "", + Flags: []cli.Flag{ + &cli.StringFlag{ + Name: apiMethodFlagName, + Aliases: []string{"X"}, + Usage: "HTTP method: GET, POST, PUT, or DELETE", + Value: http.MethodGet, + }, + &cli.StringSliceFlag{ + Name: apiHeaderFlagName, + Aliases: []string{"H"}, + Usage: "Add an additional request header as 'key:value' (repeatable)", + }, + &cli.StringFlag{ + Name: apiInputFlagName, + Usage: "Read the request body from a file, or '-' for stdin", + }, + &cli.StringSliceFlag{ + Name: apiFieldFlagName, + Aliases: []string{"F"}, + Usage: "Add a typed JSON body field as 'key=value' (repeatable)", + }, + &cli.StringSliceFlag{ + Name: apiRawFieldFlagName, + Usage: "Add a string JSON body field as 'key=value' (repeatable)", + }, + &cli.StringFlag{ + Name: apiQueryFlagName, + Aliases: []string{"q"}, + Usage: "Filter the response body through a gjson expression before output", + }, + &cli.StringFlag{ + Name: apiHostnameFlagName, + Usage: "Override the LFX API base URL (advanced; independent of the IdP domain)", + }, }, + Action: runAPI, + } +} + +func runAPI(ctx context.Context, cmd *cli.Command) error { + path := cmd.Args().First() + if path == "" { + return errors.New("usage: lfx api ") + } + + method := strings.ToUpper(cmd.String(apiMethodFlagName)) + if !apiAllowedMethods[method] { + return fmt.Errorf("invalid --%s %q (must be one of GET, POST, PUT, DELETE)", apiMethodFlagName, method) + } + + token, audience, err := resolveAccessToken(ctx, cmd) + if err != nil { + return err + } + + baseURL := cmd.String(apiHostnameFlagName) + if baseURL == "" { + baseURL = audience + } + if baseURL == "" { + return errors.New("no API base URL available; log in with `lfx auth login` or pass --hostname") + } + + body, contentType, err := apiRequestBody(cmd) + if err != nil { + return err + } + + url, err := apiJoinURL(baseURL, path) + if err != nil { + return err + } + + req, err := http.NewRequestWithContext(ctx, method, url, bytes.NewReader(body)) + if err != nil { + return fmt.Errorf("build request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+token) + if contentType != "" { + req.Header.Set("Content-Type", contentType) + } + for _, h := range cmd.StringSlice(apiHeaderFlagName) { + key, value, ok := strings.Cut(h, ":") + if !ok { + return fmt.Errorf("invalid --%s %q (expected 'key:value')", apiHeaderFlagName, h) + } + req.Header.Set(strings.TrimSpace(key), strings.TrimSpace(value)) + } + + resp, err := http.DefaultClient.Do(req) + if err != nil { + return fmt.Errorf("request failed: %w", err) + } + defer resp.Body.Close() + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("read response body: %w", err) + } + + output := respBody + if query := cmd.String(apiQueryFlagName); query != "" { + output = []byte(gjson.GetBytes(respBody, query).String()) + } + fmt.Println(string(output)) + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + fmt.Fprintf(os.Stderr, "HTTP %d\n", resp.StatusCode) + return cli.Exit("", 1) + } + + return nil +} + +// apiRequestBody constructs the request body and its Content-Type from the +// command's --input, --field, and --raw-field flags (mutually exclusive: +// --input vs. --field/--raw-field), falling back to stdin when none are +// passed and stdin is piped (non-TTY). +func apiRequestBody(cmd *cli.Command) (body []byte, contentType string, err error) { + input := cmd.String(apiInputFlagName) + fields := cmd.StringSlice(apiFieldFlagName) + rawFields := cmd.StringSlice(apiRawFieldFlagName) + + if input != "" && (len(fields) > 0 || len(rawFields) > 0) { + return nil, "", fmt.Errorf("--%s cannot be combined with --%s or --%s", apiInputFlagName, apiFieldFlagName, apiRawFieldFlagName) + } + + if input != "" { + if input == "-" { + data, err := io.ReadAll(os.Stdin) + if err != nil { + return nil, "", fmt.Errorf("read stdin: %w", err) + } + return data, "", nil + } + data, err := os.ReadFile(input) + if err != nil { + return nil, "", fmt.Errorf("read --%s file: %w", apiInputFlagName, err) + } + return data, "", nil + } + + if len(fields) > 0 || len(rawFields) > 0 { + obj := make(map[string]any, len(fields)+len(rawFields)) + for _, f := range fields { + key, value, ok := strings.Cut(f, "=") + if !ok { + return nil, "", fmt.Errorf("invalid --%s %q (expected 'key=value')", apiFieldFlagName, f) + } + obj[key] = coerceFieldValue(value) + } + for _, f := range rawFields { + key, value, ok := strings.Cut(f, "=") + if !ok { + return nil, "", fmt.Errorf("invalid --%s %q (expected 'key=value')", apiRawFieldFlagName, f) + } + obj[key] = value + } + data, err := json.Marshal(obj) + if err != nil { + return nil, "", fmt.Errorf("build JSON body: %w", err) + } + return data, "application/json", nil + } + + stat, err := os.Stdin.Stat() + if err == nil && (stat.Mode()&os.ModeCharDevice) == 0 { + data, err := io.ReadAll(os.Stdin) + if err != nil { + return nil, "", fmt.Errorf("read stdin: %w", err) + } + return data, "", nil + } + + return nil, "", nil +} + +// coerceFieldValue applies gh-style type coercion to a --field value: +// "true"/"false" become booleans, "null" becomes nil, and numeric strings +// become JSON numbers. Everything else stays a string. +func coerceFieldValue(value string) any { + switch value { + case "true": + return true + case "false": + return false + case "null": + return nil + } + if n, err := strconv.ParseFloat(value, 64); err == nil { + return n + } + return value +} + +// apiJoinURL joins base and path into a single URL, ensuring exactly one +// slash separates them. +func apiJoinURL(base, path string) (string, error) { + if base == "" { + return "", errors.New("empty base URL") } + return strings.TrimRight(base, "/") + "/" + strings.TrimLeft(path, "/"), nil } diff --git a/internal/commands/auth.go b/internal/commands/auth.go index d43ffb4..02cfbd2 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -438,70 +438,84 @@ func newAuthTokenCommand() *cli.Command { Name: "token", Usage: "Print a valid access token for the LFX platform", Action: func(ctx context.Context, cmd *cli.Command) error { - store, creds, found, err := loadStoredCredentials(cmd) + token, _, err := resolveAccessToken(ctx, cmd) if err != nil { return err } - if !found { - return errors.New("not logged in; run `lfx auth login` first") - } + fmt.Println(token) + return nil + }, + } +} - // Validate the persisted device state (insecure-storage and - // --backend pinning) before trusting or returning anything - // from creds, including the ValidAccessToken fast path below - // -- otherwise omitting a pinned --backend could still open - // some other auto-detected backend and print its cached - // token, defeating the pin. - _, domain, clientID, err := loadDeviceStateForBackend(store, cmd) - if err != nil { - return fmt.Errorf("load device state: %w", err) - } +// resolveAccessToken returns a valid access token for the current login, +// refreshing it (and persisting the refreshed credentials) if the cached +// one is missing or expired. It also returns the audience recorded at +// login time, so callers (e.g. `lfx api`) can use it as their default API +// base URL. Both `lfx auth token` and `lfx api` share this single code +// path so their refresh, error, and credential-persistence behavior never +// drifts apart. +func resolveAccessToken(ctx context.Context, cmd *cli.Command) (token, audience string, err error) { + store, creds, found, err := loadStoredCredentials(cmd) + if err != nil { + return "", "", err + } + if !found { + return "", "", errors.New("not logged in; run `lfx auth login` first") + } - if creds.ValidAccessToken() { - fmt.Println(creds.AccessToken) - return nil - } + // Validate the persisted device state (insecure-storage and + // --backend pinning) before trusting or returning anything from + // creds, including the ValidAccessToken fast path below -- otherwise + // omitting a pinned --backend could still open some other + // auto-detected backend and return its cached token, defeating the + // pin. + state, domain, clientID, err := loadDeviceStateForBackend(store, cmd) + if err != nil { + return "", "", fmt.Errorf("load device state: %w", err) + } - if creds.RefreshToken == "" { - return errors.New("no refresh token available; run `lfx auth login` again") - } + if creds.ValidAccessToken() { + return creds.AccessToken, state.Audience, nil + } - cfg := &oauth2.Config{ - ClientID: clientID, - Endpoint: oauth2.Endpoint{ - DeviceAuthURL: "https://" + domain + "/oauth/device/code", - TokenURL: "https://" + domain + "/oauth/token", - AuthStyle: oauth2.AuthStyleInParams, - }, - } + if creds.RefreshToken == "" { + return "", "", errors.New("no refresh token available; run `lfx auth login` again") + } - token, err := cfg.TokenSource(ctx, &oauth2.Token{RefreshToken: creds.RefreshToken}).Token() - var retrieveErr *oauth2.RetrieveError - if errors.As(err, &retrieveErr) && retrieveErr.ErrorCode == "invalid_grant" { - return errors.New("session expired or revoked; run `lfx auth login` to log in again") - } - if err != nil { - return fmt.Errorf("refresh access token: %w", err) - } + cfg := &oauth2.Config{ + ClientID: clientID, + Endpoint: oauth2.Endpoint{ + DeviceAuthURL: "https://" + domain + "/oauth/device/code", + TokenURL: "https://" + domain + "/oauth/token", + AuthStyle: oauth2.AuthStyleInParams, + }, + } - refreshToken := token.RefreshToken - if refreshToken == "" { - // Auth0 may not rotate the refresh token on every - // exchange; keep the existing one in that case. - refreshToken = creds.RefreshToken - } - if err := store.SaveCredentials(credstore.Credentials{ - RefreshToken: refreshToken, - AccessToken: token.AccessToken, - AccessTokenExpiry: token.Expiry, - }); err != nil { - return fmt.Errorf("save refreshed credentials: %w", err) - } + refreshed, err := cfg.TokenSource(ctx, &oauth2.Token{RefreshToken: creds.RefreshToken}).Token() + var retrieveErr *oauth2.RetrieveError + if errors.As(err, &retrieveErr) && retrieveErr.ErrorCode == "invalid_grant" { + return "", "", errors.New("session expired or revoked; run `lfx auth login` to log in again") + } + if err != nil { + return "", "", fmt.Errorf("refresh access token: %w", err) + } - fmt.Println(token.AccessToken) - return nil - }, + refreshToken := refreshed.RefreshToken + if refreshToken == "" { + // Auth0 may not rotate the refresh token on every exchange; keep + // the existing one in that case. + refreshToken = creds.RefreshToken } + if err := store.SaveCredentials(credstore.Credentials{ + RefreshToken: refreshToken, + AccessToken: refreshed.AccessToken, + AccessTokenExpiry: refreshed.Expiry, + }); err != nil { + return "", "", fmt.Errorf("save refreshed credentials: %w", err) + } + + return refreshed.AccessToken, state.Audience, nil } func newAuthStatusCommand() *cli.Command { From f3484be3d1a1645e3e16a838aa7093ede6124298 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 26 Aug 2026 14:41:05 -0700 Subject: [PATCH 02/16] Add unit tests for lfx api's pure/testable helpers Cover coerceFieldValue, apiJoinURL, and apiRequestBody (--field/ --raw-field merging and type coercion, --input from a file, piped stdin auto-detection, and the mutual-exclusion/malformed-flag error paths), following the table-driven and cli.Command-based patterns already used in auth_test.go. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api_test.go | 205 ++++++++++++++++++++++++++++++++++ 1 file changed, 205 insertions(+) create mode 100644 internal/commands/api_test.go diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go new file mode 100644 index 0000000..09b7ae6 --- /dev/null +++ b/internal/commands/api_test.go @@ -0,0 +1,205 @@ +// Copyright The Linux Foundation and each contributor to LFX. +// SPDX-License-Identifier: MIT + +package commands + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/urfave/cli/v3" +) + +// newAPITestCommand builds a *cli.Command with the `api` command's flags +// registered, parses args against it, and hands the parsed *cli.Command to +// fn, mirroring newTestCommand in auth_test.go for the auth flags. +func newAPITestCommand(t *testing.T, args []string, fn func(cmd *cli.Command)) { + t.Helper() + cmd := &cli.Command{ + Name: "test", + Flags: NewAPICommand().Flags, + Action: func(_ context.Context, cmd *cli.Command) error { + fn(cmd) + return nil + }, + } + if err := cmd.Run(context.Background(), append([]string{"test"}, args...)); err != nil { + t.Fatalf("cmd.Run: %v", err) + } +} + +func TestCoerceFieldValue(t *testing.T) { + tests := []struct { + name string + value string + want any + }{ + {name: "true", value: "true", want: true}, + {name: "false", value: "false", want: false}, + {name: "null", value: "null", want: nil}, + {name: "integer", value: "42", want: float64(42)}, + {name: "float", value: "3.14", want: 3.14}, + {name: "plain string", value: "hello", want: "hello"}, + {name: "numeric-looking but not fully numeric", value: "42abc", want: "42abc"}, + {name: "empty string", value: "", want: ""}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + if got := coerceFieldValue(tc.value); got != tc.want { + t.Errorf("coerceFieldValue(%q) = %#v, want %#v", tc.value, got, tc.want) + } + }) + } +} + +func TestAPIJoinURL(t *testing.T) { + tests := []struct { + name string + base string + path string + want string + }{ + {name: "no trailing/leading slash", base: "https://api.example.com", path: "projects", want: "https://api.example.com/projects"}, + {name: "trailing slash on base", base: "https://api.example.com/", path: "projects", want: "https://api.example.com/projects"}, + {name: "leading slash on path", base: "https://api.example.com", path: "/projects", want: "https://api.example.com/projects"}, + {name: "both slashes", base: "https://api.example.com/", path: "/projects", want: "https://api.example.com/projects"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := apiJoinURL(tc.base, tc.path) + if err != nil { + t.Fatalf("apiJoinURL(%q, %q): %v", tc.base, tc.path, err) + } + if got != tc.want { + t.Errorf("apiJoinURL(%q, %q) = %q, want %q", tc.base, tc.path, got, tc.want) + } + }) + } + + t.Run("empty base", func(t *testing.T) { + if _, err := apiJoinURL("", "/projects"); err == nil { + t.Fatal("apiJoinURL: got nil error, want error for empty base") + } + }) +} + +func TestAPIRequestBodyFields(t *testing.T) { + newAPITestCommand(t, []string{ + "--field", "name=example", + "--field", "active=true", + "--field", "count=3", + "--raw-field", "note=42", + }, func(cmd *cli.Command) { + body, contentType, err := apiRequestBody(cmd) + if err != nil { + t.Fatalf("apiRequestBody: %v", err) + } + if contentType != "application/json" { + t.Errorf("contentType = %q, want application/json", contentType) + } + + var got map[string]any + if err := json.Unmarshal(body, &got); err != nil { + t.Fatalf("json.Unmarshal(body): %v", err) + } + want := map[string]any{ + "name": "example", + "active": true, + "count": float64(3), + // --raw-field always stays a JSON string, even though "42" + // would otherwise coerce to a number via --field. + "note": "42", + } + if len(got) != len(want) { + t.Fatalf("body = %v, want %v", got, want) + } + for k, v := range want { + if got[k] != v { + t.Errorf("body[%q] = %#v, want %#v", k, got[k], v) + } + } + }) +} + +func TestAPIRequestBodyInvalidField(t *testing.T) { + newAPITestCommand(t, []string{"--field", "no-equals-sign"}, func(cmd *cli.Command) { + if _, _, err := apiRequestBody(cmd); err == nil { + t.Fatal("apiRequestBody: got nil error, want error for malformed --field") + } + }) +} + +func TestAPIRequestBodyInvalidRawField(t *testing.T) { + newAPITestCommand(t, []string{"--raw-field", "no-equals-sign"}, func(cmd *cli.Command) { + if _, _, err := apiRequestBody(cmd); err == nil { + t.Fatal("apiRequestBody: got nil error, want error for malformed --raw-field") + } + }) +} + +func TestAPIRequestBodyInputFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "body.json") + if err := os.WriteFile(path, []byte(`{"raw":true}`), 0o600); err != nil { + t.Fatalf("os.WriteFile: %v", err) + } + + newAPITestCommand(t, []string{"--input", path}, func(cmd *cli.Command) { + body, contentType, err := apiRequestBody(cmd) + if err != nil { + t.Fatalf("apiRequestBody: %v", err) + } + if contentType != "" { + t.Errorf("contentType = %q, want empty (raw --input sets no Content-Type)", contentType) + } + if string(body) != `{"raw":true}` { + t.Errorf("body = %q, want %q", body, `{"raw":true}`) + } + }) +} + +func TestAPIRequestBodyInputRejectsCombiningWithFields(t *testing.T) { + newAPITestCommand(t, []string{"--input", "/dev/null", "--field", "name=example"}, func(cmd *cli.Command) { + if _, _, err := apiRequestBody(cmd); err == nil { + t.Fatal("apiRequestBody: got nil error, want error for --input combined with --field") + } + }) +} + +func TestAPIRequestBodyNoneWithPipedStdin(t *testing.T) { + // os.Stdin under `go test` is not a real TTY, so apiRequestBody's + // non-char-device check treats it as piped; substitute an explicit, + // already-closed pipe end so the test deterministically exercises + // the "read piped stdin" branch without depending on -- or + // blocking on -- whatever stdin the test binary happened to + // inherit. + r, w, err := os.Pipe() + if err != nil { + t.Fatalf("os.Pipe: %v", err) + } + if _, err := w.WriteString(`{"from":"stdin"}`); err != nil { + t.Fatalf("write to pipe: %v", err) + } + if err := w.Close(); err != nil { + t.Fatalf("close pipe writer: %v", err) + } + + orig := os.Stdin + os.Stdin = r + t.Cleanup(func() { os.Stdin = orig }) + + newAPITestCommand(t, nil, func(cmd *cli.Command) { + body, contentType, err := apiRequestBody(cmd) + if err != nil { + t.Fatalf("apiRequestBody: %v", err) + } + if contentType != "" { + t.Errorf("contentType = %q, want empty (piped stdin sets no Content-Type)", contentType) + } + if string(body) != `{"from":"stdin"}` { + t.Errorf("body = %q, want %q", body, `{"from":"stdin"}`) + } + }) +} From c831d3dfab3186b5922e7f76dc1b627d5c2fd474 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 26 Aug 2026 14:42:01 -0700 Subject: [PATCH 03/16] Drop stale Jira ticket reference from credstore comment The design rationale it points to stands on its own; no need to keep a pointer to the tickets that originally prompted it. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/credstore/credstore.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/credstore/credstore.go b/internal/credstore/credstore.go index f69626e..69e317b 100644 --- a/internal/credstore/credstore.go +++ b/internal/credstore/credstore.go @@ -178,8 +178,8 @@ func (c Credentials) ValidAccessToken() bool { // re-specify the environment, IdP domain, or audience used at login. // // Note: this deliberately does not include a persistent "device ID". One -// was considered (see LFXV2-2515/LFXV2-2509 discussion) on the assumption -// that `gh` uses one as part of its OAuth device flow, but `gh`'s +// was considered on the assumption that `gh` uses one as part of its +// OAuth device flow, but `gh`'s // `~/.local/state/gh/device-id` is actually just an anonymous telemetry // identifier (see `internal/telemetry.getOrCreateDeviceID` in // github.com/cli/cli) -- it plays no role in the OAuth device From 1e1db1bab4d49b557f83364e5a02aa0f8d673c23 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 26 Aug 2026 14:44:57 -0700 Subject: [PATCH 04/16] Clarify why lfx api excludes PATCH Point to the actual design reason (the LFX API has no PATCH endpoints, favoring PUT with ETag/If-Match concurrency control) instead of an internal reference to the command's planning notes. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index a5af6e9..d7598cb 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -32,7 +32,9 @@ const ( ) // apiAllowedMethods enumerates the HTTP methods `lfx api` accepts via -// --method. PATCH is deliberately excluded per the command's plan. +// --method. PATCH is deliberately excluded: the LFX API, by design, +// currently includes no PATCH endpoints, favoring PUT with ETag/If-Match +// concurrency control instead. var apiAllowedMethods = map[string]bool{ http.MethodGet: true, http.MethodPost: true, From 8c3f7ce0e1e569f585c1590ed2d504f836b1eacf Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 26 Aug 2026 14:51:02 -0700 Subject: [PATCH 05/16] Auto-promote GET to POST when a body is explicitly supplied; fix stdin Stat() error handling - lfx api now switches its default --method from GET to POST when the caller passes --field/--raw-field/--input but no explicit --method, matching the conventional default gh api and curl both use. A body implicitly picked up from piped stdin (no explicit body flag) does not trigger this, since that alone isn't a clear signal of intent. - apiRequestBody now surfaces a stdin Stat() failure as an error instead of silently falling back to an empty body. - Reworked the README PUT example to use --input - with a redirected file instead of a single --field, and added an -H "If-Match: " header to illustrate concurrency control on updates. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- README.md | 3 ++- internal/commands/api.go | 29 +++++++++++++++++++-- internal/commands/api_test.go | 47 +++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 483ffa6..604ee7f 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,8 @@ lfx auth logout # Make an authenticated call to an LFX platform API endpoint. lfx api -lfx api -X POST /projects --field name=example +lfx api /projects --field name=example # auto-promotes to POST +lfx api -X PUT /projects/123 --input - -H "If-Match: " < input.json ``` Credentials (refresh token, cached access token) are stored in your diff --git a/internal/commands/api.go b/internal/commands/api.go index d7598cb..77c78db 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -53,7 +53,7 @@ func NewAPICommand() *cli.Command { &cli.StringFlag{ Name: apiMethodFlagName, Aliases: []string{"X"}, - Usage: "HTTP method: GET, POST, PUT, or DELETE", + Usage: "HTTP method: GET, POST, PUT, or DELETE (default: GET, or POST if a body is explicitly supplied)", Value: http.MethodGet, }, &cli.StringSliceFlag{ @@ -95,6 +95,17 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { } method := strings.ToUpper(cmd.String(apiMethodFlagName)) + if !cmd.IsSet(apiMethodFlagName) && apiHasExplicitBody(cmd) { + // No --method was passed, but the caller explicitly supplied a + // body via --field/--raw-field/--input; a GET request wouldn't + // carry it anywhere useful. Auto-promote to POST, matching the + // conventional default `gh api` and `curl` both use when a body + // is present. This never triggers for a body implicitly picked + // up from piped stdin with no explicit body flag, since that's + // not a clear enough signal of intent to override the GET + // default. + method = http.MethodPost + } if !apiAllowedMethods[method] { return fmt.Errorf("invalid --%s %q (must be one of GET, POST, PUT, DELETE)", apiMethodFlagName, method) } @@ -163,6 +174,17 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { return nil } +// apiHasExplicitBody reports whether cmd was given an explicit +// body-supplying flag (--input, --field, or --raw-field), as opposed to a +// body implicitly picked up from piped stdin with none of those flags set. +// Used to decide whether the default --method should be promoted from GET; +// see runAPI. +func apiHasExplicitBody(cmd *cli.Command) bool { + return cmd.String(apiInputFlagName) != "" || + len(cmd.StringSlice(apiFieldFlagName)) > 0 || + len(cmd.StringSlice(apiRawFieldFlagName)) > 0 +} + // apiRequestBody constructs the request body and its Content-Type from the // command's --input, --field, and --raw-field flags (mutually exclusive: // --input vs. --field/--raw-field), falling back to stdin when none are @@ -215,7 +237,10 @@ func apiRequestBody(cmd *cli.Command) (body []byte, contentType string, err erro } stat, err := os.Stdin.Stat() - if err == nil && (stat.Mode()&os.ModeCharDevice) == 0 { + if err != nil { + return nil, "", fmt.Errorf("stat stdin: %w", err) + } + if stat.Mode()&os.ModeCharDevice == 0 { data, err := io.ReadAll(os.Stdin) if err != nil { return nil, "", fmt.Errorf("read stdin: %w", err) diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go index 09b7ae6..a93114f 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -168,6 +168,53 @@ func TestAPIRequestBodyInputRejectsCombiningWithFields(t *testing.T) { }) } +func TestAPIHasExplicitBody(t *testing.T) { + tests := []struct { + name string + args []string + want bool + }{ + {name: "no body flags", args: nil, want: false}, + {name: "field", args: []string{"--field", "name=example"}, want: true}, + {name: "raw-field", args: []string{"--raw-field", "name=example"}, want: true}, + {name: "input file", args: []string{"--input", "/dev/null"}, want: true}, + {name: "input stdin marker", args: []string{"--input", "-"}, want: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + newAPITestCommand(t, tc.args, func(cmd *cli.Command) { + if got := apiHasExplicitBody(cmd); got != tc.want { + t.Errorf("apiHasExplicitBody() = %v, want %v", got, tc.want) + } + }) + }) + } +} + +func TestAPIRequestBodyStatError(t *testing.T) { + // Substitute an already-closed file for os.Stdin so Stat() fails, + // simulating the rare platforms/conditions where stdin can't be + // probed at all; apiRequestBody should surface that error rather + // than silently falling back to an empty body. + f, err := os.CreateTemp(t.TempDir(), "closed-stdin") + if err != nil { + t.Fatalf("os.CreateTemp: %v", err) + } + if err := f.Close(); err != nil { + t.Fatalf("close temp file: %v", err) + } + + orig := os.Stdin + os.Stdin = f + t.Cleanup(func() { os.Stdin = orig }) + + newAPITestCommand(t, nil, func(cmd *cli.Command) { + if _, _, err := apiRequestBody(cmd); err == nil { + t.Fatal("apiRequestBody: got nil error, want error from failed stdin Stat()") + } + }) +} + func TestAPIRequestBodyNoneWithPipedStdin(t *testing.T) { // os.Stdin under `go test` is not a real TTY, so apiRequestBody's // non-char-device check treats it as piped; substitute an explicit, From cce07c7016a0e77397a092c6b8ab59078157fd72 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 26 Aug 2026 14:53:06 -0700 Subject: [PATCH 06/16] Use a real query-string endpoint in the README GET example Replace the generic placeholder with a live, manually-verified example (lfx api '/my-grants?v=1&object_type=projects'), demonstrating query-string usage alongside the existing POST/PUT examples. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 604ee7f..42e0960 100644 --- a/README.md +++ b/README.md @@ -39,7 +39,7 @@ lfx auth token lfx auth logout # Make an authenticated call to an LFX platform API endpoint. -lfx api +lfx api '/my-grants?v=1&object_type=projects' lfx api /projects --field name=example # auto-promotes to POST lfx api -X PUT /projects/123 --input - -H "If-Match: " < input.json ``` From b970ee0a0459d47171a70fc117221d007d14701f Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Wed, 26 Aug 2026 16:27:22 -0700 Subject: [PATCH 07/16] Address Copilot review feedback on lfx api - Move --insecure-storage/--backend to the root lfx command (new exported commands.CredentialStoreFlags) so lfx api can resolve credentials the same way lfx auth subcommands do. Previously these flags were only declared on the auth command group, so lfx api rejected them outright. - Reject extra positional arguments to lfx api instead of silently ignoring everything after the first. - Avoid a spurious blank "Error: " line on non-2xx responses by returning the HTTP status through cli.Exit's message instead of writing directly to stderr. - Replace strconv.ParseFloat-based --field number coercion with a JSON-number-syntax regex plus json.Number, so NaN/Inf/hex floats stay strings and large integers don't silently lose precision. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- README.md | 10 ++++---- cmd/lfx/main.go | 1 + internal/commands/api.go | 31 +++++++++++++++++------- internal/commands/api_test.go | 9 +++++-- internal/commands/auth.go | 44 +++++++++++++++++++--------------- internal/commands/auth_test.go | 15 +++++------- 6 files changed, 66 insertions(+), 44 deletions(-) diff --git a/README.md b/README.md index 42e0960..1086882 100644 --- a/README.md +++ b/README.md @@ -52,11 +52,11 @@ Service reachable in one shell session but not another); pass `--backend` to pin it to one explicitly (see `lfx auth backends` for the available names). Once a login has pinned a backend, later commands must pass the same `--backend` value. Pass `--insecure-storage` to -any `auth` subcommand to instead store credentials in a plain, unencrypted, -owner-only file, at the cost of weaker protection for the stored tokens. On -Windows, this owner-only mode relies on inherited directory permissions -rather than a real ACL, since Go's `Chmod(0600)` maps to the read-only -attribute there rather than restricting access to the current user. +instead store credentials in a plain, unencrypted, owner-only file, at the +cost of weaker protection for the stored tokens. On Windows, this +owner-only mode relies on inherited directory permissions rather than a +real ACL, since Go's `Chmod(0600)` maps to the read-only attribute there +rather than restricting access to the current user. ```bash lfx auth login --insecure-storage diff --git a/cmd/lfx/main.go b/cmd/lfx/main.go index 6a34b18..a108cec 100644 --- a/cmd/lfx/main.go +++ b/cmd/lfx/main.go @@ -38,6 +38,7 @@ func main() { Usage: "Authenticate with and call the LFX platform APIs", Version: version, EnableShellCompletion: true, + Flags: commands.CredentialStoreFlags, Commands: []*cli.Command{ commands.NewAuthCommand(), commands.NewAPICommand(), diff --git a/internal/commands/api.go b/internal/commands/api.go index 77c78db..f8c7f0d 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -13,7 +13,7 @@ import ( "io" "net/http" "os" - "strconv" + "regexp" "strings" "github.com/tidwall/gjson" @@ -89,10 +89,10 @@ func NewAPICommand() *cli.Command { } func runAPI(ctx context.Context, cmd *cli.Command) error { - path := cmd.Args().First() - if path == "" { + if cmd.Args().Len() != 1 { return errors.New("usage: lfx api ") } + path := cmd.Args().First() method := strings.ToUpper(cmd.String(apiMethodFlagName)) if !cmd.IsSet(apiMethodFlagName) && apiHasExplicitBody(cmd) { @@ -167,8 +167,10 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { fmt.Println(string(output)) if resp.StatusCode < 200 || resp.StatusCode >= 300 { - fmt.Fprintf(os.Stderr, "HTTP %d\n", resp.StatusCode) - return cli.Exit("", 1) + // Use cli.Exit's message (rather than writing directly to + // stderr) so main's generic `Error: ` handling only emits + // one diagnostic line instead of two. + return cli.Exit(fmt.Sprintf("HTTP %d", resp.StatusCode), 1) } return nil @@ -251,9 +253,20 @@ func apiRequestBody(cmd *cli.Command) (body []byte, contentType string, err erro return nil, "", nil } +// jsonNumberPattern matches valid JSON number syntax (RFC 8259), which is +// stricter than Go's strconv.ParseFloat: it rejects "NaN", "Inf", and +// hexadecimal floats (e.g. "0x1p2"), all of which ParseFloat accepts but +// which are not valid JSON numbers and would otherwise either fail +// json.Marshal outright or silently coerce a value the user likely meant +// as a literal string. +var jsonNumberPattern = regexp.MustCompile(`^-?(0|[1-9]\d*)(\.\d+)?([eE][+-]?\d+)?$`) + // coerceFieldValue applies gh-style type coercion to a --field value: -// "true"/"false" become booleans, "null" becomes nil, and numeric strings -// become JSON numbers. Everything else stays a string. +// "true"/"false" become booleans, "null" becomes nil, and values matching +// JSON number syntax become json.Number (preserving the original digits +// verbatim in the request body, rather than round-tripping through +// float64 and silently losing precision for integers beyond 2^53). +// Everything else stays a string. func coerceFieldValue(value string) any { switch value { case "true": @@ -263,8 +276,8 @@ func coerceFieldValue(value string) any { case "null": return nil } - if n, err := strconv.ParseFloat(value, 64); err == nil { - return n + if jsonNumberPattern.MatchString(value) { + return json.Number(value) } return value } diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go index a93114f..59e0d97 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -40,11 +40,16 @@ func TestCoerceFieldValue(t *testing.T) { {name: "true", value: "true", want: true}, {name: "false", value: "false", want: false}, {name: "null", value: "null", want: nil}, - {name: "integer", value: "42", want: float64(42)}, - {name: "float", value: "3.14", want: 3.14}, + {name: "integer", value: "42", want: json.Number("42")}, + {name: "float", value: "3.14", want: json.Number("3.14")}, {name: "plain string", value: "hello", want: "hello"}, {name: "numeric-looking but not fully numeric", value: "42abc", want: "42abc"}, {name: "empty string", value: "", want: ""}, + {name: "NaN stays a string", value: "NaN", want: "NaN"}, + {name: "Infinity stays a string", value: "Infinity", want: "Infinity"}, + {name: "hex float stays a string", value: "0x1p2", want: "0x1p2"}, + {name: "leading zero stays a string (not valid JSON number)", value: "042", want: "042"}, + {name: "large integer preserves precision", value: "9007199254740993", want: json.Number("9007199254740993")}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 02cfbd2..009479d 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -43,35 +43,41 @@ const ( audienceFlagName = "audience" ) +// CredentialStoreFlags are the --insecure-storage and --backend flags +// used by credStoreFromCommand. They are registered on the root `lfx` +// command in cmd/lfx/main.go (rather than per-subcommand) so they're +// inherited via cmd.Bool/cmd.String without redeclaration wherever +// they're needed -- both the auth-specific commands and API/method calls. +var CredentialStoreFlags = []cli.Flag{ + &cli.BoolFlag{ + Name: insecureStorageFlagName, + Usage: "Store & retrieve credentials in a plain (unencrypted) file instead of the system backend", + }, + &cli.StringFlag{ + Name: backendFlagName, + Usage: "Pin credential storage to a specific system backend (see `lfx auth backends`); mutually exclusive with --insecure-storage", + }, +} + // scopes requested during the device code flow. offline_access is required // to receive a refresh token; the rest identify the user for `auth status`. var loginScopes = []string{"openid", "profile", "email", "offline_access"} // NewAuthCommand builds the `lfx auth` command group with its subcommands. // -// The --insecure-storage and --backend flags are shared by all -// subcommands (they are not declared as "Local" flags, so urfave/cli -// resolves them for subcommand actions via cmd.Bool/cmd.String). -// --insecure-storage controls whether credentials bypass the system -// backend in favor of credstore's plain (unencrypted) file fallback, e.g. -// for headless/CI use. --backend pins credential storage to a -// single system backend rather than letting keyring.Open silently pick -// whichever one currently opens; see credstore.DeviceState.Backend for why -// that matters once a login has pinned one. +// --insecure-storage and --backend (registered on the root `lfx` command; +// see CredentialStoreFlags) control credential storage for all +// subcommands here. --insecure-storage controls whether credentials +// bypass the system backend in favor of credstore's plain (unencrypted) +// file fallback, e.g. for headless/CI use. --backend pins credential +// storage to a single system backend rather than letting keyring.Open +// silently pick whichever one currently opens; see +// credstore.DeviceState.Backend for why that matters once a login has +// pinned one. func NewAuthCommand() *cli.Command { return &cli.Command{ Name: "auth", Usage: "Manage authentication with the LFX platform", - Flags: []cli.Flag{ - &cli.BoolFlag{ - Name: insecureStorageFlagName, - Usage: "Store credentials in a plain (unencrypted) file instead of the system backend", - }, - &cli.StringFlag{ - Name: backendFlagName, - Usage: "Pin credential storage to a specific system backend (see `lfx auth backends`); mutually exclusive with --insecure-storage", - }, - }, Commands: []*cli.Command{ newAuthLoginCommand(), newAuthTokenCommand(), diff --git a/internal/commands/auth_test.go b/internal/commands/auth_test.go index a795d1b..078a7aa 100644 --- a/internal/commands/auth_test.go +++ b/internal/commands/auth_test.go @@ -15,18 +15,15 @@ import ( ) // newTestCommand builds a *cli.Command with the --insecure-storage and -// --backend flags registered (as newAuthLoginCommand and friends -// do), parses args against it, and returns the parsed *cli.Command handed -// to fn's Action so tests can read flag values the way the real commands -// do. +// --backend flags registered (as the root `lfx` command does via +// CredentialStoreFlags), parses args against it, and returns the parsed +// *cli.Command handed to fn's Action so tests can read flag values the +// way the real commands do. func newTestCommand(t *testing.T, args []string, fn func(cmd *cli.Command)) { t.Helper() cmd := &cli.Command{ - Name: "test", - Flags: []cli.Flag{ - &cli.BoolFlag{Name: insecureStorageFlagName}, - &cli.StringFlag{Name: backendFlagName}, - }, + Name: "test", + Flags: CredentialStoreFlags, Action: func(_ context.Context, cmd *cli.Command) error { fn(cmd) return nil From 5be4fb3f28eeeec824d9fb4e368f0c6c67e458af Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Thu, 27 Aug 2026 11:49:38 -0700 Subject: [PATCH 08/16] Address additional Copilot review feedback on lfx api - Write the response body verbatim via os.Stdout.Write instead of fmt.Println, which appended an extra newline byte not present in the original response, corrupting raw/binary output and files produced by redirecting lfx api's stdout. - Default Content-Type to application/json for POST/PUT requests with a body, unless the caller already set it via -H (or it was set by --field/--raw-field, which already produce JSON). LFX platform APIs are overwhelmingly JSON, so this avoids needing an explicit -H on the common --input/stdin body case. - Fix the README --input example, which sent a body with no Content-Type before this change. - Update AGENTS.md's stale "Current State" section, which still described lfx api as an unimplemented stub. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- AGENTS.md | 12 +++++++----- README.md | 2 +- internal/commands/api.go | 17 +++++++++++++++-- 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index ca03943..1d20caf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -46,11 +46,13 @@ lfx-cli/ ### Current State -`lfx auth login` / `status` / `token` / `logout` are fully implemented, -including the Auth0 Device Code flow, refresh-token exchange, and -credential storage (system keychain via `99designs/keyring`, with a plain -`--insecure-storage` fallback). `lfx api` remains a stub; its -implementation lands in follow-on work. +The CLI implements `lfx auth login` / `status` / `token` / `logout` +(Auth0 Device Code flow, refresh-token exchange, and credential storage +via the system keychain with `99designs/keyring`, or a plain +`--insecure-storage` fallback) and `lfx api` (raw authenticated calls +against LFX platform APIs, with custom methods/headers, JSON body +construction via `--field`/`--raw-field`, raw bodies via `--input`/stdin, +and response filtering via `--query`). **No container build**: this project produces binary artifacts only, distributed via GitHub Releases, the `install.sh` curl-style installer diff --git a/README.md b/README.md index 1086882..9d215a2 100644 --- a/README.md +++ b/README.md @@ -41,7 +41,7 @@ lfx auth logout # Make an authenticated call to an LFX platform API endpoint. lfx api '/my-grants?v=1&object_type=projects' lfx api /projects --field name=example # auto-promotes to POST -lfx api -X PUT /projects/123 --input - -H "If-Match: " < input.json +lfx api -X PUT /projects/123 --input - -H "If-Match: " < input.json # Content-Type: application/json is added automatically ``` Credentials (refresh token, cached access token) are stored in your diff --git a/internal/commands/api.go b/internal/commands/api.go index f8c7f0d..e7efb14 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -63,7 +63,7 @@ func NewAPICommand() *cli.Command { }, &cli.StringFlag{ Name: apiInputFlagName, - Usage: "Read the request body from a file, or '-' for stdin", + Usage: "Read the request body from a file, or '-' for stdin (Content-Type defaults to application/json for POST/PUT unless overridden with -H)", }, &cli.StringSliceFlag{ Name: apiFieldFlagName, @@ -148,6 +148,13 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { } req.Header.Set(strings.TrimSpace(key), strings.TrimSpace(value)) } + if len(body) > 0 && req.Header.Get("Content-Type") == "" && (method == http.MethodPost || method == http.MethodPut) { + // LFX platform APIs are overwhelmingly JSON, so default to that + // for any POST/PUT body unless the caller set Content-Type + // explicitly (via --field/--raw-field, which already produce + // JSON and set this above, or via -H, handled just above). + req.Header.Set("Content-Type", "application/json") + } resp, err := http.DefaultClient.Do(req) if err != nil { @@ -164,7 +171,13 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { if query := cmd.String(apiQueryFlagName); query != "" { output = []byte(gjson.GetBytes(respBody, query).String()) } - fmt.Println(string(output)) + // Write the response body verbatim rather than through fmt.Println, + // which would append an extra newline byte not present in the + // original response, corrupting raw/binary output and any files + // produced by redirecting `lfx api`'s stdout. + if _, err := os.Stdout.Write(output); err != nil { + return fmt.Errorf("write response body: %w", err) + } if resp.StatusCode < 200 || resp.StatusCode >= 300 { // Use cli.Exit's message (rather than writing directly to From b00127b7aa87ce748f9396935b36f6e28f1a89c1 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Thu, 27 Aug 2026 12:05:31 -0700 Subject: [PATCH 09/16] Address further Copilot review feedback on lfx api - Reject non-HTTPS base URLs (--hostname or a non-HTTPS --audience) before sending the bearer token, since the Authorization header would otherwise be transmitted in cleartext. Loopback hosts (localhost, 127.0.0.1, ::1) are exempted for local debugging. - Stream the response body directly to stdout when --query isn't set, instead of always buffering the full body in memory first; buffering is now only used when GJSON filtering requires it. - Add unit test coverage for the new apiRequireHTTPS helper. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api.go | 55 ++++++++++++++++++++++++++--------- internal/commands/api_test.go | 28 ++++++++++++++++++ 2 files changed, 69 insertions(+), 14 deletions(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index e7efb14..dd5c33e 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -12,6 +12,7 @@ import ( "fmt" "io" "net/http" + "net/url" "os" "regexp" "strings" @@ -122,6 +123,9 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { if baseURL == "" { return errors.New("no API base URL available; log in with `lfx auth login` or pass --hostname") } + if err := apiRequireHTTPS(baseURL); err != nil { + return err + } body, contentType, err := apiRequestBody(cmd) if err != nil { @@ -162,21 +166,24 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { } defer resp.Body.Close() - respBody, err := io.ReadAll(resp.Body) - if err != nil { - return fmt.Errorf("read response body: %w", err) - } - - output := respBody if query := cmd.String(apiQueryFlagName); query != "" { - output = []byte(gjson.GetBytes(respBody, query).String()) - } - // Write the response body verbatim rather than through fmt.Println, - // which would append an extra newline byte not present in the - // original response, corrupting raw/binary output and any files - // produced by redirecting `lfx api`'s stdout. - if _, err := os.Stdout.Write(output); err != nil { - return fmt.Errorf("write response body: %w", err) + // --query needs the whole body in memory to run gjson against + // it, so buffer and filter before writing. + respBody, err := io.ReadAll(resp.Body) + if err != nil { + return fmt.Errorf("read response body: %w", err) + } + output := []byte(gjson.GetBytes(respBody, query).String()) + if _, err := os.Stdout.Write(output); err != nil { + return fmt.Errorf("write response body: %w", err) + } + } else { + // Stream the body straight through rather than buffering it + // all in memory first, since this command explicitly supports + // redirecting its output for large or binary responses. + if _, err := io.Copy(os.Stdout, resp.Body); err != nil { + return fmt.Errorf("write response body: %w", err) + } } if resp.StatusCode < 200 || resp.StatusCode >= 300 { @@ -303,3 +310,23 @@ func apiJoinURL(base, path string) (string, error) { } return strings.TrimRight(base, "/") + "/" + strings.TrimLeft(path, "/"), nil } + +// apiRequireHTTPS rejects base URLs that would send the bearer token over +// a non-HTTPS connection, since the request's Authorization header is +// otherwise transmitted in cleartext. Loopback hosts are exempted, since +// http:// there is a common and low-risk way to point --hostname at a +// local mock server for debugging. +func apiRequireHTTPS(rawURL string) error { + parsed, err := url.Parse(rawURL) + if err != nil { + return fmt.Errorf("invalid base URL %q: %w", rawURL, err) + } + if parsed.Scheme == "https" { + return nil + } + switch parsed.Hostname() { + case "localhost", "127.0.0.1", "::1": + return nil + } + return fmt.Errorf("refusing to send credentials to non-HTTPS URL %q (use an https:// --hostname, or localhost/127.0.0.1 for local debugging)", rawURL) +} diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go index 59e0d97..b8664ee 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -91,6 +91,34 @@ func TestAPIJoinURL(t *testing.T) { }) } +func TestAPIRequireHTTPS(t *testing.T) { + tests := []struct { + name string + rawURL string + wantErr bool + }{ + {name: "https", rawURL: "https://api.example.com", wantErr: false}, + {name: "https with port", rawURL: "https://api.example.com:8443", wantErr: false}, + {name: "http rejected", rawURL: "http://api.example.com", wantErr: true}, + {name: "http localhost allowed", rawURL: "http://localhost:8080", wantErr: false}, + {name: "http 127.0.0.1 allowed", rawURL: "http://127.0.0.1:8080", wantErr: false}, + {name: "http ::1 allowed", rawURL: "http://[::1]:8080", wantErr: false}, + {name: "http on non-loopback hostname resembling localhost rejected", rawURL: "http://localhost.attacker.example", wantErr: true}, + {name: "invalid URL rejected", rawURL: "http://[::1", wantErr: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := apiRequireHTTPS(tc.rawURL) + if tc.wantErr && err == nil { + t.Fatalf("apiRequireHTTPS(%q): got nil error, want error", tc.rawURL) + } + if !tc.wantErr && err != nil { + t.Fatalf("apiRequireHTTPS(%q): got error %v, want nil", tc.rawURL, err) + } + }) + } +} + func TestAPIRequestBodyFields(t *testing.T) { newAPITestCommand(t, []string{ "--field", "name=example", From 2183d19d48e9f4b00cde79ca1d7cb0aea25d8094 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Thu, 27 Aug 2026 12:14:48 -0700 Subject: [PATCH 10/16] Address further Copilot review feedback on lfx api - Disable automatic redirect-following in the HTTP client used by runAPI: http.DefaultClient can preserve the Authorization header across a same-host redirect even when it downgrades from HTTPS to HTTP, which would bypass apiRequireHTTPS and leak the bearer token. 3xx responses are now reported like any other non-2xx status instead. - Reject an explicitly empty --hostname= rather than silently falling back to the login audience, so a script with an unset hostname variable fails loudly instead of hitting the default production API. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api.go | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index dd5c33e..d5c0426 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -117,6 +117,9 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { } baseURL := cmd.String(apiHostnameFlagName) + if cmd.IsSet(apiHostnameFlagName) && baseURL == "" { + return errors.New("--hostname was set to an empty value; omit the flag to use the login audience instead") + } if baseURL == "" { baseURL = audience } @@ -160,7 +163,17 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { req.Header.Set("Content-Type", "application/json") } - resp, err := http.DefaultClient.Do(req) + // Don't auto-follow redirects: http.DefaultClient can preserve the + // Authorization header across a same-host redirect even when it + // downgrades from HTTPS to HTTP, which would bypass + // apiRequireHTTPS and leak the bearer token. A 3xx response is + // instead reported like any other non-2xx status below. + client := &http.Client{ + CheckRedirect: func(_ *http.Request, _ []*http.Request) error { + return http.ErrUseLastResponse + }, + } + resp, err := client.Do(req) if err != nil { return fmt.Errorf("request failed: %w", err) } From 3833f799e9a10f8c707f27f9a6783a69b4b0a45c Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Fri, 28 Aug 2026 11:57:10 -0700 Subject: [PATCH 11/16] Address remaining PR review nits on lfx api - Check resp.Body.Close() error (fixes an errcheck failure surfaced by an updated local golangci-lint). - Use cmd.IsSet instead of a string-empty check in apiHasExplicitBody. - Use url.JoinPath instead of manual string concatenation in apiJoinURL: it normalizes dot-segments (../) and percent-encodes path segments, which the plain concat never did. - Rename the local url variable in runAPI to endpoint so it no longer shadows the imported net/url package. - Document the Header.Set-vs-Add and no-trailing-newline design choices flagged as nits, rather than changing the behavior. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api.go | 27 ++++++++++++++++++++------- 1 file changed, 20 insertions(+), 7 deletions(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index d5c0426..9a52677 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -135,12 +135,12 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { return err } - url, err := apiJoinURL(baseURL, path) + endpoint, err := apiJoinURL(baseURL, path) if err != nil { return err } - req, err := http.NewRequestWithContext(ctx, method, url, bytes.NewReader(body)) + req, err := http.NewRequestWithContext(ctx, method, endpoint, bytes.NewReader(body)) if err != nil { return fmt.Errorf("build request: %w", err) } @@ -153,6 +153,11 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { if !ok { return fmt.Errorf("invalid --%s %q (expected 'key:value')", apiHeaderFlagName, h) } + // Header.Set (rather than Add) is deliberate: it lets a + // repeated -H for the same key (e.g. an explicit + // "Authorization:" or "Content-Type:") cleanly override the + // value set above, at the cost of not accumulating multiple + // values for the same header name the way curl/gh api do. req.Header.Set(strings.TrimSpace(key), strings.TrimSpace(value)) } if len(body) > 0 && req.Header.Get("Content-Type") == "" && (method == http.MethodPost || method == http.MethodPut) { @@ -177,7 +182,9 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { if err != nil { return fmt.Errorf("request failed: %w", err) } - defer resp.Body.Close() + defer func() { + _ = resp.Body.Close() + }() if query := cmd.String(apiQueryFlagName); query != "" { // --query needs the whole body in memory to run gjson against @@ -187,6 +194,10 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { return fmt.Errorf("read response body: %w", err) } output := []byte(gjson.GetBytes(respBody, query).String()) + // Write output verbatim with no added trailing newline, + // matching `gh api -q` so a scalar result (e.g. a single + // field) can be captured cleanly by a shell command + // substitution without a stray newline. if _, err := os.Stdout.Write(output); err != nil { return fmt.Errorf("write response body: %w", err) } @@ -215,7 +226,7 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { // Used to decide whether the default --method should be promoted from GET; // see runAPI. func apiHasExplicitBody(cmd *cli.Command) bool { - return cmd.String(apiInputFlagName) != "" || + return cmd.IsSet(apiInputFlagName) || len(cmd.StringSlice(apiFieldFlagName)) > 0 || len(cmd.StringSlice(apiRawFieldFlagName)) > 0 } @@ -315,13 +326,15 @@ func coerceFieldValue(value string) any { return value } -// apiJoinURL joins base and path into a single URL, ensuring exactly one -// slash separates them. +// apiJoinURL joins base and path into a single URL. Unlike a plain string +// concatenation, url.JoinPath percent-encodes path segments and resolves +// traversal components such as "../", which matters since path can come +// from user input and base can come from a user-controlled --hostname. func apiJoinURL(base, path string) (string, error) { if base == "" { return "", errors.New("empty base URL") } - return strings.TrimRight(base, "/") + "/" + strings.TrimLeft(path, "/"), nil + return url.JoinPath(base, path) } // apiRequireHTTPS rejects base URLs that would send the bearer token over From 67af91760dbd106b1853f1f24693ad780bbc03eb Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Fri, 28 Aug 2026 11:59:40 -0700 Subject: [PATCH 12/16] Clarify that --hostname sends the bearer token to the named host Addresses @dealako's review question about the trust model for --hostname: it validates HTTPS but not that the host belongs to LFX, so it's an advanced escape hatch, not a safety net. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index 9a52677..446dad2 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -82,7 +82,7 @@ func NewAPICommand() *cli.Command { }, &cli.StringFlag{ Name: apiHostnameFlagName, - Usage: "Override the LFX API base URL (advanced; independent of the IdP domain)", + Usage: "Override the LFX API base URL (advanced; independent of the IdP domain). Sends your bearer token to whatever HTTPS host you name here, so only point it at a trusted LFX endpoint.", }, }, Action: runAPI, From bfb23c5ec9fbbffd9ec3d1c4cc35045faebc3a99 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Fri, 28 Aug 2026 12:05:31 -0700 Subject: [PATCH 13/16] Restrict --hostname to development-environment logins resolveAccessToken now also returns the login environment recorded at `lfx auth login --env=...`. runAPI uses it to reject --hostname (remote or loopback) unless the current login used --env=development, narrowing the blast radius of a leaked or misdirected prod/staging bearer token per @dealako's review question. This is a heuristic tied to how login is normally done (--env and --audience are set together), not a cryptographic guarantee about what the token itself is scoped to. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api.go | 20 ++++++++++++++++---- internal/commands/auth.go | 33 +++++++++++++++++---------------- 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index 446dad2..b80714a 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -82,7 +82,7 @@ func NewAPICommand() *cli.Command { }, &cli.StringFlag{ Name: apiHostnameFlagName, - Usage: "Override the LFX API base URL (advanced; independent of the IdP domain). Sends your bearer token to whatever HTTPS host you name here, so only point it at a trusted LFX endpoint.", + Usage: "Override the LFX API base URL (advanced; independent of the IdP domain). Requires a development-environment login (`lfx auth login --env=development`).", }, }, Action: runAPI, @@ -111,14 +111,26 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { return fmt.Errorf("invalid --%s %q (must be one of GET, POST, PUT, DELETE)", apiMethodFlagName, method) } - token, audience, err := resolveAccessToken(ctx, cmd) + token, audience, env, err := resolveAccessToken(ctx, cmd) if err != nil { return err } baseURL := cmd.String(apiHostnameFlagName) - if cmd.IsSet(apiHostnameFlagName) && baseURL == "" { - return errors.New("--hostname was set to an empty value; omit the flag to use the login audience instead") + if cmd.IsSet(apiHostnameFlagName) { + if baseURL == "" { + return errors.New("--hostname was set to an empty value; omit the flag to use the login audience instead") + } + // --hostname sends the live bearer token to whatever host is + // named, so it's restricted to development-environment logins + // (`lfx auth login --env=development`) to limit the blast + // radius of a leaked or misdirected prod/staging token. This is + // a heuristic tied to how login is normally done (--env and + // --audience are set together), not a cryptographic guarantee + // about what the token itself is scoped to. + if env != envDevelopment { + return fmt.Errorf("--%s requires a development-environment login (run `lfx auth login --%s=%s`); the current login's environment is %q", apiHostnameFlagName, envFlagName, envDevelopment, env) + } } if baseURL == "" { baseURL = audience diff --git a/internal/commands/auth.go b/internal/commands/auth.go index 009479d..79ef3a6 100644 --- a/internal/commands/auth.go +++ b/internal/commands/auth.go @@ -444,7 +444,7 @@ func newAuthTokenCommand() *cli.Command { Name: "token", Usage: "Print a valid access token for the LFX platform", Action: func(ctx context.Context, cmd *cli.Command) error { - token, _, err := resolveAccessToken(ctx, cmd) + token, _, _, err := resolveAccessToken(ctx, cmd) if err != nil { return err } @@ -456,18 +456,19 @@ func newAuthTokenCommand() *cli.Command { // resolveAccessToken returns a valid access token for the current login, // refreshing it (and persisting the refreshed credentials) if the cached -// one is missing or expired. It also returns the audience recorded at -// login time, so callers (e.g. `lfx api`) can use it as their default API -// base URL. Both `lfx auth token` and `lfx api` share this single code -// path so their refresh, error, and credential-persistence behavior never -// drifts apart. -func resolveAccessToken(ctx context.Context, cmd *cli.Command) (token, audience string, err error) { +// one is missing or expired. It also returns the audience and login +// environment recorded at login time, so callers (e.g. `lfx api`) can use +// the audience as their default API base URL and the environment to gate +// features like --hostname to non-production logins. Both `lfx auth +// token` and `lfx api` share this single code path so their refresh, +// error, and credential-persistence behavior never drifts apart. +func resolveAccessToken(ctx context.Context, cmd *cli.Command) (token, audience string, env authEnvironment, err error) { store, creds, found, err := loadStoredCredentials(cmd) if err != nil { - return "", "", err + return "", "", "", err } if !found { - return "", "", errors.New("not logged in; run `lfx auth login` first") + return "", "", "", errors.New("not logged in; run `lfx auth login` first") } // Validate the persisted device state (insecure-storage and @@ -478,15 +479,15 @@ func resolveAccessToken(ctx context.Context, cmd *cli.Command) (token, audience // pin. state, domain, clientID, err := loadDeviceStateForBackend(store, cmd) if err != nil { - return "", "", fmt.Errorf("load device state: %w", err) + return "", "", "", fmt.Errorf("load device state: %w", err) } if creds.ValidAccessToken() { - return creds.AccessToken, state.Audience, nil + return creds.AccessToken, state.Audience, authEnvironment(state.Environment), nil } if creds.RefreshToken == "" { - return "", "", errors.New("no refresh token available; run `lfx auth login` again") + return "", "", "", errors.New("no refresh token available; run `lfx auth login` again") } cfg := &oauth2.Config{ @@ -501,10 +502,10 @@ func resolveAccessToken(ctx context.Context, cmd *cli.Command) (token, audience refreshed, err := cfg.TokenSource(ctx, &oauth2.Token{RefreshToken: creds.RefreshToken}).Token() var retrieveErr *oauth2.RetrieveError if errors.As(err, &retrieveErr) && retrieveErr.ErrorCode == "invalid_grant" { - return "", "", errors.New("session expired or revoked; run `lfx auth login` to log in again") + return "", "", "", errors.New("session expired or revoked; run `lfx auth login` to log in again") } if err != nil { - return "", "", fmt.Errorf("refresh access token: %w", err) + return "", "", "", fmt.Errorf("refresh access token: %w", err) } refreshToken := refreshed.RefreshToken @@ -518,10 +519,10 @@ func resolveAccessToken(ctx context.Context, cmd *cli.Command) (token, audience AccessToken: refreshed.AccessToken, AccessTokenExpiry: refreshed.Expiry, }); err != nil { - return "", "", fmt.Errorf("save refreshed credentials: %w", err) + return "", "", "", fmt.Errorf("save refreshed credentials: %w", err) } - return refreshed.AccessToken, state.Audience, nil + return refreshed.AccessToken, state.Audience, authEnvironment(state.Environment), nil } func newAuthStatusCommand() *cli.Command { From 450baaea966e513661b5fe5375f89d226b70b664 Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Fri, 28 Aug 2026 12:10:57 -0700 Subject: [PATCH 14/16] Fix query-string loss in apiJoinURL and empty --input= handling - apiJoinURL now parses path as a URL reference and reattaches its RawQuery/Fragment after joining, since url.JoinPath treats its second argument as a bare pathname and would otherwise percent-encode a query string (e.g. "?v=1&object_type=projects") straight into the path, dropping the query entirely. - apiRequestBody now uses cmd.IsSet(apiInputFlagName) instead of a string-empty check, so an explicitly empty --input= is rejected (and correctly participates in the --input/--field mutual-exclusion check) instead of silently falling through to implicit stdin handling. Addresses two Copilot findings from the latest review pass. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api.go | 32 +++++++++++++++++++++++++++----- internal/commands/api_test.go | 19 +++++++++++++++++++ 2 files changed, 46 insertions(+), 5 deletions(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index b80714a..6ff3cb9 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -248,15 +248,19 @@ func apiHasExplicitBody(cmd *cli.Command) bool { // --input vs. --field/--raw-field), falling back to stdin when none are // passed and stdin is piped (non-TTY). func apiRequestBody(cmd *cli.Command) (body []byte, contentType string, err error) { + inputSet := cmd.IsSet(apiInputFlagName) input := cmd.String(apiInputFlagName) fields := cmd.StringSlice(apiFieldFlagName) rawFields := cmd.StringSlice(apiRawFieldFlagName) - if input != "" && (len(fields) > 0 || len(rawFields) > 0) { + if inputSet && (len(fields) > 0 || len(rawFields) > 0) { return nil, "", fmt.Errorf("--%s cannot be combined with --%s or --%s", apiInputFlagName, apiFieldFlagName, apiRawFieldFlagName) } - if input != "" { + if inputSet { + if input == "" { + return nil, "", fmt.Errorf("--%s was set to an empty value; omit the flag to read the body from stdin instead", apiInputFlagName) + } if input == "-" { data, err := io.ReadAll(os.Stdin) if err != nil { @@ -338,15 +342,33 @@ func coerceFieldValue(value string) any { return value } -// apiJoinURL joins base and path into a single URL. Unlike a plain string -// concatenation, url.JoinPath percent-encodes path segments and resolves +// apiJoinURL joins base and path into a single URL. path is parsed as a +// URL reference first so its query string and fragment (e.g. +// "/projects?limit=10") are preserved rather than being percent-encoded +// into the path by url.JoinPath, which only understands its second +// argument as a pathname. Only the parsed path component is joined with +// base -- url.JoinPath percent-encodes path segments and resolves // traversal components such as "../", which matters since path can come // from user input and base can come from a user-controlled --hostname. func apiJoinURL(base, path string) (string, error) { if base == "" { return "", errors.New("empty base URL") } - return url.JoinPath(base, path) + ref, err := url.Parse(path) + if err != nil { + return "", fmt.Errorf("invalid path %q: %w", path, err) + } + joined, err := url.JoinPath(base, ref.Path) + if err != nil { + return "", err + } + full, err := url.Parse(joined) + if err != nil { + return "", err + } + full.RawQuery = ref.RawQuery + full.Fragment = ref.Fragment + return full.String(), nil } // apiRequireHTTPS rejects base URLs that would send the bearer token over diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go index b8664ee..b5bf94a 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -71,6 +71,9 @@ func TestAPIJoinURL(t *testing.T) { {name: "trailing slash on base", base: "https://api.example.com/", path: "projects", want: "https://api.example.com/projects"}, {name: "leading slash on path", base: "https://api.example.com", path: "/projects", want: "https://api.example.com/projects"}, {name: "both slashes", base: "https://api.example.com/", path: "/projects", want: "https://api.example.com/projects"}, + {name: "query string preserved", base: "https://api.example.com", path: "/my-grants?v=1&object_type=projects", want: "https://api.example.com/my-grants?v=1&object_type=projects"}, + {name: "fragment preserved", base: "https://api.example.com", path: "/projects#frag", want: "https://api.example.com/projects#frag"}, + {name: "dot-segments resolved", base: "https://api.example.com", path: "../secret", want: "https://api.example.com/secret"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -201,6 +204,22 @@ func TestAPIRequestBodyInputRejectsCombiningWithFields(t *testing.T) { }) } +func TestAPIRequestBodyRejectsExplicitEmptyInput(t *testing.T) { + newAPITestCommand(t, []string{"--input="}, func(cmd *cli.Command) { + if _, _, err := apiRequestBody(cmd); err == nil { + t.Fatal("apiRequestBody: got nil error, want error for explicit empty --input=") + } + }) +} + +func TestAPIRequestBodyExplicitEmptyInputRejectsCombiningWithFields(t *testing.T) { + newAPITestCommand(t, []string{"--input=", "--field", "name=example"}, func(cmd *cli.Command) { + if _, _, err := apiRequestBody(cmd); err == nil { + t.Fatal("apiRequestBody: got nil error, want error for explicit empty --input= combined with --field") + } + }) +} + func TestAPIHasExplicitBody(t *testing.T) { tests := []struct { name string From 8cf2701c2716d77a9d6fef4fa5770014f757659e Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Fri, 28 Aug 2026 12:22:26 -0700 Subject: [PATCH 15/16] Revert apiJoinURL to simple concatenation; fix scheme case-sensitivity apiJoinURL's url.JoinPath-based version had accumulated three distinct correctness bugs across two review rounds (query-string loss, escaped- slash unescaping, and the underlying complexity risk of a third): the simple TrimRight/TrimLeft concatenation never had any of them, since it passes path through byte-for-byte instead of round-tripping it through url.Parse/url.JoinPath. Reverting to it, per @andrest50/@dealako's original review consensus that the plain version was acceptable (hardening nit, not a defect), especially now that --hostname is gated to development-environment logins rather than relying on URL sanitization as the only safeguard. Also fixed apiRequireHTTPS's scheme comparison to be case-insensitive (strings.EqualFold), so "HTTPS://..." is accepted like "https://...". Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api.go | 34 ++++++++++------------------------ internal/commands/api_test.go | 3 ++- 2 files changed, 12 insertions(+), 25 deletions(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index 6ff3cb9..584eb09 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -342,33 +342,19 @@ func coerceFieldValue(value string) any { return value } -// apiJoinURL joins base and path into a single URL. path is parsed as a -// URL reference first so its query string and fragment (e.g. -// "/projects?limit=10") are preserved rather than being percent-encoded -// into the path by url.JoinPath, which only understands its second -// argument as a pathname. Only the parsed path component is joined with -// base -- url.JoinPath percent-encodes path segments and resolves -// traversal components such as "../", which matters since path can come -// from user input and base can come from a user-controlled --hostname. +// apiJoinURL joins base and path into a single URL, ensuring exactly one +// slash separates them. path is passed through unmodified (aside from a +// leading-slash trim) rather than parsed/re-escaped: base is already +// trusted (the login audience, or --hostname, which is gated to +// development-environment logins in runAPI) and path is meant to be sent +// verbatim, including its query string, exactly as the caller wrote it -- +// a URL-parsing round trip risks subtly rewriting it (see the query +// string and escaped-slash regressions caught in review). func apiJoinURL(base, path string) (string, error) { if base == "" { return "", errors.New("empty base URL") } - ref, err := url.Parse(path) - if err != nil { - return "", fmt.Errorf("invalid path %q: %w", path, err) - } - joined, err := url.JoinPath(base, ref.Path) - if err != nil { - return "", err - } - full, err := url.Parse(joined) - if err != nil { - return "", err - } - full.RawQuery = ref.RawQuery - full.Fragment = ref.Fragment - return full.String(), nil + return strings.TrimRight(base, "/") + "/" + strings.TrimLeft(path, "/"), nil } // apiRequireHTTPS rejects base URLs that would send the bearer token over @@ -381,7 +367,7 @@ func apiRequireHTTPS(rawURL string) error { if err != nil { return fmt.Errorf("invalid base URL %q: %w", rawURL, err) } - if parsed.Scheme == "https" { + if strings.EqualFold(parsed.Scheme, "https") { return nil } switch parsed.Hostname() { diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go index b5bf94a..4aa4ae0 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -73,7 +73,7 @@ func TestAPIJoinURL(t *testing.T) { {name: "both slashes", base: "https://api.example.com/", path: "/projects", want: "https://api.example.com/projects"}, {name: "query string preserved", base: "https://api.example.com", path: "/my-grants?v=1&object_type=projects", want: "https://api.example.com/my-grants?v=1&object_type=projects"}, {name: "fragment preserved", base: "https://api.example.com", path: "/projects#frag", want: "https://api.example.com/projects#frag"}, - {name: "dot-segments resolved", base: "https://api.example.com", path: "../secret", want: "https://api.example.com/secret"}, + {name: "encoded slash preserved verbatim", base: "https://api.example.com", path: "/objects/a%2Fb", want: "https://api.example.com/objects/a%2Fb"}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { @@ -101,6 +101,7 @@ func TestAPIRequireHTTPS(t *testing.T) { wantErr bool }{ {name: "https", rawURL: "https://api.example.com", wantErr: false}, + {name: "https uppercase scheme", rawURL: "HTTPS://api.example.com", wantErr: false}, {name: "https with port", rawURL: "https://api.example.com:8443", wantErr: false}, {name: "http rejected", rawURL: "http://api.example.com", wantErr: true}, {name: "http localhost allowed", rawURL: "http://localhost:8080", wantErr: false}, From 3cb2cbd9f54593984524d98bd65a9f5b594f713d Mon Sep 17 00:00:00 2001 From: Eric Searcy Date: Fri, 28 Aug 2026 12:32:45 -0700 Subject: [PATCH 16/16] Clarify rationale for the --hostname development-environment gate The restriction exists to keep a prod/staging token -- the ones with real authority -- from being redirected to a third-party host at all; --hostname's troubleshooting use case only comes up in development. Allowing it there is independently safe because OAuth2 resource servers validate a token's issuer, not just its audience, so even a development-issued token could never be honored by prod/staging. Previous wording conflated these two points. Assisted-by: github-copilot:claude-sonnet-5 Signed-off-by: Eric Searcy --- internal/commands/api.go | 17 +++++++++++------ internal/commands/environment.go | 10 ++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index 584eb09..58aa4c3 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -122,12 +122,17 @@ func runAPI(ctx context.Context, cmd *cli.Command) error { return errors.New("--hostname was set to an empty value; omit the flag to use the login audience instead") } // --hostname sends the live bearer token to whatever host is - // named, so it's restricted to development-environment logins - // (`lfx auth login --env=development`) to limit the blast - // radius of a leaked or misdirected prod/staging token. This is - // a heuristic tied to how login is normally done (--env and - // --audience are set together), not a cryptographic guarantee - // about what the token itself is scoped to. + // named. The purpose of this restriction is to keep a prod or + // staging token -- the ones with real authority -- from being + // redirected to a third-party host at all; --hostname's actual + // use case (pointing at a local mock server or alternate + // deployment for advanced troubleshooting) only comes up in + // development anyway. Allowing it there is also independently + // safe: OAuth2 resource servers validate a token's issuer + // (`iss`), not just its audience, against the specific IdP(s) + // they trust, so even in the hypothetical where a development + // login held a token claiming the prod audience, prod would + // still reject it as issued by an untrusted IdP. if env != envDevelopment { return fmt.Errorf("--%s requires a development-environment login (run `lfx auth login --%s=%s`); the current login's environment is %q", apiHostnameFlagName, envFlagName, envDevelopment, env) } diff --git a/internal/commands/environment.go b/internal/commands/environment.go index 04a4667..dfd49da 100644 --- a/internal/commands/environment.go +++ b/internal/commands/environment.go @@ -30,6 +30,16 @@ const ( // since this CLI never calls the Auth0 Management API (only the device // code and token endpoints), there's no need to separately track the // underlying tenant name. +// +// `runAPI`'s --hostname gate (api.go) restricts redirecting the API base +// URL to development-environment logins: the goal is to keep a prod or +// staging token -- the ones with real authority -- from ever being sent +// to a third-party host, since --hostname's troubleshooting use case only +// comes up in development anyway. That restriction is also independently +// safe, since each tenant here is a distinct OAuth2 issuer and resource +// servers validate a token's issuer (`iss`), not just its audience: even +// a development-issued token claiming the prod audience would still be +// rejected by prod as untrusted. var authDomains = map[authEnvironment]string{ envProd: "sso.linuxfoundation.org", envStaging: "linuxfoundation-staging.auth0.com",