Repository: numtide/treefmt
Pull request #513 (2025-01-23 21:08:29)

Enforce formatter name doesn't contain bad characters
As the title suggests. 

Recently discovered in https://github.com/numtide/treefmt-nix that names with `/` could be passed through and would break execution. This enforces simple formatted names in config and when passing as `formatters` arg. 

Also fixes misuse of the assertions API.  


Patch:
diff --git a/config/config.go b/config/config.go
index c10df3f..23c70af 100644
--- a/config/config.go
+++ b/config/config.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"regexp"
 	"strings"
 
 	"github.com/numtide/treefmt/v2/walk"
@@ -210,12 +211,32 @@ func FromViper(v *viper.Viper) (*Config, error) {
 		cfg.Excludes = cfg.Global.Excludes
 	}
 
+	// validate formatter names do not contain invalid characters
+
+	nameRegex := regexp.MustCompile("^[a-zA-Z0-9_-]+$")
+
+	for name := range cfg.FormatterConfigs {
+		if !nameRegex.MatchString(name) {
+			return nil, fmt.Errorf(
+				"formatter name %q is invalid, must be of the form %s",
+				name, nameRegex.String(),
+			)
+		}
+	}
+
 	// filter formatters based on provided names
 	if len(cfg.Formatters) > 0 {
 		filtered := make(map[string]*Formatter)
 
 		// check if the provided names exist in the config
 		for _, name := range cfg.Formatters {
+			if !nameRegex.MatchString(name) {
+				return nil, fmt.Errorf(
+					"formatter name %q is invalid, must be of the form %s",
+					name, nameRegex.String(),
+				)
+			}
+
 			formatterCfg, ok := cfg.FormatterConfigs[name]
 			if !ok {
 				return nil, fmt.Errorf("formatter %v not found in config", name)
diff --git a/go.mod b/go.mod
index ef2629e..1e3309c 100644
--- a/go.mod
+++ b/go.mod
@@ -10,7 +10,7 @@ require (
 	github.com/otiai10/copy v1.14.1
 	github.com/rogpeppe/go-internal v1.13.1
 	github.com/spf13/cobra v1.8.1
-	github.com/spf13/pflag v1.0.6
+	github.com/spf13/pflag v1.0.5
 	github.com/spf13/viper v1.19.0
 	github.com/stretchr/testify v1.10.0
 	go.etcd.io/bbolt v1.3.11
diff --git a/go.sum b/go.sum
index 236ecec..213b3c4 100644
--- a/go.sum
+++ b/go.sum
@@ -94,8 +94,6 @@ github.com/spf13/cobra v1.8.1 h1:e5/vxKd/rZsfSJMUX1agtjeTDf+qv1/JdBF8gg5k9ZM=
 github.com/spf13/cobra v1.8.1/go.mod h1:wHxEcudfqmLYa8iTfL+OuZPbBZkmvliBWKIezN3kD9Y=
 github.com/spf13/pflag v1.0.5 h1:iy+VFUOCP1a+8yFto/drg2CJ5u0yRoB7fZw3DKv/JXA=
 github.com/spf13/pflag v1.0.5/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
-github.com/spf13/pflag v1.0.6 h1:jFzHGLGAlb3ruxLB8MhbI6A8+AQX/2eW4qeyNZXNp2o=
-github.com/spf13/pflag v1.0.6/go.mod h1:McXfInJRrz4CZXVZOBLb0bTZqETkiAhM9Iw0y3An2Bg=
 github.com/spf13/viper v1.19.0 h1:RWq5SEjt8o25SROyN3z2OrDB9l7RPd3lwTWU8EcEdcI=
 github.com/spf13/viper v1.19.0/go.mod h1:GQUN9bilAbhU/jgc1bKs99f/suXKeUMct8Adx5+Ntkg=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
diff --git a/nix/packages/treefmt/gomod2nix.toml b/nix/packages/treefmt/gomod2nix.toml
index e5f3afa..354e612 100644
--- a/nix/packages/treefmt/gomod2nix.toml
+++ b/nix/packages/treefmt/gomod2nix.toml
@@ -96,8 +96,8 @@ schema = 3
     version = "v1.8.1"
     hash = "sha256-yDF6yAHycV1IZOrt3/hofR+QINe+B2yqkcIaVov3Ky8="
   [mod."github.com/spf13/pflag"]
-    version = "v1.0.6"
-    hash = "sha256-NjrK0FZPIfO/p2xtL1J7fOBQNTZAPZOC6Cb4aMMvhxI="
+    version = "v1.0.5"
+    hash = "sha256-w9LLYzxxP74WHT4ouBspH/iQZXjuAh2WQCHsuvyEjAw="
   [mod."github.com/spf13/viper"]
     version = "v1.19.0"
     hash = "sha256-MZ8EAvdgpGbw6kmUz8UOaAAAMdPPGd14TrCBAY+A1T4="


Test patch:
diff --git a/cmd/root_test.go b/cmd/root_test.go
index d47d3df..abbc3e8 100644
--- a/cmd/root_test.go
+++ b/cmd/root_test.go
@@ -79,7 +79,7 @@ func TestOnUnmatched(t *testing.T) {
 
 	// should exit with error when using fatal
 	t.Run("fatal", func(t *testing.T) {
-		errorFn := func(err error) {
+		errorFn := func(as *require.Assertions, err error) {
 			as.ErrorContains(err, "no formatter for path: "+expectedPaths[0])
 		}
 
@@ -114,8 +114,8 @@ func TestOnUnmatched(t *testing.T) {
 
 	t.Run("invalid", func(t *testing.T) {
 		// test bad value
-		errorFn := func(arg string) func(err error) {
-			return func(err error) {
+		errorFn := func(arg string) func(as *require.Assertions, err error) {
+			return func(as *require.Assertions, err error) {
 				as.ErrorContains(err, fmt.Sprintf(`invalid level: "%s"`, arg))
 			}
 		}
@@ -153,7 +153,7 @@ func TestQuiet(t *testing.T) {
 	t.Setenv("TREEFMT_ALLOW_MISSING_FORMATTER", "false")
 
 	// check it doesn't suppress errors
-	treefmt(t, withError(func(err error) {
+	treefmt(t, withError(func(as *require.Assertions, err error) {
 		as.ErrorContains(err, "error looking up 'foo-fmt'")
 	}))
 }
@@ -183,8 +183,6 @@ func TestCpuProfile(t *testing.T) {
 }
 
 func TestAllowMissingFormatter(t *testing.T) {
-	as := require.New(t)
-
 	tempDir := test.TempExamples(t)
 	configPath := filepath.Join(tempDir, "treefmt.toml")
 
@@ -200,7 +198,7 @@ func TestAllowMissingFormatter(t *testing.T) {
 
 	t.Run("default", func(t *testing.T) {
 		treefmt(t,
-			withError(func(err error) {
+			withError(func(as *require.Assertions, err error) {
 				as.ErrorIs(err, format.ErrCommandNotFound)
 			}),
 		)
@@ -226,8 +224,6 @@ func TestAllowMissingFormatter(t *testing.T) {
 }
 
 func TestSpecifyingFormatters(t *testing.T) {
-	as := require.New(t)
-
 	// we use the test formatter to append some whitespace
 	cfg := &config.Config{
 		FormatterConfigs: map[string]*config.Formatter{
@@ -308,8 +304,8 @@ func TestSpecifyingFormatters(t *testing.T) {
 		// bad name
 		treefmt(t,
 			withArgs("--formatters", "foo"),
-			withError(func(err error) {
-				as.Errorf(err, "formatter not found in config: foo")
+			withError(func(as *require.Assertions, err error) {
+				as.ErrorContains(err, "formatter foo not found in config")
 			}),
 		)
 	})
@@ -331,11 +327,49 @@ func TestSpecifyingFormatters(t *testing.T) {
 		t.Setenv("TREEFMT_FORMATTERS", "bar,foo")
 
 		treefmt(t,
-			withError(func(err error) {
-				as.Errorf(err, "formatter not found in config: bar")
+			withError(func(as *require.Assertions, err error) {
+				as.ErrorContains(err, "formatter bar not found in config")
 			}),
 		)
 	})
+
+	t.Run("bad names", func(t *testing.T) {
+		for _, name := range []string{"foo$", "/bar", "baz%"} {
+			treefmt(t,
+				withArgs("--formatters", name),
+				withError(func(as *require.Assertions, err error) {
+					as.ErrorContains(err, fmt.Sprintf("formatter name %q is invalid", name))
+				}),
+			)
+
+			t.Setenv("TREEFMT_FORMATTERS", name)
+
+			treefmt(t,
+				withError(func(as *require.Assertions, err error) {
+					as.ErrorContains(err, fmt.Sprintf("formatter name %q is invalid", name))
+				}),
+			)
+
+			t.Setenv("TREEFMT_FORMATTERS", "")
+
+			cfg.FormatterConfigs[name] = &config.Formatter{
+				Command:  "echo",
+				Includes: []string{"*"},
+			}
+
+			test.WriteConfig(t, configPath, cfg)
+
+			treefmt(t,
+				withError(func(as *require.Assertions, err error) {
+					as.ErrorContains(err, fmt.Sprintf("formatter name %q is invalid", name))
+				}),
+			)
+
+			delete(cfg.FormatterConfigs, name)
+
+			test.WriteConfig(t, configPath, cfg)
+		}
+	})
 }
 
 func TestIncludesAndExcludes(t *testing.T) {
@@ -541,7 +575,7 @@ func TestConfigFile(t *testing.T) {
 				withEnv(map[string]string{
 					"PRJ_ROOT": configSubDir,
 				}),
-				withError(func(err error) {
+				withError(func(as *require.Assertions, err error) {
 					as.ErrorContains(err, "failed to find treefmt config file")
 				}),
 			)
@@ -550,8 +584,6 @@ func TestConfigFile(t *testing.T) {
 }
 
 func TestCache(t *testing.T) {
-	as := require.New(t)
-
 	tempDir := test.TempExamples(t)
 	configPath := filepath.Join(tempDir, "treefmt.toml")
 
@@ -657,7 +689,7 @@ func TestCache(t *testing.T) {
 	// running should match but not format anything
 
 	treefmt(t,
-		withError(func(err error) {
+		withError(func(as *require.Assertions, err error) {
 			as.ErrorIs(err, format.ErrFormattingFailures)
 		}),
 		withStats(t, map[stats.Type]int{
@@ -670,7 +702,7 @@ func TestCache(t *testing.T) {
 
 	// running again should provide the same result
 	treefmt(t,
-		withError(func(err error) {
+		withError(func(as *require.Assertions, err error) {
 			as.ErrorIs(err, format.ErrFormattingFailures)
 		}),
 		withStats(t, map[stats.Type]int{
@@ -733,7 +765,7 @@ func TestChangeWorkingDirectory(t *testing.T) {
 
 		treefmt(t,
 			withConfig(configPath, cfg),
-			withError(func(err error) {
+			withError(func(as *require.Assertions, err error) {
 				as.ErrorContains(err, "failed to find treefmt config file")
 			}),
 		)
@@ -804,8 +836,6 @@ func TestChangeWorkingDirectory(t *testing.T) {
 }
 
 func TestFailOnChange(t *testing.T) {
-	as := require.New(t)
-
 	t.Run("change size", func(t *testing.T) {
 		tempDir := test.TempExamples(t)
 		configPath := filepath.Join(tempDir, "treefmt.toml")
@@ -829,7 +859,7 @@ func TestFailOnChange(t *testing.T) {
 		treefmt(t,
 			withArgs("--fail-on-change"),
 			withConfig(configPath, cfg),
-			withError(func(err error) {
+			withError(func(as *require.Assertions, err error) {
 				as.ErrorIs(err, formatCmd.ErrFailOnChange)
 			}),
 			withStats(t, map[stats.Type]int{
@@ -890,7 +920,7 @@ func TestFailOnChange(t *testing.T) {
 					},
 				}
 			}),
-			withError(func(err error) {
+			withError(func(as *require.Assertions, err error) {
 				as.ErrorIs(err, formatCmd.ErrFailOnChange)
 			}),
 			withStats(t, map[stats.Type]int{
@@ -1428,7 +1458,7 @@ func TestGit(t *testing.T) {
 	treefmt(t,
 		withArgs("-C", tempDir, "haskell", "foo"),
 		withConfig(configPath, cfg),
-		withError(func(err error) {
+		withError(func(as *require.Assertions, err error) {
 			as.ErrorContains(err, "path foo not found")
 		}),
 	)
@@ -1555,8 +1585,8 @@ func TestPathsArg(t *testing.T) {
 	// specify a bad path
 	treefmt(t,
 		withArgs("elm/elm.json", "haskell/Nested/Bar.hs"),
-		withError(func(err error) {
-			as.Errorf(err, "path haskell/Nested/Bar.hs not found")
+		withError(func(as *require.Assertions, err error) {
+			as.ErrorContains(err, "path haskell/Nested/Bar.hs not found")
 		}),
 	)
 
@@ -1567,8 +1597,8 @@ func TestPathsArg(t *testing.T) {
 
 	treefmt(t,
 		withArgs(absoluteExternalPath),
-		withError(func(err error) {
-			as.Errorf(err, "path %s not found within the tree root", absoluteExternalPath)
+		withError(func(as *require.Assertions, err error) {
+			as.ErrorContains(err, fmt.Sprintf("path %s not inside the tree root", absoluteExternalPath))
 		}),
 	)
 
@@ -1578,8 +1608,8 @@ func TestPathsArg(t *testing.T) {
 
 	treefmt(t,
 		withArgs(relativeExternalPath),
-		withError(func(err error) {
-			as.Errorf(err, "path %s not found within the tree root", relativeExternalPath)
+		withError(func(as *require.Assertions, err error) {
+			as.ErrorContains(err, fmt.Sprintf("path %s not inside the tree root", relativeExternalPath))
 		}),
 	)
 }
@@ -1607,7 +1637,7 @@ func TestStdin(t *testing.T) {
 	// we get an error about the missing filename parameter.
 	treefmt(t,
 		withArgs("--stdin"),
-		withError(func(err error) {
+		withError(func(as *require.Assertions, err error) {
 			as.EqualError(err, "exactly one path should be specified when using the --stdin flag")
 		}),
 		withStderr(func(out []byte) {
@@ -1640,8 +1670,8 @@ func TestStdin(t *testing.T) {
 
 	treefmt(t,
 		withArgs("--stdin", "../test.nix"),
-		withError(func(err error) {
-			as.Errorf(err, "path ../test.nix not inside the tree root %s", tempDir)
+		withError(func(as *require.Assertions, err error) {
+			as.ErrorContains(err, "path ../test.nix not inside the tree root "+tempDir)
 		}),
 		withStderr(func(out []byte) {
 			as.Contains(string(out), "Error: path ../test.nix not inside the tree root")
@@ -1804,7 +1834,7 @@ func TestRunInSubdir(t *testing.T) {
 			// this should not work, as we're in a subdirectory
 			treefmt(t,
 				withArgs("-c", "elm/elm.json", "haskell/Nested/Foo.hs"),
-				withError(func(err error) {
+				withError(func(as *require.Assertions, err error) {
 					as.ErrorContains(err, "path elm/elm.json not found")
 				}),
 			)
@@ -1836,7 +1866,7 @@ type options struct {
 	assertStdout func([]byte)
 	assertStderr func([]byte)
 
-	assertError func(error)
+	assertError func(*require.Assertions, error)
 	assertStats func(*stats.Stats)
 
 	bump struct {
@@ -1886,7 +1916,7 @@ func withStats(t *testing.T, expected map[stats.Type]int) option {
 	}
 }
 
-func withError(fn func(error)) option {
+func withError(fn func(*require.Assertions, error)) option {
 	return func(o *options) {
 		o.assertError = fn
 	}
@@ -1896,8 +1926,8 @@ func withNoError(t *testing.T) option {
 	t.Helper()
 
 	return func(o *options) {
-		o.assertError = func(err error) {
-			require.NoError(t, err)
+		o.assertError = func(as *require.Assertions, err error) {
+			as.NoError(err)
 		}
 	}
 }
@@ -1928,6 +1958,8 @@ func treefmt(
 ) {
 	t.Helper()
 
+	as := require.New(t)
+
 	// build options
 	opts := &options{}
 	for _, option := range opt {
@@ -2037,6 +2069,6 @@ func treefmt(
 	}
 
 	if opts.assertError != nil {
-		opts.assertError(cmdErr)
+		opts.assertError(as, cmdErr)
 	}
 }