diff --git a/CLAUDE.md b/CLAUDE.md index 08060f6..9443d5e 100644 --- a/CLAUDE.md +++ b/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 `, child of `TRACEPARENT`, diff --git a/cmd/config.go b/cmd/config.go index 5a5c44a..52422a4 100644 --- a/cmd/config.go +++ b/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) diff --git a/cmd/config_test.go b/cmd/config_test.go index 5492762..15bdbb2 100644 --- a/cmd/config_test.go +++ b/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") diff --git a/cmd/webdav.go b/cmd/webdav.go index 565b44a..c8a7def 100644 --- a/cmd/webdav.go +++ b/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, diff --git a/cmd/webdav_metrics.go b/cmd/webdav_metrics.go index e5ba4e0..fe9a5e0 100644 --- a/cmd/webdav_metrics.go +++ b/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) } diff --git a/cmd/webdav_metrics_test.go b/cmd/webdav_metrics_test.go index 2aeb4e1..aec0df9 100644 --- a/cmd/webdav_metrics_test.go +++ b/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) + } + }) } } diff --git a/doc/webdav.md b/doc/webdav.md index b71004e..9c6492c 100644 --- a/doc/webdav.md +++ b/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 \ diff --git a/internal/auth/oidc_test.go b/internal/auth/oidc_test.go index f1be2ab..8c5dffa 100644 --- a/internal/auth/oidc_test.go +++ b/internal/auth/oidc_test.go @@ -594,4 +594,3 @@ func TestGetValidToken_NoToken(t *testing.T) { t.Errorf("error = %v, want ErrNoToken", err) } } - diff --git a/internal/ui/escape.go b/internal/ui/escape.go index 9602d33..a38f774 100644 --- a/internal/ui/escape.go +++ b/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}, + }, +} diff --git a/internal/ui/escape_test.go b/internal/ui/escape_test.go index 5367fcc..5286a48 100644 --- a/internal/ui/escape_test.go +++ b/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 {