From 5585ceec85270ef3f312bb8580f93fc56239bffb Mon Sep 17 00:00:00 2001 From: mlsmaycon Date: Tue, 11 Aug 2026 02:16:22 +0000 Subject: [PATCH] [management,client] Normalize anonymize_level and sanity-check the bundle upload URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review follow-ups. The API validated anonymize_level after trimming and lowercasing but persisted the value verbatim, so " default " passed as default yet reached the client — which only lowercases — as an unrecognized value it resolves to strict. Persist the normalized form so what was validated is what the client parses. The remote debug bundle job forwarded the management-supplied upload URL to the uploader unchecked and logged it at info level, where it can leak a host, credentials, or query tokens. Reject a malformed or non-https URL before generating the bundle, and keep the URL out of the info-level line while leaving the full parameters at debug. The accepted host is left unrestricted for now, pending a decision on management-directed uploads. --- client/internal/engine.go | 32 +++++++++++++++++++++++- client/internal/engine_bundle_test.go | 35 +++++++++++++++++++++++++++ management/server/types/job.go | 9 +++++-- management/server/types/job_test.go | 19 +++++++++++++++ 4 files changed, 92 insertions(+), 3 deletions(-) create mode 100644 client/internal/engine_bundle_test.go diff --git a/client/internal/engine.go b/client/internal/engine.go index 6ca2115be..2b8189490 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -1365,7 +1365,17 @@ func (e *Engine) receiveJobEvents() { } func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobResponse_Bundle, error) { - log.Infof("handle remote debug bundle request: %s", params.String()) + // The upload URL can carry a host, credentials, or query tokens, so it is + // kept out of the info-level line; the full parameters stay available at + // debug level for troubleshooting. + log.Infof("handle remote debug bundle request: anonymize=%v anonymize_level=%q log_file_count=%d bundle_for=%v bundle_for_time=%d", + params.GetAnonymize(), params.GetAnonymizeLevel(), params.GetLogFileCount(), params.GetBundleFor(), params.GetBundleForTime()) + log.Debugf("remote debug bundle request parameters: %s", params.String()) + + if err := validateBundleUploadURL(params.GetUploadUrl()); err != nil { + return nil, err + } + syncResponse, err := e.GetLatestSyncResponse() if err != nil { log.Warnf("get latest sync response: %v", err) @@ -1406,6 +1416,26 @@ func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobR return response, nil } +// validateBundleUploadURL sanity-checks a management-supplied upload URL for a +// remote debug bundle job. An empty value is accepted — the executor falls back +// to the default upload service. A non-empty value must be a well-formed https +// URL with a host; a malformed value or a plaintext scheme is rejected. This +// deliberately does not constrain which host may receive the bundle; that +// policy is left open pending a decision on management-directed uploads. +func validateBundleUploadURL(raw string) error { + if raw == "" { + return nil + } + parsed, err := url.Parse(raw) + if err != nil { + return fmt.Errorf("parse upload URL: %w", err) + } + if parsed.Scheme != "https" || parsed.Host == "" { + return fmt.Errorf("upload URL must be an https URL with a host") + } + return nil +} + // receiveManagementEvents connects to the Management Service event stream to receive updates from the management service // E.g. when a new peer has been registered and we are allowed to connect to it. func (e *Engine) receiveManagementEvents() { diff --git a/client/internal/engine_bundle_test.go b/client/internal/engine_bundle_test.go new file mode 100644 index 000000000..d736e2591 --- /dev/null +++ b/client/internal/engine_bundle_test.go @@ -0,0 +1,35 @@ +package internal + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// TestValidateBundleUploadURL covers the sanity check applied to a +// management-supplied upload URL before a remote debug bundle is generated. +func TestValidateBundleUploadURL(t *testing.T) { + for _, tc := range []struct { + name string + raw string + wantErr bool + }{ + {name: "empty falls back to default", raw: ""}, + {name: "https with host", raw: "https://upload.debug.netbird.io/upload"}, + {name: "https self-hosted host", raw: "https://upload.example.com"}, + {name: "plaintext rejected", raw: "http://upload.example.com", wantErr: true}, + {name: "missing host rejected", raw: "https:///upload", wantErr: true}, + {name: "non-url scheme rejected", raw: "ftp://upload.example.com", wantErr: true}, + {name: "garbage rejected", raw: "://not a url", wantErr: true}, + } { + t.Run(tc.name, func(t *testing.T) { + err := validateBundleUploadURL(tc.raw) + if tc.wantErr { + require.Error(t, err, "an invalid upload URL must be rejected") + return + } + assert.NoError(t, err, "a valid or empty upload URL must be accepted") + }) + } +} diff --git a/management/server/types/job.go b/management/server/types/job.go index e6d1be0ab..db2d0d42c 100644 --- a/management/server/types/job.go +++ b/management/server/types/job.go @@ -154,13 +154,18 @@ func validateAndBuildBundleParams(req api.WorkloadRequest, workload *Workload) e } // validate anonymize_level: omitted or empty defaults on the client; // otherwise it must name a known level. An unknown value is rejected here - // rather than silently escalated, so a typo surfaces at job creation. + // rather than silently escalated, so a typo surfaces at job creation. The + // normalized (trimmed, lowercased) value is persisted so it matches what + // the client parses — the client only lowercases, so a stored " default " + // would otherwise resolve to strict. if lvl := bundle.Parameters.AnonymizeLevel; lvl != nil { - switch strings.ToLower(strings.TrimSpace(*lvl)) { + normalized := strings.ToLower(strings.TrimSpace(*lvl)) + switch normalized { case "", anonymize.LevelDefaultString, anonymize.LevelStrictString: default: return fmt.Errorf("anonymize_level must be %q or %q, got %q", anonymize.LevelDefaultString, anonymize.LevelStrictString, *lvl) } + bundle.Parameters.AnonymizeLevel = &normalized } workload.Parameters, err = json.Marshal(bundle.Parameters) diff --git a/management/server/types/job_test.go b/management/server/types/job_test.go index aad3e720e..428c2215b 100644 --- a/management/server/types/job_test.go +++ b/management/server/types/job_test.go @@ -80,6 +80,7 @@ func TestNewJob_AnonymizeLevelValidation(t *testing.T) { {name: "default", level: strPtr("default")}, {name: "strict", level: strPtr("strict")}, {name: "mixed case", level: strPtr("Strict")}, + {name: "padded", level: strPtr(" default ")}, {name: "unknown", level: strPtr("verbose"), wantErr: true}, } { t.Run(tc.name, func(t *testing.T) { @@ -96,6 +97,24 @@ func TestNewJob_AnonymizeLevelValidation(t *testing.T) { } } +// TestNewJob_AnonymizeLevelNormalized verifies an accepted level is persisted +// trimmed and lowercased, so it reaches the client as a value the client's +// lowercase-only parser resolves correctly rather than escalating to strict. +func TestNewJob_AnonymizeLevelNormalized(t *testing.T) { + job, err := NewJob("user-1", "acc-1", "peer-1", newBundleJobRequest(t, api.BundleParameters{ + BundleFor: false, + LogFileCount: 100, + Anonymize: true, + AnonymizeLevel: strPtr(" Default "), + })) + require.NoError(t, err, "a padded known level must be accepted") + + req, err := job.ToStreamJobRequest() + require.NoError(t, err, "ToStreamJobRequest must succeed") + assert.Equal(t, "default", req.GetBundle().GetAnonymizeLevel(), + "the persisted level must be normalized so the client does not resolve it to strict") +} + // TestBuildStreamBundleResponse_OmittedFieldsMapToEmpty verifies that omitted // optional fields map to the empty proto string, which the client resolves to // its defaults (default anonymization level, default upload server).