improve validation of "--detach-keys" options

Before this change, the detach-keys were not validated, and the code either
fell back to the default sequence, or returned an obscure error if the
invalid sequence would produce an error on the daemon;

Before this patch:

    docker run -it --rm --detach-keys=shift-a,b busybox
    unable to upgrade to tcp, received 400

With this patch:

    docker run -it --rm --detach-keys=shift-a,b busybox
    invalid detach keys (shift-a,b): Unknown character: 'shift-a'

Note that the "unable to upgrade to tcp, received 400" error is still
something to be looked into; the client currently discards error messages
coming from the daemon.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Author: thaJeztah

cli/command/container/attach.go

@@ -70,6 +70,14 @@ func newAttachCommand(dockerCLI command.Cli) *cobra.Command {
 
 // RunAttach executes an `attach` command
 func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, opts *AttachOptions) error {
+	detachKeys := opts.DetachKeys
+	if detachKeys == "" {
+		detachKeys = dockerCLI.ConfigFile().DetachKeys
+	}
+	if err := validateDetachKeys(detachKeys); err != nil {
+		return err
+	}
+
 	apiClient := dockerCLI.Client()
 
 	// request channel to wait for client
@@ -85,11 +93,6 @@ func RunAttach(ctx context.Context, dockerCLI command.Cli, containerID string, o
 		return err
 	}
 
-	detachKeys := dockerCLI.ConfigFile().DetachKeys
-	if opts.DetachKeys != "" {
-		detachKeys = opts.DetachKeys
-	}
-
 	options := client.ContainerAttachOptions{
 		Stream:     true,
 		Stdin:      !opts.NoStdin && c.Config.OpenStdin,

cli/command/container/attach_test.go

@@ -27,6 +27,14 @@ func TestNewAttachCommandErrors(t *testing.T) {
 				return client.ContainerInspectResult{}, errors.New("something went wrong")
 			},
 		},
+		{
+			name:          "invalid-detach-keys",
+			args:          []string{"--detach-keys", "shift-b", "5cb5bb5e4a3b"},
+			expectedError: "invalid detach keys (shift-b):",
+			containerInspectFunc: func(containerID string) (client.ContainerInspectResult, error) {
+				return client.ContainerInspectResult{}, errors.New("something went wrong")
+			},
+		},
 		{
 			name:          "client-stopped",
 			args:          []string{"5cb5bb5e4a3b"},

cli/command/container/exec.go

@@ -248,5 +248,8 @@ func parseExec(execOpts ExecOptions, configFile *configfile.ConfigFile) (*client
 	} else {
 		execOptions.DetachKeys = configFile.DetachKeys
 	}
+	if err := validateDetachKeys(execOpts.DetachKeys); err != nil {
+		return nil, err
+	}
 	return execOptions, nil
 }

cli/command/container/exec_test.go

@@ -147,13 +147,23 @@ TWO=2
 	}
 }
 
-func TestParseExecNoSuchFile(t *testing.T) {
-	execOpts := withDefaultOpts(ExecOptions{})
-	assert.Check(t, execOpts.EnvFile.Set("no-such-env-file"))
-	execConfig, err := parseExec(execOpts, &configfile.ConfigFile{})
-	assert.ErrorContains(t, err, "no-such-env-file")
-	assert.Check(t, os.IsNotExist(err))
-	assert.Check(t, execConfig == nil)
+func TestParseExecErrors(t *testing.T) {
+	t.Run("missing env-file", func(t *testing.T) {
+		execOpts := withDefaultOpts(ExecOptions{})
+		assert.Check(t, execOpts.EnvFile.Set("no-such-env-file"))
+		execConfig, err := parseExec(execOpts, &configfile.ConfigFile{})
+		assert.ErrorContains(t, err, "no-such-env-file")
+		assert.Check(t, os.IsNotExist(err))
+		assert.Check(t, execConfig == nil)
+	})
+	t.Run("invalid detach keys", func(t *testing.T) {
+		execOpts := withDefaultOpts(ExecOptions{
+			DetachKeys: "shift-a",
+		})
+		execConfig, err := parseExec(execOpts, &configfile.ConfigFile{})
+		assert.Check(t, is.ErrorContains(err, "invalid detach keys (shift-a):"))
+		assert.Check(t, is.Nil(execConfig))
+	})
 }
 
 func TestRunExec(t *testing.T) {

cli/command/container/hijack.go

@@ -30,6 +30,16 @@ func (r *readCloserWrapper) Close() error {
 	return r.closer()
 }
 
+func validateDetachKeys(keys string) error {
+	if keys == "" {
+		return nil
+	}
+	if _, err := term.ToBytes(keys); err != nil {
+		return invalidParameter(fmt.Errorf("invalid detach keys (%s): %w", keys, err))
+	}
+	return nil
+}
+
 // A hijackedIOStreamer handles copying input to and output from streams to the
 // connection.
 type hijackedIOStreamer struct {
@@ -88,7 +98,9 @@ func (h *hijackedIOStreamer) setupInput() (restore func(), err error) {
 		// The restore func is a nop.
 		return func() {}, nil
 	}
-
+	if err := validateDetachKeys(h.detachKeys); err != nil {
+		return nil, err
+	}
 	if err := setRawTerminal(h.streams); err != nil {
 		return nil, fmt.Errorf("unable to set IO streams as raw terminal: %s", err)
 	}
@@ -103,11 +115,9 @@ func (h *hijackedIOStreamer) setupInput() (restore func(), err error) {
 	// Use default escape keys if an invalid sequence is given.
 	escapeKeys := defaultEscapeKeys
 	if h.detachKeys != "" {
-		customEscapeKeys, err := term.ToBytes(h.detachKeys)
+		escapeKeys, err = term.ToBytes(h.detachKeys)
 		if err != nil {
-			logrus.Warnf("invalid detach escape keys, using default: %s", err)
-		} else {
-			escapeKeys = customEscapeKeys
+			return restore, err
 		}
 	}
 

cli/command/container/run.go

@@ -140,6 +140,14 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
 		config.StdinOnce = false
 	}
 
+	detachKeys := runOpts.detachKeys
+	if detachKeys == "" {
+		detachKeys = dockerCli.ConfigFile().DetachKeys
+	}
+	if err := validateDetachKeys(runOpts.detachKeys); err != nil {
+		return err
+	}
+
 	containerID, err := createContainer(ctx, dockerCli, containerCfg, &runOpts.createOptions)
 	if err != nil {
 		return toStatusError(err)
@@ -172,11 +180,6 @@ func runContainer(ctx context.Context, dockerCli command.Cli, runOpts *runOption
 		}()
 	}
 	if attach {
-		detachKeys := dockerCli.ConfigFile().DetachKeys
-		if runOpts.detachKeys != "" {
-			detachKeys = runOpts.detachKeys
-		}
-
 		// ctx should not be cancellable here, as this would kill the stream to the container
 		// and we want to keep the stream open until the process in the container exits or until
 		// the user forcefully terminates the CLI.

cli/command/container/run_test.go

@@ -34,6 +34,11 @@ func TestRunValidateFlags(t *testing.T) {
 			args:        []string{"--attach", "stdin", "--detach", "myimage"},
 			expectedErr: "conflicting options: cannot specify both --attach and --detach",
 		},
+		{
+			name:        "with invalid --detach-keys",
+			args:        []string{"--detach-keys", "shift-a", "myimage"},
+			expectedErr: "invalid detach keys (shift-a):",
+		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			cmd := newRunCommand(test.NewFakeCli(&fakeClient{}))

cli/command/container/start.go

@@ -70,6 +70,14 @@ func RunStart(ctx context.Context, dockerCli command.Cli, opts *StartOptions) er
 	ctx, cancelFun := context.WithCancel(ctx)
 	defer cancelFun()
 
+	detachKeys := opts.DetachKeys
+	if detachKeys == "" {
+		detachKeys = dockerCli.ConfigFile().DetachKeys
+	}
+	if err := validateDetachKeys(detachKeys); err != nil {
+		return err
+	}
+
 	switch {
 	case opts.Attach || opts.OpenStdin:
 		// We're going to attach to a container.
@@ -93,11 +101,6 @@ func RunStart(ctx context.Context, dockerCli command.Cli, opts *StartOptions) er
 			defer signal.StopCatch(sigc)
 		}
 
-		detachKeys := dockerCli.ConfigFile().DetachKeys
-		if opts.DetachKeys != "" {
-			detachKeys = opts.DetachKeys
-		}
-
 		options := client.ContainerAttachOptions{
 			Stream:     true,
 			Stdin:      opts.OpenStdin && c.Container.Config.OpenStdin,

cli/command/container/start_test.go

@@ -0,0 +1,38 @@
+package container
+
+import (
+	"io"
+	"testing"
+
+	"github.com/docker/cli/internal/test"
+	"gotest.tools/v3/assert"
+	is "gotest.tools/v3/assert/cmp"
+)
+
+func TestStartValidateFlags(t *testing.T) {
+	for _, tc := range []struct {
+		name        string
+		args        []string
+		expectedErr string
+	}{
+		{
+			name:        "with invalid --detach-keys",
+			args:        []string{"--detach-keys", "shift-a", "myimage"},
+			expectedErr: "invalid detach keys (shift-a):",
+		},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			cmd := newStartCommand(test.NewFakeCli(&fakeClient{}))
+			cmd.SetOut(io.Discard)
+			cmd.SetErr(io.Discard)
+			cmd.SetArgs(tc.args)
+
+			err := cmd.Execute()
+			if tc.expectedErr != "" {
+				assert.Check(t, is.ErrorContains(err, tc.expectedErr))
+			} else {
+				assert.Check(t, is.Nil(err))
+			}
+		})
+	}
+}