From 35749e7341bd6613302c00cc433914a86385d876 Mon Sep 17 00:00:00 2001 From: Ogulcan Aydogan Date: Mon, 26 Oct 2026 22:16:38 +0000 Subject: [PATCH] fix: do not show the password of a plugin download URL notation plugin install --url put the URL given by the user into its messages as it is. A URL with user info (https://user:pass@host/...) therefore showed the password in the parse error, in the "Downloading plugin from" line on stdout, in the download error and in the bad status and size limit errors of DownloadPluginFromURL. Use the redacted form of the URL for these messages and drop the URL from the parse error, because the error of url.Parse contains it. Signed-off-by: Ogulcan Aydogan --- cmd/notation/internal/plugin/plugin.go | 4 +- cmd/notation/internal/plugin/plugin_test.go | 42 +++++++++++++++++++++ cmd/notation/plugin/install.go | 13 +++++-- cmd/notation/plugin/install_test.go | 41 +++++++++++++++++++- 4 files changed, 94 insertions(+), 6 deletions(-) create mode 100644 cmd/notation/internal/plugin/plugin_test.go diff --git a/cmd/notation/internal/plugin/plugin.go b/cmd/notation/internal/plugin/plugin.go index 1bcdec693..191875a2c 100644 --- a/cmd/notation/internal/plugin/plugin.go +++ b/cmd/notation/internal/plugin/plugin.go @@ -68,7 +68,7 @@ func DownloadPluginFromURL(ctx context.Context, pluginURL string, tmpFile io.Wri defer resp.Body.Close() // Check server response if resp.StatusCode != http.StatusOK { - return fmt.Errorf("%s %q: https response bad status: %s", resp.Request.Method, resp.Request.URL, resp.Status) + return fmt.Errorf("%s %q: https response bad status: %s", resp.Request.Method, resp.Request.URL.Redacted(), resp.Status) } // Write the body to file lr := &io.LimitedReader{ @@ -80,7 +80,7 @@ func DownloadPluginFromURL(ctx context.Context, pluginURL string, tmpFile io.Wri return err } if lr.N == 0 { - return fmt.Errorf("%s %q: https response reached the %d MiB size limit", resp.Request.Method, resp.Request.URL, MaxPluginSourceBytes/1024/1024) + return fmt.Errorf("%s %q: https response reached the %d MiB size limit", resp.Request.Method, resp.Request.URL.Redacted(), MaxPluginSourceBytes/1024/1024) } return nil } diff --git a/cmd/notation/internal/plugin/plugin_test.go b/cmd/notation/internal/plugin/plugin_test.go new file mode 100644 index 000000000..31ca59f7b --- /dev/null +++ b/cmd/notation/internal/plugin/plugin_test.go @@ -0,0 +1,42 @@ +// Copyright The Notary Project Authors. +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package plugin + +import ( + "context" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +func TestDownloadPluginFromURL_BadStatusRedactsCredentials(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.NotFound(w, nil) + })) + defer server.Close() + pluginURL := strings.Replace(server.URL, "http://", "http://user:s3cret@", 1) + "/plugin.tar.gz" + + err := DownloadPluginFromURL(context.Background(), pluginURL, io.Discard) + if err == nil { + t.Fatal("DownloadPluginFromURL() expected an error, but got nil") + } + if !strings.Contains(err.Error(), "https response bad status: 404 Not Found") { + t.Errorf("DownloadPluginFromURL() error = %q, want the bad status", err) + } + if strings.Contains(err.Error(), "s3cret") { + t.Errorf("DownloadPluginFromURL() error contains the password: %q", err) + } +} diff --git a/cmd/notation/plugin/install.go b/cmd/notation/plugin/install.go index a0849c42d..757a9e0d6 100644 --- a/cmd/notation/plugin/install.go +++ b/cmd/notation/plugin/install.go @@ -132,7 +132,12 @@ func install(command *cobra.Command, opts *pluginInstallOpts) error { } pluginURL, err := url.Parse(opts.pluginSource) if err != nil { - return fmt.Errorf("failed to parse plugin download URL %s with error: %w", opts.pluginSource, err) + // the error of url.Parse contains the URL, which can have credentials + var urlErr *url.Error + if errors.As(err, &urlErr) { + err = urlErr.Err + } + return fmt.Errorf("failed to parse plugin download URL with error: %w", err) } if pluginURL.Scheme != "https" { return fmt.Errorf("failed to download plugin from URL: only the HTTPS scheme is supported, but got %s", pluginURL.Scheme) @@ -143,10 +148,12 @@ func install(command *cobra.Command, opts *pluginInstallOpts) error { } defer os.Remove(tmpFile.Name()) defer tmpFile.Close() - fmt.Printf("Downloading plugin from %s\n", opts.pluginSource) + // do not show the password of the URL in the output + redactedURL := pluginURL.Redacted() + fmt.Printf("Downloading plugin from %s\n", redactedURL) err = notationplugin.DownloadPluginFromURL(ctx, opts.pluginSource, tmpFile) if err != nil { - return fmt.Errorf("failed to download plugin from URL %s with error: %w", opts.pluginSource, err) + return fmt.Errorf("failed to download plugin from URL %s with error: %w", redactedURL, err) } fmt.Println("Download completed") if err := installPlugin(ctx, tmpFile.Name(), opts.inputChecksum, opts.force); err != nil { diff --git a/cmd/notation/plugin/install_test.go b/cmd/notation/plugin/install_test.go index d739ef8d3..c7a26a164 100644 --- a/cmd/notation/plugin/install_test.go +++ b/cmd/notation/plugin/install_test.go @@ -33,13 +33,52 @@ func TestInstall(t *testing.T) { inputChecksum: "dummy", pluginSource: "http://[::1]/%", } - expectedErrMsg := `failed to parse plugin download URL http://[::1]/% with error: parse "http://[::1]/%": invalid URL escape "%"` + expectedErrMsg := `failed to parse plugin download URL with error: invalid URL escape "%"` err := install(&cobra.Command{}, opts) if err == nil || err.Error() != expectedErrMsg { t.Fatalf("expected error %s, but got %s", expectedErrMsg, err) } }) + t.Run("invalid plugin source url with credentials", func(t *testing.T) { + opts := &pluginInstallOpts{ + pluginSourceType: notationplugin.PluginSourceTypeURL, + inputChecksum: "dummy", + pluginSource: "https://user:s3cret@localhost:badport/plugin.tar.gz", + } + err := install(&cobra.Command{}, opts) + if err == nil { + t.Fatal("install() expected an error, but got nil") + } + if !strings.Contains(err.Error(), "failed to parse plugin download URL") { + t.Errorf("install() error = %q, want a parse error", err) + } + if strings.Contains(err.Error(), "s3cret") { + t.Errorf("install() error contains the password: %q", err) + } + }) + + t.Run("failed download with credentials in the url", func(t *testing.T) { + cmd := &cobra.Command{} + cmd.SetContext(context.Background()) + // nothing listens on port 1, so the connection is refused + opts := &pluginInstallOpts{ + pluginSourceType: notationplugin.PluginSourceTypeURL, + inputChecksum: "dummy", + pluginSource: "https://user:s3cret@127.0.0.1:1/plugin.tar.gz", + } + err := install(cmd, opts) + if err == nil { + t.Fatal("install() expected an error, but got nil") + } + if want := "failed to download plugin from URL https://user:xxxxx@127.0.0.1:1/plugin.tar.gz"; !strings.Contains(err.Error(), want) { + t.Errorf("install() error = %q, want it to contain %q", err, want) + } + if strings.Contains(err.Error(), "s3cret") { + t.Errorf("install() error contains the password: %q", err) + } + }) + t.Run("unknown plugin source type", func(t *testing.T) { opts := &pluginInstallOpts{ pluginSourceType: -1,