fix(exec): honour empty --detach-keys in remote exec by DeveshB-1 · Pull Request #28405 · containers/podman

DeveshB-1 · 2026-03-29T18:21:20Z

podman --remote exec --detach-keys "" was silently ignoring the empty value and falling back to the default ctrl-p,ctrl-q detach sequence.

Root cause

In pkg/api/handlers/compat/exec.go, the guard was:

if input.DetachKeys != "" {
    libpodConfig.DetachKeys = &input.DetachKeys
}

Go unmarshals both an absent field and "DetachKeys": "" to the same zero value, so an explicit empty string was indistinguishable from absent — and was ignored.

Fix

Read the raw JSON body alongside the typed decode to check whether DetachKeys was explicitly present. Apply the value whenever the key is present, even when empty. Mirrors the fix applied to podman run in v5.8.0.

cc @mheon @baude

packit-as-a-service · 2026-03-29T19:15:14Z

[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore.

mheon · 2026-03-30T01:41:41Z

As I commented on the other - quite similar - PR we have open for this issue: this is greatly overcomplicated. Why not just change DetachKeys to a *string and let the JSON parser do the work for us?

DeveshB-1 · 2026-03-30T02:08:25Z

Good call - updated to shadow DetachKeys with a *string in ExecCreateConfig so the JSON decoder handles presence detection natively. nil = field absent, *"" = explicitly disabled. Also added an e2e test to test/e2e/exec_test.go covering --detach-keys="". Thanks for the direction!

mheon · 2026-03-30T02:09:59Z


 	input := new(handlers.ExecCreateConfig)
-	if err := json.NewDecoder(r.Body).Decode(&input); err != nil {
+	if err := json.NewDecoder(r.Body).Decode(input); err != nil {


Why was the & dropped?

Good catch - accidentally dropped the & when rewriting the decode call. Restored.

mheon · 2026-03-30T02:11:22Z

 		execSession.WaitWithDefaultTimeout()
 		Expect(execSession).Should(ExitWithError(137, ""))
+
+	It("podman exec --detach-keys empty string disables detach", func() {


This test does nothing. I don't think we have any testing for detach keys because it's difficult to verify in an automated fashion.

You're right, that test was meaningless. Replaced it with a unit test in pkg/api/handlers/exec_create_config_test.go that directly verifies the JSON decoding of DetachKeys as a *string - covering absent field (nil), empty string (*""), and a real value. That's the actual behaviour the fix guarantees.

DeveshB-1 · 2026-03-30T02:14:42Z

Thanks for the review @mheon, really appreciate the guidance!

Honny1

Code LGTM, I would like to see some extension of tests. Also would be great to add API tests.

Honny1 · 2026-03-30T12:03:36Z

+// This is the regression test for the bug where DetachKeys:"" was silently
+// ignored because the zero value of string is indistinguishable from absent.
+func TestExecCreateConfigDetachKeys(t *testing.T) {
+	tests := []struct {


I miss the test scenario with an empty body.

Ok will add it

Added an empty body ({}) test case - committed in the latest push.

DeveshB-1 · 2026-03-30T15:18:03Z

Added API tests in test/apiv2/20-containers.at covering both the empty DetachKeys case (disable detach) and a non-empty value, with inspect verification via GET /exec/{id}/json. Also added the empty body {} unit test case as requested.

DeveshB-1 · 2026-03-30T15:19:04Z

@Honny1 do look into this

mheon · 2026-03-30T15:19:29Z

Please squash down to 1 commit

DeveshB-1 · 2026-03-30T15:23:59Z

Done - squashed to a single commit.

Honny1 · 2026-03-30T15:26:25Z

+# Explicitly empty DetachKeys disables detach - exec create must accept it.
+t POST containers/exec-detach-keys-test/exec \
+  Cmd='["true"]' \
+  DetachKeys='' \


Nice, the last thing I miss is a scenario without DetachKeys.

Added - the test now also covers exec create without DetachKeys and verifies the default is non-empty via .DetachKeys~.+.

DeveshB-1 · 2026-03-30T15:36:20Z

Done @Honny1 - added the no-DetachKeys scenario as well.

mheon

LGTM

Honny1 · 2026-03-30T16:44:41Z

+  201 \
+  .Id~[0-9a-f]\\{64\\}
+eid=$(jq -r '.Id' <<<"$output")
+t GET exec/$eid/json 200 .DetachKeys~.+


not ok 1195 [20-containers] GET exec/74000e0395b9fcfd953a21e3a0330c5abceafbcb996aa19aba4afb6392d2afc9/json : .DetachKeys # expected: ~ .+ # actual:

Fixed — when DetachKeys is absent from the request, libpodConfig.DetachKeys was left as nil, and libpod's Inspect() returns "" for nil. Now we fall back to runtimeConfig.Engine.DetachKeys explicitly so the system default is stored and returned by inspect.

…efault Use a *string field for DetachKeys in ExecCreateConfig so the JSON decoder can distinguish an absent field (nil) from an explicitly-empty one (""). Previously both mapped to the same empty string, causing DetachKeys:"" to be silently ignored. When DetachKeys is absent from the request, store the system default from runtimeConfig.Engine.DetachKeys so that exec inspect returns a meaningful value rather than an empty string. Adds unit tests for the JSON decoding and API tests covering all three scenarios: no DetachKeys, empty DetachKeys, and a non-empty value. Fixes: containers#28307 Signed-off-by: Devesh B <98201065+DeveshB-1@users.noreply.github.com>

baude · 2026-03-30T19:59:03Z

looks like the api tests are failing still.

DeveshB-1 · 2026-03-31T01:42:37Z

looks like the api tests are failing still.

Got it will look into it

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Mar 29, 2026

DeveshB-1 force-pushed the fix/remote-exec-detach-keys branch from bf350c3 to ffb5019 Compare March 29, 2026 18:46

mheon reviewed Mar 30, 2026

View reviewed changes

DeveshB-1 force-pushed the fix/remote-exec-detach-keys branch 2 times, most recently from b44ddd7 to f0b995e Compare March 30, 2026 04:37

Honny1 reviewed Mar 30, 2026

View reviewed changes

DeveshB-1 force-pushed the fix/remote-exec-detach-keys branch from 08a2726 to 8ac0d8b Compare March 30, 2026 15:22

Honny1 reviewed Mar 30, 2026

View reviewed changes

DeveshB-1 force-pushed the fix/remote-exec-detach-keys branch from 0cd9721 to e218ce7 Compare March 30, 2026 15:35

mheon approved these changes Mar 30, 2026

View reviewed changes

Honny1 requested changes Mar 30, 2026

View reviewed changes

DeveshB-1 force-pushed the fix/remote-exec-detach-keys branch from 2393461 to 2147234 Compare March 30, 2026 16:56

Conversation

DeveshB-1 commented Mar 29, 2026

Root cause

Fix

Uh oh!

packit-as-a-service bot commented Mar 29, 2026

Uh oh!

mheon commented Mar 30, 2026

Uh oh!

DeveshB-1 commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeveshB-1 Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeveshB-1 Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeveshB-1 commented Mar 30, 2026

Uh oh!

Honny1 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeveshB-1 commented Mar 30, 2026

Uh oh!

DeveshB-1 commented Mar 30, 2026

Uh oh!

mheon commented Mar 30, 2026

Uh oh!

DeveshB-1 commented Mar 30, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DeveshB-1 commented Mar 30, 2026

Uh oh!

mheon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baude commented Mar 30, 2026

Uh oh!

DeveshB-1 commented Mar 31, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DeveshB-1 commented Mar 30, 2026 •

edited

Loading

DeveshB-1 Mar 30, 2026 •

edited

Loading

DeveshB-1 Mar 30, 2026 •

edited

Loading