From e3048805752112404108cc70095c9e06cb556ab4 Mon Sep 17 00:00:00 2001 From: Emily Zhang Date: Wed, 5 Aug 2026 16:32:48 -0700 Subject: [PATCH 1/5] fix(client): bound API response size and JSON nesting depth The generated client buffers every response whole before parsing, so an oversized or endless body was bounded only by available memory. Guard the response body at the transport seam, which also covers the SDK's own unmarshal calls and the signing key fetch without touching generated code. Addresses NSPECT-ZJGA-VOED threat 3 (resource exhaustion during response deserialization): byte ceiling via a bounded reader, JSON nesting ceiling, and typed errors raised outside the retry loop so an oversized body cannot drive a retry loop. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Emily Zhang --- nvfleetint/client.go | 37 ++-- nvfleetint/responseguard.go | 212 ++++++++++++++++++++++ nvfleetint/responseguard_test.go | 293 +++++++++++++++++++++++++++++++ nvfleetint/verify_test.go | 7 +- 4 files changed, 535 insertions(+), 14 deletions(-) create mode 100644 nvfleetint/responseguard.go create mode 100644 nvfleetint/responseguard_test.go diff --git a/nvfleetint/client.go b/nvfleetint/client.go index b0afb28..0264ec6 100644 --- a/nvfleetint/client.go +++ b/nvfleetint/client.go @@ -46,12 +46,14 @@ var ( // Calls the Fleet Intelligence customer API type Client struct { - baseURL *url.URL - apiKey string - httpClient *http.Client - requestDoer fleetapi.HttpRequestDoer - timeout time.Duration - api *fleetapi.ClientWithResponses + baseURL *url.URL + apiKey string + httpClient *http.Client + requestDoer fleetapi.HttpRequestDoer + timeout time.Duration + maxResponseBytes int64 + maxJSONDepth int + api *fleetapi.ClientWithResponses } // Customizes client construction behavior @@ -106,18 +108,27 @@ func NewClient(baseURL, apiKey string, opts ...Option) (*Client, error) { } client := &Client{ - baseURL: parsedBaseURL, - apiKey: apiKey, - httpClient: defaultHTTPClient(), - timeout: DefaultTimeout, + baseURL: parsedBaseURL, + apiKey: apiKey, + httpClient: defaultHTTPClient(), + timeout: DefaultTimeout, + maxResponseBytes: DefaultMaxResponseBytes, + maxJSONDepth: DefaultMaxJSONDepth, } for _, opt := range opts { opt(client) } - client.requestDoer = &retryingDoer{ - inner: client.httpClient, - maxAttempts: defaultRequestAttempts, + // The size and depth guard sits outside the retry loop so it bounds the one + // response that is actually handed back for parsing; bodies discarded + // between retries are already drained through a small bounded reader. + client.requestDoer = &limitingDoer{ + inner: &retryingDoer{ + inner: client.httpClient, + maxAttempts: defaultRequestAttempts, + }, + maxBytes: client.maxResponseBytes, + maxDepth: client.maxJSONDepth, } api, err := fleetapi.NewClientWithResponses( diff --git a/nvfleetint/responseguard.go b/nvfleetint/responseguard.go new file mode 100644 index 0000000..4a4cf25 --- /dev/null +++ b/nvfleetint/responseguard.go @@ -0,0 +1,212 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nvfleetint + +import ( + "errors" + "fmt" + "io" + "net/http" + "strings" + + "github.com/NVIDIA/fleet-intelligence-client/internal/generated/fleetapi" +) + +// DefaultMaxResponseBytes is the ceiling applied to a single API response body +// when none is configured. The generated client buffers every response whole +// (io.ReadAll) before parsing, so an oversized or endless body would otherwise +// be bounded only by available memory. The value leaves generous headroom over +// the largest responses seen in practice (tens of MiB for a wide `alert +// describe` or a signed inventory report); raise it with WithMaxResponseBytes +// if a deployment legitimately returns more. +const DefaultMaxResponseBytes int64 = 64 << 20 + +// DefaultMaxJSONDepth is the nesting depth allowed in a JSON response body when +// none is configured. Real payloads from the Fleet Intelligence API nest a +// handful of levels; the limit exists to reject adversarially nested documents +// long before encoding/json's own 10000-level ceiling turns into wasted CPU. +const DefaultMaxJSONDepth = 64 + +// ErrResponseTooLarge reports that a response body exceeded the configured size +// limit and was not parsed. See WithMaxResponseBytes. +var ErrResponseTooLarge = errors.New("response body too large") + +// ErrResponseTooDeep reports that a JSON response body nested more deeply than +// the configured limit and was not parsed. See WithMaxJSONDepth. +var ErrResponseTooDeep = errors.New("response JSON nested too deeply") + +// WithMaxResponseBytes sets the maximum number of bytes read from a single API +// response body. Reading past the limit fails the call with +// ErrResponseTooLarge rather than buffering the remainder. Non-positive values +// are ignored, leaving the existing limit in place. +func WithMaxResponseBytes(maxBytes int64) Option { + return func(c *Client) { + if maxBytes > 0 { + c.maxResponseBytes = maxBytes + } + } +} + +// WithMaxJSONDepth sets the maximum object and array nesting allowed in a JSON +// response body. Exceeding it fails the call with ErrResponseTooDeep. +// Non-positive values are ignored, leaving the existing limit in place. +func WithMaxJSONDepth(maxDepth int) Option { + return func(c *Client) { + if maxDepth > 0 { + c.maxJSONDepth = maxDepth + } + } +} + +// Wraps a Doer so every response body it hands back is bounded before anything +// downstream buffers or parses it. +// +// The limits live here, at the transport seam, because the code that actually +// calls io.ReadAll is generated (internal/generated/fleetapi) and must not be +// hand-edited. Guarding the body instead of the parser also covers the SDK's +// own json.Unmarshal calls and the raw CSV and ZIP report payloads, which never +// reach a JSON decoder at all. +type limitingDoer struct { + inner fleetapi.HttpRequestDoer + maxBytes int64 + maxDepth int +} + +// Performs the request and returns it with a guarded body. +func (d *limitingDoer) Do(req *http.Request) (*http.Response, error) { + response, err := d.inner.Do(req) + if err != nil || response == nil || response.Body == nil { + return response, err + } + + body := newGuardedBody(response.Body, d.maxBytes, responseDepthLimit(response, d.maxDepth)) + body.rejectDeclaredLength(response.ContentLength) + response.Body = body + + return response, nil +} + +// Returns the depth limit to apply to this response, or 0 to skip the check. +// +// Depth is checked only for bodies the server labels as JSON — the same +// condition the generated parser uses to decide whether to unmarshal. Scanning +// a ZIP or CSV report payload for brace nesting would be meaningless and could +// reject a legitimate download on arbitrary binary content. A body that is +// unmarshaled despite a non-JSON content type is still covered by the byte +// limit and by encoding/json's own nesting ceiling. +func responseDepthLimit(response *http.Response, maxDepth int) int { + if maxDepth <= 0 || !strings.Contains(strings.ToLower(response.Header.Get("Content-Type")), "json") { + return 0 + } + + return maxDepth +} + +// Enforces a byte ceiling, and optionally a JSON nesting ceiling, as a response +// body is read. Both are checked incrementally so an oversized body is rejected +// while it streams rather than after it has been buffered. +type guardedBody struct { + inner io.ReadCloser + remaining int64 + limit int64 + scanner *jsonDepthScanner + err error +} + +// Builds a guarded body +func newGuardedBody(inner io.ReadCloser, maxBytes int64, maxDepth int) *guardedBody { + body := &guardedBody{inner: inner, remaining: maxBytes, limit: maxBytes} + if maxDepth > 0 { + body.scanner = &jsonDepthScanner{max: maxDepth} + } + + return body +} + +// Fails the body up front when the server declares a length above the limit, so +// an oversized payload is refused instead of streamed only to be rejected at +// the end. A chunked response declares -1 and is caught while reading instead; +// a declared length that understates the real body is caught the same way. +func (b *guardedBody) rejectDeclaredLength(declared int64) { + if declared > b.limit { + b.err = fmt.Errorf("%w: declared %d bytes, limit is %d", ErrResponseTooLarge, declared, b.limit) + } +} + +// Reads from the underlying body, failing once either limit is crossed. The +// error is sticky: a caller that ignores it and reads again gets the same +// failure instead of a partial, silently truncated document. +func (b *guardedBody) Read(p []byte) (int, error) { + if b.err != nil { + return 0, b.err + } + + n, err := b.inner.Read(p) + if n > 0 { + if int64(n) > b.remaining { + b.err = fmt.Errorf("%w: exceeds limit of %d bytes", ErrResponseTooLarge, b.limit) + return 0, b.err + } + b.remaining -= int64(n) + + if b.scanner != nil { + if scanErr := b.scanner.scan(p[:n]); scanErr != nil { + b.err = scanErr + return 0, b.err + } + } + } + if err != nil { + b.err = err + } + + return n, err +} + +// Closes the underlying body +func (b *guardedBody) Close() error { + return b.inner.Close() +} + +// Tracks JSON object and array nesting across successive chunks of a streamed +// body. It deliberately does not validate the document: malformed input is the +// decoder's business, and this only has to bound how deep a well-formed one may +// go before the decoder recurses into it. +type jsonDepthScanner struct { + max int + depth int + inString bool + escaped bool +} + +// Scans one chunk, reporting ErrResponseTooDeep as soon as the limit is passed. +func (s *jsonDepthScanner) scan(chunk []byte) error { + for _, c := range chunk { + if s.inString { + switch { + case s.escaped: + s.escaped = false + case c == '\\': + s.escaped = true + case c == '"': + s.inString = false + } + continue + } + + switch c { + case '"': + s.inString = true + case '{', '[': + s.depth++ + if s.depth > s.max { + return fmt.Errorf("%w: exceeds limit of %d levels", ErrResponseTooDeep, s.max) + } + case '}', ']': + s.depth-- + } + } + + return nil +} diff --git a/nvfleetint/responseguard_test.go b/nvfleetint/responseguard_test.go new file mode 100644 index 0000000..b596cc6 --- /dev/null +++ b/nvfleetint/responseguard_test.go @@ -0,0 +1,293 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nvfleetint + +import ( + "context" + "errors" + "io" + "net/http" + "net/http/httptest" + "strconv" + "strings" + "testing" +) + +// Builds nested JSON arrays of the requested depth wrapped in an object, so the +// document stays valid at any depth. The returned body nests depth+1 levels. +func nestedJSON(depth int) string { + return `{"a":` + strings.Repeat("[", depth) + strings.Repeat("]", depth) + `}` +} + +// Verifies the client applies the documented defaults +func TestNewClientDefaultsResponseLimits(t *testing.T) { + client, err := NewClient("https://api.example.com", "key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + if client.maxResponseBytes != DefaultMaxResponseBytes { + t.Fatalf("unexpected byte limit: %d", client.maxResponseBytes) + } + if client.maxJSONDepth != DefaultMaxJSONDepth { + t.Fatalf("unexpected depth limit: %d", client.maxJSONDepth) + } +} + +// Verifies the limit options are applied and that non-positive values are +// ignored rather than disabling the guard +func TestResponseLimitOptions(t *testing.T) { + client, err := NewClient("https://api.example.com", "key", + WithMaxResponseBytes(1024), WithMaxJSONDepth(4)) + if err != nil { + t.Fatalf("new client failed: %v", err) + } + if client.maxResponseBytes != 1024 || client.maxJSONDepth != 4 { + t.Fatalf("options not applied: %d / %d", client.maxResponseBytes, client.maxJSONDepth) + } + + unchanged, err := NewClient("https://api.example.com", "key", + WithMaxResponseBytes(0), WithMaxJSONDepth(-1)) + if err != nil { + t.Fatalf("new client failed: %v", err) + } + if unchanged.maxResponseBytes != DefaultMaxResponseBytes || unchanged.maxJSONDepth != DefaultMaxJSONDepth { + t.Fatalf("non-positive limits were applied: %d / %d", + unchanged.maxResponseBytes, unchanged.maxJSONDepth) + } +} + +// Verifies an oversized response is rejected instead of being buffered whole +func TestResponseByteLimitRejectsOversizedBody(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"nodesCount":` + strings.Repeat("1", 4096) + `}`)) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key", WithMaxResponseBytes(256)) + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + _, err = client.GetOverview(context.Background(), OverviewOptions{}) + if !errors.Is(err, ErrResponseTooLarge) { + t.Fatalf("expected ErrResponseTooLarge, got %v", err) + } +} + +// Verifies a body at the limit still succeeds, so the guard is off-by-one safe +func TestResponseByteLimitAllowsBodyAtLimit(t *testing.T) { + body := `{"nodesCount":10}` + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(body)) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key", WithMaxResponseBytes(int64(len(body)))) + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + got, err := client.GetOverview(context.Background(), OverviewOptions{}) + if err != nil { + t.Fatalf("overview failed: %v", err) + } + if got.NodesCount == nil || *got.NodesCount != 10 { + t.Fatalf("unexpected nodes count: %#v", got.NodesCount) + } +} + +// Verifies a declared Content-Length above the limit fails before the payload +// is transferred +func TestResponseByteLimitRejectsDeclaredLength(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + payload := []byte(`{"nodesCount":` + strings.Repeat("1", 4096) + `}`) + w.Header().Set("Content-Type", "application/json") + w.Header().Set("Content-Length", strconv.Itoa(len(payload))) + _, _ = w.Write(payload) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key", WithMaxResponseBytes(64)) + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + _, err = client.GetOverview(context.Background(), OverviewOptions{}) + if !errors.Is(err, ErrResponseTooLarge) { + t.Fatalf("expected ErrResponseTooLarge, got %v", err) + } + if !strings.Contains(err.Error(), "declared") { + t.Fatalf("expected the declared-length path, got %v", err) + } +} + +// Verifies a chunked response with no declared length is still bounded +func TestResponseByteLimitRejectsUnboundedChunkedBody(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + flusher, ok := w.(http.Flusher) + if !ok { + t.Fatalf("response writer is not a flusher") + } + _, _ = w.Write([]byte(`{"nodesCount":`)) + flusher.Flush() + for range 64 { + _, _ = w.Write([]byte(strings.Repeat("1", 512))) + flusher.Flush() + } + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key", WithMaxResponseBytes(1024)) + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + _, err = client.GetOverview(context.Background(), OverviewOptions{}) + if !errors.Is(err, ErrResponseTooLarge) { + t.Fatalf("expected ErrResponseTooLarge, got %v", err) + } +} + +// Verifies a deeply nested JSON response is rejected before decoding +func TestResponseDepthLimitRejectsDeeplyNestedJSON(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(nestedJSON(200))) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + _, err = client.GetOverview(context.Background(), OverviewOptions{}) + if !errors.Is(err, ErrResponseTooDeep) { + t.Fatalf("expected ErrResponseTooDeep, got %v", err) + } +} + +// Verifies a realistically nested response is untouched by the depth guard +func TestResponseDepthLimitAllowsOrdinaryPayload(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"nodesCount":10,"metrics":[{"name":"gpu_utilization","value":42.5}]}`)) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + got, err := client.GetOverview(context.Background(), OverviewOptions{}) + if err != nil { + t.Fatalf("overview failed: %v", err) + } + if len(got.Metrics) != 1 { + t.Fatalf("unexpected metrics: %#v", got.Metrics) + } +} + +// Verifies non-JSON payloads skip the depth scan, so brace-heavy binary report +// downloads are not mistaken for nested documents +func TestResponseDepthLimitSkipsNonJSONContentType(t *testing.T) { + response := &http.Response{Header: http.Header{}} + response.Header.Set("Content-Type", "application/zip") + if got := responseDepthLimit(response, 8); got != 0 { + t.Fatalf("expected depth checking to be skipped, got %d", got) + } + + response.Header.Set("Content-Type", "application/json; charset=utf-8") + if got := responseDepthLimit(response, 8); got != 8 { + t.Fatalf("expected depth limit 8, got %d", got) + } + + response.Header.Set("Content-Type", "application/problem+json") + if got := responseDepthLimit(response, 8); got != 8 { + t.Fatalf("expected depth limit 8 for a +json type, got %d", got) + } + + if got := responseDepthLimit(response, 0); got != 0 { + t.Fatalf("expected a non-positive limit to disable the scan, got %d", got) + } +} + +// Verifies the depth scanner ignores structural characters inside strings and +// tracks depth across chunk boundaries +func TestJSONDepthScanner(t *testing.T) { + cases := []struct { + name string + chunks []string + max int + wantErr bool + }{ + {name: "at limit", chunks: []string{`{"a":[1,2]}`}, max: 2}, + {name: "over limit", chunks: []string{`{"a":[[1]]}`}, max: 2, wantErr: true}, + {name: "braces in string", chunks: []string{`{"a":"[[[[[[[[[["}`}, max: 1}, + {name: "escaped quote in string", chunks: []string{`{"a":"\"[[[[["}`}, max: 1}, + {name: "escaped backslash ends string", chunks: []string{`{"a":"x\\"}`}, max: 1}, + {name: "split across chunks", chunks: []string{`{"a":`, `[[1]]}`}, max: 2, wantErr: true}, + {name: "string split across chunks", chunks: []string{`{"a":"[[`, `[["}`}, max: 1}, + {name: "siblings do not accumulate", chunks: []string{`{"a":[1],"b":[2],"c":[3]}`}, max: 2}, + } + + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + scanner := &jsonDepthScanner{max: testCase.max} + var err error + for _, chunk := range testCase.chunks { + if err = scanner.scan([]byte(chunk)); err != nil { + break + } + } + if testCase.wantErr != (err != nil) { + t.Fatalf("wantErr=%v, got %v", testCase.wantErr, err) + } + if testCase.wantErr && !errors.Is(err, ErrResponseTooDeep) { + t.Fatalf("expected ErrResponseTooDeep, got %v", err) + } + }) + } +} + +// Verifies the guarded body keeps failing once a limit is crossed, so an +// ignored error cannot yield a silently truncated document +func TestGuardedBodyErrorIsSticky(t *testing.T) { + body := newGuardedBody(io.NopCloser(strings.NewReader(strings.Repeat("x", 64))), 8, 0) + + buf := make([]byte, 32) + if _, err := body.Read(buf); !errors.Is(err, ErrResponseTooLarge) { + t.Fatalf("expected ErrResponseTooLarge, got %v", err) + } + n, err := body.Read(buf) + if n != 0 || !errors.Is(err, ErrResponseTooLarge) { + t.Fatalf("expected the error to stick, got n=%d err=%v", n, err) + } + if err := body.Close(); err != nil { + t.Fatalf("close failed: %v", err) + } +} + +// Verifies the signing key fetch, which reads its body outside the generated +// client, is bounded too +func TestFetchSigningKeyIsBounded(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/x-pem-file") + _, _ = w.Write([]byte(strings.Repeat("A", 8192))) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key", WithMaxResponseBytes(128)) + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + if _, err := client.FetchSigningKey(context.Background()); !errors.Is(err, ErrResponseTooLarge) { + t.Fatalf("expected ErrResponseTooLarge, got %v", err) + } +} diff --git a/nvfleetint/verify_test.go b/nvfleetint/verify_test.go index 38c7616..74cb7e2 100644 --- a/nvfleetint/verify_test.go +++ b/nvfleetint/verify_test.go @@ -149,10 +149,15 @@ func TestFetchSigningKeyRetriesTransientFailure(t *testing.T) { if err != nil { t.Fatalf("new client failed: %v", err) } - retryer, ok := client.requestDoer.(*retryingDoer) + // The retry layer sits inside the response size and depth guard. + limiter, ok := client.requestDoer.(*limitingDoer) if !ok { t.Fatalf("unexpected request doer: %T", client.requestDoer) } + retryer, ok := limiter.inner.(*retryingDoer) + if !ok { + t.Fatalf("unexpected inner doer: %T", limiter.inner) + } retryer.delay = func(int, *http.Response) time.Duration { return 0 } key, err := client.FetchSigningKey(context.Background()) From f64bacfcbd6a5e67509dcba7e9e6308c926caa8d Mon Sep 17 00:00:00 2001 From: Emily Zhang Date: Wed, 5 Aug 2026 16:42:06 -0700 Subject: [PATCH 2/5] fix(client): validate identifiers and paging before building requests Path parameters are percent-escaped by the generated client, but dot segments survive escaping and are resolved away when the operation path is joined to the base URL: a node UUID of ".." turned GET /v1/nodes/{id} into GET /v1/, and an empty one into a request against the collection. Validate identifiers through one shared nvfleetint.ValidateResourceID, called from both the SDK and the CLI, so caller-supplied input cannot change which endpoint is called. Also bound page and page size in the SDK. Those limits previously existed only in the CLI flag layer, leaving them unenforced for programs that use the SDK directly. Addresses NSPECT-ZJGA-VOED threat 5, requirement 1. Requirements 2 and 3 of that threat are server-side controls and are out of scope for this repo. The threat as written describes corrupting backend records through request bodies; no command here writes, so this covers the reachable surface, which is the path and query parameters of read requests. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Emily Zhang --- cmd/nvfleetint/alert.go | 23 ++- cmd/nvfleetint/node.go | 6 +- cmd/nvfleetint/node_health.go | 6 +- cmd/nvfleetint/node_test.go | 49 ++++++ internal/clihelpers/pagination.go | 7 +- nvfleetint/alert.go | 26 ++- nvfleetint/computezone.go | 3 + nvfleetint/event.go | 3 + nvfleetint/node.go | 7 +- nvfleetint/node_health.go | 6 +- nvfleetint/nodegroup.go | 2 +- nvfleetint/params.go | 72 ++++++++ nvfleetint/params_test.go | 271 ++++++++++++++++++++++++++++++ nvfleetint/report.go | 5 +- 14 files changed, 453 insertions(+), 33 deletions(-) create mode 100644 nvfleetint/params.go create mode 100644 nvfleetint/params_test.go diff --git a/cmd/nvfleetint/alert.go b/cmd/nvfleetint/alert.go index f001163..a98fd2a 100644 --- a/cmd/nvfleetint/alert.go +++ b/cmd/nvfleetint/alert.go @@ -4,7 +4,6 @@ package main import ( - "errors" "fmt" "io" "strings" @@ -212,8 +211,13 @@ func runAlertTimeline(cmd *cobra.Command, flags alertTimelineFlags, common resol return err } - nodeUUID := strings.TrimSpace(flags.node) - if nodeUUID != "" { + // An omitted --node lists every node with timeline history, so only a + // supplied value is validated as a path identifier. + if strings.TrimSpace(flags.node) != "" { + nodeUUID, err := nvfleetint.ValidateResourceID("--node", flags.node) + if err != nil { + return err + } return runNodeAlertTimeline(cmd, client, flags, nodeUUID, common) } return runAlertTimelineNodes(cmd, client, flags, common) @@ -330,13 +334,14 @@ func runAlertDescribe(cmd *cobra.Command, alertUUID string, flags alertDescribeF return err } - nodeUUID := strings.TrimSpace(flags.node) - alertUUID = strings.TrimSpace(alertUUID) - if nodeUUID == "" { - return errors.New("--node is required") + // Named for the flag so an omitted value reports "--node is required". + nodeUUID, err := nvfleetint.ValidateResourceID("--node", flags.node) + if err != nil { + return err } - if alertUUID == "" { - return errors.New("alert UUID is required") + alertUUID, err = nvfleetint.ValidateResourceID("alert UUID", alertUUID) + if err != nil { + return err } client, err := newConfiguredClient(common) diff --git a/cmd/nvfleetint/node.go b/cmd/nvfleetint/node.go index 1b63e3f..84c814e 100644 --- a/cmd/nvfleetint/node.go +++ b/cmd/nvfleetint/node.go @@ -256,9 +256,9 @@ func runNodeDescribe(cmd *cobra.Command, nodeUUID string, common resolvedCommonF return err } - nodeUUID = strings.TrimSpace(nodeUUID) - if nodeUUID == "" { - return errors.New("node UUID is required") + nodeUUID, err := nvfleetint.ValidateResourceID("node UUID", nodeUUID) + if err != nil { + return err } client, err := newConfiguredClient(common) diff --git a/cmd/nvfleetint/node_health.go b/cmd/nvfleetint/node_health.go index b2f7f58..9cbf749 100644 --- a/cmd/nvfleetint/node_health.go +++ b/cmd/nvfleetint/node_health.go @@ -53,9 +53,9 @@ func runNodeHealth(cmd *cobra.Command, nodeUUID string, flags nodeHealthFlags, c return err } - nodeUUID = strings.TrimSpace(nodeUUID) - if nodeUUID == "" { - return errors.New("node UUID is required") + nodeUUID, err := nvfleetint.ValidateResourceID("node UUID", nodeUUID) + if err != nil { + return err } start := strings.TrimSpace(flags.start) diff --git a/cmd/nvfleetint/node_test.go b/cmd/nvfleetint/node_test.go index bf79549..637707b 100644 --- a/cmd/nvfleetint/node_test.go +++ b/cmd/nvfleetint/node_test.go @@ -446,3 +446,52 @@ func TestListAllRejectsPage(t *testing.T) { t.Fatalf("unexpected error: %v", err) } } + +// Verifies the CLI rejects an identifier that would re-target the request +// before any call reaches the API. The SDK enforces this too; the check is +// duplicated here so a hostile argument never reaches a configured client. +func TestResourceIDArgsRejectedBeforeRequest(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer server.Close() + + saveTestConfig(t, server.URL, "test-key") + + invocations := [][]string{ + {"node", "describe", ".."}, + {"node", "describe", "../../v1/tags"}, + {"node", "health", "..", "--start", "2026-04-07T00:00:00Z", "--end", "2026-04-14T00:00:00Z"}, + {"alert", "describe", "alert-1", "--node", ".."}, + {"alert", "describe", "..", "--node", "node-1"}, + {"alert", "timeline", "--node", ".."}, + } + + for _, args := range invocations { + t.Run(strings.Join(args, " "), func(t *testing.T) { + var out bytes.Buffer + cmd := newRootCmd() + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SetArgs(args) + + err := cmd.Execute() + if err == nil { + t.Fatalf("expected an error, got output %q", out.String()) + } + if !strings.Contains(err.Error(), "different API path") && + !strings.Contains(err.Error(), "single path segment") { + t.Fatalf("unexpected error: %v", err) + } + }) + } + + if requests.Load() != 0 { + t.Fatalf("expected no requests to be issued, server saw %d", requests.Load()) + } +} diff --git a/internal/clihelpers/pagination.go b/internal/clihelpers/pagination.go index 7859a2a..9426775 100644 --- a/internal/clihelpers/pagination.go +++ b/internal/clihelpers/pagination.go @@ -9,9 +9,12 @@ import ( ) const ( - // MinPageSize is the smallest page size accepted by list commands + // MinPageSize is the smallest page size accepted by list commands. + // Kept in step with nvfleetint.MinPageSize, which enforces the same bound + // for callers who use the SDK directly. MinPageSize = 1 - // MaxPageSize is the largest page size accepted by list commands + // MaxPageSize is the largest page size accepted by list commands. + // Kept in step with nvfleetint.MaxPageSize. MaxPageSize = 100 // MaxPages bounds all-page pagination when the API keeps reporting more data MaxPages = 10000 diff --git a/nvfleetint/alert.go b/nvfleetint/alert.go index 96e2e0f..8d97992 100644 --- a/nvfleetint/alert.go +++ b/nvfleetint/alert.go @@ -207,6 +207,10 @@ func (c *Client) ListAlertTimelineNodes(ctx context.Context, opts ListAlertTimel ctx, cancel := c.requestContext(ctx) defer cancel() + if err := validatePagination(opts.Page, opts.PageSize); err != nil { + return AlertTimelineNodesPage{}, err + } + params := fleetapi.GetV1AlertTimelineNodesParams{} if opts.Active { params.Active = boolPointer(opts.Active) @@ -234,8 +238,12 @@ func (c *Client) ListNodeAlertTimeline(ctx context.Context, opts ListNodeAlertTi ctx, cancel := c.requestContext(ctx) defer cancel() - if opts.NodeUUID == "" { - return NodeAlertTimelinePage{}, fmt.Errorf("node UUID is required") + nodeUUID, err := ValidateResourceID("node UUID", opts.NodeUUID) + if err != nil { + return NodeAlertTimelinePage{}, err + } + if err := validatePagination(opts.Page, opts.PageSize); err != nil { + return NodeAlertTimelinePage{}, err } params := fleetapi.GetV1AlertTimelineNodesNodeUuidAlertsParams{} @@ -249,7 +257,7 @@ func (c *Client) ListNodeAlertTimeline(ctx context.Context, opts ListNodeAlertTi params.PageSize = cloneInt(opts.PageSize) } - resp, err := c.api.GetV1AlertTimelineNodesNodeUuidAlertsWithResponse(ctx, opts.NodeUUID, ¶ms) + resp, err := c.api.GetV1AlertTimelineNodesNodeUuidAlertsWithResponse(ctx, nodeUUID, ¶ms) if err != nil { return NodeAlertTimelinePage{}, err } @@ -265,11 +273,13 @@ func (c *Client) DescribeAlertTimeline(ctx context.Context, nodeUUID, alertUUID ctx, cancel := c.requestContext(ctx) defer cancel() - if nodeUUID == "" { - return AlertTimelineDetails{}, fmt.Errorf("node UUID is required") + nodeUUID, err := ValidateResourceID("node UUID", nodeUUID) + if err != nil { + return AlertTimelineDetails{}, err } - if alertUUID == "" { - return AlertTimelineDetails{}, fmt.Errorf("alert UUID is required") + alertUUID, err = ValidateResourceID("alert UUID", alertUUID) + if err != nil { + return AlertTimelineDetails{}, err } resp, err := c.api.GetV1AlertTimelineNodesNodeUuidAlertsAlertUuidWithResponse(ctx, nodeUUID, alertUUID, &fleetapi.GetV1AlertTimelineNodesNodeUuidAlertsAlertUuidParams{}) @@ -298,7 +308,7 @@ func validateAlertOptions(opts ListAlertsOptions) error { if opts.State != "" && !opts.State.Valid() { return fmt.Errorf("invalid alert state %q: expected Detected, Triggered, or Resolved", opts.State) } - return nil + return validatePagination(opts.Page, opts.PageSize) } // alertsAPIPageOffset bridges the /v1/alerts endpoint's 1-indexed paging to the diff --git a/nvfleetint/computezone.go b/nvfleetint/computezone.go index acb2ba6..1c1b257 100644 --- a/nvfleetint/computezone.go +++ b/nvfleetint/computezone.go @@ -61,6 +61,9 @@ func (c *Client) ListComputeZones(ctx context.Context, opts ListComputeZonesOpti if err != nil { return ComputeZonesPage{}, err } + if err := validatePagination(opts.Page, opts.PageSize); err != nil { + return ComputeZonesPage{}, err + } params := fleetapi.GetV1ComputezonesParams{ View: computeZoneViewParam(view), diff --git a/nvfleetint/event.go b/nvfleetint/event.go index eda27e0..4924d40 100644 --- a/nvfleetint/event.go +++ b/nvfleetint/event.go @@ -94,6 +94,9 @@ func (c *Client) ListEvents(ctx context.Context, opts EventListOptions) (EventsP if err != nil { return EventsPage{}, err } + if err := validatePagination(opts.Page, opts.PageSize); err != nil { + return EventsPage{}, err + } mode := fleetapi.GetV1EventsParamsTimeMode(timeRange.timeMode) params := fleetapi.GetV1EventsParams{TimeMode: &mode} diff --git a/nvfleetint/node.go b/nvfleetint/node.go index 8d99409..338798f 100644 --- a/nvfleetint/node.go +++ b/nvfleetint/node.go @@ -394,8 +394,9 @@ func (c *Client) DescribeNode(ctx context.Context, nodeUUID string) (NodeDetails ctx, cancel := c.requestContext(ctx) defer cancel() - if nodeUUID == "" { - return NodeDetails{}, fmt.Errorf("node UUID is required") + nodeUUID, err := ValidateResourceID("node UUID", nodeUUID) + if err != nil { + return NodeDetails{}, err } resp, err := c.api.GetV1NodesNodeUuidWithResponse(ctx, nodeUUID) @@ -470,7 +471,7 @@ func validateNodeOptions(view NodeView, opts ListNodesOptions) error { } } - return nil + return validatePagination(opts.Page, opts.PageSize) } // Reports whether a sort field works with basic view diff --git a/nvfleetint/node_health.go b/nvfleetint/node_health.go index 2ce0d5d..e98cbab 100644 --- a/nvfleetint/node_health.go +++ b/nvfleetint/node_health.go @@ -52,12 +52,12 @@ func (c *Client) NodeHealthHistory(ctx context.Context, nodeUUID string, opts No ctx, cancel := c.requestContext(ctx) defer cancel() - nodeUUID = strings.TrimSpace(nodeUUID) opts.StartTime = strings.TrimSpace(opts.StartTime) opts.EndTime = strings.TrimSpace(opts.EndTime) - if nodeUUID == "" { - return NodeHealthHistory{}, fmt.Errorf("node UUID is required") + nodeUUID, err := ValidateResourceID("node UUID", nodeUUID) + if err != nil { + return NodeHealthHistory{}, err } if opts.StartTime == "" || opts.EndTime == "" { return NodeHealthHistory{}, fmt.Errorf("start and end times are required") diff --git a/nvfleetint/nodegroup.go b/nvfleetint/nodegroup.go index ece253d..65c3bfe 100644 --- a/nvfleetint/nodegroup.go +++ b/nvfleetint/nodegroup.go @@ -191,7 +191,7 @@ func validateNodeGroupOptions(view NodeGroupView, opts ListNodeGroupsOptions) er return fmt.Errorf("basic node group view is incompatible with sort order %q", opts.Order) } - return nil + return validatePagination(opts.Page, opts.PageSize) } // Converts a normalized view into the generated parameter type diff --git a/nvfleetint/params.go b/nvfleetint/params.go new file mode 100644 index 0000000..388fc5e --- /dev/null +++ b/nvfleetint/params.go @@ -0,0 +1,72 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nvfleetint + +import ( + "fmt" + "strings" +) + +// MinPageSize and MaxPageSize bound the page size the API accepts, per +// api/openapi/openapi.yaml. The CLI mirrors these in internal/clihelpers; they +// are restated here because the SDK is used directly by callers who never go +// through the CLI's flag validation. +const ( + MinPageSize = 1 + MaxPageSize = 100 +) + +// ValidateResourceID checks an identifier that will be interpolated into the +// request URL path and returns it trimmed. name is the caller-facing label used +// in the error message, e.g. "node UUID". +// +// It is exported so the CLI can reject a hostile identifier before it builds a +// client, without the two layers drifting apart on what "valid" means. +// +// The generated client percent-escapes path parameters, so separators, query +// markers, and fragments cannot break out of their segment. Dot segments are +// the exception: "." and ".." survive escaping intact and are then resolved +// away when the operation path is joined to the base URL, so a caller-supplied +// ".." silently re-targets the request at a different endpoint (/v1/nodes/.. +// resolves to /v1/). An empty value collapses the same way, turning an +// item request into a request against the collection. +// +// Path parameters are declared as bare strings in the OpenAPI spec — there is +// no UUID format to enforce — so this rejects only what would change which +// endpoint is called or what would smuggle control characters into the URL. +func ValidateResourceID(name, value string) (string, error) { + trimmed := strings.TrimSpace(value) + if trimmed == "" { + return "", fmt.Errorf("%s is required", name) + } + if trimmed == "." || trimmed == ".." { + return "", fmt.Errorf("invalid %s %q: value would redirect the request to a different API path", name, trimmed) + } + if strings.ContainsAny(trimmed, "/\\") { + return "", fmt.Errorf("invalid %s %q: expected a single path segment", name, trimmed) + } + for _, r := range trimmed { + // Reported without the value: echoing control bytes back through a + // terminal is its own small hazard. + if r < 0x20 || r == 0x7f { + return "", fmt.Errorf("invalid %s: contains a control character", name) + } + } + + return trimmed, nil +} + +// Checks the paging parameters shared by every list call. Page is 0-based in +// the SDK (the CLI presents it 1-based). Both are pointers because an unset +// value means "let the backend apply its default" and is always allowed. +func validatePagination(page, pageSize *int) error { + if page != nil && *page < 0 { + return fmt.Errorf("invalid page %d: expected a non-negative page number", *page) + } + if pageSize != nil && (*pageSize < MinPageSize || *pageSize > MaxPageSize) { + return fmt.Errorf("invalid page size %d: expected %d-%d", *pageSize, MinPageSize, MaxPageSize) + } + + return nil +} diff --git a/nvfleetint/params_test.go b/nvfleetint/params_test.go new file mode 100644 index 0000000..1a68e03 --- /dev/null +++ b/nvfleetint/params_test.go @@ -0,0 +1,271 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nvfleetint + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "sync/atomic" + "testing" +) + +// Verifies identifier validation, including the dot segments that survive +// percent-escaping and would otherwise re-target the request +func TestValidateResourceID(t *testing.T) { + cases := []struct { + name string + value string + want string + wantErr string + }{ + {name: "uuid", value: "1e9c0d2a-0000-4a1b-9c3d-000000000001", want: "1e9c0d2a-0000-4a1b-9c3d-000000000001"}, + {name: "trims surrounding space", value: " node-1 ", want: "node-1"}, + {name: "dotted identifier is fine", value: "node.example.1", want: "node.example.1"}, + {name: "empty", value: "", wantErr: "node UUID is required"}, + {name: "whitespace only", value: " ", wantErr: "node UUID is required"}, + {name: "parent dot segment", value: "..", wantErr: "different API path"}, + {name: "padded parent dot segment", value: " .. ", wantErr: "different API path"}, + {name: "current dot segment", value: ".", wantErr: "different API path"}, + {name: "forward slash", value: "../../v1/tags", wantErr: "single path segment"}, + {name: "backslash", value: `a\b`, wantErr: "single path segment"}, + {name: "control character", value: "node\x00id", wantErr: "control character"}, + {name: "newline", value: "node\nid", wantErr: "control character"}, + } + + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + got, err := ValidateResourceID("node UUID", testCase.value) + if testCase.wantErr == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != testCase.want { + t.Fatalf("got %q, want %q", got, testCase.want) + } + return + } + if err == nil { + t.Fatalf("expected an error, got %q", got) + } + if !strings.Contains(err.Error(), testCase.wantErr) { + t.Fatalf("error %q does not mention %q", err, testCase.wantErr) + } + }) + } +} + +// Verifies a rejected control character is not echoed back in the message +func TestValidateResourceIDDoesNotEchoControlCharacters(t *testing.T) { + _, err := ValidateResourceID("node UUID", "node\x1b[31mid") + if err == nil { + t.Fatal("expected an error") + } + if strings.ContainsRune(err.Error(), 0x1b) { + t.Fatalf("error echoed a control character: %q", err.Error()) + } +} + +// Verifies paging bounds, including the unset case that defers to the backend +func TestValidatePagination(t *testing.T) { + page := func(v int) *int { return &v } + + cases := []struct { + name string + page *int + pageSize *int + wantErr string + }{ + {name: "both unset"}, + {name: "first page", page: page(0), pageSize: page(50)}, + {name: "minimum page size", pageSize: page(MinPageSize)}, + {name: "maximum page size", pageSize: page(MaxPageSize)}, + {name: "negative page", page: page(-1), wantErr: "non-negative"}, + {name: "zero page size", pageSize: page(0), wantErr: "expected 1-100"}, + {name: "oversized page size", pageSize: page(MaxPageSize + 1), wantErr: "expected 1-100"}, + {name: "negative page size", pageSize: page(-5), wantErr: "expected 1-100"}, + } + + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + err := validatePagination(testCase.page, testCase.pageSize) + switch { + case testCase.wantErr == "" && err != nil: + t.Fatalf("unexpected error: %v", err) + case testCase.wantErr != "" && err == nil: + t.Fatal("expected an error") + case testCase.wantErr != "" && !strings.Contains(err.Error(), testCase.wantErr): + t.Fatalf("error %q does not mention %q", err, testCase.wantErr) + } + }) + } +} + +// Verifies no SDK call that interpolates an identifier into the URL path can be +// steered off its endpoint by a hostile identifier. Each case must fail before +// a request is issued, so the server counter stays at zero. +func TestPathParamsCannotRetargetRequests(t *testing.T) { + var requests atomic.Int32 + var observed []string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests.Add(1) + observed = append(observed, r.URL.Path) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + calls := map[string]func(id string) error{ + "DescribeNode": func(id string) error { + _, err := client.DescribeNode(context.Background(), id) + return err + }, + "NodeHealthHistory": func(id string) error { + _, err := client.NodeHealthHistory(context.Background(), id, NodeHealthHistoryOptions{ + StartTime: "2026-04-07T00:00:00Z", + EndTime: "2026-04-14T00:00:00Z", + }) + return err + }, + "ListNodeAlertTimeline": func(id string) error { + _, err := client.ListNodeAlertTimeline(context.Background(), ListNodeAlertTimelineOptions{NodeUUID: id}) + return err + }, + "DescribeAlertTimeline/node": func(id string) error { + _, err := client.DescribeAlertTimeline(context.Background(), id, "alert-1") + return err + }, + "DescribeAlertTimeline/alert": func(id string) error { + _, err := client.DescribeAlertTimeline(context.Background(), "node-1", id) + return err + }, + } + + hostile := []string{"", " ", ".", "..", "../..", "../../v1/tags", `a\b`, "node\nid"} + + for name, call := range calls { + for _, id := range hostile { + t.Run(name+"/"+strings.ReplaceAll(id, "\n", "\\n"), func(t *testing.T) { + if err := call(id); err == nil { + t.Fatalf("hostile identifier %q was accepted", id) + } + }) + } + } + + if requests.Load() != 0 { + t.Fatalf("expected no requests to be issued, server saw %d: %v", requests.Load(), observed) + } +} + +// Verifies a legitimate identifier still reaches the intended endpoint, so the +// validation above is not simply rejecting everything +func TestPathParamsReachIntendedEndpoint(t *testing.T) { + var path string + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + path = r.URL.Path + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"nodeUUID":"node-1"}`)) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + if _, err := client.DescribeNode(context.Background(), " node-1 "); err != nil { + t.Fatalf("describe node failed: %v", err) + } + if path != "/v1/nodes/node-1" { + t.Fatalf("unexpected request path: %q", path) + } +} + +// Verifies every paginated list call rejects out-of-range paging before issuing +// a request +func TestListCallsRejectOutOfRangePagination(t *testing.T) { + var requests atomic.Int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + requests.Add(1) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{}`)) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + badPage := -1 + badPageSize := MaxPageSize + 1 + + calls := map[string]func(page, pageSize *int) error{ + "ListAlerts": func(page, pageSize *int) error { + _, err := client.ListAlerts(context.Background(), ListAlertsOptions{Page: page, PageSize: pageSize}) + return err + }, + "ListAlertTimelineNodes": func(page, pageSize *int) error { + _, err := client.ListAlertTimelineNodes(context.Background(), ListAlertTimelineNodesOptions{Page: page, PageSize: pageSize}) + return err + }, + "ListNodeAlertTimeline": func(page, pageSize *int) error { + _, err := client.ListNodeAlertTimeline(context.Background(), ListNodeAlertTimelineOptions{ + NodeUUID: "node-1", Page: page, PageSize: pageSize, + }) + return err + }, + "ListComputeZones": func(page, pageSize *int) error { + _, err := client.ListComputeZones(context.Background(), ListComputeZonesOptions{Page: page, PageSize: pageSize}) + return err + }, + "ListEvents": func(page, pageSize *int) error { + _, err := client.ListEvents(context.Background(), EventListOptions{Window: "24h", Page: page, PageSize: pageSize}) + return err + }, + "ListNodes": func(page, pageSize *int) error { + _, err := client.ListNodes(context.Background(), ListNodesOptions{Page: page, PageSize: pageSize}) + return err + }, + "ListNodeGroups": func(page, pageSize *int) error { + _, err := client.ListNodeGroups(context.Background(), ListNodeGroupsOptions{Page: page, PageSize: pageSize}) + return err + }, + "GetInventoryReport": func(page, pageSize *int) error { + _, err := client.GetInventoryReport(context.Background(), InventoryReportOptions{Page: page, PageSize: pageSize}) + return err + }, + "GetErrorReport": func(page, pageSize *int) error { + _, err := client.GetErrorReport(context.Background(), ErrorReportOptions{ + View: ErrorReportViewOverview, TimeMode: ErrorReportTimeModeRelative, Window: "24h", + Page: page, PageSize: pageSize, + }) + return err + }, + } + + for name, call := range calls { + t.Run(name+"/page", func(t *testing.T) { + if err := call(&badPage, nil); err == nil { + t.Fatal("negative page was accepted") + } + }) + t.Run(name+"/pageSize", func(t *testing.T) { + if err := call(nil, &badPageSize); err == nil { + t.Fatal("oversized page size was accepted") + } + }) + } + + if requests.Load() != 0 { + t.Fatalf("expected no requests to be issued, server saw %d", requests.Load()) + } +} diff --git a/nvfleetint/report.go b/nvfleetint/report.go index cbf07d5..82b6f40 100644 --- a/nvfleetint/report.go +++ b/nvfleetint/report.go @@ -380,6 +380,9 @@ func normalizeErrorReportOptions(opts ErrorReportOptions) (ErrorReportOptions, e if err := validateErrorReportTime(opts); err != nil { return ErrorReportOptions{}, err } + if err := validatePagination(opts.Page, opts.PageSize); err != nil { + return ErrorReportOptions{}, err + } return opts, nil } @@ -452,7 +455,7 @@ func validateInventoryReportOptions(opts InventoryReportOptions) error { if opts.Order != "" && !opts.Order.Valid() { return fmt.Errorf("invalid inventory report order %q: expected asc or desc", opts.Order) } - return nil + return validatePagination(opts.Page, opts.PageSize) } // Builds generated inventory report query parameters From 5efbb49a162e78453143e2b3024282d7ce4b143b Mon Sep 17 00:00:00 2001 From: Emily Zhang Date: Wed, 5 Aug 2026 16:46:53 -0700 Subject: [PATCH 3/5] fix(client): bound the outbound connection pool per host Go leaves MaxConnsPerHost unlimited, so a concurrent SDK embedder opened one socket per in-flight call and could turn a single program into a load spike on a shared backend. Cap connections per host in the shared hardened transport, and raise MaxIdleConnsPerHost to match so the cap does not cost the backend extra TLS handshakes through connection churn. A caller's stricter settings are preserved, and WithHTTPClient remains the escape hatch for a different pool. Addresses NSPECT-ZJGA-VOED threat 6, requirement 3. The other two parts of that requirement were already in place: per-request timeouts through Client.requestContext, and exponential backoff with jitter and Retry-After in retryingDoer. Both now have tests asserting the property rather than relying on it incidentally. Requirements 1 and 2 of this threat are server-side rate limiting and are out of scope for this repo. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Emily Zhang --- nvfleetint/client.go | 37 +++++++++-- nvfleetint/client_test.go | 136 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 169 insertions(+), 4 deletions(-) diff --git a/nvfleetint/client.go b/nvfleetint/client.go index 0264ec6..f9a9fbd 100644 --- a/nvfleetint/client.go +++ b/nvfleetint/client.go @@ -248,10 +248,26 @@ func originHostPort(u *url.URL) string { return net.JoinHostPort(u.Hostname(), port) } -// Clones base (preserving proxy, keep-alive, and HTTP/2 defaults) and pins at -// least a TLS 1.2 floor, since Go's default has none. An existing stricter -// floor is left alone. Returns nil when base is not an *http.Transport, leaving -// the caller to fall back to net/http's own default. +const ( + // maxConnsPerHost caps how many connections this process opens to any one + // API host at a time. Go's default is unlimited, so a concurrent embedder + // could otherwise open a socket per in-flight call and turn one misbehaving + // program into a load spike on a shared backend. Requests past the cap wait + // for a free connection rather than failing. + maxConnsPerHost = 16 + // maxIdleConnsPerHost matches the cap so connections earned under it stay + // warm and get reused. Go's default of 2 would otherwise close and reopen + // the rest, making the throttle cost the backend extra TLS handshakes. + // MaxIdleConns is left at net/http's default of 100, which is already + // bounded and is a process-wide total across hosts. + maxIdleConnsPerHost = maxConnsPerHost +) + +// Clones base (preserving proxy, keep-alive, and HTTP/2 defaults), pins at +// least a TLS 1.2 floor since Go's default has none, and bounds the connection +// pool. Existing stricter settings are left alone. Returns nil when base is not +// an *http.Transport, leaving the caller to fall back to net/http's own +// default. func hardenedTransport(base http.RoundTripper) *http.Transport { transport, ok := base.(*http.Transport) if !ok { @@ -265,6 +281,19 @@ func hardenedTransport(base http.RoundTripper) *http.Transport { if cloned.TLSClientConfig.MinVersion < tls.VersionTLS12 { cloned.TLSClientConfig.MinVersion = tls.VersionTLS12 } + // Zero means unlimited for MaxConnsPerHost, so it has to be treated as + // looser than any cap rather than as "already strict". + if cloned.MaxConnsPerHost <= 0 || cloned.MaxConnsPerHost > maxConnsPerHost { + cloned.MaxConnsPerHost = maxConnsPerHost + } + // Zero here means net/http's default of 2, which is stricter than the cap + // but only in the sense of holding fewer idle sockets; raise it so reuse + // tracks MaxConnsPerHost. A caller who deliberately set a higher number + // keeps it only up to the cap, since more idle connections than the + // per-host limit cannot be used anyway. + if cloned.MaxIdleConnsPerHost <= 0 || cloned.MaxIdleConnsPerHost > maxIdleConnsPerHost { + cloned.MaxIdleConnsPerHost = maxIdleConnsPerHost + } return cloned } diff --git a/nvfleetint/client_test.go b/nvfleetint/client_test.go index c75912b..9629e0b 100644 --- a/nvfleetint/client_test.go +++ b/nvfleetint/client_test.go @@ -9,11 +9,13 @@ import ( "crypto/tls" "errors" "io" + "net" "net/http" "net/http/httptest" "net/url" "strconv" "strings" + "sync" "sync/atomic" "testing" "time" @@ -502,3 +504,137 @@ func TestTimeoutEnforcedWithSharedHTTPClient(t *testing.T) { t.Fatalf("expected shared HTTP client timeout to remain unset, got %v", shared.Timeout) } } + +// Verifies the default transport bounds its connection pool +func TestHardenedTransportBoundsConnectionPool(t *testing.T) { + hardened := hardenedTransport(http.DefaultTransport) + if hardened == nil { + t.Fatal("expected a hardened transport") + } + if hardened.MaxConnsPerHost != maxConnsPerHost { + t.Fatalf("unexpected MaxConnsPerHost: %d", hardened.MaxConnsPerHost) + } + if hardened.MaxIdleConnsPerHost != maxIdleConnsPerHost { + t.Fatalf("unexpected MaxIdleConnsPerHost: %d", hardened.MaxIdleConnsPerHost) + } + // net/http's default is already a bound, so it is deliberately left alone. + if hardened.MaxIdleConns != http.DefaultTransport.(*http.Transport).MaxIdleConns { + t.Fatalf("MaxIdleConns was changed: %d", hardened.MaxIdleConns) + } +} + +// Verifies a caller's stricter pool settings survive hardening +func TestHardenedTransportPreservesStricterConnectionLimits(t *testing.T) { + base := &http.Transport{MaxConnsPerHost: 4, MaxIdleConnsPerHost: 3} + + hardened := hardenedTransport(base) + if hardened == nil { + t.Fatal("expected a hardened transport") + } + if hardened.MaxConnsPerHost != 4 { + t.Fatalf("stricter MaxConnsPerHost was overwritten: %d", hardened.MaxConnsPerHost) + } + if hardened.MaxIdleConnsPerHost != 3 { + t.Fatalf("stricter MaxIdleConnsPerHost was overwritten: %d", hardened.MaxIdleConnsPerHost) + } +} + +// Verifies the pool cap actually throttles concurrent calls, so a parallel +// caller cannot open a socket per in-flight request against one API host +func TestConcurrentCallsRespectConnectionCap(t *testing.T) { + var mu sync.Mutex + open, peak := 0, 0 + + server := httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + // Hold the connection long enough that callers overlap; without the + // cap every goroutine would get its own socket. + time.Sleep(20 * time.Millisecond) + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"nodesCount":1}`)) + })) + server.Config.ConnState = func(_ net.Conn, state http.ConnState) { + mu.Lock() + defer mu.Unlock() + switch state { + case http.StateNew: + open++ + if open > peak { + peak = open + } + case http.StateClosed, http.StateHijacked: + open-- + } + } + server.Start() + defer server.Close() + + client, err := NewClient(server.URL, "test-key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + const callers = 64 + var wg sync.WaitGroup + errs := make(chan error, callers) + for range callers { + wg.Add(1) + go func() { + defer wg.Done() + if _, err := client.GetOverview(context.Background(), OverviewOptions{}); err != nil { + errs <- err + } + }() + } + wg.Wait() + close(errs) + for err := range errs { + t.Fatalf("overview failed: %v", err) + } + + mu.Lock() + defer mu.Unlock() + if peak > maxConnsPerHost { + t.Fatalf("peak connections %d exceeded the cap of %d", peak, maxConnsPerHost) + } + // Guards against a vacuous pass if the calls never actually overlapped. + if peak < 2 { + t.Fatalf("calls did not run concurrently, peak connections was %d", peak) + } +} + +// Verifies retry delays grow exponentially and stay capped, so a client that +// keeps hitting a struggling backend backs away from it instead of hammering +// it at a fixed interval +func TestDefaultRetryDelayBacksOffExponentially(t *testing.T) { + const samples = 200 + + var previousBase time.Duration + for attempt := 1; attempt <= 8; attempt++ { + base := initialRetryDelay << (attempt - 1) + if base > maximumRetryDelay { + base = maximumRetryDelay + } + // Jitter spreads each delay over 50%-150% of the base. + low, high := base/2, base*3/2 + + for range samples { + delay := defaultRetryDelay(attempt, nil) + if delay < low || delay > high { + t.Fatalf("attempt %d delay %v outside [%v, %v]", attempt, delay, low, high) + } + } + + if attempt > 1 && base < previousBase { + t.Fatalf("attempt %d base %v shrank from %v", attempt, base, previousBase) + } + if base > maximumRetryDelay { + t.Fatalf("attempt %d base %v exceeded the cap %v", attempt, base, maximumRetryDelay) + } + previousBase = base + } + + // The growth has to actually happen, not just stay within bounds. + if initialRetryDelay<<3 <= initialRetryDelay { + t.Fatal("retry delay does not grow between attempts") + } +} From 473da9fd2c0e5947583449c6164aabda8c545dc6 Mon Sep 17 00:00:00 2001 From: Emily Zhang Date: Thu, 6 Aug 2026 10:07:26 -0700 Subject: [PATCH 4/5] fix(install): retry, time out, and fall back when downloads fail Neither installer bounded its downloads: a dropped or throttled connection left curl or Invoke-WebRequest waiting on defaults, and a single failed request aborted the install with no second attempt and no alternative source. A degraded GitHub Releases could therefore hang a provisioning pipeline instead of failing it. Both scripts now fetch through one retry helper with explicit connect and request timeouts, a bounded attempt count, and exponential backoff capped at a maximum delay. Retries are limited to transient failures: transport errors and 408/425/429/5xx are retried, while a 404 fails immediately rather than delaying a certain failure. Exhaustion logs the reason and the attempt count and exits non-zero. Adds optional fallback sources: NVFLEETINT_BASE_URL overrides the download root, NVFLEETINT_FALLBACK_BASE_URL is tried after the primary is exhausted, and NVFLEETINT_CACHE_DIR is read before the network. The cache is populated only after checksum verification, so a later run never reuses an artifact the current one could not vouch for. Mirror URLs must be https, with plain http allowed only for loopback, matching the rule the SDK already applies to its own base URL, so adding a mirror cannot downgrade the transport. Addresses NSPECT-ZJGA-VOED threat 9, all three requirements. install.sh was verified against a mock release server covering retry with backoff, non-retryable 404, attempt exhaustion, request timeout, mirror fallback, cache read and populate, and input rejection. install.ps1 mirrors it but is unverified by execution: no PowerShell is available on the development machine and CI has no Windows job. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Emily Zhang --- install.ps1 | 165 +++++++++++++++++++++++++++++++++++++++++-- install.sh | 197 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 2 files changed, 352 insertions(+), 10 deletions(-) diff --git a/install.ps1 b/install.ps1 index 425af22..5d13077 100644 --- a/install.ps1 +++ b/install.ps1 @@ -9,7 +9,27 @@ param( } else { Join-Path $env:LOCALAPPDATA "Programs\nvfleetint\bin" }), - [switch]$NoModifyPath + [switch]$NoModifyPath, + + # Download resilience. Every request carries an explicit timeout and a + # bounded number of attempts, so a dropped or throttled network fails the + # install instead of hanging a provisioning pipeline indefinitely. + [ValidateRange(1, 3600)] + [int]$TimeoutSeconds = $(if ($env:NVFLEETINT_MAX_TIME) { $env:NVFLEETINT_MAX_TIME } else { 120 }), + [ValidateRange(1, 100)] + [int]$RetryAttempts = $(if ($env:NVFLEETINT_RETRY_ATTEMPTS) { $env:NVFLEETINT_RETRY_ATTEMPTS } else { 4 }), + [ValidateRange(1, 3600)] + [int]$RetryDelaySeconds = $(if ($env:NVFLEETINT_RETRY_DELAY) { $env:NVFLEETINT_RETRY_DELAY } else { 2 }), + [ValidateRange(1, 3600)] + [int]$RetryMaxDelaySeconds = $(if ($env:NVFLEETINT_RETRY_MAX_DELAY) { $env:NVFLEETINT_RETRY_MAX_DELAY } else { 30 }), + + # Fallback sources. BaseUrl replaces the default download root, assets are + # read from //; FallbackBaseUrl is tried only after the + # primary is exhausted; CacheDir is consulted before the network and + # populated after a successful checksum verification. + [string]$BaseUrl = $(if ($env:NVFLEETINT_BASE_URL) { $env:NVFLEETINT_BASE_URL } else { "" }), + [string]$FallbackBaseUrl = $(if ($env:NVFLEETINT_FALLBACK_BASE_URL) { $env:NVFLEETINT_FALLBACK_BASE_URL } else { "" }), + [string]$CacheDir = $(if ($env:NVFLEETINT_CACHE_DIR) { $env:NVFLEETINT_CACHE_DIR } else { "" }) ) $ErrorActionPreference = "Stop" @@ -17,8 +37,92 @@ $ProgressPreference = "SilentlyContinue" $repository = "NVIDIA/fleet-intelligence-client" [Net.ServicePointManager]::SecurityProtocol = [Net.SecurityProtocolType]::Tls12 +if (-not $BaseUrl) { + $BaseUrl = "https://github.com/$repository/releases/download" +} + +# Keeps a caller-supplied mirror from downgrading the transport to plaintext. +# Plain http is accepted only for loopback, matching the rule the SDK applies to +# its own base URL (nvfleetint/baseurl.go) so local mock servers keep working. +function Assert-SecureUrl { + param([string]$Name, [string]$Value) + + $uri = $null + if (-not [Uri]::TryCreate($Value, [UriKind]::Absolute, [ref]$uri)) { + throw "$Name must be an absolute URL, got: $Value" + } + if ($uri.Scheme -eq "https") { return } + if ($uri.Scheme -eq "http" -and $uri.IsLoopback) { return } + throw "$Name must be an https:// URL (plain http is allowed only for localhost), got: $Value" +} + +# Extracts the HTTP status from a failed request, or 0 when the request never +# got a response at all (DNS failure, refused connection, timeout). +function Get-HttpStatusCode { + param($ErrorRecord) + + $response = $ErrorRecord.Exception.Response + if (-not $response) { return 0 } + try { + return [int]$response.StatusCode + } catch { + return 0 + } +} + +# Reports whether a failure is worth another attempt. A transport-level failure +# has no status and is always transient enough to retry; a 404 means the release +# or asset does not exist, so retrying only delays a certain failure. +function Test-RetryableFailure { + param($ErrorRecord) + + $code = Get-HttpStatusCode $ErrorRecord + if ($code -eq 0) { return $true } + return @(408, 425, 429, 500, 502, 503, 504) -contains $code +} + +# Runs a request with bounded retries and exponential backoff, throwing a clear +# message once the attempts are exhausted or the failure is deterministic. +function Invoke-WithRetry { + param([string]$Description, [scriptblock]$Action) + + $attempt = 1 + $delay = $RetryDelaySeconds + while ($true) { + try { + return & $Action + } catch { + $record = $_ + $code = Get-HttpStatusCode $record + $reason = if ($code -ne 0) { "HTTP $code" } else { $record.Exception.Message } + + if (-not (Test-RetryableFailure $record)) { + throw "$Description failed ($reason); not retryable." + } + if ($attempt -ge $RetryAttempts) { + throw "$Description failed after $RetryAttempts attempts ($reason)." + } + + Write-Warning "$Description failed ($reason); retrying in ${delay}s (attempt $($attempt + 1)/$RetryAttempts)." + Start-Sleep -Seconds $delay + $attempt++ + $delay = [Math]::Min($delay * 2, $RetryMaxDelaySeconds) + } + } +} + +Assert-SecureUrl -Name "BaseUrl" -Value $BaseUrl +$BaseUrl = $BaseUrl.TrimEnd("/") +if ($FallbackBaseUrl) { + Assert-SecureUrl -Name "FallbackBaseUrl" -Value $FallbackBaseUrl + $FallbackBaseUrl = $FallbackBaseUrl.TrimEnd("/") +} + if ($Version -eq "latest") { - $release = Invoke-RestMethod -Uri "https://api.github.com/repos/$repository/releases/latest" + $release = Invoke-WithRetry -Description "latest release lookup" -Action { + Invoke-RestMethod -Uri "https://api.github.com/repos/$repository/releases/latest" ` + -TimeoutSec $TimeoutSeconds + } $Version = $release.tag_name } @@ -44,17 +148,60 @@ $architecture = switch ($machineArchitecture.ToUpperInvariant()) { } $asset = "nvfleetint_${releaseVersion}_windows_${architecture}.zip" -$baseUrl = "https://github.com/$repository/releases/download/$tag" $workDir = Join-Path ([System.IO.Path]::GetTempPath()) ("nvfleetint-install-" + [guid]::NewGuid()) $archive = Join-Path $workDir $asset $checksumPath = Join-Path $workDir "checksums.txt" $extractDir = Join-Path $workDir "extract" +# Resolves one release file into the work directory: the cache first, then each +# configured download root in turn. Every source is fully retried before the +# next is tried. +function Get-ReleaseFile { + param([string]$Name, [string]$Destination) + + if ($CacheDir) { + $cached = Join-Path (Join-Path $CacheDir $tag) $Name + if (Test-Path -LiteralPath $cached) { + Write-Host "Using cached $Name from $(Join-Path $CacheDir $tag)" + Copy-Item -LiteralPath $cached -Destination $Destination -Force + return + } + } + + $roots = @($BaseUrl) + if ($FallbackBaseUrl) { $roots += $FallbackBaseUrl } + + foreach ($root in $roots) { + try { + Invoke-WithRetry -Description "download of $Name from $root" -Action { + Invoke-WebRequest -Uri "$root/$tag/$Name" -OutFile $Destination ` + -UseBasicParsing -TimeoutSec $TimeoutSeconds + } + return + } catch { + Write-Warning "Giving up on $root for ${Name}: $($_.Exception.Message)" + } + } + + throw "Could not obtain $Name from any configured source." +} + +# Stores a verified file in the cache. Only called after checksum verification, +# so a later run never reuses an artifact this run could not vouch for. +function Save-CachedFile { + param([string]$Name, [string]$Path) + + if (-not $CacheDir) { return } + $target = Join-Path $CacheDir $tag + New-Item -ItemType Directory -Path $target -Force | Out-Null + Copy-Item -LiteralPath $Path -Destination (Join-Path $target $Name) -Force +} + try { New-Item -ItemType Directory -Path $workDir | Out-Null Write-Host "Downloading nvfleetint $tag for windows/$architecture" - Invoke-WebRequest -Uri "$baseUrl/$asset" -OutFile $archive -UseBasicParsing - Invoke-WebRequest -Uri "$baseUrl/checksums.txt" -OutFile $checksumPath -UseBasicParsing + Get-ReleaseFile -Name $asset -Destination $archive + Get-ReleaseFile -Name "checksums.txt" -Destination $checksumPath $escapedAsset = [regex]::Escape($asset) $checksumLine = Get-Content $checksumPath | Where-Object { @@ -70,6 +217,9 @@ try { throw "Checksum verification failed for $asset" } + Save-CachedFile -Name $asset -Path $archive + Save-CachedFile -Name "checksums.txt" -Path $checksumPath + New-Item -ItemType Directory -Path $extractDir | Out-Null Expand-Archive -Path $archive -DestinationPath $extractDir $binary = Get-ChildItem -Path $extractDir -Filter "nvfleetint.exe" -File -Recurse | @@ -100,6 +250,11 @@ try { Write-Host "Installed nvfleetint to $destination" & $destination version +} catch { + # Fail deterministically: an automated caller sees a non-zero exit code and + # one clear reason, rather than a partially installed tree and exit 0. + $Host.UI.WriteErrorLine("Error: $($_.Exception.Message)") + exit 1 } finally { if (Test-Path -LiteralPath $workDir) { Remove-Item -LiteralPath $workDir -Recurse -Force diff --git a/install.sh b/install.sh index 2de7eed..0b54d24 100755 --- a/install.sh +++ b/install.sh @@ -6,10 +6,27 @@ set -euo pipefail readonly REPOSITORY="NVIDIA/fleet-intelligence-client" +readonly DEFAULT_BASE_URL="https://github.com/${REPOSITORY}/releases/download" version="${NVFLEETINT_VERSION:-latest}" install_dir="${NVFLEETINT_INSTALL_DIR:-${HOME}/.local/bin}" +# Download resilience. Every fetch is bounded by a connect and a total timeout +# and a fixed number of attempts, so a dropped or throttled network fails the +# install instead of hanging a provisioning pipeline indefinitely. +connect_timeout="${NVFLEETINT_CONNECT_TIMEOUT:-10}" +max_time="${NVFLEETINT_MAX_TIME:-120}" +retry_attempts="${NVFLEETINT_RETRY_ATTEMPTS:-4}" +retry_delay="${NVFLEETINT_RETRY_DELAY:-2}" +retry_max_delay="${NVFLEETINT_RETRY_MAX_DELAY:-30}" + +# Fallback sources. base_url replaces the default download root; fallback_url is +# tried only after the primary is exhausted; cache_dir is consulted before the +# network and populated after a successful checksum verification. +base_url="${NVFLEETINT_BASE_URL:-$DEFAULT_BASE_URL}" +fallback_url="${NVFLEETINT_FALLBACK_BASE_URL:-}" +cache_dir="${NVFLEETINT_CACHE_DIR:-}" + usage() { cat <<'EOF' Install nvfleetint for macOS or Linux. @@ -19,9 +36,54 @@ Usage: install.sh [--version ] [--install-dir ] Environment variables: NVFLEETINT_VERSION Release version, for example v1.2.3 (default: latest) NVFLEETINT_INSTALL_DIR Installation directory (default: $HOME/.local/bin) + +Download resilience: + NVFLEETINT_CONNECT_TIMEOUT Per-connection timeout in seconds (default: 10) + NVFLEETINT_MAX_TIME Per-request timeout in seconds (default: 120) + NVFLEETINT_RETRY_ATTEMPTS Attempts per source (default: 4) + NVFLEETINT_RETRY_DELAY Initial backoff in seconds, doubling (default: 2) + NVFLEETINT_RETRY_MAX_DELAY Maximum backoff in seconds (default: 30) + +Fallback sources: + NVFLEETINT_BASE_URL Download root, must be https (default: GitHub releases). + Assets are read from //. + NVFLEETINT_FALLBACK_BASE_URL Mirror tried after the primary root is exhausted + NVFLEETINT_CACHE_DIR Local artifact cache, read before the network and + populated after checksum verification EOF } +# Rejects a non-numeric or non-positive tunable before it reaches curl or sleep +require_positive_int() { + local name=$1 value=$2 + case "$value" in + ''|*[!0-9]*) + echo "Error: ${name} must be a positive integer, got: ${value}" >&2 + exit 1 + ;; + esac + [[ "$value" -gt 0 ]] || { + echo "Error: ${name} must be a positive integer, got: ${value}" >&2 + exit 1 + } +} + +# Keeps a caller-supplied mirror from downgrading the transport to plaintext. +# Plain http is accepted only for loopback, matching the rule the SDK applies to +# its own base URL (nvfleetint/baseurl.go) so local mock servers keep working. +require_secure_url() { + local name=$1 value=$2 + case "$value" in + https://*) return 0 ;; + http://127.0.0.1|http://127.0.0.1[:/]*) return 0 ;; + http://localhost|http://localhost[:/]*) return 0 ;; + http://\[::1\]|http://\[::1\][:/]*) return 0 ;; + esac + + echo "Error: ${name} must be an https:// URL (plain http is allowed only for localhost), got: ${value}" >&2 + exit 1 +} + while [[ $# -gt 0 ]]; do case "$1" in --version) @@ -46,6 +108,95 @@ while [[ $# -gt 0 ]]; do esac done +require_positive_int NVFLEETINT_CONNECT_TIMEOUT "$connect_timeout" +require_positive_int NVFLEETINT_MAX_TIME "$max_time" +require_positive_int NVFLEETINT_RETRY_ATTEMPTS "$retry_attempts" +require_positive_int NVFLEETINT_RETRY_DELAY "$retry_delay" +require_positive_int NVFLEETINT_RETRY_MAX_DELAY "$retry_max_delay" + +require_secure_url NVFLEETINT_BASE_URL "$base_url" +base_url="${base_url%/}" +if [[ -n "$fallback_url" ]]; then + require_secure_url NVFLEETINT_FALLBACK_BASE_URL "$fallback_url" + fallback_url="${fallback_url%/}" +fi + +# Reports whether a curl exit status is a transient transport failure. +# 6 DNS, 7 connect, 18 partial transfer, 28 timeout, 35 TLS handshake, +# 52 empty reply, 55 send error, 56 receive error. +is_retryable_curl_status() { + case "$1" in + 6|7|18|28|35|52|55|56) return 0 ;; + *) return 1 ;; + esac +} + +# Reports whether an HTTP status is worth another attempt. A 404 means the +# release or asset does not exist, so retrying only delays a certain failure. +is_retryable_http_code() { + case "$1" in + 408|425|429|500|502|503|504) return 0 ;; + *) return 1 ;; + esac +} + +# Fetches url into dest with bounded retries and exponential backoff, echoing +# the value of write_out on success. Every attempt carries an explicit connect +# and total timeout. Returns non-zero once the attempts are exhausted or the +# failure is deterministic, always after logging why. +fetch_with_retry() { + local description=$1 url=$2 dest=$3 write_out=$4 + local attempt=1 delay="$retry_delay" + local result status http_code payload reason + + while :; do + status=0 + # The status code is appended last and is always three digits, so the + # caller's write_out value can be recovered by trimming it. + result="$(curl -sSL \ + --connect-timeout "$connect_timeout" \ + --max-time "$max_time" \ + -w "${write_out}%{http_code}" \ + -o "$dest" \ + "$url")" || status=$? + + http_code="${result: -3}" + payload="${result%???}" + + if [[ $status -eq 0 && "$http_code" == 2?? ]]; then + printf '%s' "$payload" + return 0 + fi + + if [[ $status -ne 0 ]]; then + reason="curl exit status ${status}" + is_retryable_curl_status "$status" || { + echo "Error: ${description} failed (${reason}); not retryable." >&2 + return 1 + } + else + reason="HTTP ${http_code}" + is_retryable_http_code "$http_code" || { + echo "Error: ${description} failed (${reason}); not retryable." >&2 + return 1 + } + fi + + if [[ "$attempt" -ge "$retry_attempts" ]]; then + echo "Error: ${description} failed after ${retry_attempts} attempts (${reason})." >&2 + return 1 + fi + + echo "Warning: ${description} failed (${reason}); retrying in ${delay}s (attempt $((attempt + 1))/${retry_attempts})." >&2 + sleep "$delay" + attempt=$((attempt + 1)) + delay=$((delay * 2)) + if [[ "$delay" -gt "$retry_max_delay" ]]; then + delay="$retry_max_delay" + fi + done +} + for command in awk curl find install mkdir uname; do command -v "$command" >/dev/null 2>&1 || { echo "Required command not found: $command" >&2 @@ -82,8 +233,8 @@ case "$(uname -m)" in esac if [[ "$version" == "latest" ]]; then - latest_url="$(curl -fsSL -o /dev/null -w '%{url_effective}' \ - "https://github.com/${REPOSITORY}/releases/latest")" + latest_url="$(fetch_with_retry "latest release lookup" \ + "https://github.com/${REPOSITORY}/releases/latest" /dev/null '%{url_effective}')" || exit 1 version="${latest_url##*/}" [[ "$version" == v* ]] || { echo "Could not determine the latest release version" >&2 @@ -99,7 +250,6 @@ tag="$version" } release_version="${tag#v}" asset="nvfleetint_${release_version}_${os}_${arch}.${extension}" -base_url="https://github.com/${REPOSITORY}/releases/download/${tag}" work_dir="$(mktemp -d "${TMPDIR:-/tmp}/nvfleetint-install.XXXXXX")" cleanup() { @@ -110,9 +260,43 @@ cleanup() { } trap cleanup EXIT +# Resolves one release file into work_dir: the cache first, then each configured +# download root in turn. Every source is fully retried before the next is tried. +obtain_file() { + local name=$1 + local dest="${work_dir}/${name}" + local root + + if [[ -n "$cache_dir" && -f "${cache_dir}/${tag}/${name}" ]]; then + echo "Using cached ${name} from ${cache_dir}/${tag}" + cp "${cache_dir}/${tag}/${name}" "$dest" + return 0 + fi + + for root in "$base_url" ${fallback_url:+"$fallback_url"}; do + if fetch_with_retry "download of ${name} from ${root}" \ + "${root}/${tag}/${name}" "$dest" ''; then + return 0 + fi + echo "Warning: giving up on ${root} for ${name}." >&2 + done + + echo "Error: could not obtain ${name} from any configured source." >&2 + return 1 +} + +# Stores a verified file in the cache. Only called after checksum verification, +# so a later run never reuses an artifact this run could not vouch for. +cache_store() { + local name=$1 + [[ -n "$cache_dir" ]] || return 0 + mkdir -p "${cache_dir}/${tag}" + cp "${work_dir}/${name}" "${cache_dir}/${tag}/${name}" +} + echo "Downloading nvfleetint ${tag} for ${os}/${arch}" -curl -fsSL "${base_url}/${asset}" -o "${work_dir}/${asset}" -curl -fsSL "${base_url}/${checksum_file}" -o "${work_dir}/${checksum_file}" +obtain_file "$asset" || exit 1 +obtain_file "$checksum_file" || exit 1 expected_checksum="$(awk -v name="$asset" '$2 == name || $2 == "*" name { print $1; exit }' \ "${work_dir}/${checksum_file}")" @@ -135,6 +319,9 @@ fi exit 1 } +cache_store "$asset" +cache_store "$checksum_file" + extract_dir="${work_dir}/extract" mkdir -p "$extract_dir" if [[ "$extension" == "zip" ]]; then From 638b4af99e03ca9b4d18b9f446fdd20c22b6c514 Mon Sep 17 00:00:00 2001 From: Emily Zhang Date: Thu, 6 Aug 2026 10:23:51 -0700 Subject: [PATCH 5/5] fix(client): validate response fields against the API contract MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The generated client decodes into strong Go types, so a value of the wrong JSON kind already fails to unmarshal. What that did not catch was a well-formed payload carrying values the contract forbids: an IP field that is not an address, a hostname carrying terminal escape sequences, an alert severity outside the enum. Those reached the operator as rendered fleet state, which is what a tampered or compromised backend would rely on. Validate every JSON response before it is mapped into domain types, at all 16 SDK entry points that decode one. Control characters are refused in any string, most importantly ESC, which starts a sequence that could rewrite an operator's terminal; tab, newline, and return stay allowed because alert messages legitimately wrap and the table renderer already collapses them. Fields the contract constrains are checked by name wherever they appear: publicIP and privateIP must parse as addresses, hostname must be within the DNS length and character set, and severity and state must fall in the generated enums, so regenerating from an updated spec widens them automatically. The walk is streaming rather than decoding into an interface tree, so validating a large response does not add a second copy of it to memory and undo the bounds added for threat 3. Bodies that are not JSON — the CSV and ZIP report payloads — are skipped. Addresses NSPECT-ZJGA-VOED threat 2, requirement 3, and requirement 2 in part. This validates the constraints openapi.yaml actually declares for the named fields; it is not a general JSON Schema engine bound to each operation's response schema, which would need a schema validator dependency and the spec embedded in the binary. Verified against the live dev backend: overview, node list, node describe, node health, alert list, alert timeline, event list, tag list, computezone list, nodegroup list, report inventory, and report error all pass unchanged. Co-Authored-By: Claude Opus 5 (1M context) Signed-off-by: Emily Zhang --- nvfleetint/alert.go | 13 ++ nvfleetint/auth.go | 4 + nvfleetint/computezone.go | 4 + nvfleetint/event.go | 8 + nvfleetint/node.go | 7 + nvfleetint/node_health.go | 4 + nvfleetint/nodegroup.go | 3 + nvfleetint/overview.go | 4 + nvfleetint/report.go | 6 + nvfleetint/responsevalidate.go | 234 ++++++++++++++++++++++++++++ nvfleetint/responsevalidate_test.go | 230 +++++++++++++++++++++++++++ nvfleetint/tag.go | 4 + 12 files changed, 521 insertions(+) create mode 100644 nvfleetint/responsevalidate.go create mode 100644 nvfleetint/responsevalidate_test.go diff --git a/nvfleetint/alert.go b/nvfleetint/alert.go index 8d97992..76107b3 100644 --- a/nvfleetint/alert.go +++ b/nvfleetint/alert.go @@ -199,6 +199,9 @@ func (c *Client) ListAlerts(ctx context.Context, opts ListAlertsOptions) (Alerts return AlertsPage{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return AlertsPage{}, err + } return decodeAlerts(resp.Body) } @@ -230,6 +233,9 @@ func (c *Client) ListAlertTimelineNodes(ctx context.Context, opts ListAlertTimel return AlertTimelineNodesPage{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return AlertTimelineNodesPage{}, err + } return decodeAlertTimelineNodes(resp.Body) } @@ -265,6 +271,9 @@ func (c *Client) ListNodeAlertTimeline(ctx context.Context, opts ListNodeAlertTi return NodeAlertTimelinePage{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return NodeAlertTimelinePage{}, err + } return decodeNodeAlertTimeline(resp.Body) } @@ -290,6 +299,10 @@ func (c *Client) DescribeAlertTimeline(ctx context.Context, nodeUUID, alertUUID return AlertTimelineDetails{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return AlertTimelineDetails{}, err + } + var data fleetapi.ModelsAlertTimelineDetailResponse if err := json.Unmarshal(resp.Body, &data); err != nil { return AlertTimelineDetails{}, err diff --git a/nvfleetint/auth.go b/nvfleetint/auth.go index 0b8b3c2..c075f4a 100644 --- a/nvfleetint/auth.go +++ b/nvfleetint/auth.go @@ -35,6 +35,10 @@ func (c *Client) GetAuthStatus(ctx context.Context) (AuthStatus, error) { return AuthStatus{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return AuthStatus{}, err + } + var data fleetapi.ModelsAuthStatusResponse if err := json.Unmarshal(resp.Body, &data); err != nil { return AuthStatus{}, err diff --git a/nvfleetint/computezone.go b/nvfleetint/computezone.go index 1c1b257..537372f 100644 --- a/nvfleetint/computezone.go +++ b/nvfleetint/computezone.go @@ -87,6 +87,10 @@ func (c *Client) ListComputeZones(ctx context.Context, opts ListComputeZonesOpti return ComputeZonesPage{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return ComputeZonesPage{}, err + } + if view == ComputeZoneViewBasic { return decodeBasicComputeZones(resp.Body) } diff --git a/nvfleetint/event.go b/nvfleetint/event.go index 4924d40..e784760 100644 --- a/nvfleetint/event.go +++ b/nvfleetint/event.go @@ -133,6 +133,10 @@ func (c *Client) ListEvents(ctx context.Context, opts EventListOptions) (EventsP return EventsPage{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return EventsPage{}, err + } + var data fleetapi.ModelsEventsResponse if err := json.Unmarshal(resp.Body, &data); err != nil { return EventsPage{}, err @@ -199,6 +203,10 @@ func (c *Client) GetEventBuckets(ctx context.Context, opts EventBucketsOptions) return EventBuckets{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return EventBuckets{}, err + } + var data fleetapi.ModelsEventBucketsResponse if err := json.Unmarshal(resp.Body, &data); err != nil { return EventBuckets{}, err diff --git a/nvfleetint/node.go b/nvfleetint/node.go index 338798f..03cf135 100644 --- a/nvfleetint/node.go +++ b/nvfleetint/node.go @@ -381,6 +381,9 @@ func (c *Client) ListNodes(ctx context.Context, opts ListNodesOptions) (NodesPag if resp.StatusCode() != http.StatusOK { return NodesPage{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return NodesPage{}, err + } if view == NodeViewBasic { return decodeBasicNodes(resp.Body) @@ -407,6 +410,10 @@ func (c *Client) DescribeNode(ctx context.Context, nodeUUID string) (NodeDetails return NodeDetails{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return NodeDetails{}, err + } + var data fleetapi.ModelsNodeDetailsResponse if err := json.Unmarshal(resp.Body, &data); err != nil { return NodeDetails{}, err diff --git a/nvfleetint/node_health.go b/nvfleetint/node_health.go index e98cbab..89c0539 100644 --- a/nvfleetint/node_health.go +++ b/nvfleetint/node_health.go @@ -82,6 +82,10 @@ func (c *Client) NodeHealthHistory(ctx context.Context, nodeUUID string, opts No return NodeHealthHistory{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return NodeHealthHistory{}, err + } + var data fleetapi.ModelsNodeHealthHistoryResponse if err := json.Unmarshal(resp.Body, &data); err != nil { return NodeHealthHistory{}, err diff --git a/nvfleetint/nodegroup.go b/nvfleetint/nodegroup.go index 65c3bfe..b78620e 100644 --- a/nvfleetint/nodegroup.go +++ b/nvfleetint/nodegroup.go @@ -148,6 +148,9 @@ func (c *Client) ListNodeGroups(ctx context.Context, opts ListNodeGroupsOptions) if resp.StatusCode() != http.StatusOK { return NodeGroupsPage{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return NodeGroupsPage{}, err + } if view == NodeGroupViewBasic { return decodeBasicNodeGroups(resp.Body) diff --git a/nvfleetint/overview.go b/nvfleetint/overview.go index f6c03e7..c85f54d 100644 --- a/nvfleetint/overview.go +++ b/nvfleetint/overview.go @@ -64,6 +64,10 @@ func (c *Client) GetOverview(ctx context.Context, opts OverviewOptions) (Overvie return Overview{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return Overview{}, err + } + var data fleetapi.ModelsOverviewResponse if err := json.Unmarshal(resp.Body, &data); err != nil { return Overview{}, err diff --git a/nvfleetint/report.go b/nvfleetint/report.go index 82b6f40..e86a62e 100644 --- a/nvfleetint/report.go +++ b/nvfleetint/report.go @@ -294,6 +294,9 @@ func (c *Client) GetInventoryReport(ctx context.Context, opts InventoryReportOpt return InventoryReport{RawCSV: append([]byte(nil), resp.Body...)}, nil } + if err := validateResponsePayload(resp.Body); err != nil { + return InventoryReport{}, err + } return decodeInventoryReport(resp.Body) } @@ -323,6 +326,9 @@ func (c *Client) GetErrorReport(ctx context.Context, opts ErrorReportOptions) (E }, nil } + if err := validateResponsePayload(resp.Body); err != nil { + return ErrorReport{}, err + } return decodeErrorReport(resp.Body, normalized.View, normalized.GroupBy) } diff --git a/nvfleetint/responsevalidate.go b/nvfleetint/responsevalidate.go new file mode 100644 index 0000000..3d40954 --- /dev/null +++ b/nvfleetint/responsevalidate.go @@ -0,0 +1,234 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nvfleetint + +// This file checks response payloads against the constraints the OpenAPI +// contract actually declares, before the SDK maps them into domain types. +// +// The generated client decodes into strong Go types, so a value of the wrong +// JSON kind already fails to unmarshal. What that does not catch is a +// well-formed payload carrying values the contract forbids: an IP field that is +// not an address, a hostname carrying terminal escape sequences, an alert +// severity outside the enum. Those reach the operator as rendered fleet state +// and are what a tampered or compromised backend would use. +// +// Scope, stated plainly: this validates the constraints openapi.yaml declares +// for the fields the threat model names, keyed by field name. It is not a +// general JSON Schema engine bound to each operation's response schema — that +// would need a schema validator dependency and the spec embedded in the binary. + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "net" + "strings" + + "github.com/NVIDIA/fleet-intelligence-client/internal/generated/fleetapi" +) + +// ErrInvalidResponse indicates an API response did not match the contract and +// was rejected rather than decoded. Match it with errors.Is. +var ErrInvalidResponse = errors.New("invalid API response") + +// maxHostnameLength is the DNS limit on a fully qualified name +const maxHostnameLength = 253 + +// ResponseValidationError names the response field that failed validation. +// The value is rendered with %q so a hostile value cannot emit raw control +// bytes through an operator's terminal by way of the error message. +type ResponseValidationError struct { + Field string + Reason string +} + +// Error renders the field and the reason it was rejected +func (e *ResponseValidationError) Error() string { + return fmt.Sprintf("%s: field %q %s", ErrInvalidResponse, e.Field, e.Reason) +} + +// Unwrap ties the error to ErrInvalidResponse for errors.Is +func (e *ResponseValidationError) Unwrap() error { + return ErrInvalidResponse +} + +// fieldValidators maps a lowercased JSON field name to the check applied +// wherever that name appears in a response. Field names are matched +// case-insensitively because the contract spells the same concept both +// nodeUUID and nodeUuid depending on the endpoint. +// +// Only fields whose shape the spec constrains are listed. alertStatus, for +// example, is declared as a bare string whose description merely gives +// examples, so validating it against a value set would enforce a rule the +// contract does not make. +var fieldValidators = map[string]func(string) error{ + "hostname": validateResponseHostname, + "publicip": validateResponseIP, + "privateip": validateResponseIP, + "severity": validateResponseAlertSeverity, + "state": validateResponseAlertState, +} + +// Checks a JSON response body against the contract, returning a +// ResponseValidationError for the first field that violates it. +// +// The walk is streaming: it reads tokens rather than decoding the payload into +// an interface tree, so validating a large response does not add a second copy +// of it to memory. A body that is not JSON at all — the CSV and ZIP report +// payloads — is skipped, as is malformed JSON, which the caller's typed +// unmarshal reports with a better message. +func validateResponsePayload(data []byte) error { + decoder := json.NewDecoder(bytes.NewReader(data)) + decoder.UseNumber() + + // containers records the open nesting, true for an object. expectKey is + // true when the next string inside the innermost object is a field name + // rather than a value. + var containers []bool + expectKey := false + key := "" + + for { + token, err := decoder.Token() + if err != nil { + return nil + } + + switch value := token.(type) { + case json.Delim: + switch value { + case '{': + containers = append(containers, true) + expectKey = true + continue + case '[': + containers = append(containers, false) + expectKey = false + continue + default: + if len(containers) > 0 { + containers = containers[:len(containers)-1] + } + } + case string: + if expectKey { + key = value + expectKey = false + continue + } + if err := validateResponseField(key, value); err != nil { + return err + } + } + + // A completed value means the innermost object expects its next key. + // Inside an array, elements keep the array's own field name. + expectKey = len(containers) > 0 && containers[len(containers)-1] + } +} + +// Applies the universal and field-specific checks to one string value +func validateResponseField(key, value string) error { + if err := validateNoControlCharacters(key, value); err != nil { + return err + } + validate, ok := fieldValidators[strings.ToLower(key)] + if !ok { + return nil + } + if err := validate(value); err != nil { + return &ResponseValidationError{Field: key, Reason: err.Error()} + } + + return nil +} + +// Rejects control characters in any response string. Tab, newline, and return +// are allowed because alert messages legitimately wrap; the table renderer +// collapses them (internal/output.sanitizeTableCell) so they cannot forge rows. +// Everything else in C0 and C1 — most importantly ESC, which starts an ANSI +// sequence that could rewrite an operator's terminal — has no legitimate place +// in fleet data. +func validateNoControlCharacters(key, value string) error { + for _, r := range value { + if r == '\t' || r == '\n' || r == '\r' { + continue + } + if r < 0x20 || r == 0x7f || (r >= 0x80 && r <= 0x9f) { + return &ResponseValidationError{ + Field: key, + Reason: fmt.Sprintf("contains control character %U", r), + } + } + } + + return nil +} + +// Rejects an address field that is not an IP address. An empty value means the +// backend did not report one, which the contract allows. +func validateResponseIP(value string) error { + trimmed := strings.TrimSpace(value) + if trimmed == "" { + return nil + } + if net.ParseIP(trimmed) == nil { + return fmt.Errorf("is not a valid IP address: %q", value) + } + + return nil +} + +// Rejects a hostname carrying characters that cannot appear in a DNS name. +// This is a charset and length check rather than full RFC 1123 label parsing: +// real fleets carry names that bend the RFC, and rejecting a whole inventory +// listing over a leading digit or an underscore would turn a display concern +// into an outage of the tool. Anything that could carry markup, escapes, or +// whitespace into rendered output is still refused. +func validateResponseHostname(value string) error { + if value == "" { + return nil + } + if len(value) > maxHostnameLength { + return fmt.Errorf("exceeds the %d character DNS limit", maxHostnameLength) + } + for _, r := range value { + switch { + case r >= 'a' && r <= 'z', r >= 'A' && r <= 'Z', r >= '0' && r <= '9': + case r == '-', r == '.', r == '_': + default: + return fmt.Errorf("contains an invalid hostname character: %q", value) + } + } + + return nil +} + +// Rejects a severity outside the contract's enum. The allowed set comes from +// the generated types, so regenerating the client from an updated spec widens +// it automatically. This mirrors the SDK's existing rejection of an unknown +// severity on the request side. +func validateResponseAlertSeverity(value string) error { + if value == "" { + return nil + } + if !fleetapi.ModelsAlertSeverity(value).Valid() { + return fmt.Errorf("is not a valid alert severity: %q", value) + } + + return nil +} + +// Rejects an alert state outside the contract's enum +func validateResponseAlertState(value string) error { + if value == "" { + return nil + } + if !fleetapi.ModelsAlertState(value).Valid() { + return fmt.Errorf("is not a valid alert state: %q", value) + } + + return nil +} diff --git a/nvfleetint/responsevalidate_test.go b/nvfleetint/responsevalidate_test.go new file mode 100644 index 0000000..258067f --- /dev/null +++ b/nvfleetint/responsevalidate_test.go @@ -0,0 +1,230 @@ +// SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. +// SPDX-License-Identifier: Apache-2.0 + +package nvfleetint + +import ( + "context" + "errors" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// Verifies the field checks the OpenAPI contract declares, and that payload +// shapes which are not JSON objects of interest are left alone +func TestValidateResponsePayload(t *testing.T) { + cases := []struct { + name string + payload string + wantErr string + }{ + {name: "ordinary node", payload: `{"nodeUUID":"n-1","hostname":"gpu-001","publicIP":"10.0.0.1","privateIP":"192.168.1.1"}`}, + {name: "ipv6", payload: `{"publicIP":"2001:db8::1"}`}, + {name: "absent address", payload: `{"publicIP":"","privateIP":""}`}, + {name: "nested object", payload: `{"node":{"publicIP":"10.0.0.1"}}`}, + {name: "array of objects", payload: `{"nodes":[{"hostname":"gpu-1"},{"hostname":"gpu-2"}]}`}, + {name: "valid alert", payload: `{"severity":"Critical","state":"Resolved"}`}, + {name: "alert message may wrap", payload: `{"message":"line one\nline two\ttabbed"}`}, + + {name: "falsified ip", payload: `{"publicIP":"10.0.0.1; rm -rf /"}`, wantErr: "not a valid IP address"}, + {name: "hostname as ip", payload: `{"privateIP":"gpu-001.example.com"}`, wantErr: "not a valid IP address"}, + {name: "nested falsified ip", payload: `{"node":{"publicIP":"nope"}}`, wantErr: "not a valid IP address"}, + {name: "falsified ip in array", payload: `{"nodes":[{"hostname":"ok"},{"publicIP":"nope"}]}`, wantErr: "not a valid IP address"}, + {name: "hostname with markup", payload: `{"hostname":""}`, wantErr: "invalid hostname character"}, + {name: "hostname with space", payload: `{"hostname":"gpu 001"}`, wantErr: "invalid hostname character"}, + {name: "overlong hostname", payload: `{"hostname":"` + strings.Repeat("a", 254) + `"}`, wantErr: "DNS limit"}, + {name: "unknown severity", payload: `{"severity":"Catastrophic"}`, wantErr: "not a valid alert severity"}, + {name: "unknown state", payload: `{"state":"Exploded"}`, wantErr: "not a valid alert state"}, + + // A field name appearing as someone else's value must not be validated. + {name: "field name used as a value", payload: `{"note":"publicIP","other":"hostname"}`}, + // Payloads that are not JSON at all are the CSV and ZIP report bodies. + {name: "csv body", payload: "hostname,publicIP\ngpu-001,not-an-ip\n"}, + {name: "malformed json", payload: `{"publicIP":`}, + {name: "empty body", payload: ``}, + } + + for _, testCase := range cases { + t.Run(testCase.name, func(t *testing.T) { + err := validateResponsePayload([]byte(testCase.payload)) + if testCase.wantErr == "" { + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + return + } + if err == nil { + t.Fatal("expected an error") + } + if !errors.Is(err, ErrInvalidResponse) { + t.Fatalf("error is not ErrInvalidResponse: %v", err) + } + if !strings.Contains(err.Error(), testCase.wantErr) { + t.Fatalf("error %q does not mention %q", err, testCase.wantErr) + } + }) + } +} + +// Verifies control characters are refused in any response string, whatever the +// field, since they are what would let a tampered payload rewrite an operator's +// terminal through rendered output +func TestValidateResponseRejectsControlCharacters(t *testing.T) { + hostile := map[string]string{ + "ansi escape": "{\"hostname\":\"gpu-001\\u001b[31mFAKE\"}", + "null byte": "{\"message\":\"a\\u0000b\"}", + "bell": "{\"component\":\"\\u0007\"}", + "c1 control": "{\"message\":\"a\\u0085b\"}", + "escape in nested": "{\"node\":{\"error\":\"\\u001b]0;title\\u0007\"}}", + } + + for name, payload := range hostile { + t.Run(name, func(t *testing.T) { + err := validateResponsePayload([]byte(payload)) + if err == nil { + t.Fatal("expected a control character to be rejected") + } + if !errors.Is(err, ErrInvalidResponse) { + t.Fatalf("error is not ErrInvalidResponse: %v", err) + } + if !strings.Contains(err.Error(), "control character") { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} + +// Verifies the rejection message cannot itself carry escape sequences into the +// terminal that is about to print it +func TestResponseValidationErrorDoesNotEmitControlBytes(t *testing.T) { + err := validateResponsePayload([]byte("{\"hostname\":\"gpu\\u001b[31m\"}")) + if err == nil { + t.Fatal("expected an error") + } + if strings.ContainsRune(err.Error(), 0x1b) { + t.Fatalf("error echoed a raw escape byte: %q", err.Error()) + } +} + +// Verifies a tampered payload fails the SDK call rather than being decoded and +// rendered as fleet state +func TestTamperedResponseFailsTheCall(t *testing.T) { + cases := map[string]string{ + "falsified ip": `{"nodeUUID":"node-1","hostname":"gpu-001","publicIP":"attacker-controlled"}`, + "escaped hostname": "{\"nodeUUID\":\"node-1\",\"hostname\":\"gpu-001\\u001b[2K\"}", + } + + for name, payload := range cases { + t.Run(name, func(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(payload)) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + if _, err := client.DescribeNode(context.Background(), "node-1"); !errors.Is(err, ErrInvalidResponse) { + t.Fatalf("expected ErrInvalidResponse, got %v", err) + } + }) + } +} + +// Verifies an untampered response still decodes, so the validation is not +// simply failing everything +func TestValidResponseStillDecodes(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"nodeUUID":"node-1","hostname":"gpu-001","publicIP":"10.0.0.1","privateIP":"192.168.1.1"}`)) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + + node, err := client.DescribeNode(context.Background(), "node-1") + if err != nil { + t.Fatalf("describe node failed: %v", err) + } + if node.Hostname != "gpu-001" || node.PublicIP != "10.0.0.1" { + t.Fatalf("unexpected node: %#v", node.Node) + } +} + +// Verifies validation runs on every SDK entry point that decodes JSON, so a +// tampered field cannot slip through whichever endpoint happens to carry it +func TestEveryJSONEndpointValidates(t *testing.T) { + // A hostname carrying an ANSI escape is accepted by every response shape, + // since the check is keyed on the field name wherever it appears. + const tampered = "{\"hostname\":\"gpu-001\\u001b[31m\"}" + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(tampered)) + })) + defer server.Close() + + client, err := NewClient(server.URL, "test-key") + if err != nil { + t.Fatalf("new client failed: %v", err) + } + ctx := context.Background() + + calls := map[string]func() error{ + "GetAuthStatus": func() error { _, err := client.GetAuthStatus(ctx); return err }, + "GetOverview": func() error { _, err := client.GetOverview(ctx, OverviewOptions{}); return err }, + "ListNodes": func() error { _, err := client.ListNodes(ctx, ListNodesOptions{}); return err }, + "DescribeNode": func() error { _, err := client.DescribeNode(ctx, "node-1"); return err }, + "ListNodeGroups": func() error { _, err := client.ListNodeGroups(ctx, ListNodeGroupsOptions{}); return err }, + "ListComputeZones": func() error { _, err := client.ListComputeZones(ctx, ListComputeZonesOptions{}); return err }, + "ListAlerts": func() error { _, err := client.ListAlerts(ctx, ListAlertsOptions{}); return err }, + "ListAlertTimelineNodes": func() error { + _, err := client.ListAlertTimelineNodes(ctx, ListAlertTimelineNodesOptions{}) + return err + }, + "ListNodeAlertTimeline": func() error { + _, err := client.ListNodeAlertTimeline(ctx, ListNodeAlertTimelineOptions{NodeUUID: "node-1"}) + return err + }, + "DescribeAlertTimeline": func() error { + _, err := client.DescribeAlertTimeline(ctx, "node-1", "alert-1") + return err + }, + "ListEvents": func() error { _, err := client.ListEvents(ctx, EventListOptions{Window: "24h"}); return err }, + "GetEventBuckets": func() error { _, err := client.GetEventBuckets(ctx, EventBucketsOptions{Window: "24h"}); return err }, + "ListTags": func() error { _, err := client.ListTags(ctx, TagListOptions{}); return err }, + "NodeHealthHistory": func() error { + _, err := client.NodeHealthHistory(ctx, "node-1", NodeHealthHistoryOptions{ + StartTime: "2026-04-07T00:00:00Z", + EndTime: "2026-04-14T00:00:00Z", + }) + return err + }, + "GetInventoryReport": func() error { + _, err := client.GetInventoryReport(ctx, InventoryReportOptions{}) + return err + }, + "GetErrorReport": func() error { + _, err := client.GetErrorReport(ctx, ErrorReportOptions{ + View: ErrorReportViewOverview, TimeMode: ErrorReportTimeModeRelative, Window: "24h", + }) + return err + }, + } + + for name, call := range calls { + t.Run(name, func(t *testing.T) { + if err := call(); !errors.Is(err, ErrInvalidResponse) { + t.Fatalf("tampered payload was not rejected, got %v", err) + } + }) + } +} diff --git a/nvfleetint/tag.go b/nvfleetint/tag.go index f13c998..11e965f 100644 --- a/nvfleetint/tag.go +++ b/nvfleetint/tag.go @@ -66,6 +66,10 @@ func (c *Client) ListTags(ctx context.Context, opts TagListOptions) (TagList, er return TagList{}, newAPIError(resp.StatusCode(), resp.Status(), resp.Body) } + if err := validateResponsePayload(resp.Body); err != nil { + return TagList{}, err + } + var data fleetapi.ModelsListTagsResponse if err := json.Unmarshal(resp.Body, &data); err != nil { return TagList{}, err