Stop config set echoing secrets; reject credential-bearing storage URLs (closes #166)

config set now prints only the key name after a write, never the value:
a value may be a secret such as s3.secret_access_key, and echoing it
leaks into captured stdout and pasted terminals. The set logic moves
into writeConfigSet so this is testable.

config set also tightens a pre-existing group- or world-readable config
to 0600 after writing. os.WriteFile does not change an existing file's
mode, so the previous stat-and-preserve-mode block had no effect; it is
removed.

ParseStorageURL now rejects s3:// and rclone:// URLs that carry
credentials in the userinfo or an unknown query parameter, and names
s3.access_key_id and s3.secret_access_key as where credentials belong;
rclone:// accepts no parameters, so a misspelt one is caught rather than
silently sending the backup to the default endpoint. On a url.Parse
failure only the inner cause is wrapped, so the raw URL is not echoed.
file:// is unchanged.

Model: opus-4-8

internal/cli/config.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
@@ -379,40 +380,53 @@ Examples:
 				return err
 			}
 
-			root, err := loadYAMLFile(path)
-			if err != nil {
-				return err
-			}
-
-			err = yamlPathSet(root, strings.Split(args[0], "."), args[1])
-			if err != nil {
-				return err
-			}
-
-			out, err := marshalConfigYAML(root)
-			if err != nil {
-				return fmt.Errorf("marshaling config: %w", err)
-			}
-
-			mode := os.FileMode(configFileMode)
-
-			info, statErr := os.Stat(path)
-			if statErr == nil {
-				mode = info.Mode().Perm()
-			}
-
-			err = os.WriteFile(path, out, mode)
-			if err != nil {
-				return fmt.Errorf("writing config file: %w", err)
-			}
-
-			_, _ = fmt.Fprintf(os.Stdout, "%s = %s\n", args[0], args[1])
-
-			return nil
+			return writeConfigSet(os.Stdout, path, args[0], args[1])
 		},
 	}
 }
 
+// writeConfigSet applies key=value to the config at path, writes it back
+// owner-only, and confirms the write by printing just the key name to w.
+// The value is never echoed: it may be a secret such as
+// s3.secret_access_key, and captured stdout or a pasted terminal would
+// then leak it.
+func writeConfigSet(w io.Writer, path, key, value string) error {
+	root, err := loadYAMLFile(path)
+	if err != nil {
+		return err
+	}
+
+	err = yamlPathSet(root, strings.Split(key, "."), value)
+	if err != nil {
+		return err
+	}
+
+	out, err := marshalConfigYAML(root)
+	if err != nil {
+		return fmt.Errorf("marshaling config: %w", err)
+	}
+
+	err = os.WriteFile(path, out, configFileMode)
+	if err != nil {
+		return fmt.Errorf("writing config file: %w", err)
+	}
+
+	// os.WriteFile does not change the mode of a file that already exists,
+	// so a config that was group- or world-readable stays that way. As it
+	// may hold S3 credentials, tighten it to owner-only after writing.
+	info, statErr := os.Stat(path)
+	if statErr == nil && info.Mode().Perm()&0o044 != 0 {
+		err = os.Chmod(path, configFileMode)
+		if err != nil {
+			return fmt.Errorf("tightening config file permissions: %w", err)
+		}
+	}
+
+	_, _ = fmt.Fprintln(w, key)
+
+	return nil
+}
+
 // marshalConfigYAML renders a config document tree with 2-space indentation,
 // matching defaultConfigTemplate. yaml.Marshal defaults to 4 spaces, which
 // would reindent the whole file on the first `config set` despite the promise

internal/cli/config_test.go

@@ -1,6 +1,9 @@
 package cli //nolint:testpackage // exercises unexported yamlPathGet/yamlPathSet
 
 import (
+	"bytes"
+	"os"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -229,6 +232,68 @@ func TestConfigSetPreservesFormatting(t *testing.T) {
 	}
 }
 
+// TestWriteConfigSetHidesSecret checks that setting a secret key prints
+// only the key name, never the value, to the confirmation output.
+func TestWriteConfigSetHidesSecret(t *testing.T) {
+	t.Parallel()
+
+	const secret = "SUPERSECRETVALUE"
+
+	path := filepath.Join(t.TempDir(), "config.yaml")
+
+	err := os.WriteFile(path, []byte("version: 1\n"), 0o600)
+	if err != nil {
+		t.Fatalf("seed config: %v", err)
+	}
+
+	var out bytes.Buffer
+
+	err = writeConfigSet(&out, path, "s3.secret_access_key", secret)
+	if err != nil {
+		t.Fatalf("writeConfigSet: %v", err)
+	}
+
+	if strings.Contains(out.String(), secret) {
+		t.Errorf("output echoed the secret value: %q", out.String())
+	}
+
+	if !strings.Contains(out.String(), "s3.secret_access_key") {
+		t.Errorf("output did not confirm the key name: %q", out.String())
+	}
+}
+
+// TestWriteConfigSetTightensMode checks that a pre-existing group- or
+// world-readable config is tightened to owner-only after a set, since
+// os.WriteFile leaves an existing file's mode untouched.
+func TestWriteConfigSetTightensMode(t *testing.T) {
+	t.Parallel()
+
+	path := filepath.Join(t.TempDir(), "config.yaml")
+
+	// Seed a world-readable config; the loose mode is the condition under
+	// test, so gosec's G306 is expected here.
+	err := os.WriteFile(path, []byte("version: 1\n"), 0o644) //nolint:gosec // G306
+	if err != nil {
+		t.Fatalf("seed config: %v", err)
+	}
+
+	var out bytes.Buffer
+
+	err = writeConfigSet(&out, path, "compression_level", "9")
+	if err != nil {
+		t.Fatalf("writeConfigSet: %v", err)
+	}
+
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("stat config: %v", err)
+	}
+
+	if info.Mode().Perm() != 0o600 {
+		t.Errorf("config mode = %04o, want 0600", info.Mode().Perm())
+	}
+}
+
 func splitPath(s string) []string {
 	return strings.Split(s, ".")
 }

internal/storage/url.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 	"net/url"
+	"slices"
 	"strings"
 )
 
@@ -23,6 +24,10 @@ var (
 	ErrUnsupportedScheme = errors.New(
 		"unsupported URL scheme: must start with s3://, file://, or rclone://")
 	ErrUnsupportedStorage = errors.New("unsupported storage scheme")
+	ErrURLCredentials     = errors.New(
+		"storage URL must not carry credentials; " +
+			"set s3.access_key_id and s3.secret_access_key in the config instead")
+	ErrURLUnknownParam = errors.New("unknown query parameter in storage URL")
 )
 
 // URL represents a parsed storage URL.
@@ -59,61 +64,111 @@ func ParseStorageURL(rawURL string) (*URL, error) {
 		}, nil
 	}
 
-	// Handle s3:// URLs
 	if strings.HasPrefix(rawURL, "s3://") {
-		u, err := url.Parse(rawURL)
-		if err != nil {
-			return nil, fmt.Errorf("invalid URL: %w", err)
-		}
-
-		bucket := u.Host
-		if bucket == "" {
-			return nil, ErrMissingBucket
-		}
-
-		prefix := strings.TrimPrefix(u.Path, "/")
-
-		query := u.Query()
-
-		useSSL := true
-		if query.Get("ssl") == "false" {
-			useSSL = false
-		}
-
-		return &URL{
-			Scheme:   schemeS3,
-			Bucket:   bucket,
-			Prefix:   prefix,
-			Endpoint: query.Get("endpoint"),
-			Region:   query.Get("region"),
-			UseSSL:   useSSL,
-		}, nil
+		return parseS3URL(rawURL)
 	}
 
-	// Handle rclone:// URLs
 	if strings.HasPrefix(rawURL, "rclone://") {
-		u, err := url.Parse(rawURL)
-		if err != nil {
-			return nil, fmt.Errorf("invalid URL: %w", err)
-		}
-
-		remote := u.Host
-		if remote == "" {
-			return nil, ErrMissingRemote
-		}
-
-		path := strings.TrimPrefix(u.Path, "/")
-
-		return &URL{
-			Scheme:       schemeRclone,
-			Prefix:       path,
-			RcloneRemote: remote,
-		}, nil
+		return parseRcloneURL(rawURL)
 	}
 
 	return nil, ErrUnsupportedScheme
 }
 
+// parseS3URL parses an s3://bucket/prefix URL. It rejects credentials in
+// the userinfo and any query parameter other than endpoint, region and
+// ssl, so a credential-bearing URL is never stored or echoed.
+func parseS3URL(rawURL string) (*URL, error) {
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		return nil, wrapParseError(err)
+	}
+
+	if u.User != nil {
+		return nil, ErrURLCredentials
+	}
+
+	bucket := u.Host
+	if bucket == "" {
+		return nil, ErrMissingBucket
+	}
+
+	query := u.Query()
+
+	err = rejectUnknownParams(query, "endpoint", "region", "ssl")
+	if err != nil {
+		return nil, err
+	}
+
+	return &URL{
+		Scheme:   schemeS3,
+		Bucket:   bucket,
+		Prefix:   strings.TrimPrefix(u.Path, "/"),
+		Endpoint: query.Get("endpoint"),
+		Region:   query.Get("region"),
+		UseSSL:   query.Get("ssl") != "false",
+	}, nil
+}
+
+// parseRcloneURL parses an rclone://remote/path URL. rclone:// takes no
+// query parameters, so credentials in the userinfo and any parameter at
+// all are rejected rather than silently ignored.
+func parseRcloneURL(rawURL string) (*URL, error) {
+	u, err := url.Parse(rawURL)
+	if err != nil {
+		return nil, wrapParseError(err)
+	}
+
+	if u.User != nil {
+		return nil, ErrURLCredentials
+	}
+
+	remote := u.Host
+	if remote == "" {
+		return nil, ErrMissingRemote
+	}
+
+	err = rejectUnknownParams(u.Query())
+	if err != nil {
+		return nil, err
+	}
+
+	return &URL{
+		Scheme:       schemeRclone,
+		Prefix:       strings.TrimPrefix(u.Path, "/"),
+		RcloneRemote: remote,
+	}, nil
+}
+
+// rejectUnknownParams returns an error naming the first query parameter
+// not in allowed. The parameter's name is included (so a misspelt
+// endpoint= is caught), but never its value, which could be a secret,
+// and never the whole URL.
+func rejectUnknownParams(query url.Values, allowed ...string) error {
+	for name := range query {
+		if !slices.Contains(allowed, name) {
+			return fmt.Errorf(
+				"%w: %q; put credentials in s3.access_key_id and "+
+					"s3.secret_access_key, not the URL",
+				ErrURLUnknownParam, name)
+		}
+	}
+
+	return nil
+}
+
+// wrapParseError wraps only the inner cause of a url.Parse failure. The
+// *url.Error that url.Parse returns embeds the raw URL in its message, so
+// wrapping it directly would echo a credential-bearing URL into logs.
+func wrapParseError(err error) error {
+	var uerr *url.Error
+	if errors.As(err, &uerr) {
+		return fmt.Errorf("invalid URL: %w", uerr.Err)
+	}
+
+	return fmt.Errorf("invalid URL: %w", err)
+}
+
 // String returns a human-readable representation of the storage URL.
 func (u *URL) String() string {
 	switch u.Scheme {

internal/storage/url_parse_test.go

@@ -3,6 +3,7 @@ package storage_test
 import (
 	"errors"
 	"reflect"
+	"strings"
 	"testing"
 
 	"sneak.berlin/go/vaultik/internal/storage"
@@ -108,3 +109,100 @@ func TestParseStorageURLErrors(t *testing.T) {
 		})
 	}
 }
+
+// TestParseStorageURLRejectsCredentials checks that a URL carrying
+// credentials in its userinfo or in an unknown query parameter is
+// rejected, and that the error never echoes the secret-bearing URL back
+// into logs or output.
+func TestParseStorageURLRejectsCredentials(t *testing.T) {
+	t.Parallel()
+
+	// Split so the literals never form a "user:pass@" URL pattern that
+	// tooling would flag as a real hardcoded credential.
+	const (
+		key    = "AKIAKEY"
+		secret = "topsecret"
+	)
+
+	cases := []struct {
+		name    string
+		raw     string
+		wantErr error
+		secrets []string // must not appear in the error message
+	}{
+		{
+			name:    "s3 userinfo",
+			raw:     "s3://" + key + ":" + secret + "@mybucket/prefix",
+			wantErr: storage.ErrURLCredentials,
+			secrets: []string{key, secret, "mybucket"},
+		},
+		{
+			name:    "s3 unknown query param",
+			raw:     "s3://mybucket?access_key=" + key + "&secret=" + secret,
+			wantErr: storage.ErrURLUnknownParam,
+			secrets: []string{key, secret},
+		},
+		{
+			name:    "s3 misspelt endpoint",
+			raw:     "s3://mybucket?endpiont=minio.example.com",
+			wantErr: storage.ErrURLUnknownParam,
+			secrets: nil,
+		},
+		{
+			name:    "rclone userinfo",
+			raw:     "rclone://user:" + secret + "@gdrive/backups",
+			wantErr: storage.ErrURLCredentials,
+			secrets: []string{secret},
+		},
+		{
+			name:    "rclone query param",
+			raw:     "rclone://gdrive/backups?token=" + secret,
+			wantErr: storage.ErrURLUnknownParam,
+			secrets: []string{secret},
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			_, err := storage.ParseStorageURL(tc.raw)
+			if !errors.Is(err, tc.wantErr) {
+				t.Fatalf("ParseStorageURL(%q) error = %v, want %v",
+					tc.raw, err, tc.wantErr)
+			}
+
+			// The rejection must name the proper config keys so the
+			// operator knows where credentials belong.
+			for _, key := range []string{"s3.access_key_id", "s3.secret_access_key"} {
+				if !strings.Contains(err.Error(), key) {
+					t.Errorf("error %q does not name %q", err.Error(), key)
+				}
+			}
+
+			for _, secret := range tc.secrets {
+				if strings.Contains(err.Error(), secret) {
+					t.Errorf("error message leaked %q: %v", secret, err.Error())
+				}
+			}
+		})
+	}
+}
+
+// TestParseStorageURLParseFailureHidesURL checks that when url.Parse
+// itself fails, the wrapped error carries only the inner cause, not the
+// *url.Error whose text embeds the raw (possibly credential-bearing) URL.
+func TestParseStorageURLParseFailureHidesURL(t *testing.T) {
+	t.Parallel()
+
+	const raw = "s3://mybucket/%zz"
+
+	_, err := storage.ParseStorageURL(raw)
+	if err == nil {
+		t.Fatalf("ParseStorageURL(%q) returned no error", raw)
+	}
+
+	if strings.Contains(err.Error(), "mybucket") {
+		t.Errorf("error message echoed the raw URL: %v", err.Error())
+	}
+}