From fe3157419c53ab578ea547defa6a6ba1fdab2215 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 14 Jan 2026 13:22:59 +0100 Subject: [PATCH] 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 --- cli/command/container/attach.go | 13 ++++++---- cli/command/container/attach_test.go | 8 ++++++ cli/command/container/exec.go | 3 +++ cli/command/container/exec_test.go | 34 ++++++++++++++++--------- cli/command/container/hijack.go | 24 +++++++++++++----- cli/command/container/run.go | 13 ++++++---- cli/command/container/run_test.go | 5 ++++ cli/command/container/start.go | 13 ++++++---- cli/command/container/start_test.go | 38 ++++++++++++++++++++++++++++ 9 files changed, 118 insertions(+), 33 deletions(-) create mode 100644 cli/command/container/start_test.go diff --git a/cli/command/container/attach.go b/cli/command/container/attach.go index 8316c56fb0ce..f31a13f27b86 100644 --- a/cli/command/container/attach.go +++ b/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, diff --git a/cli/command/container/attach_test.go b/cli/command/container/attach_test.go index 60b9c51ed1e3..3f1410b16494 100644 --- a/cli/command/container/attach_test.go +++ b/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"}, diff --git a/cli/command/container/exec.go b/cli/command/container/exec.go index 02510d94bced..bd4f9bc6242c 100644 --- a/cli/command/container/exec.go +++ b/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 } diff --git a/cli/command/container/exec_test.go b/cli/command/container/exec_test.go index 0b2d0d043a36..47b6574ec513 100644 --- a/cli/command/container/exec_test.go +++ b/cli/command/container/exec_test.go @@ -92,21 +92,21 @@ TWO=2 }, { options: withDefaultOpts(ExecOptions{Detach: true}), - configFile: configfile.ConfigFile{DetachKeys: "de"}, + configFile: configfile.ConfigFile{DetachKeys: "ctrl-d,e"}, expected: client.ExecCreateOptions{ Cmd: []string{"command"}, - DetachKeys: "de", + DetachKeys: "ctrl-d,e", }, }, { options: withDefaultOpts(ExecOptions{ Detach: true, - DetachKeys: "ab", + DetachKeys: "ctrl-a,b", }), - configFile: configfile.ConfigFile{DetachKeys: "de"}, + configFile: configfile.ConfigFile{DetachKeys: "ctrl-d,e"}, expected: client.ExecCreateOptions{ Cmd: []string{"command"}, - DetachKeys: "ab", + DetachKeys: "ctrl-a,b", }, }, { @@ -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) { diff --git a/cli/command/container/hijack.go b/cli/command/container/hijack.go index 2a27052040fb..1ce40962c927 100644 --- a/cli/command/container/hijack.go +++ b/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 { @@ -82,13 +92,15 @@ func (h *hijackedIOStreamer) stream(ctx context.Context) error { } } -func (h *hijackedIOStreamer) setupInput() (restore func(), err error) { +func (h *hijackedIOStreamer) setupInput() (restore func(), _ error) { if h.inputStream == nil || !h.tty { // No need to setup input TTY. // 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,11 @@ 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) + var err error + escapeKeys, err = term.ToBytes(h.detachKeys) if err != nil { - logrus.Warnf("invalid detach escape keys, using default: %s", err) - } else { - escapeKeys = customEscapeKeys + restore() + return nil, err } } diff --git a/cli/command/container/run.go b/cli/command/container/run.go index 336a437c90f9..3c25630d6b78 100644 --- a/cli/command/container/run.go +++ b/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. diff --git a/cli/command/container/run_test.go b/cli/command/container/run_test.go index c60ff0629a79..c0049e8b4547 100644 --- a/cli/command/container/run_test.go +++ b/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{})) diff --git a/cli/command/container/start.go b/cli/command/container/start.go index 0973c51f8011..916f5808d717 100644 --- a/cli/command/container/start.go +++ b/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, diff --git a/cli/command/container/start_test.go b/cli/command/container/start_test.go new file mode 100644 index 000000000000..a19a7998cc08 --- /dev/null +++ b/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)) + } + }) + } +}