From 6f50b4df4087e2e822d582cfbe555e274762cfb2 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" Date: Fri, 15 May 2026 15:27:02 +0200 Subject: [PATCH 1/2] JGC-437 - Enable JSON error output for http commands --- general/api/cli.go | 17 ++- general/api/cli_test.go | 81 +++++++++++++ go.mod | 2 +- go.sum | 6 +- main.go | 14 +++ utils/cliutils/cli_consts.go | 9 ++ utils/cliutils/errorformat.go | 151 +++++++++++++++++++++++ utils/cliutils/errorformat_test.go | 188 +++++++++++++++++++++++++++++ 8 files changed, 464 insertions(+), 4 deletions(-) create mode 100644 utils/cliutils/errorformat.go create mode 100644 utils/cliutils/errorformat_test.go diff --git a/general/api/cli.go b/general/api/cli.go index 7a00f24ed..a4f6c5735 100644 --- a/general/api/cli.go +++ b/general/api/cli.go @@ -185,6 +185,21 @@ func exchangeAndPrint(client *httpclient.HttpClient, c commandContext, method, f log.Info("Http Status:", resp.StatusCode) + isError := resp.StatusCode < 200 || resp.StatusCode > 399 + + // Opt-in (JFROG_CLI_ERROR_OUTPUT_FORMAT=json, or --format=json auto-promote): + // for HTTP error responses, emit the response as a structured JSON object on + // stdout — the same data channel where the successful body would have gone — + // and skip dumping the raw body. Stderr keeps its log lines untouched, so + // `jf api ... | jq` works in both success and error cases. + if isError && cliutils.HandleHTTPErrorAsJSON(stdOut, &errorutils.HttpResponseError{ + StatusCode: resp.StatusCode, + Status: resp.Status, + Body: respBody, + }) { + return cli.NewExitError("", 1) + } + if _, err = stdOut.Write(respBody); err != nil { return errorutils.CheckError(err) } @@ -194,7 +209,7 @@ func exchangeAndPrint(client *httpclient.HttpClient, c commandContext, method, f } } - if resp.StatusCode < 200 || resp.StatusCode > 399 { + if isError { log.Warn("jf api:", method, fullURL, "returned", resp.Status) // Exit code only: a non-empty ExitError message would be printed again by urfave/cli's // HandleExitCoder after the response body when stdout and stderr are combined. diff --git a/general/api/cli_test.go b/general/api/cli_test.go index c47d5820e..83df3e5de 100644 --- a/general/api/cli_test.go +++ b/general/api/cli_test.go @@ -2,6 +2,7 @@ package api import ( "bytes" + "encoding/json" "fmt" "net/http" "net/http/httptest" @@ -777,3 +778,83 @@ func TestRunApiCmd_UsageReportCompletes(t *testing.T) { // Reporter signals immediately, so the call should not approach the timeout. assert.Less(t, time.Since(start), 2*time.Second) } + +// newTestServerWithStatus starts an httptest server returning the given status +// and body. Used to drive the HTTP-error code path in `jf api`. +func newTestServerWithStatus(t *testing.T, status int, body []byte, contentType string) *coreConfig.ServerDetails { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if contentType != "" { + w.Header().Set("Content-Type", contentType) + } + w.WriteHeader(status) + if _, err := w.Write(body); err != nil { + t.Log(err) + } + })) + t.Cleanup(srv.Close) + return &coreConfig.ServerDetails{Url: srv.URL, AccessToken: "my-token"} +} + +// TestApiJSONErrorMode_EmitsJSONOnStdout verifies that when the opt-in +// JFROG_CLI_ERROR_OUTPUT_FORMAT=json env var is set, `jf api` emits a +// structured JSON object describing the HTTP error on stdout (the data +// channel) rather than the raw body. Stderr keeps its log lines untouched. +func TestApiJSONErrorMode_EmitsJSONOnStdout(t *testing.T) { + t.Setenv("JFROG_CLI_ERROR_OUTPUT_FORMAT", "json") + + body := []byte(`{"errors":[{"code":"UNAUTHORIZED","message":"bad creds"}]}`) + serverDetails := newTestServerWithStatus(t, http.StatusUnauthorized, body, "application/json") + ctx := newMockContext(&commandArgs{path: "/unauthorized"}) + + var stdOut bytes.Buffer + err := runApiCmd(ctx, serverDetails, &stdOut, nil) + + // Must signal failure (urfave/cli ExitError with code 1, empty message). + assert.Error(t, err) + + // stdout must contain a parseable JSON object with the structured fields — + // not the raw response body as in legacy mode. + var out map[string]interface{} + require.NoError(t, json.Unmarshal(stdOut.Bytes(), &out), "stdout must be parseable JSON") + assert.EqualValues(t, http.StatusUnauthorized, out["status_code"]) + assert.Equal(t, "401 Unauthorized", out["status"]) + bodyOut, ok := out["body"].(map[string]interface{}) + require.True(t, ok, "body field must be a nested JSON object") + errorsArr, ok := bodyOut["errors"].([]interface{}) + require.True(t, ok) + assert.NotEmpty(t, errorsArr) +} + +// TestApiJSONErrorMode_SuccessUnchanged verifies that 2xx responses still +// write the body to stdout even when the env var is set — JSON mode only +// changes the error path. +func TestApiJSONErrorMode_SuccessUnchanged(t *testing.T) { + t.Setenv("JFROG_CLI_ERROR_OUTPUT_FORMAT", "json") + + body := []byte(`{"ok":true}`) + serverDetails := newTestServerWithStatus(t, http.StatusOK, body, "application/json") + ctx := newMockContext(&commandArgs{path: "/ok"}) + + var stdOut bytes.Buffer + require.NoError(t, runApiCmd(ctx, serverDetails, &stdOut, nil)) + assert.Equal(t, string(body), strings.TrimSpace(stdOut.String())) +} + +// TestApiDefaultMode_ErrorStillDumpsBody guards against regressing the legacy +// curl-like behavior: with the env var unset, errors still dump the body to +// stdout and the command exits non-zero. +func TestApiDefaultMode_ErrorStillDumpsBody(t *testing.T) { + t.Setenv("JFROG_CLI_ERROR_OUTPUT_FORMAT", "") + + // 4xx is returned as a non-retried response; 5xx would trip the client's + // retry loop and never reach the body-write branch under test. + body := []byte(`{"errors":["nope"]}`) + serverDetails := newTestServerWithStatus(t, http.StatusUnauthorized, body, "application/json") + ctx := newMockContext(&commandArgs{path: "/unauthorized-default"}) + + var stdOut bytes.Buffer + err := runApiCmd(ctx, serverDetails, &stdOut, nil) + assert.Error(t, err) + assert.Equal(t, string(body), strings.TrimSpace(stdOut.String())) +} diff --git a/go.mod b/go.mod index f1607e9eb..690482949 100644 --- a/go.mod +++ b/go.mod @@ -24,7 +24,7 @@ require ( github.com/jfrog/jfrog-cli-evidence v0.9.4 github.com/jfrog/jfrog-cli-platform-services v1.10.1-0.20260430094150-ce7d9b371c6f github.com/jfrog/jfrog-cli-security v1.29.0 - github.com/jfrog/jfrog-client-go v1.55.1-0.20260521115926-32f082854b39 + github.com/jfrog/jfrog-client-go v1.55.1-0.20260522071027-8b60a715d6e4 github.com/jszwec/csvutil v1.10.0 github.com/spf13/viper v1.21.0 github.com/stretchr/testify v1.11.1 diff --git a/go.sum b/go.sum index f4eebaf00..9fdb26b64 100644 --- a/go.sum +++ b/go.sum @@ -428,8 +428,10 @@ github.com/jfrog/jfrog-cli-platform-services v1.10.1-0.20260430094150-ce7d9b371c github.com/jfrog/jfrog-cli-platform-services v1.10.1-0.20260430094150-ce7d9b371c6f/go.mod h1:JUdq/dQoNscpta62FDCAcaSVbvcCOr5VkH8UeGTG1HQ= github.com/jfrog/jfrog-cli-security v1.29.0 h1:TN2OCA5i/iPbikQWzSwVqGvySvIvw1P6rPga+DbVBOI= github.com/jfrog/jfrog-cli-security v1.29.0/go.mod h1:q38TPlxortIJvbyD3u9P9UhHwyx007tEb9WbXlXw2E0= -github.com/jfrog/jfrog-client-go v1.55.1-0.20260521115926-32f082854b39 h1:cXWtxiTOWFha3yBWh6FDxr0qCNVd0Q40rB/rB+bU3eY= -github.com/jfrog/jfrog-client-go v1.55.1-0.20260521115926-32f082854b39/go.mod h1:k3PqoFpS6XDt9/4xg3pS8J8JUvxtaz1w2vdTdodknGk= +github.com/jfrog/jfrog-client-go v1.55.1-0.20260518090239-a4f67e2bd8cb h1:ew1foDyxCyqKbSz06ybrrHruf4OIbhti/YLwaa0J8dI= +github.com/jfrog/jfrog-client-go v1.55.1-0.20260518090239-a4f67e2bd8cb/go.mod h1:sCE06+GngPoyrGO0c+vmhgMoVSP83UMNiZnIuNPzU8U= +github.com/jfrog/jfrog-client-go v1.55.1-0.20260522071027-8b60a715d6e4 h1:ujVu255rk51l9Uz1t75DdsVoa2MH+lYNV2cB2xDWjPM= +github.com/jfrog/jfrog-client-go v1.55.1-0.20260522071027-8b60a715d6e4/go.mod h1:k3PqoFpS6XDt9/4xg3pS8J8JUvxtaz1w2vdTdodknGk= github.com/jhump/protoreflect v1.17.0 h1:qOEr613fac2lOuTgWN4tPAtLL7fUSbuJL5X5XumQh94= github.com/jhump/protoreflect v1.17.0/go.mod h1:h9+vUUL38jiBzck8ck+6G/aeMX8Z4QUY/NiJPwPNi+8= github.com/jmespath/go-jmespath v0.4.1-0.20220621161143-b0104c826a24 h1:liMMTbpW34dhU4az1GN0pTPADwNmvoRSeoZ6PItiqnY= diff --git a/main.go b/main.go index 0a5dbb7d9..198bdf4e9 100644 --- a/main.go +++ b/main.go @@ -79,6 +79,15 @@ func main() { if cleanupErr := fileutils.CleanOldDirs(); cleanupErr != nil { clientlog.Warn("failed while attempting to cleanup old CLI temp directories:", cleanupErr) } + // Opt-in (JFROG_CLI_ERROR_OUTPUT_FORMAT=json, or --format=json auto-promoted + // in execMain): emit HTTP response errors as structured JSON on stdout — + // the data channel — so scripts can pipe to jq. Stderr continues to receive + // the existing logger output (Info/Warn lines, trace ID) unchanged. Covers + // every command that hits the platform via the standard jfrog-client-go + // helpers, including OIDC token-exchange failures. + if cliutils.HandleHTTPErrorAsJSON(os.Stdout, err) { + os.Exit(coreutils.GetExitCode(err, 0, 0, false).Code) + } coreutils.ExitOnErr(err) } @@ -97,6 +106,11 @@ func execMain() error { app.Version = cliutils.GetVersion() args := os.Args cliutils.SetCliExecutableName(args[0]) + // Auto-promote --format=json (already supported by many commands and by + // commands.Exec subcommands) to JFROG_CLI_ERROR_OUTPUT_FORMAT=json so that + // HTTP error responses are emitted as JSON on stderr without requiring a + // second env var. Explicit env var wins. + cliutils.EnableJSONErrorIfFormatJSON(args) app.EnableBashCompletion = true commands, err := getCommands() if err != nil { diff --git a/utils/cliutils/cli_consts.go b/utils/cliutils/cli_consts.go index c709e53ba..2b20cafde 100644 --- a/utils/cliutils/cli_consts.go +++ b/utils/cliutils/cli_consts.go @@ -28,4 +28,13 @@ const ( //#nosec G101 JfrogCliGithubToken = "JFROG_CLI_GITHUB_TOKEN" JfrogCliHideSurvey = "JFROG_CLI_HIDE_SURVEY" + // JfrogCliErrorOutputFormat controls how HTTP response errors are surfaced. + // Set to "json" to emit the structured response (status code + body) as JSON + // on stderr instead of the default human-readable text. Unset or "text" keeps + // the legacy behavior. Applies uniformly to all commands, including OIDC + // token-exchange failures. + JfrogCliErrorOutputFormat = "JFROG_CLI_ERROR_OUTPUT_FORMAT" ) + +// ErrorFormatJSON is the env-var value that switches HTTP error reporting to JSON-on-stderr. +const ErrorFormatJSON = "json" diff --git a/utils/cliutils/errorformat.go b/utils/cliutils/errorformat.go new file mode 100644 index 000000000..1e611490c --- /dev/null +++ b/utils/cliutils/errorformat.go @@ -0,0 +1,151 @@ +package cliutils + +import ( + "encoding/json" + "errors" + "fmt" + "io" + "os" + "strconv" + "strings" + + "github.com/jfrog/jfrog-client-go/utils/errorutils" +) + +// legacyHTTPErrPrefix matches the text produced by errorutils.HttpResponseError.Error() +// (and historically by GenerateResponseError). When intermediate code wraps the +// typed error in a way that strips the chain (e.g. errors.New(err.Error() + ...)), +// we fall back to parsing this prefix so that JSON emission still applies. +const legacyHTTPErrPrefix = "server response: " + +// EnableJSONErrorIfFormatJSON inspects args for "--format=json" / "--format json" +// (or the single-dash "-format" variant) and, when found, sets +// JFROG_CLI_ERROR_OUTPUT_FORMAT=json for the lifetime of this process. +// +// Rationale: any command (direct or one dispatched via commands.Exec) that +// already accepts --format=json for its success output should produce +// machine-readable JSON for HTTP error responses too, so a single flag covers +// both code paths and scripts can rely on `jq` end-to-end. +// +// Must be called early in main() — the env var is read by HandleHTTPErrorAsJSON, +// which fires both during command execution (e.g. `jf api`) and at process exit. +// An explicit env var setting wins: if JFROG_CLI_ERROR_OUTPUT_FORMAT is already +// set, the function is a no-op. +func EnableJSONErrorIfFormatJSON(args []string) { + if os.Getenv(JfrogCliErrorOutputFormat) != "" { + return + } + for i, a := range args { + if val, ok := matchFormatEquals(a); ok && strings.EqualFold(val, ErrorFormatJSON) { + _ = os.Setenv(JfrogCliErrorOutputFormat, ErrorFormatJSON) + return + } + if (a == "--format" || a == "-format") && i+1 < len(args) && + strings.EqualFold(args[i+1], ErrorFormatJSON) { + _ = os.Setenv(JfrogCliErrorOutputFormat, ErrorFormatJSON) + return + } + } +} + +func matchFormatEquals(a string) (string, bool) { + for _, prefix := range []string{"--format=", "-format="} { + if strings.HasPrefix(a, prefix) { + return a[len(prefix):], true + } + } + return "", false +} + +// HandleHTTPErrorAsJSON emits err as a JSON object to w when: +// +// 1. JFROG_CLI_ERROR_OUTPUT_FORMAT is set to "json", and +// 2. err wraps a *errorutils.HttpResponseError (the typed error returned by +// jfrog-client-go's CheckResponseStatus / CheckResponseStatusWithBody), +// or its legacy "server response: ..." text equivalent. +// +// w is expected to be the caller's stdout sink: the JSON object is *data* a +// script consumer wants to parse alongside successful command output. Stderr +// continues to carry the human-readable logger output (Info/Warn/Error lines) +// untouched, which a downstream `| jq` pipeline can simply ignore. +// +// Returns true when JSON was emitted. The caller is then responsible for setting +// the process exit code and suppressing any subsequent text logging of the same +// error. Returns false in every other case (env var unset/other value, err nil, +// or err is not an HTTP response error), so callers can fall through to the +// default text reporting path (which uses stderr). +// +// This covers all command-paths that use the standard jfrog-client-go HTTP +// helpers, including OIDC token exchange (errors are wrapped with %w so +// errors.As walks through them). +func HandleHTTPErrorAsJSON(w io.Writer, err error) bool { + if err == nil { + return false + } + if !strings.EqualFold(strings.TrimSpace(os.Getenv(JfrogCliErrorOutputFormat)), ErrorFormatJSON) { + return false + } + httpErr := extractHTTPResponseError(err) + if httpErr == nil { + return false + } + payload := map[string]interface{}{ + "status_code": httpErr.StatusCode, + "status": httpErr.Status, + } + if len(httpErr.Body) > 0 { + var parsed interface{} + if json.Unmarshal(httpErr.Body, &parsed) == nil { + payload["body"] = parsed + } else { + payload["body"] = string(httpErr.Body) + } + } + encoded, marshalErr := json.MarshalIndent(payload, "", " ") + if marshalErr != nil { + return false + } + _, _ = fmt.Fprintln(w, string(encoded)) + return true +} + +// extractHTTPResponseError returns the underlying *errorutils.HttpResponseError +// if err wraps one (via errors.As). When no typed error is found, it falls back +// to parsing the legacy "server response: \n" text format produced +// by errorutils.GenerateResponseError. The fallback handles cases where +// intermediate code stripped the wrap chain (e.g. errors.New(err.Error() + ...)) +// while preserving the human-readable message. +func extractHTTPResponseError(err error) *errorutils.HttpResponseError { + var httpErr *errorutils.HttpResponseError + if errors.As(err, &httpErr) && httpErr != nil { + return httpErr + } + return parseLegacyHTTPResponseError(err.Error()) +} + +func parseLegacyHTTPResponseError(msg string) *errorutils.HttpResponseError { + idx := strings.Index(msg, legacyHTTPErrPrefix) + if idx < 0 { + return nil + } + rest := msg[idx+len(legacyHTTPErrPrefix):] + statusLine, bodyStr, _ := strings.Cut(rest, "\n") + statusLine = strings.TrimSpace(statusLine) + + // First token must be a numeric status code (e.g. "401 Unauthorized"). + // Cap the digit count so Atoi cannot overflow on malformed input — real + // HTTP statuses are 3 digits; 9 leaves plenty of headroom under int32. + codeEnd := 0 + for codeEnd < len(statusLine) && codeEnd < 9 && statusLine[codeEnd] >= '0' && statusLine[codeEnd] <= '9' { + codeEnd++ + } + if codeEnd == 0 { + return nil + } + code, _ := strconv.Atoi(statusLine[:codeEnd]) + return &errorutils.HttpResponseError{ + StatusCode: code, + Status: statusLine, + Body: []byte(strings.TrimRight(bodyStr, "\n")), + } +} diff --git a/utils/cliutils/errorformat_test.go b/utils/cliutils/errorformat_test.go new file mode 100644 index 000000000..555a16a55 --- /dev/null +++ b/utils/cliutils/errorformat_test.go @@ -0,0 +1,188 @@ +package cliutils + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "net/http" + "os" + "testing" + + "github.com/jfrog/jfrog-client-go/utils/errorutils" + "github.com/stretchr/testify/assert" +) + +func newHTTPErr(status int, statusText string, body []byte) error { + resp := &http.Response{StatusCode: status, Status: statusText} + return errorutils.CheckResponseStatusWithBody(resp, body, http.StatusOK) +} + +func TestHandleHTTPErrorAsJSON_DisabledByDefault(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "") + var buf bytes.Buffer + err := newHTTPErr(401, "401 Unauthorized", []byte(`{"x":1}`)) + assert.False(t, HandleHTTPErrorAsJSON(&buf, err)) + assert.Empty(t, buf.String()) +} + +func TestHandleHTTPErrorAsJSON_TextValueDoesNothing(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "text") + var buf bytes.Buffer + err := newHTTPErr(500, "500 Internal Server Error", []byte(`{"x":1}`)) + assert.False(t, HandleHTTPErrorAsJSON(&buf, err)) + assert.Empty(t, buf.String()) +} + +func TestHandleHTTPErrorAsJSON_NilErr(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "json") + var buf bytes.Buffer + assert.False(t, HandleHTTPErrorAsJSON(&buf, nil)) + assert.Empty(t, buf.String()) +} + +func TestHandleHTTPErrorAsJSON_NonHTTPErrorFallsThrough(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "json") + var buf bytes.Buffer + assert.False(t, HandleHTTPErrorAsJSON(&buf, errors.New("plain"))) + assert.Empty(t, buf.String()) +} + +func TestHandleHTTPErrorAsJSON_EmitsParsedBody(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "json") + var buf bytes.Buffer + body := []byte(`{"errors":[{"code":"UNAUTHORIZED","message":"bad creds"}]}`) + err := newHTTPErr(http.StatusUnauthorized, "401 Unauthorized", body) + assert.True(t, HandleHTTPErrorAsJSON(&buf, err)) + + var out map[string]interface{} + assert.NoError(t, json.Unmarshal(buf.Bytes(), &out)) + assert.EqualValues(t, 401, out["status_code"]) + assert.Equal(t, "401 Unauthorized", out["status"]) + assert.NotNil(t, out["body"]) + // body must be embedded as a structured object, not a string. + _, isObject := out["body"].(map[string]interface{}) + assert.True(t, isObject, "body should be embedded as JSON object, got: %T", out["body"]) +} + +func TestHandleHTTPErrorAsJSON_NonJSONBodyKeptAsString(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "json") + var buf bytes.Buffer + err := newHTTPErr(503, "503 Service Unavailable", []byte("down")) + assert.True(t, HandleHTTPErrorAsJSON(&buf, err)) + + var out map[string]interface{} + assert.NoError(t, json.Unmarshal(buf.Bytes(), &out)) + assert.EqualValues(t, 503, out["status_code"]) + assert.Equal(t, "down", out["body"]) +} + +func TestHandleHTTPErrorAsJSON_UnwrapsThroughFmtErrorf(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "json") + var buf bytes.Buffer + inner := newHTTPErr(http.StatusForbidden, "403 Forbidden", []byte(`{"msg":"nope"}`)) + wrapped := fmt.Errorf("failed to exchange OIDC token: %w", inner) + assert.True(t, HandleHTTPErrorAsJSON(&buf, wrapped)) + + var out map[string]interface{} + assert.NoError(t, json.Unmarshal(buf.Bytes(), &out)) + assert.EqualValues(t, 403, out["status_code"]) +} + +func TestHandleHTTPErrorAsJSON_CaseInsensitiveValue(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, " JSON ") + var buf bytes.Buffer + err := newHTTPErr(404, "404 Not Found", nil) + assert.True(t, HandleHTTPErrorAsJSON(&buf, err)) + assert.Contains(t, buf.String(), `"status_code": 404`) +} + +// TestHandleHTTPErrorAsJSON_LegacyTextFallback covers the case where intermediate +// code stripped the typed-error wrap chain (e.g. errors.New(err.Error() + ...)). +// The legacy text format is the only signal left, and we parse it back to JSON. +func TestHandleHTTPErrorAsJSON_LegacyTextFallback(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "json") + var buf bytes.Buffer + // Mimic a callsite that flattened the typed error into plain text. + plain := errors.New("server response: 502 Bad Gateway\n{\"err\":\"upstream down\"}") + assert.True(t, HandleHTTPErrorAsJSON(&buf, plain)) + + var out map[string]interface{} + assert.NoError(t, json.Unmarshal(buf.Bytes(), &out)) + assert.EqualValues(t, 502, out["status_code"]) + assert.Equal(t, "502 Bad Gateway", out["status"]) + body, ok := out["body"].(map[string]interface{}) + assert.True(t, ok, "body must reparse as JSON object") + assert.Equal(t, "upstream down", body["err"]) +} + +func TestParseLegacyHTTPResponseError_NoPrefixReturnsNil(t *testing.T) { + assert.Nil(t, parseLegacyHTTPResponseError("totally unrelated message")) +} + +func TestParseLegacyHTTPResponseError_StatusOnly(t *testing.T) { + got := parseLegacyHTTPResponseError("server response: 204 No Content") + if assert.NotNil(t, got) { + assert.Equal(t, 204, got.StatusCode) + assert.Equal(t, "204 No Content", got.Status) + assert.Empty(t, got.Body) + } +} + +func TestEnableJSONErrorIfFormatJSON_EqualsForm(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "") + EnableJSONErrorIfFormatJSON([]string{"jf", "rt", "ping", "--format=json"}) + assert.Equal(t, ErrorFormatJSON, os.Getenv(JfrogCliErrorOutputFormat)) +} + +func TestEnableJSONErrorIfFormatJSON_SpaceForm(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "") + EnableJSONErrorIfFormatJSON([]string{"jf", "rt", "search", "--format", "json", "repo/*"}) + assert.Equal(t, ErrorFormatJSON, os.Getenv(JfrogCliErrorOutputFormat)) +} + +func TestEnableJSONErrorIfFormatJSON_SingleDashEquals(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "") + EnableJSONErrorIfFormatJSON([]string{"jf", "rt", "ping", "-format=json"}) + assert.Equal(t, ErrorFormatJSON, os.Getenv(JfrogCliErrorOutputFormat)) +} + +func TestEnableJSONErrorIfFormatJSON_CaseInsensitiveValue(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "") + EnableJSONErrorIfFormatJSON([]string{"jf", "x", "--format=JSON"}) + assert.Equal(t, ErrorFormatJSON, os.Getenv(JfrogCliErrorOutputFormat)) +} + +func TestEnableJSONErrorIfFormatJSON_NonJSONValueIgnored(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "") + EnableJSONErrorIfFormatJSON([]string{"jf", "x", "--format=table"}) + assert.Empty(t, os.Getenv(JfrogCliErrorOutputFormat)) +} + +func TestEnableJSONErrorIfFormatJSON_NoFlag(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "") + EnableJSONErrorIfFormatJSON([]string{"jf", "rt", "ping"}) + assert.Empty(t, os.Getenv(JfrogCliErrorOutputFormat)) +} + +func TestEnableJSONErrorIfFormatJSON_LooksLikeFormatInOtherFlag(t *testing.T) { + // Reject false positives: "--format=json" must be a standalone arg, not + // part of another flag's value. + t.Setenv(JfrogCliErrorOutputFormat, "") + EnableJSONErrorIfFormatJSON([]string{"jf", "rt", "ping", "--header=x-format=json"}) + assert.Empty(t, os.Getenv(JfrogCliErrorOutputFormat)) +} + +func TestEnableJSONErrorIfFormatJSON_DanglingFormatFlag(t *testing.T) { + // Trailing "--format" with no value: do not panic, do not set. + t.Setenv(JfrogCliErrorOutputFormat, "") + EnableJSONErrorIfFormatJSON([]string{"jf", "rt", "ping", "--format"}) + assert.Empty(t, os.Getenv(JfrogCliErrorOutputFormat)) +} + +func TestEnableJSONErrorIfFormatJSON_ExplicitEnvWins(t *testing.T) { + // User explicitly set the env var (to anything) — auto-promotion stays out. + t.Setenv(JfrogCliErrorOutputFormat, "text") + EnableJSONErrorIfFormatJSON([]string{"jf", "x", "--format=json"}) + assert.Equal(t, "text", os.Getenv(JfrogCliErrorOutputFormat)) +} From aa089ab51bff0e6d464639be8496f68896486b0d Mon Sep 17 00:00:00 2001 From: ehl-jf Date: Fri, 22 May 2026 09:54:31 +0200 Subject: [PATCH 2/2] Reject malformed legacy HTTP codes longer than 9 digits The digit-count cap in parseLegacyHTTPResponseError previously truncated 10+ digit inputs to their first 9 digits, producing a JSON status_code the server never sent. Change the loop to count all leading digits and return nil when the count exceeds 9, so the caller falls back to the text path for clearly malformed inputs. Adds TestLegacyHTTPErrorCodeDigitLimits with four subtests covering the upper boundary (9 digits accepted), one digit over the cap (10 digits rejected), a far-past-the-cap input (20 digits rejected), and an end-to-end check that HandleHTTPErrorAsJSON emits no JSON for the too-long case (text-path fallthrough). --- utils/cliutils/errorformat.go | 10 ++++++---- utils/cliutils/errorformat_test.go | 32 ++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/utils/cliutils/errorformat.go b/utils/cliutils/errorformat.go index 1e611490c..b4855c0e2 100644 --- a/utils/cliutils/errorformat.go +++ b/utils/cliutils/errorformat.go @@ -133,13 +133,15 @@ func parseLegacyHTTPResponseError(msg string) *errorutils.HttpResponseError { statusLine = strings.TrimSpace(statusLine) // First token must be a numeric status code (e.g. "401 Unauthorized"). - // Cap the digit count so Atoi cannot overflow on malformed input — real - // HTTP statuses are 3 digits; 9 leaves plenty of headroom under int32. + // Real HTTP statuses are 3 digits. We accept up to 9 to leave headroom + // under int32, but anything longer is malformed — return nil so the + // caller falls back to the text path rather than emitting a truncated + // status_code the server never sent. codeEnd := 0 - for codeEnd < len(statusLine) && codeEnd < 9 && statusLine[codeEnd] >= '0' && statusLine[codeEnd] <= '9' { + for codeEnd < len(statusLine) && statusLine[codeEnd] >= '0' && statusLine[codeEnd] <= '9' { codeEnd++ } - if codeEnd == 0 { + if codeEnd == 0 || codeEnd > 9 { return nil } code, _ := strconv.Atoi(statusLine[:codeEnd]) diff --git a/utils/cliutils/errorformat_test.go b/utils/cliutils/errorformat_test.go index 555a16a55..7c51e8995 100644 --- a/utils/cliutils/errorformat_test.go +++ b/utils/cliutils/errorformat_test.go @@ -129,6 +129,38 @@ func TestParseLegacyHTTPResponseError_StatusOnly(t *testing.T) { } } +// TestLegacyHTTPErrorCodeDigitLimits exercises the status-code digit-count +// boundary in parseLegacyHTTPResponseError and the end-to-end behavior in +// HandleHTTPErrorAsJSON. Real HTTP statuses are 3 digits; the parser accepts +// up to 9 (overflow protection under int32) and rejects longer inputs as +// malformed so the caller falls back to the text path rather than emitting +// a truncated status_code the server never sent. +func TestLegacyHTTPErrorCodeDigitLimits(t *testing.T) { + t.Run("9-digit code accepted (upper boundary)", func(t *testing.T) { + got := parseLegacyHTTPResponseError("server response: 123456789 Pathological") + if assert.NotNil(t, got) { + assert.Equal(t, 123456789, got.StatusCode) + assert.Equal(t, "123456789 Pathological", got.Status) + } + }) + + t.Run("10-digit code rejected (one over the cap)", func(t *testing.T) { + assert.Nil(t, parseLegacyHTTPResponseError("server response: 1234567890 Custom")) + }) + + t.Run("very long code rejected", func(t *testing.T) { + assert.Nil(t, parseLegacyHTTPResponseError("server response: 99999999999999999999 Custom")) + }) + + t.Run("HandleHTTPErrorAsJSON falls back to text on too-long code", func(t *testing.T) { + t.Setenv(JfrogCliErrorOutputFormat, "json") + var buf bytes.Buffer + plain := errors.New("server response: 1234567890 Custom\n{\"err\":\"x\"}") + assert.False(t, HandleHTTPErrorAsJSON(&buf, plain)) + assert.Empty(t, buf.String()) + }) +} + func TestEnableJSONErrorIfFormatJSON_EqualsForm(t *testing.T) { t.Setenv(JfrogCliErrorOutputFormat, "") EnableJSONErrorIfFormatJSON([]string{"jf", "rt", "ping", "--format=json"})