Skip to content

chore: prevent nil dereferences on cmd handlers #8319

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 5, 2023
Merged

chore: prevent nil dereferences on cmd handlers #8319

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9b453f7
chore: detect nil cmd handlers
Emyrk Jul 5, 2023
9fa88f8
Fix typo
Emyrk Jul 5, 2023
7de7e77
Parallelize tests
Emyrk Jul 5, 2023
63073c4
Expand comment lanugage on the nil handler
Emyrk Jul 5, 2023
eaa2218
Merge remote-tracking branch 'origin/main' into stevenmasley/cmd_help
Emyrk Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions cli/clitest/handlers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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())
}
})
}
5 changes: 5 additions & 0 deletions cli/root.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,11 @@ 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.
return
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This explains what, but not why? I'm guessing it's the call to h below, but why don't we just default to h being func(inv *clibase.Invocation) error { return inv.Command.HelpHandler(inv) } in this case?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mafredri I was wondering if defaulting to the help command would be a good solution.

Atm we manually invoke the help on all cmds except this one. So if I made that change it would be a new behavior that is probably fine, but being explicit is also pretty easy. Now that I have a unit test that will fail if you add a new cmd without a help, I figured I'd keep it being manual.

I'll update the comment with more explanation as to why. The unit test failure has that language near it

}
cmd.Handler = func(i *clibase.Invocation) error {
if !debugOptions {
return h(i)
Expand Down
10 changes: 10 additions & 0 deletions cli/root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
3 changes: 3 additions & 0 deletions enterprise/cli/provisionerdaemons.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
},
Expand Down
10 changes: 10 additions & 0 deletions enterprise/cli/root_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}