From 38da4b5e896d86f6c37f580ff3f8aeba0bf160a0 Mon Sep 17 00:00:00 2001 From: Luca Di Maio Date: Mon, 29 Jun 2026 13:12:43 +0200 Subject: [PATCH] fix(list): scope distrobox detection to known label keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit IsDistrobox matched any label key OR value containing "distrobox", so unrelated containers carrying a path or project tag that happened to include the substring (e.g. a workdir under a "distrobox" directory) leaked into `distrobox list` — and into the rm/upgrade/stop pipelines that filter through the same call. Label values are never authoritative; only `manager=distrobox` and the `distrobox.*` key namespace are ever set by us. Check those directly, keeping 24b31ed8 working for manager-overridden boxes (apx). Signed-off-by: Luca Di Maio --- pkg/commands/list_test.go | 73 +++++++++++++++++++ pkg/containermanager/containermanager.go | 24 ++++-- pkg/containermanager/containermanager_test.go | 52 +++++++++++++ 3 files changed, 142 insertions(+), 7 deletions(-) create mode 100644 pkg/commands/list_test.go diff --git a/pkg/commands/list_test.go b/pkg/commands/list_test.go new file mode 100644 index 00000000..88c05e19 --- /dev/null +++ b/pkg/commands/list_test.go @@ -0,0 +1,73 @@ +package commands_test + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/89luca89/distrobox/pkg/commands" + "github.com/89luca89/distrobox/pkg/config" + "github.com/89luca89/distrobox/pkg/containermanager" + "github.com/89luca89/distrobox/pkg/internal/testutil" +) + +// ListCommand must drop containers that are not distrobox-owned, even when a +// foreign tool's label value happens to contain the substring "distrobox" +// (the regression that motivated tightening Container.IsDistrobox). +func TestListCommand_FiltersNonDistroboxContainers(t *testing.T) { + mock := &testutil.MockContainerManager{ + ListContainersResult: []containermanager.Container{ + { + Name: "real-box", + Image: "registry.fedoraproject.org/fedora-toolbox:latest", + Status: "Up", + Labels: map[string]string{"manager": "distrobox", "distrobox.unshare_groups": "0"}, + }, + { + Name: "apx-box", + Image: "docker.io/library/ubuntu:22.04", + Status: "Exited", + Labels: map[string]string{"manager": "apx", "distrobox.unshare_groups": "0"}, + }, + { + Name: "plain-box", + Image: "docker.io/library/alpine:latest", + Status: "Up", + Labels: map[string]string{"foo": "bar"}, + }, + { + Name: "dir-box", + Image: "docker.io/library/alpine:latest", + Status: "Up", + Labels: map[string]string{ + "example.dir": "/home/luca/distrobox", + }, + }, + }, + } + + cmd := commands.NewListCommand(&config.Values{}, mock) + result, err := cmd.Execute(context.Background()) + require.NoError(t, err) + + names := make([]string, 0, len(result.Containers)) + for _, c := range result.Containers { + names = append(names, c.Name) + } + assert.Equal(t, []string{"apx-box", "real-box"}, names, + "plain-box (no distrobox labels) and dir-box (distrobox only inside a label value) must be filtered out; result must be sorted by name") +} + +// Empty container list propagates through the filter without surprise. +func TestListCommand_EmptyResult(t *testing.T) { + mock := &testutil.MockContainerManager{ + ListContainersResult: []containermanager.Container{}, + } + + cmd := commands.NewListCommand(&config.Values{}, mock) + result, err := cmd.Execute(context.Background()) + require.NoError(t, err) + assert.Empty(t, result.Containers) +} diff --git a/pkg/containermanager/containermanager.go b/pkg/containermanager/containermanager.go index 9fd4810b..18586967 100644 --- a/pkg/containermanager/containermanager.go +++ b/pkg/containermanager/containermanager.go @@ -102,14 +102,24 @@ type RmOptions struct { ContainerHome string } -// IsDistrobox returns true if any label key or value contains "distrobox". -// We can't just check manager=distrobox because users can override it with -// --additional-flags --label=manager=foo (apx does this). The -// distrobox.unshare_groups label is always set on creation, so the -// substring match catches those containers too. +// IsDistrobox returns true when the container was created by distrobox. +// +// Two label shapes count as distrobox-owned, mirroring what the create path +// always sets (pkg/containermanager/providers/{docker,podman}.go): +// - manager=distrobox (the standard case) +// - any label key prefixed with "distrobox." (e.g. distrobox.unshare_groups) +// +// The key-prefix branch keeps the 24b31ed8 fix working for containers whose +// manager label is overridden via --additional-flags --label=manager=apx, +// while staying out of label *values* — those are arbitrary tool/user +// strings (workdirs, mount paths, project names) and substring-matching +// them produces false positives on unrelated containers. func (c Container) IsDistrobox() bool { - for key, value := range c.Labels { - if strings.Contains(key, "distrobox") || strings.Contains(value, "distrobox") { + if c.Labels["manager"] == "distrobox" { + return true + } + for key := range c.Labels { + if strings.HasPrefix(key, "distrobox.") { return true } } diff --git a/pkg/containermanager/containermanager_test.go b/pkg/containermanager/containermanager_test.go index 0e38c34f..ca1032d9 100644 --- a/pkg/containermanager/containermanager_test.go +++ b/pkg/containermanager/containermanager_test.go @@ -37,3 +37,55 @@ func TestContainer_IsDistrobox_NilLabels(t *testing.T) { c := containermanager.Container{Labels: nil} assert.False(t, c.IsDistrobox()) } + +// Regression: a label *value* that happens to contain the substring "distrobox" +// (e.g. another tool tagging the container with a workdir under a directory +// named "distrobox") must NOT make us claim the container as ours. Only label +// keys with the distrobox. prefix, or manager=distrobox, count. +func TestContainer_IsDistrobox_LabelValueSubstringIgnored(t *testing.T) { + c := containermanager.Container{ + Labels: map[string]string{ + "example.dir": "/home/luca/distrobox", + }, + } + assert.False(t, c.IsDistrobox()) +} + +// A foreign manager label combined with a distrobox-suffixed value must not +// trigger detection on the value side either. The container has to actually +// carry a distrobox.* key (or manager=distrobox) to be ours. +func TestContainer_IsDistrobox_ForeignManagerWithDistroboxValue(t *testing.T) { + c := containermanager.Container{ + Labels: map[string]string{ + "manager": "compose", + "com.example.image": "registry.opensuse.org/opensuse/distrobox", + }, + } + assert.False(t, c.IsDistrobox()) +} + +// Keys that merely contain the substring "distrobox" but don't use the +// reserved distrobox. namespace (e.g. another project's labels) must not +// be treated as ours. Matches the docs at pkg/containermanager/providers +// where create always sets distrobox.=… keys. +func TestContainer_IsDistrobox_UnrelatedKeyContainingSubstring(t *testing.T) { + c := containermanager.Container{ + Labels: map[string]string{ + "my-distrobox-thing": "1", + }, + } + assert.False(t, c.IsDistrobox()) +} + +// The manager-label override case (24b31ed8): the user supplied +// --additional-flags --label=manager=apx, so manager!=distrobox, but the +// distrobox.unshare_groups label is still set by the create path and is +// enough to identify the container. +func TestContainer_IsDistrobox_DistroboxKeyPrefix(t *testing.T) { + c := containermanager.Container{ + Labels: map[string]string{ + "distrobox.unshare_groups": "0", + }, + } + assert.True(t, c.IsDistrobox()) +}