🎨 Review cleanup

CLAUDE.md

@@ -220,8 +220,9 @@ lose the proxy and CA settings.
 - `InitTLSRoots()` is called from `rootCmd.PersistentPreRunE`: the bundle is
   loaded and validated once at startup so a bad path fails immediately instead
   of mid-transfer. It returns a description of the roots, printed under `--debug`.
-  Commands annotated `annotationOffline` (`version`, `mcp manifest`) skip it —
-  they open no connection, and the release CI runs `retyc mcp manifest`.
+  Commands annotated `annotationOffline` (`version`, `mcp manifest`,
+  `config path`, `config show`) skip it — they open no connection, and the
+  release CI runs `retyc mcp manifest`.
   Beware: cobra runs only the closest `PersistentPreRun(E)` unless
   `cobra.EnableTraverseRunHooks` is set, so adding one to a subcommand would
   silently skip the CA loading.
@@ -542,8 +543,8 @@ environment value.
 `grpc` on `OTEL_EXPORTER_OTLP_PROTOCOL=grpc`. `Init` runs in
 `rootCmd.PersistentPreRunE`, `run()` closes the command span and flushes
 within 2 s; a failed init or export never changes the exit code.
-`annotationOffline` commands (`version`, `mcp manifest`) never initialise
-tracing, since `Init` runs in `PersistentPreRunE` after that early return.
+`annotationOffline` commands (`version`, `mcp manifest`, `config path`,
+`config show`) never initialise tracing, since `Init` runs in `PersistentPreRunE` after that early return.
 
 Span model:
 - one-shot command: span `retyc <command path>`, child of `TRACEPARENT`,

cmd/config.go

@@ -137,9 +137,11 @@ func effectiveValue(key string) string {
 	if key == "insecure" {
 		return strconv.FormatBool(insecure)
 	}
-	// List keys (webdav.metrics.labels) would render as "" through GetString.
-	if items, ok := viper.Get(key).([]string); ok {
-		return strings.Join(items, " ")
+	// List keys (webdav.metrics.labels) would render as "" through GetString:
+	// []string from the environment, []any from a YAML list.
+	switch viper.Get(key).(type) {
+	case []string, []any:
+		return strings.Join(viper.GetStringSlice(key), " ")
 	}
 
 	return viper.GetString(key)

cmd/config_test.go

@@ -172,6 +172,22 @@ func TestConfigFileLoaded_ReadableFile(t *testing.T) {
 	}
 }
 
+func TestEffectiveValue_JoinsYAMLLists(t *testing.T) {
+	isolateConfig(t)
+	path := filepath.Join(t.TempDir(), "config.yaml")
+	yaml := "webdav:\n  metrics:\n    labels:\n      - identity=abc\n      - pod=x\n"
+	if err := os.WriteFile(path, []byte(yaml), 0600); err != nil {
+		t.Fatalf("WriteFile() error = %v", err)
+	}
+	cfgFile = path
+
+	initConfig()
+
+	if got := effectiveValue("webdav.metrics.labels"); got != "identity=abc pod=x" {
+		t.Errorf("effectiveValue = %q, want %q", got, "identity=abc pod=x")
+	}
+}
+
 func TestEffectiveValue_JoinsLists(t *testing.T) {
 	isolateConfig(t)
 	t.Setenv("RETYC_WEBDAV_METRICS_LABELS", "identity=abc pod=x")

cmd/webdav.go

@@ -2171,11 +2171,16 @@ func bindUnsafeWrite(flags *pflag.FlagSet) {
 
 // resolveWebdavAddr returns the host:port to bind, with the usual precedence
 // (flag > env > config file > default); same binding strategy as
-// resolveMetricsAddr.
-func resolveWebdavAddr(flags *pflag.FlagSet) string {
+// resolveMetricsAddr. A value without a port ("0.0.0.0", the form --addr took
+// before it included the port) is an error.
+func resolveWebdavAddr(flags *pflag.FlagSet) (string, error) {
 	_ = viper.BindPFlag("webdav.addr", flags.Lookup("addr"))
+	addr := viper.GetString("webdav.addr")
+	if _, port, err := net.SplitHostPort(addr); err != nil || port == "" {
+		return "", fmt.Errorf("--addr %q: expected host:port, e.g. 127.0.0.1:8888", addr)
+	}
 
-	return viper.GetString("webdav.addr")
+	return addr, nil
 }
 
 // tokenKeepalive pings tokenSource every 60s to keep the access token warm.
@@ -2279,6 +2284,13 @@ Example:
 			}
 		}()
 
+		// Fail-fast: a malformed address would otherwise only surface once the
+		// token, the API and the key passphrase have all been checked.
+		addr, err := resolveWebdavAddr(cmd.Flags())
+		if err != nil {
+			return err
+		}
+
 		// Fail-fast: passphrase must be set before any crypto operation.
 		if config.KeyPassphrase() == "" {
 			return config.ErrNoKeyPassphrase
@@ -2304,8 +2316,6 @@ Example:
 			return err
 		}
 
-		addr := resolveWebdavAddr(cmd.Flags())
-
 		fs := &webdavFS{
 			cfg:    cfg,
 			client: client,
@@ -2370,7 +2380,7 @@ Example:
 		})
 
 		authEnabled, _ := cmd.Flags().GetBool("auth")
-		var rootHandler = instrumentWebdav(mux)
+		var rootHandler http.Handler = mux
 		if authEnabled {
 			password := config.WebdavPassword()
 			if password == "" {
@@ -2390,6 +2400,8 @@ Example:
 				"WARNING: binding to %s without authentication exposes all dataroom contents "+
 					"in cleartext to the network; consider --auth\n", addr)
 		}
+		// Outermost, so rejected credentials (401) are counted and traced too.
+		rootHandler = instrumentWebdav(rootHandler)
 
 		srv := &http.Server{ //nolint:gosec // G112: local-only server; Slowloris not a concern
 			Addr:              addr,

cmd/webdav_metrics.go

@@ -126,6 +126,12 @@ func parseMetricsLabels(items []string) (prometheus.Labels, error) {
 			return nil, fmt.Errorf(
 				"metrics label %q: invalid label name (letters, digits and _ only, not starting with a digit)", item)
 		}
+		if key == "le" || key == "quantile" || strings.HasPrefix(key, "__") {
+			// Histograms and summaries add le / quantile to their series, and
+			// Prometheus reserves "__": the registry would accept them, then
+			// every scrape would be rejected.
+			return nil, fmt.Errorf("metrics label %q: reserved label name", item)
+		}
 		if _, dup := labels[key]; dup {
 			return nil, fmt.Errorf("metrics label %q: key given twice", key)
 		}

cmd/webdav_metrics_test.go

@@ -467,6 +467,11 @@ func TestParseMetricsLabels(t *testing.T) {
 			t.Errorf("%q: expected an error", bad)
 		}
 	}
+	for _, reserved := range []string{"le=x", "quantile=x", "__name__=x"} {
+		if _, err := parseMetricsLabels([]string{reserved}); err == nil {
+			t.Errorf("%q: expected a reserved-name error", reserved)
+		}
+	}
 	if _, err := parseMetricsLabels([]string{"a=1", "a=2"}); err == nil {
 		t.Error("duplicate key: expected an error")
 	}
@@ -542,14 +547,14 @@ func TestResolveWebdavAddr_FlagOverridesEnv(t *testing.T) {
 
 	flags := pflag.NewFlagSet("serve", pflag.ContinueOnError)
 	flags.String("addr", "127.0.0.1:8888", "")
-	if got := resolveWebdavAddr(flags); got != "0.0.0.0:9000" {
-		t.Errorf("without flag: got %q, want env value 0.0.0.0:9000", got)
+	if got, err := resolveWebdavAddr(flags); err != nil || got != "0.0.0.0:9000" {
+		t.Errorf("without flag: got %q (%v), want env value 0.0.0.0:9000", got, err)
 	}
 	if err := flags.Set("addr", "127.0.0.1:9999"); err != nil {
 		t.Fatal(err)
 	}
-	if got := resolveWebdavAddr(flags); got != "127.0.0.1:9999" {
-		t.Errorf("with flag: got %q, want flag value 127.0.0.1:9999", got)
+	if got, err := resolveWebdavAddr(flags); err != nil || got != "127.0.0.1:9999" {
+		t.Errorf("with flag: got %q (%v), want flag value 127.0.0.1:9999", got, err)
 	}
 }
 
@@ -558,8 +563,25 @@ func TestResolveWebdavAddr_Default(t *testing.T) {
 	config.SetDefaults()
 	flags := pflag.NewFlagSet("serve", pflag.ContinueOnError)
 	flags.String("addr", "127.0.0.1:8888", "")
-	if got := resolveWebdavAddr(flags); got != "127.0.0.1:8888" {
-		t.Errorf("got %q, want 127.0.0.1:8888", got)
+	if got, err := resolveWebdavAddr(flags); err != nil || got != "127.0.0.1:8888" {
+		t.Errorf("got %q (%v), want 127.0.0.1:8888", got, err)
+	}
+}
+
+func TestResolveWebdavAddr_RejectsMissingPort(t *testing.T) {
+	for _, addr := range []string{"0.0.0.0", "localhost", "127.0.0.1:", ""} {
+		t.Run(addr, func(t *testing.T) {
+			isolateConfig(t)
+			config.SetDefaults()
+			flags := pflag.NewFlagSet("serve", pflag.ContinueOnError)
+			flags.String("addr", "127.0.0.1:8888", "")
+			if err := flags.Set("addr", addr); err != nil {
+				t.Fatal(err)
+			}
+			if _, err := resolveWebdavAddr(flags); err == nil {
+				t.Errorf("--addr %q: expected an error", addr)
+			}
+		})
 	}
 }
 

doc/webdav.md

@@ -257,8 +257,9 @@ setting only matters together with `--metrics-addr`.
 `RETYC_WEBDAV_METRICS_LABELS="identity=abc pod=x"` separated by spaces) adds
 constant labels to every series, runtime metrics included, so the parent can
 tell its instances apart. The first `=` separates key and value. A key that
-collides with a metric label (`method`, `route`, `status`, ...) or an invalid
-label name stops the server at startup.
+collides with a metric label (`method`, `route`, `status`, ...), a reserved
+name (`le`, `quantile`, anything starting with `__`) or an invalid label name
+stops the server at startup.
 
 ```sh
 retyc webdav serve --metrics-addr 127.0.0.1:9090 --metrics-runtime=false \

internal/auth/oidc_test.go

@@ -594,4 +594,3 @@ func TestGetValidToken_NoToken(t *testing.T) {
 		t.Errorf("error = %v, want ErrNoToken", err)
 	}
 }
-

internal/ui/escape.go

@@ -12,8 +12,12 @@ import (
 // rewrite previous output lines, hide part of a listing or set the terminal
 // title, or bidirectional formatting characters that visually reorder a name
 // ("Trojan Source" spoofing). A rune is safe when unicode.IsGraphic accepts
-// it: this rejects C0/C1 controls, DEL and every format character (Cf: bidi
-// overrides and isolates, zero-width characters), and invalid UTF-8.
+// it: this rejects C0/C1 controls, DEL and format characters (Cf: bidi
+// marks, overrides and isolates, zero-width space), and invalid UTF-8.
+//
+// A few format characters are ordinary text, not markup, and are kept (see
+// textFormat): refusing them would mangle real names, and FileName would
+// write the mangled name to disk, irreversibly.
 
 // Escape makes s safe to print on a terminal. A safe string is returned
 // unchanged; any other is returned quoted by strconv.QuoteToGraphic, so the
@@ -43,7 +47,7 @@ func EscapeLines(s string) string {
 // is a path separator on Windows. Invalid UTF-8 becomes U+FFFD.
 func FileName(name string) string {
 	return strings.Map(func(r rune) rune {
-		if !unicode.IsGraphic(r) {
+		if !isSafe(r) {
 			return '_'
 		}
 
@@ -56,10 +60,32 @@ func isGraphic(s string) bool {
 	for _, r := range s {
 		// Ranging over invalid UTF-8 yields utf8.RuneError, which is graphic:
 		// check validity separately.
-		if !unicode.IsGraphic(r) {
+		if !isSafe(r) {
 			return false
 		}
 	}
 
 	return utf8.ValidString(s)
 }
+
+// isSafe reports whether r may be printed or written to a file name as is.
+func isSafe(r rune) bool {
+	return unicode.IsGraphic(r) || unicode.Is(textFormat, r)
+}
+
+// textFormat lists the format characters (Cf) that carry no layout or
+// terminal effect and that real names contain: the soft hyphen, the zero
+// width non-joiner (Persian, Urdu, Indic scripts) and joiner (Indic scripts,
+// composed emoji such as 👨‍💻), and the tag characters of subdivision flag
+// emoji. Bidi marks (U+200E, U+200F, U+061C) stay refused even though some
+// systems insert them in right-to-left names: they reorder what surrounds
+// them, which is the spoofing this file guards against.
+var textFormat = &unicode.RangeTable{
+	R16: []unicode.Range16{
+		{Lo: 0x00AD, Hi: 0x00AD, Stride: 1},
+		{Lo: 0x200C, Hi: 0x200D, Stride: 1},
+	},
+	R32: []unicode.Range32{
+		{Lo: 0xE0020, Hi: 0xE007F, Stride: 1},
+	},
+}

internal/ui/escape_test.go

@@ -2,6 +2,10 @@ package ui
 
 import "testing"
 
+// flagEngland is the England flag emoji: a black flag followed by the tag
+// characters "gbeng" and a cancel tag, all format characters (Cf).
+const flagEngland = "🏴\U000E0067\U000E0062\U000E0065\U000E006E\U000E0067\U000E007F"
+
 func TestEscape(t *testing.T) {
 	tests := []struct {
 		name, in, want string
@@ -20,6 +24,11 @@ func TestEscape(t *testing.T) {
 		{"raw 0x9b byte", "a\x9b31mb", `"a\x9b31mb"`},
 		{"bidi override", "invoice\u202Efdp.exe", `"invoice\u202efdp.exe"`},
 		{"zero width space", "a\u200Bb", `"a\u200bb"`},
+		{"bidi mark", "a\u200Fb", `"a\u200fb"`},
+		{"ZWNJ kept", "می\u200Cخواهم.docx", "می\u200Cخواهم.docx"},
+		{"ZWJ emoji kept", "👨\u200D💻 notes.txt", "👨\u200D💻 notes.txt"},
+		{"soft hyphen kept", "Donau\u00ADdampf", "Donau\u00ADdampf"},
+		{"subdivision flag kept", flagEngland, flagEngland},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -46,6 +55,9 @@ func TestFileName(t *testing.T) {
 		{"a\x1b[2K\rb\x07", "a_[2K_b_"},
 		{"invoice\u202Efdp.exe", "invoice_fdp.exe"},
 		{"a\x9bb", "a\uFFFDb"},
+		{"a\u200Bb\u200Fc", "a_b_c"},
+		{"می\u200Cخواهم.docx", "می\u200Cخواهم.docx"},
+		{"👨\u200D💻 notes.txt", "👨\u200D💻 notes.txt"},
 	}
 	for _, tt := range tests {
 		if got := FileName(tt.in); got != tt.want {