fix: validate derived backend endpoints#152
Merged
jingxiang-z merged 4 commits intomainfrom Apr 1, 2026
Merged
Conversation
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
📝 WalkthroughWalkthroughIntroduces an internal endpoint package for URL parsing/validation and joining; integrates it across CLI commands, attestation, and exporter modules to validate server and backend endpoints (loopback/local checks and HTTPS enforcement) and replace ad-hoc URL building with centralized helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/fleetint/inject.go (1)
114-117:⚠️ Potential issue | 🟠 MajorAdd explicit HTTP timeouts for inject/clear requests.
http.Postuses the default client without a timeout, so these calls can hang indefinitely under network/server stalls. The codebase already establishes a pattern of using&http.Client{Timeout: 5 * time.Second}instatus.go—apply the same approach here.Suggested patch
import ( "bytes" "encoding/json" "fmt" "net/http" "net/url" + "time" @@ - resp, err := http.Post(url, "application/json", bytes.NewBuffer(jsonData)) + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Post(url, "application/json", bytes.NewBuffer(jsonData)) @@ - resp, err := http.Post(url, "application/json", bytes.NewBuffer(jsonData)) + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Post(url, "application/json", bytes.NewBuffer(jsonData))Also applies to: 187-190
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetint/inject.go` around lines 114 - 117, Replace the use of the package-level http.Post (which has no timeout) with a local http.Client configured with a timeout (e.g. client := &http.Client{Timeout: 5 * time.Second}) and call client.Post for both places where http.Post is used in this file; ensure you keep the existing error wrapping (fmt.Errorf("failed to make request to %s: %w", url, err)), handle resp as before, and remember to close resp.Body after a successful response. Refer to the http.Post calls in this file (the inject/clear request handlers) and mirror the timeout pattern used in status.go.
🧹 Nitpick comments (2)
internal/exporter/exporter.go (1)
268-309: Consider extracting shared endpoint-refresh logic for metrics/logs.Both branches duplicate read/validate/apply/log flow. A small helper would reduce drift risk and keep future policy changes consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/exporter/exporter.go` around lines 268 - 309, Extract the duplicated read/validate/apply/log flow into a small helper (e.g., refreshEndpoint(ctx context.Context, metaKey string, current string) string or a method on the exporter like (e *Exporter) refreshEndpoint(ctx context.Context, metaKey string, getCurrent func() string, setCurrent func(string))) that uses pkgmetadata.ReadMetadata and endpoint.ValidateBackendEndpoint, preserves the existing behavior for invalid values (log.Logger.Errorw + treat as empty), error-on-read (log.Logger.Errorw), and the update/clear logging (log.Logger.Infow) and returns or applies the final endpoint string; then replace the two blocks that handle "metrics_endpoint" and "logs_endpoint" to call this helper and set e.options.config.MetricsEndpoint / e.options.config.LogsEndpoint accordingly so semantics remain identical but duplication is removed.cmd/fleetint/enroll.go (1)
62-86: Optional: reduce repeated endpoint join blocks.This section is correct; you could simplify maintenance by extracting a tiny local helper for repeated
"api/v1/health/*"joins.♻️ Proposed refactor
- // Construct enroll endpoint - enrollEndpoint, err := endpoint.JoinPath(baseURL, "api", "v1", "health", "enroll") + buildHealthEndpoint := func(name string) (string, error) { + return endpoint.JoinPath(baseURL, "api", "v1", "health", name) + } + + // Construct enroll endpoint + enrollEndpoint, err := buildHealthEndpoint("enroll") if err != nil { return fmt.Errorf("failed to construct enroll endpoint: %w", err) } @@ - metricsEndpoint, err := endpoint.JoinPath(baseURL, "api", "v1", "health", "metrics") + metricsEndpoint, err := buildHealthEndpoint("metrics") if err != nil { return fmt.Errorf("failed to construct metrics endpoint: %w", err) } - logsEndpoint, err := endpoint.JoinPath(baseURL, "api", "v1", "health", "logs") + logsEndpoint, err := buildHealthEndpoint("logs") if err != nil { return fmt.Errorf("failed to construct logs endpoint: %w", err) } - nonceEndpoint, err := endpoint.JoinPath(baseURL, "api", "v1", "health", "nonce") + nonceEndpoint, err := buildHealthEndpoint("nonce") if err != nil { return fmt.Errorf("failed to construct nonce endpoint: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetint/enroll.go` around lines 62 - 86, Extract a small helper function (e.g., buildHealthEndpoint) to DRY the repeated endpoint.JoinPath(baseURL, "api", "v1", "health", ... ) logic used to create enrollEndpoint, metricsEndpoint, logsEndpoint, and nonceEndpoint; replace each direct endpoint.JoinPath call with buildHealthEndpoint(ctxSegment) that returns (string, error) and reuse it for "enroll", "metrics", "logs", and "nonce" so error handling remains identical but the join code is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/endpoint/endpoint.go`:
- Around line 35-43: ValidateLocalServerURL currently permits server URLs with
non-root paths; update it to reject any URL whose parsed.Path is non-empty or
not equal to "/" so local daemon base URLs must be root-only. In the function
(around the existing host/isLoopbackHost checks), add a check using parsed.Path
(or parsed.EscapedPath) and return an error like "server URL must not contain a
path; use the root URL" when the path is not empty or "/" to prevent JoinPath
from producing unintended routes. Ensure the error message references the
offending path for clarity and keep the isLoopbackHost and host validations
intact.
---
Outside diff comments:
In `@cmd/fleetint/inject.go`:
- Around line 114-117: Replace the use of the package-level http.Post (which has
no timeout) with a local http.Client configured with a timeout (e.g. client :=
&http.Client{Timeout: 5 * time.Second}) and call client.Post for both places
where http.Post is used in this file; ensure you keep the existing error
wrapping (fmt.Errorf("failed to make request to %s: %w", url, err)), handle resp
as before, and remember to close resp.Body after a successful response. Refer to
the http.Post calls in this file (the inject/clear request handlers) and mirror
the timeout pattern used in status.go.
---
Nitpick comments:
In `@cmd/fleetint/enroll.go`:
- Around line 62-86: Extract a small helper function (e.g., buildHealthEndpoint)
to DRY the repeated endpoint.JoinPath(baseURL, "api", "v1", "health", ... )
logic used to create enrollEndpoint, metricsEndpoint, logsEndpoint, and
nonceEndpoint; replace each direct endpoint.JoinPath call with
buildHealthEndpoint(ctxSegment) that returns (string, error) and reuse it for
"enroll", "metrics", "logs", and "nonce" so error handling remains identical but
the join code is centralized.
In `@internal/exporter/exporter.go`:
- Around line 268-309: Extract the duplicated read/validate/apply/log flow into
a small helper (e.g., refreshEndpoint(ctx context.Context, metaKey string,
current string) string or a method on the exporter like (e *Exporter)
refreshEndpoint(ctx context.Context, metaKey string, getCurrent func() string,
setCurrent func(string))) that uses pkgmetadata.ReadMetadata and
endpoint.ValidateBackendEndpoint, preserves the existing behavior for invalid
values (log.Logger.Errorw + treat as empty), error-on-read (log.Logger.Errorw),
and the update/clear logging (log.Logger.Infow) and returns or applies the final
endpoint string; then replace the two blocks that handle "metrics_endpoint" and
"logs_endpoint" to call this helper and set e.options.config.MetricsEndpoint /
e.options.config.LogsEndpoint accordingly so semantics remain identical but
duplication is removed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be341500-ee91-4d0c-b98c-b830cbd44e5a
📒 Files selected for processing (10)
cmd/fleetint/enroll.gocmd/fleetint/inject.gocmd/fleetint/security_test.gocmd/fleetint/status.gointernal/attestation/attestation.gointernal/attestation/attestation_test.gointernal/endpoint/endpoint.gointernal/endpoint/endpoint_test.gointernal/exporter/exporter.gointernal/exporter/exporter_test.go
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/endpoint/endpoint.go (1)
62-63: Add a nil guard inJoinPathto avoid panic on invalid callers.
base.String()on Line 63 will panic ifbase == nil. Returning an error keeps this helper fail-safe.Proposed patch
func JoinPath(base *url.URL, elems ...string) (string, error) { + if base == nil { + return "", fmt.Errorf("base URL is nil") + } return url.JoinPath(base.String(), elems...) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/endpoint/endpoint.go` around lines 62 - 63, The JoinPath helper currently calls base.String() which will panic if base is nil; add a nil check at the start of JoinPath (func JoinPath(base *url.URL, elems ...string)) and return a non-nil error when base == nil instead of calling base.String(); otherwise proceed to call url.JoinPath with base.String() and elems. Ensure the returned error is descriptive (e.g., "nil base URL") so callers can handle it.cmd/fleetint/inject.go (1)
115-117: Consider extracting shared POST logic for inject/clear paths.Both branches create the same timeout client and POST flow; a small helper would reduce duplication and drift risk.
Refactor sketch
+func postJSON(url string, body []byte) (*http.Response, error) { + client := &http.Client{Timeout: 5 * time.Second} + return client.Post(url, "application/json", bytes.NewBuffer(body)) +} ... - client := &http.Client{Timeout: 5 * time.Second} - resp, err := client.Post(url, "application/json", bytes.NewBuffer(jsonData)) + resp, err := postJSON(url, jsonData) ... - client := &http.Client{Timeout: 5 * time.Second} - resp, err := client.Post(url, "application/json", bytes.NewBuffer(jsonData)) + resp, err := postJSON(url, jsonData)Also applies to: 189-191
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/fleetint/inject.go` around lines 115 - 117, The inject.go code duplicates HTTP POST setup (creating a timeout http.Client and calling client.Post with "application/json") in the inject and clear branches; extract that shared logic into a small helper like postJSONWithTimeout(url string, jsonData []byte, timeout time.Duration) (resp *http.Response, err error) (or similarly named) and replace the duplicated blocks in the inject/clear handlers to call that helper; update callers where the helper is used (the blocks around client := &http.Client{Timeout: 5 * time.Second} and client.Post(...)) and ensure errors and resp are handled the same as before.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/fleetint/inject.go`:
- Around line 115-117: The inject.go code duplicates HTTP POST setup (creating a
timeout http.Client and calling client.Post with "application/json") in the
inject and clear branches; extract that shared logic into a small helper like
postJSONWithTimeout(url string, jsonData []byte, timeout time.Duration) (resp
*http.Response, err error) (or similarly named) and replace the duplicated
blocks in the inject/clear handlers to call that helper; update callers where
the helper is used (the blocks around client := &http.Client{Timeout: 5 *
time.Second} and client.Post(...)) and ensure errors and resp are handled the
same as before.
In `@internal/endpoint/endpoint.go`:
- Around line 62-63: The JoinPath helper currently calls base.String() which
will panic if base is nil; add a nil check at the start of JoinPath (func
JoinPath(base *url.URL, elems ...string)) and return a non-nil error when base
== nil instead of calling base.String(); otherwise proceed to call url.JoinPath
with base.String() and elems. Ensure the returned error is descriptive (e.g.,
"nil base URL") so callers can handle it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 47368f96-7175-4a0c-bc2c-920823f83827
📒 Files selected for processing (3)
cmd/fleetint/inject.gocmd/fleetint/security_test.gointernal/endpoint/endpoint.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/fleetint/security_test.go
jingxiang-z
added a commit
that referenced
this pull request
Apr 14, 2026
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
jingxiang-z
added a commit
that referenced
this pull request
Apr 14, 2026
Signed-off-by: Jingxiang Zhang <jingzhang@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Why
The agent was trusting derived or stored endpoint values too loosely in a few paths. The main trust anchor is the enrollment endpoint shape, and downstream endpoints should get at least basic validation before use.
Impact
This hardens
enroll,status,inject, attestation nonce fetches, and exporter metadata refreshes against malformed or unsafe endpoint values while preserving the intended localhost client flow.Validation
go test ./internal/exporter ./cmd/fleetint ./internal/endpoint ./internal/attestationSummary by CodeRabbit
Bug Fixes
Tests