[management, client] Take the debug-bundle upload destination from management #7514
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| package debug | ||
|
|
||
| import ( | ||
| "github.com/netbirdio/netbird/upload-server/types" | ||
| ) | ||
|
|
||
| // ResolveUploadURL decides where a debug bundle is uploaded. | ||
| // | ||
| // requested is a destination a caller named explicitly — an MDM override, the | ||
| // CLI's --upload-bundle-url, a remote job's upload_url; it always wins, and the | ||
| // callers that accept one gate it separately (see requirePrivilegeForUploadURL: | ||
| // any host other than the default needs a privileged caller). published is what | ||
| // the management server of this deployment advertises, which the engine holds | ||
| // (Engine.DebugUploadURL). With neither, the upload service NetBird runs is the | ||
| // default, for a self-hosted deployment as much as for a cloud one: an operator | ||
| // who needs the bundles to stay inside their own infrastructure points either | ||
| // knob at their own upload service, and until they do the everyday | ||
| // "collect a bundle and send it to support" flow keeps working. | ||
| func ResolveUploadURL(requested, published string) string { | ||
| if requested != "" { | ||
| return requested | ||
| } | ||
|
|
||
| if published != "" { | ||
| return published | ||
| } | ||
|
|
||
| return types.DefaultBundleURL | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,53 @@ | ||
| package debug | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
|
|
||
| "github.com/netbirdio/netbird/upload-server/types" | ||
| ) | ||
|
|
||
| func TestResolveUploadURL(t *testing.T) { | ||
| const ( | ||
| operatorURL = "https://upload.example.com/upload-url" | ||
| requestedURL = "https://requested.example.com/upload-url" | ||
| ) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| requested string | ||
| published string | ||
| want string | ||
| }{ | ||
| { | ||
| name: "requested wins over published", | ||
| requested: requestedURL, | ||
| published: operatorURL, | ||
| want: requestedURL, | ||
| }, | ||
| { | ||
| name: "requested wins with nothing published", | ||
| requested: requestedURL, | ||
| want: requestedURL, | ||
| }, | ||
| { | ||
| name: "published used when nothing requested", | ||
| published: operatorURL, | ||
| want: operatorURL, | ||
| }, | ||
| { | ||
| // The default stays the service NetBird runs whatever the | ||
| // deployment: an operator who wants the bundles elsewhere says so, | ||
| // and until then collecting one and sending it to support works. | ||
| name: "nothing configured falls back to the NetBird service", | ||
| want: types.DefaultBundleURL, | ||
| }, | ||
| } | ||
|
|
||
| for _, tc := range tests { | ||
| t.Run(tc.name, func(t *testing.T) { | ||
| assert.Equal(t, tc.want, ResolveUploadURL(tc.requested, tc.published)) | ||
| }) | ||
| } | ||
| } |
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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,39 @@ 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. The | ||
| // class stops at the delimiters an error message wraps a URL in — quotes, | ||
| // backticks, angle brackets, parens and braces — so the match does not run past | ||
| // the URL and swallow the prose after it. TrimRight below then drops trailing | ||
| // sentence punctuation, which a bare URL at the end of a clause picks up. | ||
| var urlInText = regexp.MustCompile("https?://[^\\s\"'`<>\\[\\]{}()]+") | ||
|
Contributor
There was a problem hiding this comment. 🔒 Security & Privacy | 🛡️ Analyzed with Security Review | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: set -eu
printf '%s\n' '--- upload.go outline ---'
ast-grep outline client/internal/debug/upload.go
printf '%s\n' '--- relevant source ---'
rg -n -A35 -B20 'urlInText|redactURLs|ReplaceAllString|regexp.MustCompile' client/internal/debug/upload.go
printf '%s\n' '--- module Go version ---'
rg -n '^go ' go.modRepository: netbirdio/netbird Length of output: 5109 Sensitive Data Exposure CWE: CWE-200 — Exposure of Sensitive Information to an Unauthorized Actor Match bracketed IPv6 hosts before redaction.
Proposed fix-var urlInText = regexp.MustCompile("https?://[^\\s\"'`<>\\[\\]{}()]+")
+var urlInText = regexp.MustCompile("https?://(?:\\[[^\\]\\s\"'`<>{}()]+\\]|[^\\s\"'<>\\[\\]{}()])+")🤖 Prompt for AI Agents |
||
|
|
||
| // 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} | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| 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`, | ||
| }, | ||
| { | ||
| // The match must stop at the delimiter, not run on and eat the | ||
| // words after it. | ||
| name: "closing paren and the prose after it survive", | ||
| err: errors.New(`(see https://upload.example.com/x?t=1) for details`), | ||
| want: `(see https://upload.example.com) for details`, | ||
| }, | ||
| { | ||
| name: "angle brackets survive", | ||
| err: errors.New(`tried <https://a.example.com/p?q=1> and failed`), | ||
| want: `tried <https://a.example.com> and failed`, | ||
| }, | ||
| { | ||
| name: "backticks survive", | ||
| err: errors.New("use `https://b.example.com/p` instead"), | ||
| want: "use `https://b.example.com` instead", | ||
| }, | ||
| } | ||
|
|
||
| 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) | ||
| }) | ||
| } |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
P1: When an upload or presigned URL uses a bracketed IPv6 host, this pattern matches nothing and
redactURLsInErrorcan expose the full URL, including credentials or signed query parameters. Keep valid bracketed host syntax matchable while still handling wrapper delimiters.Prompt for AI agents