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

* chore: detect nil cmd handlers

Prevent nil panic dereferences on cmd handlers. Add a unit test
to prevent future mistakes
This commit is contained in:
Steven Masley
2023-07-05 17:20:12 +00:00
committed by GitHub
parent d70e2d97de
commit 4a008a8f34
5 changed files with 59 additions and 0 deletions
+24
View File
@@ -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())
}
})
}
+12
View File
@@ -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)
+10
View File
@@ -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)
}