From 9e9f5f304c3eca600ab4fbe1e6adc8e01d5d7a7c Mon Sep 17 00:00:00 2001 From: riccardom Date: Mon, 14 Sep 2026 11:34:11 +0200 Subject: [PATCH] [client] Strip URLs out of debug bundle upload errors Redacting the URL in the log line left the error itself untouched, and Go's *url.Error prints the URL whole. That error does not stay local: it becomes UploadFailureReason for the CLI and the desktop UI, and for a remote job it is stored in the management server's job record and shown in the dashboard. A sample from a failed job: Client error: 'upload debug bundle: get presigned URL: Get "https://helloworld.asda1234:2356?id=eb23d149..."' The service URL can carry userinfo or a query token, and the presigned URL the service hands back carries credentials in its query by design, so the second step leaks more than the first. UploadDebugBundle now rewrites every URL in its error down to scheme://host, on the way out, which covers the daemon, both mobile SDKs and the job runner at once. The original error stays reachable through Unwrap. Reported by cubic on #7514. --- client/internal/debug/upload.go | 41 ++++++++++++ client/internal/debug/upload_redact_test.go | 72 +++++++++++++++++++++ 2 files changed, 113 insertions(+) create mode 100644 client/internal/debug/upload_redact_test.go diff --git a/client/internal/debug/upload.go b/client/internal/debug/upload.go index 88fde6d6f..d3a1ca9cf 100644 --- a/client/internal/debug/upload.go +++ b/client/internal/debug/upload.go @@ -10,6 +10,8 @@ import ( "net/http" neturl "net/url" "os" + "regexp" + "strings" "github.com/netbirdio/netbird/upload-server/types" ) @@ -65,6 +67,13 @@ func rejectInsecureRedirect(req *http.Request, via []*http.Request) error { } func UploadDebugBundle(ctx context.Context, url, managementURL, filePath string, insecure bool) (key string, err error) { + // Every error out of here is surfaced somewhere durable: the daemon log, the + // CLI, and — for a remote job — the management server's job record and the + // dashboard. Go's *url.Error prints the URL whole, and the presigned URL the + // service hands back can carry credentials in its query, so nothing leaves + // this function with a URL longer than scheme://host. + defer func() { err = redactURLsInError(err) }() + if !insecure { if err := requireHTTPS("upload service URL", url); err != nil { return "", err @@ -168,3 +177,35 @@ func getUploadURL(ctx context.Context, serviceURL string, managementURL string, func getURLHash(url string) string { return fmt.Sprintf("%x", sha256.Sum256([]byte(url))) } + +// urlInText matches an absolute http(s) URL inside a free-form message. +var urlInText = regexp.MustCompile(`https?://[^\s"']+`) + +// redactedError keeps the original error reachable for errors.Is/As while +// presenting a message with every URL cut down to scheme://host. +type redactedError struct { + msg string + err error +} + +func (e *redactedError) Error() string { return e.msg } +func (e *redactedError) Unwrap() error { return e.err } + +func redactURLsInError(err error) error { + if err == nil { + return nil + } + + msg := err.Error() + redacted := urlInText.ReplaceAllStringFunc(msg, func(raw string) string { + parsed, perr := neturl.Parse(strings.TrimRight(raw, `.,;:)]}"'`)) + if perr != nil || parsed.Host == "" { + return "(redacted URL)" + } + return parsed.Scheme + "://" + parsed.Host + }) + if redacted == msg { + return err + } + return &redactedError{msg: redacted, err: err} +} diff --git a/client/internal/debug/upload_redact_test.go b/client/internal/debug/upload_redact_test.go new file mode 100644 index 000000000..896201fea --- /dev/null +++ b/client/internal/debug/upload_redact_test.go @@ -0,0 +1,72 @@ +package debug + +import ( + "errors" + "fmt" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestRedactURLsInError(t *testing.T) { + sentinel := errors.New("boom") + + tests := []struct { + name string + err error + want string + }{ + {name: "nil stays nil"}, + { + name: "no URL is left alone", + err: errors.New("file too large"), + want: "file too large", + }, + { + // What a failed GET actually looks like: *url.Error prints the URL + // whole, query included. + name: "service URL loses its query", + err: fmt.Errorf("get presigned URL: %w", &url.Error{ + Op: "Get", + URL: "https://upload.example.com/upload-url?id=deadbeef", + Err: errors.New("no such host"), + }), + want: `get presigned URL: Get "https://upload.example.com": no such host`, + }, + { + // The presigned PUT URL is the one that carries credentials. + name: "presigned URL loses its credentials", + err: errors.New(`upload failed: Put "https://bucket.s3.amazonaws.com/k?X-Amz-Signature=abc123&X-Amz-Credential=AKIA": timeout`), + want: `upload failed: Put "https://bucket.s3.amazonaws.com": timeout`, + }, + { + name: "userinfo does not survive", + err: errors.New(`Get "https://user:hunter2@upload.example.com/upload-url": refused`), + want: `Get "https://upload.example.com": refused`, + }, + { + name: "two URLs are both cut", + err: errors.New(`redirect from https://a.example.com/x?t=1 to https://b.example.com/y?t=2`), + want: `redirect from https://a.example.com to https://b.example.com`, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := redactURLsInError(tc.err) + if tc.err == nil { + assert.NoError(t, got) + return + } + require.Error(t, got) + assert.Equal(t, tc.want, got.Error()) + }) + } + + t.Run("the original error stays reachable", func(t *testing.T) { + wrapped := fmt.Errorf(`Get "https://upload.example.com/x?t=1": %w`, sentinel) + assert.ErrorIs(t, redactURLsInError(wrapped), sentinel) + }) +}