From c48e2af03324e4e280e61dd0a6dc68bb0203a7ac Mon Sep 17 00:00:00 2001 From: Ross Golder Date: Wed, 29 Jul 2026 17:22:57 +0700 Subject: [PATCH] fix(flags): drop empty/whitespace entries when parsing CSV flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comma-separated flags (`--assignees`, `--labels`, `--add/remove-labels`, `--add/remove-reviewers`, `--events`, login `--scopes`) silently accepted stray commas and whitespace, producing values like `[""]` for empty input or `["alice", "", "bob"]` for `--assignees=alice,,bob`. Those blanks were then forwarded to the Gitea SDK, where they either triggered obscure server errors or silently mutated state in unexpected ways. Root cause: a mix of `strings.Split(s, ",")` and ad-hoc trim loops, none of which rejected empty segments. Introduce `modules/utils.SplitCSV` as the single parser for all CSV flag values. It: - returns `nil` for an empty or whitespace-only input (so callers can use `len(x) == 0` to mean "not set", and `cmd.IsSet("...")` keeps working), - drops empty and whitespace-only segments anywhere in the input, - otherwise trims surrounding whitespace from each remaining segment. `cmd/flags.SplitCSV` is re-exported from `cmd/flags/csvflag.go` so existing call sites read `flags.SplitCSV` without depending on a leaf package that `modules/task` already imports them. Convert the remaining raw `strings.Split(..., ",")` sites: - `cmd/flags/issue_pr.go` (issue/pr --assignees, --labels and the set/add/remove variants) - `cmd/pulls/edit.go` (`--add-reviewers`, `--remove-reviewers`) - `cmd/webhooks/create.go`, `cmd/webhooks/update.go` (`--events`) - `modules/task/login_create.go` (login `--scopes`) Drop the now-unused `"strings"` imports from the three cmd/* files. Tests: - `modules/utils/splitcsv_test.go` — table-driven coverage for empty, whitespace-only, leading/trailing commas, internal empty segments, surrounding whitespace, and re-imported identity with `flags.SplitCSV`. - `cmd/flags/csvflag_test.go` — `GetValues` happy path, error path, empty entry drop, and AvailableFields variants. - `cmd/flags/issue_pr_test.go` — regression test for `GetIssuePREditFlags` proving `SetAssignees/AddAssignees/...` produce `nil` (not `[""]`) for empty input and trimmed slices for mixed inputs. --- cmd/flags/csvflag.go | 13 +++-- cmd/flags/csvflag_test.go | 86 ++++++++++++++++++++++++++++++++++ cmd/flags/issue_pr.go | 20 +++----- cmd/flags/issue_pr_test.go | 85 +++++++++++++++++++++++++++++++++ cmd/pulls/edit.go | 5 +- cmd/webhooks/create.go | 7 +-- cmd/webhooks/update.go | 7 +-- modules/task/login_create.go | 4 +- modules/utils/splitcsv.go | 28 +++++++++++ modules/utils/splitcsv_test.go | 48 +++++++++++++++++++ 10 files changed, 270 insertions(+), 33 deletions(-) create mode 100644 cmd/flags/csvflag_test.go create mode 100644 cmd/flags/issue_pr_test.go create mode 100644 modules/utils/splitcsv.go create mode 100644 modules/utils/splitcsv_test.go diff --git a/cmd/flags/csvflag.go b/cmd/flags/csvflag.go index b3eb0972..8bd1a51c 100644 --- a/cmd/flags/csvflag.go +++ b/cmd/flags/csvflag.go @@ -38,11 +38,18 @@ func NewCsvFlag(name, usage string, aliases, availableValues, defaults []string) } } -// GetValues returns the value of the flag, parsed as a commaseparated list +// SplitCSV is re-exported here for convenience; the canonical +// implementation lives in gitea.dev/tea/modules/utils so it can be used +// from both cmd/flags and modules/task without an import cycle. +var SplitCSV = utils.SplitCSV + +// GetValues returns the value of the flag, parsed as a comma-separated list. +// Empty entries (from e.g. "alice,,bob" or a trailing comma) are dropped so +// callers never send blank identifiers to the API. func (f CsvFlag) GetValues(cmd *cli.Command) ([]string, error) { val := cmd.String(f.Name) - selection := strings.Split(val, ",") - if f.AvailableFields != nil && val != "" { + selection := utils.SplitCSV(val) + if f.AvailableFields != nil && len(selection) != 0 { for _, field := range selection { if !utils.Contains(f.AvailableFields, field) { return nil, fmt.Errorf("invalid field '%s'", field) diff --git a/cmd/flags/csvflag_test.go b/cmd/flags/csvflag_test.go new file mode 100644 index 00000000..2c21edf0 --- /dev/null +++ b/cmd/flags/csvflag_test.go @@ -0,0 +1,86 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package flags + +import ( + "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v3" +) + +func TestCsvFlagGetValues(t *testing.T) { + flag := NewCsvFlag("tags", "tag list", nil, nil, nil) + cmd := cli.Command{ + Name: "test-csv", + Flags: []cli.Flag{flag}, + Action: func(_ context.Context, _ *cli.Command) error { return nil }, + } + + cases := []struct { + name string + arg string + expected []string + }{ + {name: "empty returns nil", arg: "", expected: nil}, + {name: "single value", arg: "alice", expected: []string{"alice"}}, + {name: "two values", arg: "alice,bob", expected: []string{"alice", "bob"}}, + {name: "trailing comma dropped", arg: "alice,bob,", expected: []string{"alice", "bob"}}, + {name: "leading comma dropped", arg: ",alice,bob", expected: []string{"alice", "bob"}}, + {name: "consecutive commas dropped", arg: "alice,,bob", expected: []string{"alice", "bob"}}, + {name: "only commas returns nil", arg: ",,,", expected: nil}, + {name: "whitespace trimmed", arg: " alice , bob ", expected: []string{"alice", "bob"}}, + {name: "whitespace-only entries dropped", arg: "alice, ,bob", expected: []string{"alice", "bob"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + require.NoError(t, cmd.Run(t.Context(), []string{"test-csv", "--tags", tc.arg})) + got, err := flag.GetValues(&cmd) + require.NoError(t, err) + assert.Equal(t, tc.expected, got) + }) + } +} + +func TestCsvFlagGetValuesAvailableFields(t *testing.T) { + flag := NewCsvFlag("kind", "kind filter", []string{"K"}, []string{"issues", "pulls"}, nil) + cmd := cli.Command{ + Name: "test-csv", + Flags: []cli.Flag{flag}, + Action: func(_ context.Context, _ *cli.Command) error { return nil }, + } + + t.Run("valid value", func(t *testing.T) { + require.NoError(t, cmd.Run(t.Context(), []string{"test-csv", "--kind", "issues"})) + got, err := flag.GetValues(&cmd) + require.NoError(t, err) + assert.Equal(t, []string{"issues"}, got) + }) + + t.Run("invalid value", func(t *testing.T) { + require.NoError(t, cmd.Run(t.Context(), []string{"test-csv", "--kind", "bogus"})) + _, err := flag.GetValues(&cmd) + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid field 'bogus'") + }) + + // Regression: a stray comma in the middle of an otherwise-valid value + // no longer makes the whole flag invalid. + t.Run("trailing comma after valid value", func(t *testing.T) { + require.NoError(t, cmd.Run(t.Context(), []string{"test-csv", "--kind", "issues,"})) + got, err := flag.GetValues(&cmd) + require.NoError(t, err) + assert.Equal(t, []string{"issues"}, got) + }) +} + +func TestSplitCSVReExport(t *testing.T) { + // The flags.SplitCSV re-export must behave identically to + // utils.SplitCSV; this guards against future drift between the + // two entry points. + assert.Equal(t, []string{"alice", "bob"}, SplitCSV("alice,bob")) + assert.Nil(t, SplitCSV("")) +} diff --git a/cmd/flags/issue_pr.go b/cmd/flags/issue_pr.go index 3259c885..9b4c7547 100644 --- a/cmd/flags/issue_pr.go +++ b/cmd/flags/issue_pr.go @@ -6,7 +6,6 @@ package flags import ( stdctx "context" "fmt" - "strings" "time" gitea "gitea.dev/sdk" @@ -151,7 +150,7 @@ func GetIssuePRCreateFlags(requestCtx stdctx.Context, ctx *context.TeaContext) ( opts := gitea.CreateIssueOption{ Title: ctx.String("title"), Body: body, - Assignees: strings.Split(ctx.String("assignees"), ","), + Assignees: SplitCSV(ctx.String("assignees")), } date := ctx.String("deadline") @@ -165,7 +164,7 @@ func GetIssuePRCreateFlags(requestCtx stdctx.Context, ctx *context.TeaContext) ( client := ctx.Login.Client() - labelNames := strings.Split(ctx.String("labels"), ",") + labelNames := SplitCSV(ctx.String("labels")) if len(labelNames) != 0 { if client == nil { client = ctx.Login.Client() @@ -256,24 +255,19 @@ func GetIssuePREditFlags(ctx *context.TeaContext) (*task.EditIssueOption, error) } } if ctx.IsSet("set-assignees") { - val := ctx.String("set-assignees") - opts.SetAssignees = strings.Split(strings.ReplaceAll(val, " ", ""), ",") + opts.SetAssignees = SplitCSV(ctx.String("set-assignees")) } if ctx.IsSet("add-assignees") { - val := ctx.String("add-assignees") - opts.AddAssignees = strings.Split(strings.ReplaceAll(val, " ", ""), ",") + opts.AddAssignees = SplitCSV(ctx.String("add-assignees")) } if ctx.IsSet("remove-assignees") { - val := ctx.String("remove-assignees") - opts.RemoveAssignees = strings.Split(strings.ReplaceAll(val, " ", ""), ",") + opts.RemoveAssignees = SplitCSV(ctx.String("remove-assignees")) } if ctx.IsSet("add-labels") { - val := ctx.String("add-labels") - opts.AddLabels = strings.Split(strings.ReplaceAll(val, " ", ""), ",") + opts.AddLabels = SplitCSV(ctx.String("add-labels")) } if ctx.IsSet("remove-labels") { - val := ctx.String("remove-labels") - opts.RemoveLabels = strings.Split(strings.ReplaceAll(val, " ", ""), ",") + opts.RemoveLabels = SplitCSV(ctx.String("remove-labels")) } return &opts, nil } diff --git a/cmd/flags/issue_pr_test.go b/cmd/flags/issue_pr_test.go new file mode 100644 index 00000000..edfb652c --- /dev/null +++ b/cmd/flags/issue_pr_test.go @@ -0,0 +1,85 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package flags + +import ( + stdctx "context" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "github.com/urfave/cli/v3" + + "gitea.dev/tea/modules/context" +) + +func TestGetIssuePREditFlagsDropsEmptyCSVEntries(t *testing.T) { + cmd := cli.Command{ + Name: "edit", + Flags: IssuePREditFlags, + Action: func(_ stdctx.Context, _ *cli.Command) error { return nil }, + } + require.NoError(t, cmd.Run(t.Context(), []string{ + "edit", + "--set-assignees", "alice, ,bob", + "--add-assignees", ",carol", + "--remove-assignees", "dave,,", + "--add-labels", " bug ,,feature ", + "--remove-labels", ",wontfix,", + })) + + opts, err := GetIssuePREditFlags(&context.TeaContext{Command: &cmd}) + require.NoError(t, err) + + assert.Equal(t, []string{"alice", "bob"}, opts.SetAssignees, + "set-assignees should drop empty / whitespace-only entries") + assert.Equal(t, []string{"carol"}, opts.AddAssignees) + assert.Equal(t, []string{"dave"}, opts.RemoveAssignees) + assert.Equal(t, []string{"bug", "feature"}, opts.AddLabels, + "add-labels should trim whitespace AND drop empty entries") + assert.Equal(t, []string{"wontfix"}, opts.RemoveLabels) +} + +func TestGetIssuePREditFlagsEmptyFlagsReturnNil(t *testing.T) { + // Regression: an empty/unset flag used to materialize as a slice + // containing one empty string (from strings.Split("", ",") -> [""]). + // After SplitCSV it must be nil so callers can use len(x) == 0 + // unambiguously and downstream SDK calls don't send blank entries. + cmd := cli.Command{ + Name: "edit", + Flags: IssuePREditFlags, + Action: func(_ stdctx.Context, _ *cli.Command) error { return nil }, + } + require.NoError(t, cmd.Run(t.Context(), []string{"edit"})) + + opts, err := GetIssuePREditFlags(&context.TeaContext{Command: &cmd}) + require.NoError(t, err) + + assert.Nil(t, opts.SetAssignees) + assert.Nil(t, opts.AddAssignees) + assert.Nil(t, opts.RemoveAssignees) + assert.Nil(t, opts.AddLabels) + assert.Nil(t, opts.RemoveLabels) +} + +func TestGetIssuePREditFlagsPreservesSpacesInsideValues(t *testing.T) { + cmd := cli.Command{ + Name: "edit", + Flags: IssuePREditFlags, + Action: func(_ stdctx.Context, _ *cli.Command) error { return nil }, + } + require.NoError(t, cmd.Run(t.Context(), []string{ + "edit", + "--add-labels", "Status/Need More Info,Kind/Bug", + "--set-assignees", "john doe,jane_doe", + })) + + opts, err := GetIssuePREditFlags(&context.TeaContext{Command: &cmd}) + require.NoError(t, err) + + assert.Equal(t, []string{"Status/Need More Info", "Kind/Bug"}, opts.AddLabels, + "labels with internal spaces must be preserved") + assert.Equal(t, []string{"john doe", "jane_doe"}, opts.SetAssignees, + "assignees with internal spaces must be preserved") +} diff --git a/cmd/pulls/edit.go b/cmd/pulls/edit.go index 64604e7c..9cf00e7f 100644 --- a/cmd/pulls/edit.go +++ b/cmd/pulls/edit.go @@ -6,7 +6,6 @@ package pulls import ( stdctx "context" "fmt" - "strings" gitea "gitea.dev/sdk" @@ -103,10 +102,10 @@ func runPullsEdit(requestCtx stdctx.Context, cmd *cli.Command) error { } if cmd.IsSet("add-reviewers") { - opts.AddReviewers = strings.Split(cmd.String("add-reviewers"), ",") + opts.AddReviewers = flags.SplitCSV(cmd.String("add-reviewers")) } if cmd.IsSet("remove-reviewers") { - opts.RemoveReviewers = strings.Split(cmd.String("remove-reviewers"), ",") + opts.RemoveReviewers = flags.SplitCSV(cmd.String("remove-reviewers")) } indices, err := utils.ArgsToIndices(ctx.Args().Slice()) diff --git a/cmd/webhooks/create.go b/cmd/webhooks/create.go index 90109482..8543d06c 100644 --- a/cmd/webhooks/create.go +++ b/cmd/webhooks/create.go @@ -6,7 +6,6 @@ package webhooks import ( stdctx "context" "fmt" - "strings" gitea "gitea.dev/sdk" @@ -73,11 +72,7 @@ func runWebhooksCreate(ctx stdctx.Context, cmd *cli.Command) error { authHeader := cmd.String("authorization-header") // Parse events - eventsList := strings.Split(cmd.String("events"), ",") - events := make([]string, len(eventsList)) - for i, event := range eventsList { - events[i] = strings.TrimSpace(event) - } + events := flags.SplitCSV(cmd.String("events")) config := map[string]string{ "url": url, diff --git a/cmd/webhooks/update.go b/cmd/webhooks/update.go index 7c1b39e6..a1c92c1b 100644 --- a/cmd/webhooks/update.go +++ b/cmd/webhooks/update.go @@ -6,7 +6,6 @@ package webhooks import ( stdctx "context" "fmt" - "strings" gitea "gitea.dev/sdk" @@ -110,11 +109,7 @@ func runWebhooksUpdate(ctx stdctx.Context, cmd *cli.Command) error { // Update events if specified events := hook.Events if cmd.IsSet("events") { - eventsList := strings.Split(cmd.String("events"), ",") - events = make([]string, len(eventsList)) - for i, event := range eventsList { - events[i] = strings.TrimSpace(event) - } + events = flags.SplitCSV(cmd.String("events")) } // Update active status diff --git a/modules/task/login_create.go b/modules/task/login_create.go index 5b0b6720..298ab56d 100644 --- a/modules/task/login_create.go +++ b/modules/task/login_create.go @@ -221,8 +221,8 @@ func generateToken(ctx stdctx.Context, login config.Login, user, pass, otp, scop if len(scopes) == 0 { tokenScopes = []gitea.AccessTokenScope{gitea.AccessTokenScopeAll} } else { - for _, scope := range strings.Split(scopes, ",") { - tokenScopes = append(tokenScopes, gitea.AccessTokenScope(strings.TrimSpace(scope))) + for _, scope := range utils.SplitCSV(scopes) { + tokenScopes = append(tokenScopes, gitea.AccessTokenScope(scope)) } } diff --git a/modules/utils/splitcsv.go b/modules/utils/splitcsv.go new file mode 100644 index 00000000..6ee59820 --- /dev/null +++ b/modules/utils/splitcsv.go @@ -0,0 +1,28 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package utils + +import "strings" + +// SplitCSV parses val as a comma-separated list, trimming whitespace and +// dropping empty entries (e.g. "alice,,bob" -> ["alice", "bob"]). Returns +// nil (not an empty slice) when no non-empty entries remain, so callers +// can use len(x) == 0 unambiguously and the result round-trips cleanly +// through encoding/json (a nil slice marshals as null and is omitted by +// most Gitea API endpoints that use omitempty). +func SplitCSV(val string) []string { + if val == "" { + return nil + } + parts := strings.Split(val, ",") + var out []string + for _, p := range parts { + p = strings.TrimSpace(p) + if p == "" { + continue + } + out = append(out, p) + } + return out +} diff --git a/modules/utils/splitcsv_test.go b/modules/utils/splitcsv_test.go new file mode 100644 index 00000000..c465a566 --- /dev/null +++ b/modules/utils/splitcsv_test.go @@ -0,0 +1,48 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package utils + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestSplitCSV(t *testing.T) { + cases := []struct { + name string + in string + want []string + }{ + {name: "empty", in: "", want: nil}, + {name: "single", in: "alice", want: []string{"alice"}}, + {name: "two", in: "alice,bob", want: []string{"alice", "bob"}}, + {name: "trailing comma", in: "alice,bob,", want: []string{"alice", "bob"}}, + {name: "leading comma", in: ",alice,bob", want: []string{"alice", "bob"}}, + {name: "double comma", in: "alice,,bob", want: []string{"alice", "bob"}}, + {name: "only commas", in: ",,,", want: nil}, + {name: "whitespace around entries trimmed", in: " alice , bob ", want: []string{"alice", "bob"}}, + {name: "whitespace-only entries dropped", in: "alice, ,bob", want: []string{"alice", "bob"}}, + {name: "tabs and newlines trimmed", in: "\talice\n,\t\nbob", want: []string{"alice", "bob"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := SplitCSV(tc.in) + if !reflect.DeepEqual(got, tc.want) { + t.Errorf("SplitCSV(%q) = %#v, want %#v", tc.in, got, tc.want) + } + }) + } +} + +func TestSplitCSVEmptyReturnsNilNotEmptySlice(t *testing.T) { + // Regression: previously GetValues returned []string{""} for empty + // input, which confused length-based "is this flag set?" checks at + // every call site. nil round-trips cleanly through encoding/json. + got := SplitCSV("") + assert.Nil(t, got) + got = SplitCSV(",,,") + assert.Nil(t, got) +}