fix: match docker options by shell word when removing

Options drained out of the pre-0.38.0 `DOCKER_OPTIONS_<PHASE>` files were copied verbatim rather than re-serialized the way `docker-options:add` stores them, so `docker-options:remove` compared the canonical string it builds against a stored value that could never match it and exited successfully without removing anything. Removal now matches stored options by shell word, and stored options are rewritten into the canonical form once on upgrade, which additionally splits an entry that carried several flags on a single line into one entry per flag so a single flag can be removed and so the readers that match on a flag prefix see one value per entry. The leftover `.migrated` sentinel drain is restored to running ahead of the global short-circuit that had made it unreachable, and the plugin's Go tests are added to the test target that had never run them.

plugins/docker-options/dockeroptions.go

@@ -2,6 +2,7 @@ package dockeroptions
 
 import (
 	"fmt"
+	"slices"
 	"sort"
 	"strings"
 
@@ -36,66 +37,126 @@ func SplitOptionString(input string) (options []string, processes []string, err
 		return nil, nil, fmt.Errorf("Unable to parse docker option: %s", err.Error())
 	}
 
-	var current []string
-	flush := func() error {
-		if len(current) == 0 {
-			return nil
-		}
-		head := current[0]
+	for _, group := range groupOptionTokens(fields) {
+		head := group[0]
 		if head == "--process" {
-			if len(current) < 2 {
-				return fmt.Errorf("--process requires a value")
+			if len(group) < 2 {
+				return nil, nil, fmt.Errorf("--process requires a value")
 			}
-			if len(current) > 2 {
-				return fmt.Errorf("--process accepts a single value, got %d", len(current)-1)
+			if len(group) > 2 {
+				return nil, nil, fmt.Errorf("--process accepts a single value, got %d", len(group)-1)
 			}
-			processes = append(processes, current[1])
-			current = nil
-			return nil
+			processes = append(processes, group[1])
+			continue
 		}
 		if strings.HasPrefix(head, "--process=") {
-			if len(current) > 1 {
-				return fmt.Errorf("--process=value cannot be followed by additional tokens")
+			if len(group) > 1 {
+				return nil, nil, fmt.Errorf("--process=value cannot be followed by additional tokens")
 			}
 			processes = append(processes, head[len("--process="):])
-			current = nil
-			return nil
+			continue
 		}
-		options = append(options, joinShellTokens(current))
-		current = nil
-		return nil
-	}
-
-	for _, tok := range fields {
-		if isFlagToken(tok) && len(current) > 0 {
-			if err := flush(); err != nil {
-				return nil, nil, err
-			}
-		}
-		current = append(current, tok)
-	}
-	if err := flush(); err != nil {
-		return nil, nil, err
+		options = append(options, joinShellTokens(group))
 	}
 
 	return options, processes, nil
 }
 
+// groupOptionTokens groups shell words on flag boundaries so each group holds
+// one flag and the values that follow it. A group is emitted for every token
+// that looks like a flag, meaning `--build-arg X=Y --link a` yields
+// [["--build-arg" "X=Y"] ["--link" "a"]]. Tokens preceding the first flag stay
+// in the leading group so malformed input is never silently dropped.
+func groupOptionTokens(fields []string) [][]string {
+	var groups [][]string
+	var current []string
+	for _, tok := range fields {
+		if isFlagToken(tok) && len(current) > 0 {
+			groups = append(groups, current)
+			current = nil
+		}
+		current = append(current, tok)
+	}
+	if len(current) > 0 {
+		groups = append(groups, current)
+	}
+	return groups
+}
+
+// canonicalOptionsFromLine re-serializes a stored option line into the
+// canonical, shell-quoted form `docker-options:add` produces, splitting it on
+// flag boundaries so a line carrying several flags becomes one entry per flag.
+// Unlike SplitOptionString it never lifts `--process`: a stored line is data
+// rather than command-line input.
+//
+// The line is returned unchanged when it cannot be parsed, or when the parser
+// stops before consuming all of it - a line whose tail begins with an unquoted
+// `#` reads as a shell comment, and dropping it would lose stored data.
+func canonicalOptionsFromLine(line string) []string {
+	fields, end, err := literalFieldsWithEnd(line)
+	if err != nil || end < len(strings.TrimRight(line, " \t")) {
+		return []string{line}
+	}
+
+	groups := groupOptionTokens(fields)
+	options := make([]string, 0, len(groups))
+	for _, group := range groups {
+		options = append(options, joinShellTokens(group))
+	}
+	return options
+}
+
+// optionsEqual reports whether two option strings denote the same docker
+// option. Exact equality short-circuits; otherwise both sides are
+// shell-tokenized without expansion and compared word by word, so an option
+// held in a non-canonical form - drained verbatim out of a pre-0.38.25
+// DOCKER_OPTIONS_<PHASE> file, or written directly by another plugin - still
+// matches the canonically re-serialized string the CLI builds. Input that
+// cannot be parsed never matches.
+func optionsEqual(stored string, option string) bool {
+	if stored == option {
+		return true
+	}
+
+	storedFields, err := literalFields(stored)
+	if err != nil {
+		return false
+	}
+
+	optionFields, err := literalFields(option)
+	if err != nil {
+		return false
+	}
+
+	return slices.Equal(storedFields, optionFields)
+}
+
 // literalFields splits input into shell words using the parser directly, so
 // quotes delimit words and are stripped from the stored value, but nothing is
 // expanded. Parameter expansions, command substitutions, and other
 // metacharacters are preserved verbatim. Malformed input, such as an unbalanced
 // quote, returns the parser error.
 func literalFields(input string) ([]string, error) {
+	fields, _, err := literalFieldsWithEnd(input)
+	return fields, err
+}
+
+// literalFieldsWithEnd is literalFields plus the byte offset just past the last
+// word the parser consumed. Callers that must not lose data compare the offset
+// against the length of the input to detect a tail the parser skipped, such as
+// a shell comment.
+func literalFieldsWithEnd(input string) ([]string, int, error) {
 	parser := syntax.NewParser()
 	var fields []string
+	end := 0
 	for word, err := range parser.WordsSeq(strings.NewReader(input)) {
 		if err != nil {
-			return nil, err
+			return nil, 0, err
 		}
 		fields = append(fields, literalWordValue(input, word.Parts))
+		end = int(word.End().Offset())
 	}
-	return fields, nil
+	return fields, end, nil
 }
 
 // literalWordValue reconstructs the unquoted, unexpanded value of a shell word.
@@ -304,7 +365,11 @@ func RemoveDockerOptionFromPhases(appName string, phases []string, option string
 	return RemoveDockerOptionFromProcessPhases(appName, []string{DefaultProcessType}, phases, option)
 }
 
-// RemoveDockerOptionFromProcessPhases removes an option from the specified process types and phases.
+// RemoveDockerOptionFromProcessPhases removes an option from the specified
+// process types and phases. Stored options are matched against the requested
+// option by shell word rather than by raw string, so an entry held in a
+// non-canonical form still matches the canonically re-serialized string the
+// CLI hands down.
 func RemoveDockerOptionFromProcessPhases(appName string, processTypes []string, phases []string, option string) error {
 	if len(processTypes) == 0 {
 		processTypes = []string{DefaultProcessType}
@@ -318,7 +383,7 @@ func RemoveDockerOptionFromProcessPhases(appName string, processTypes []string,
 
 			newOptions := []string{}
 			for _, opt := range options {
-				if opt != option {
+				if !optionsEqual(opt, option) {
 					newOptions = append(newOptions, opt)
 				}
 			}

plugins/docker-options/dockeroptions_test.go

@@ -175,6 +175,142 @@ func TestQuoteShellArg(t *testing.T) {
 	}
 }
 
+func TestOptionsEqual(t *testing.T) {
+	cases := []struct {
+		name   string
+		stored string
+		option string
+		want   bool
+	}{
+		{
+			name:   "identical strings",
+			stored: "-v /tmp:/tmp",
+			option: "-v /tmp:/tmp",
+			want:   true,
+		},
+		{
+			name:   "legacy unquoted command substitution matches canonical form",
+			stored: "--group-add $(getent group docker | cut -d: -f3)",
+			option: "--group-add '$(getent group docker | cut -d: -f3)'",
+			want:   true,
+		},
+		{
+			name:   "legacy unquoted parameter expansion matches canonical form",
+			stored: "--label FOO=$BAR",
+			option: "--label 'FOO=$BAR'",
+			want:   true,
+		},
+		{
+			name:   "double quoted matches single quoted",
+			stored: `--label "hello world"`,
+			option: "--label 'hello world'",
+			want:   true,
+		},
+		{
+			name:   "different values do not match",
+			stored: "-v /tmp:/tmp",
+			option: "-v /var:/var",
+			want:   false,
+		},
+		{
+			name:   "different flags do not match",
+			stored: "--link foo",
+			option: "--link bar",
+			want:   false,
+		},
+		{
+			name:   "multi-flag entry does not match a single flag",
+			stored: "-v /a:/a -v /b:/b",
+			option: "-v /a:/a",
+			want:   false,
+		},
+		{
+			name:   "unbalanced quote never matches",
+			stored: "--label 'unbalanced",
+			option: "--label unbalanced",
+			want:   false,
+		},
+		{
+			name:   "empty stored entry does not match an option",
+			stored: "",
+			option: "-v /tmp:/tmp",
+			want:   false,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			if got := optionsEqual(tc.stored, tc.option); got != tc.want {
+				t.Errorf("optionsEqual(%q, %q) = %v, want %v", tc.stored, tc.option, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestCanonicalOptionsFromLine(t *testing.T) {
+	cases := []struct {
+		name string
+		line string
+		want []string
+	}{
+		{
+			name: "quotes an unquoted command substitution",
+			line: "--group-add $(getent group docker | cut -d: -f3)",
+			want: []string{"--group-add '$(getent group docker | cut -d: -f3)'"},
+		},
+		{
+			name: "splits a line carrying several flags",
+			line: "-v /a:/a -v /b:/b",
+			want: []string{"-v /a:/a", "-v /b:/b"},
+		},
+		{
+			name: "leaves an already canonical line untouched",
+			line: "--group-add '$(getent group docker | cut -d: -f3)'",
+			want: []string{"--group-add '$(getent group docker | cut -d: -f3)'"},
+		},
+		{
+			name: "leaves a plain option untouched",
+			line: "--restart=on-failure:5",
+			want: []string{"--restart=on-failure:5"},
+		},
+		{
+			name: "preserves a line with an unbalanced quote",
+			line: "--label 'unbalanced",
+			want: []string{"--label 'unbalanced"},
+		},
+		{
+			name: "preserves a line whose tail reads as a shell comment",
+			line: "-v /a:/a # a note",
+			want: []string{"-v /a:/a # a note"},
+		},
+		{
+			name: "returns nothing for an empty line",
+			line: "",
+			want: nil,
+		},
+	}
+
+	for _, tc := range cases {
+		t.Run(tc.name, func(t *testing.T) {
+			got := canonicalOptionsFromLine(tc.line)
+			if !equalStringSlice(got, tc.want) {
+				t.Errorf("canonicalOptionsFromLine(%q) = %q, want %q", tc.line, got, tc.want)
+			}
+
+			if len(tc.want) == 0 {
+				return
+			}
+
+			// Canonicalization must be idempotent so the one-time repair
+			// pass is a no-op on stores it has already rewritten.
+			again := canonicalizeOptionLines(got)
+			if !equalStringSlice(again, got) {
+				t.Errorf("canonicalizing %q again = %q, want %q", got, again, got)
+			}
+		})
+	}
+}
+
 func equalStringSlice(a, b []string) bool {
 	if len(a) == 0 && len(b) == 0 {
 		return true

plugins/docker-options/functions.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"slices"
 	"strings"
 
 	"github.com/dokku/dokku/plugins/common"
@@ -278,10 +279,6 @@ func convertLegacyMigratedMarker(appName, phase string) error {
 // previous release - who have `migrated-from-files` set AND `.migrated`
 // files on disk - still get those files drained into properties.
 func migrateLegacyDockerOptionsFiles() error {
-	if common.PropertyGet("docker-options", "--global", "migrated-from-files") == "true" {
-		return nil
-	}
-
 	apps, err := common.DokkuApps()
 	if err != nil {
 		if errors.Is(err, common.NoAppsExist) {
@@ -301,6 +298,10 @@ func migrateLegacyDockerOptionsFiles() error {
 		}
 	}
 
+	if common.PropertyGet("docker-options", "--global", "migrated-from-files") == "true" {
+		return nil
+	}
+
 	for _, appName := range apps {
 		for _, phase := range availablePhases {
 			legacyPath := filepath.Join(common.AppRoot(appName), "DOCKER_OPTIONS_"+strings.ToUpper(phase))
@@ -313,8 +314,9 @@ func migrateLegacyDockerOptionsFiles() error {
 				return err
 			}
 
-			if len(lines) > 0 {
-				if err := common.PropertyListWrite("docker-options", appName, propertyKey(DefaultProcessType, phase), lines); err != nil {
+			options := canonicalizeOptionLines(lines)
+			if len(options) > 0 {
+				if err := common.PropertyListWrite("docker-options", appName, propertyKey(DefaultProcessType, phase), options); err != nil {
 					return err
 				}
 				if err := common.PropertyWrite("docker-options", appName, migratedPropertyKey(phase), "true"); err != nil {
@@ -352,6 +354,17 @@ func readLegacyOptionsFile(path string) ([]string, error) {
 	return lines, nil
 }
 
+// canonicalizeOptionLines re-serializes stored option lines into the canonical
+// form `docker-options:add` writes, flattening the per-line results so a line
+// carrying several flags becomes one entry per flag. Order is preserved.
+func canonicalizeOptionLines(lines []string) []string {
+	options := make([]string, 0, len(lines))
+	for _, line := range lines {
+		options = append(options, canonicalOptionsFromLine(line)...)
+	}
+	return options
+}
+
 // traefikLabelMigrationKey gates the one-time pass that repairs docker-option
 // labels whose backticks were stored with a stray leading backslash by an
 // earlier release's option tokenizer (e.g. Traefik rules like Host(\`app\`)).
@@ -451,3 +464,63 @@ func labelSpec(option string) (string, bool) {
 	s = strings.TrimPrefix(s, "\"")
 	return s, true
 }
+
+// canonicalOptionsMigrationKey gates the one-time pass that re-serializes
+// stored docker options into the canonical form. Options drained out of the
+// legacy DOCKER_OPTIONS_<PHASE> flat files by releases before this one were
+// copied verbatim, so they never matched the canonically re-serialized string
+// `docker-options:remove` compares against.
+const canonicalOptionsMigrationKey = "migrated-canonical-options"
+
+// migrateNonCanonicalOptions rewrites stored docker options into the canonical
+// form so the whole store uses the same representation `docker-options:add`
+// writes. Two things get repaired: values whose shell metacharacters were never
+// quoted, and lines carrying several flags, which are split into one entry per
+// flag so a single flag can be removed and so the prefix-matching readers
+// (`getRestartPolicy`, `GetSpecifiedDockerOptionsForPhase`) see one value per
+// entry. Canonicalization is idempotent, so options written through the CLI are
+// left untouched. It runs once, guarded by a global property.
+func migrateNonCanonicalOptions() error {
+	if common.PropertyGet("docker-options", "--global", canonicalOptionsMigrationKey) == "true" {
+		return nil
+	}
+
+	apps, err := common.DokkuApps()
+	if err != nil {
+		if errors.Is(err, common.NoAppsExist) {
+			return common.PropertyWrite("docker-options", "--global", canonicalOptionsMigrationKey, "true")
+		}
+		return err
+	}
+
+	for _, appName := range apps {
+		properties, err := common.PropertyGetAll("docker-options", appName)
+		if err != nil {
+			return err
+		}
+
+		for key := range properties {
+			processType, phase, ok := splitPropertyKey(key)
+			if !ok {
+				continue
+			}
+
+			options, err := GetDockerOptionsForProcessPhase(appName, processType, phase)
+			if err != nil {
+				return err
+			}
+
+			canonical := canonicalizeOptionLines(options)
+			if slices.Equal(canonical, options) {
+				continue
+			}
+
+			if err := writeDockerOptionsForProcessPhase(appName, processType, phase, canonical); err != nil {
+				return err
+			}
+			common.LogInfo1(fmt.Sprintf("Rewrote docker-options into canonical form for %s (%s %s)", appName, processType, phase))
+		}
+	}
+
+	return common.PropertyWrite("docker-options", "--global", canonicalOptionsMigrationKey, "true")
+}

plugins/docker-options/migration_test.go

@@ -278,6 +278,147 @@ func TestMigrateTraefikLabelBackticks(t *testing.T) {
 	}
 }
 
+// TestMigrateLegacyDockerOptionsFiles_CanonicalizesLines covers the
+// regression behind #8904: legacy lines were drained verbatim, so an
+// option holding shell metacharacters could never match the
+// canonically re-serialized string `docker-options:remove` builds.
+func TestMigrateLegacyDockerOptionsFiles_CanonicalizesLines(t *testing.T) {
+	dokkuRoot := setupMigrationEnv(t)
+
+	writeLegacyDockerOptionsFile(t, dokkuRoot, "alpha", "DEPLOY", "--group-add $(getent group docker | cut -d: -f3)\n-v /a:/a -v /b:/b\n")
+
+	if err := migrateLegacyDockerOptionsFiles(); err != nil {
+		t.Fatalf("migrateLegacyDockerOptionsFiles: %v", err)
+	}
+
+	got, err := GetDockerOptionsForProcessPhase("alpha", "_default_", "deploy")
+	if err != nil {
+		t.Fatalf("GetDockerOptionsForProcessPhase: %v", err)
+	}
+	want := []string{
+		"--group-add '$(getent group docker | cut -d: -f3)'",
+		"-v /a:/a",
+		"-v /b:/b",
+	}
+	if !equalStrings(got, want) {
+		t.Errorf("alpha deploy = %q, want %q", got, want)
+	}
+}
+
+// TestMigrateNonCanonicalOptions covers the repair pass for installs
+// that already drained their legacy files under an earlier release, so
+// the non-canonical values are sitting in the property store.
+func TestMigrateNonCanonicalOptions(t *testing.T) {
+	dokkuRoot := setupMigrationEnv(t)
+	if err := os.MkdirAll(filepath.Join(dokkuRoot, "alpha"), 0755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+
+	if err := common.PropertyListWrite("docker-options", "alpha", "_default_.deploy", []string{
+		"--group-add $(getent group docker | cut -d: -f3)",
+		"-v /a:/a -v /b:/b",
+	}); err != nil {
+		t.Fatalf("seed default scope: %v", err)
+	}
+	if err := common.PropertyListWrite("docker-options", "alpha", "web.deploy", []string{
+		"--label FOO=$BAR",
+	}); err != nil {
+		t.Fatalf("seed web scope: %v", err)
+	}
+	if err := common.PropertyListWrite("docker-options", "alpha", "_default_.build", []string{
+		"--build-arg FOO=bar",
+	}); err != nil {
+		t.Fatalf("seed build scope: %v", err)
+	}
+
+	if err := migrateNonCanonicalOptions(); err != nil {
+		t.Fatalf("migrateNonCanonicalOptions: %v", err)
+	}
+
+	deploy, err := GetDockerOptionsForProcessPhase("alpha", "_default_", "deploy")
+	if err != nil {
+		t.Fatalf("GetDockerOptionsForProcessPhase deploy: %v", err)
+	}
+	wantDeploy := []string{
+		"--group-add '$(getent group docker | cut -d: -f3)'",
+		"-v /a:/a",
+		"-v /b:/b",
+	}
+	if !equalStrings(deploy, wantDeploy) {
+		t.Errorf("alpha deploy = %q, want %q", deploy, wantDeploy)
+	}
+
+	web, err := GetDockerOptionsForProcessPhase("alpha", "web", "deploy")
+	if err != nil {
+		t.Fatalf("GetDockerOptionsForProcessPhase web.deploy: %v", err)
+	}
+	if !equalStrings(web, []string{"--label 'FOO=$BAR'"}) {
+		t.Errorf("alpha web.deploy = %q, want [--label 'FOO=$BAR']", web)
+	}
+
+	build, err := GetDockerOptionsForProcessPhase("alpha", "_default_", "build")
+	if err != nil {
+		t.Fatalf("GetDockerOptionsForProcessPhase build: %v", err)
+	}
+	if !equalStrings(build, []string{"--build-arg FOO=bar"}) {
+		t.Errorf("alpha build = %q, want [--build-arg FOO=bar]", build)
+	}
+
+	if common.PropertyGet("docker-options", "--global", canonicalOptionsMigrationKey) != "true" {
+		t.Errorf("expected %s guard to be set", canonicalOptionsMigrationKey)
+	}
+
+	if err := migrateNonCanonicalOptions(); err != nil {
+		t.Fatalf("second migrateNonCanonicalOptions: %v", err)
+	}
+	deployAfter, err := GetDockerOptionsForProcessPhase("alpha", "_default_", "deploy")
+	if err != nil {
+		t.Fatalf("GetDockerOptionsForProcessPhase deploy (post re-run): %v", err)
+	}
+	if !equalStrings(deployAfter, wantDeploy) {
+		t.Errorf("alpha deploy after rerun = %q, want %q (idempotency violated)", deployAfter, wantDeploy)
+	}
+}
+
+// TestRemoveDockerOptionFromPhases_MatchesNonCanonicalStoredOption is
+// the regression test for #8904 itself: an option left in the store in
+// its pre-0.38.25 unquoted form must still be removable using the
+// string a user would type.
+func TestRemoveDockerOptionFromPhases_MatchesNonCanonicalStoredOption(t *testing.T) {
+	dokkuRoot := setupMigrationEnv(t)
+	if err := os.MkdirAll(filepath.Join(dokkuRoot, "alpha"), 0755); err != nil {
+		t.Fatalf("MkdirAll: %v", err)
+	}
+
+	stored := "--group-add $(getent group docker | cut -d: -f3)"
+	if err := common.PropertyListWrite("docker-options", "alpha", "_default_.deploy", []string{
+		stored,
+		"-v /keep:/keep",
+	}); err != nil {
+		t.Fatalf("seed: %v", err)
+	}
+
+	options, _, err := SplitOptionString(stored)
+	if err != nil {
+		t.Fatalf("SplitOptionString: %v", err)
+	}
+	if len(options) != 1 {
+		t.Fatalf("SplitOptionString returned %d options, want 1", len(options))
+	}
+
+	if err := RemoveDockerOptionFromPhases("alpha", []string{"deploy"}, options[0]); err != nil {
+		t.Fatalf("RemoveDockerOptionFromPhases: %v", err)
+	}
+
+	got, err := GetDockerOptionsForProcessPhase("alpha", "_default_", "deploy")
+	if err != nil {
+		t.Fatalf("GetDockerOptionsForProcessPhase: %v", err)
+	}
+	if !equalStrings(got, []string{"-v /keep:/keep"}) {
+		t.Errorf("alpha deploy = %q, want [-v /keep:/keep]", got)
+	}
+}
+
 func equalStrings(a, b []string) bool {
 	if len(a) != len(b) {
 		return false

plugins/docker-options/triggers.go

@@ -24,6 +24,10 @@ func TriggerInstall() error {
 		return fmt.Errorf("Unable to repair docker-options label backticks: %v", err)
 	}
 
+	if err := migrateNonCanonicalOptions(); err != nil {
+		return fmt.Errorf("Unable to rewrite docker-options into canonical form: %v", err)
+	}
+
 	return nil
 }