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; the previous stat-and-preserve-mode block had no effect (os.WriteFile does not change an existing file mode) and is removed.

ParseStorageURL now rejects s3:// and rclone:// URLs that carry credentials in the userinfo or an unknown query parameter, naming s3.access_key_id and s3.secret_access_key as where credentials belong. 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, ".")
 }