chore: prevent nil dereferences on cmd handlers (#8319)

Emyrk · web-flow · commit 4a008a8f340f · 2023-07-05T17:20:12.000Z
* chore: detect nil cmd handlers

Prevent nil panic dereferences on cmd handlers. Add a unit test
to prevent future mistakes
diff --git a/cli/clitest/handlers.go b/cli/clitest/handlers.go
@@ -0,0 +1,24 @@
+package clitest
+
+import (
+	"testing"
+
+	"github.com/coder/coder/cli/clibase"
+)
+
+// HandlersOK asserts that all commands have a handler.
+// Without a handler, the command has no default behavior. Even for
+// non-root commands (like 'groups' or 'users'), a handler is required.
+// These handlers are likely just the 'help' handler, but this must be
+// explicitly set.
+func HandlersOK(t *testing.T, cmd *clibase.Cmd) {
+	cmd.Walk(func(cmd *clibase.Cmd) {
+		if cmd.Handler == nil {
+			// If you see this error, make the Handler a helper invoker.
+			//   Handler: func(inv *clibase.Invocation) error {
+			//	   return inv.Command.HelpHandler(inv)
+			//	 },
+			t.Errorf("command %q has no handler, change to a helper invoker using: 'inv.Command.HelpHandler(inv)'", cmd.Name())
+		}
+	})
+}
diff --git a/cli/root.go b/cli/root.go
@@ -260,6 +260,18 @@ func (r *RootCmd) Command(subcommands []*clibase.Cmd) (*clibase.Cmd, error) {
 	// Add a wrapper to every command to enable debugging options.
 	cmd.Walk(func(cmd *clibase.Cmd) {
 		h := cmd.Handler
+		if h == nil {
+			// We should never have a nil handler, but if we do, do not
+			// wrap it. Wrapping it just hides a nil pointer dereference.
+			// If a nil handler exists, this is a developer bug. If no handler
+			// is required for a command such as command grouping (e.g. `users'
+			// and 'groups'), then the handler should be set to the helper
+			// function.
+			//	func(inv *clibase.Invocation) error {
+			//		return inv.Command.HelpHandler(inv)
+			//	}
+			return
+		}
 		cmd.Handler = func(i *clibase.Invocation) error {
 			if !debugOptions {
 				return h(i)
diff --git a/cli/root_test.go b/cli/root_test.go
@@ -181,3 +181,13 @@ func TestDERPHeaders(t *testing.T) {
 
 	require.Greater(t, atomic.LoadInt64(&derpCalled), int64(0), "expected /derp to be called at least once")
 }
+
+func TestHandlersOK(t *testing.T) {
+	t.Parallel()
+
+	var root cli.RootCmd
+	cmd, err := root.Command(root.Core())
+	require.NoError(t, err)
+
+	clitest.HandlersOK(t, cmd)
+}
diff --git a/enterprise/cli/provisionerdaemons.go b/enterprise/cli/provisionerdaemons.go
@@ -27,6 +27,9 @@ func (r *RootCmd) provisionerDaemons() *clibase.Cmd {
 	cmd := &clibase.Cmd{
 		Use:   "provisionerd",
 		Short: "Manage provisioner daemons",
+		Handler: func(inv *clibase.Invocation) error {
+			return inv.Command.HelpHandler(inv)
+		},
 		Children: []*clibase.Cmd{
 			r.provisionerDaemonStart(),
 		},
diff --git a/enterprise/cli/root_test.go b/enterprise/cli/root_test.go
@@ -17,3 +17,13 @@ func newCLI(t *testing.T, args ...string) (*clibase.Invocation, config.Root) {
 	require.NoError(t, err)
 	return clitest.NewWithCommand(t, cmd, args...)
 }
+
+func TestEnterpriseHandlersOK(t *testing.T) {
+	t.Parallel()
+
+	var root cli.RootCmd
+	cmd, err := root.Command(root.EnterpriseSubcommands())
+	require.NoError(t, err)
+
+	clitest.HandlersOK(t, cmd)
+}
