fix(flags): reject an empty element in a multi-value flag

  pflag's StringSlice splits each value with a CSV reader, and that reader yields
  nothing for an empty string. An empty element is therefore appended as nothing
  and leaves no trace: after parsing, `--attachments "" --attachments file` cannot
  be told apart from `--attachments file`, so nothing downstream can report what
  was lost.

  Two flags where that matters now use a value type that refuses an empty element
  at Set, the last point where it still exists. --attachments is the case this bug
  was filed for: an attestation is recorded without evidence its author believed
  was on it. --template is worse in kind, because it names the attestations a flow
  requires, so a dropped element weakens that flow for every artifact passing
  through it afterwards rather than spoiling one record.

  Comma splitting is kept rather than switching to pflag's StringArray, which
  stores values verbatim. An environment variable cannot be repeated, so a comma
  list is the only way to give a multi-value flag more than one value from the
  environment; a regression test pins that.

  The type implements pflag.SliceValue as well as pflag.Value, so the refusal also
  covers the config file, which bindFlags applies through Replace.

Author: JonJagger

cmd/kosli/attestGeneric_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
 )
 
@@ -203,6 +204,28 @@ func (suite *AttestGenericCommandTestSuite) TestAttestGenericCmd() {
 	runTestCmd(suite.T(), tests)
 }
 
+// TestAttestGenericRejectsEmptyAttachment pins the case this issue was filed
+// for. `--attachments "$FILE" --attachments provenance.json` with FILE unset
+// attaches only provenance.json: the empty element is discarded while the flag
+// still counts as set, so the attestation is recorded without evidence its
+// author believed was on it, and nothing in the output says so.
+//
+// The surviving attachment is a real file, so that before the refusal exists
+// the command succeeds rather than failing to stat a missing path - otherwise
+// this test would pass for the wrong reason.
+//
+// This is deliberately not part of AttestGenericCommandTestSuite: the refusal
+// happens while flags are parsed, before any request, so it needs no server.
+func TestAttestGenericRejectsEmptyAttachment(t *testing.T) {
+	_, _, _, _, err := executeCommandC(
+		`attest generic --attachments "" --attachments testdata/file1 ` +
+			`--fingerprint 0000000000000000000000000000000000000000000000000000000000000001 ` +
+			`--name foo --flow f --trail t --org demo --api-token DRY_RUN --dry-run`)
+
+	require.Error(t, err)
+	require.ErrorContains(t, err, "attachments")
+}
+
 // In order for 'go test' to run this suite, we need to create
 // a normal test function and pass our suite to suite.Run
 func TestAttestGenericCommandTestSuite(t *testing.T) {

cmd/kosli/createFlow.go

@@ -87,7 +87,7 @@ func newCreateFlowCmd(out io.Writer) *cobra.Command {
 
 	cmd.Flags().StringVar(&o.payload.Description, "description", "", flowDescriptionFlag)
 	cmd.Flags().StringVar(&o.payload.Visibility, "visibility", "", visibilityFlag)
-	cmd.Flags().StringSliceVarP(&o.payload.Template, "template", "t", []string{}, templateFlag)
+	cmd.Flags().VarP(newNonEmptyStringSlice(&o.payload.Template), "template", "t", templateFlag)
 	cmd.Flags().StringVarP(&o.TemplateFile, "template-file", "f", "", templateFileFlag)
 	cmd.Flags().BoolVar(&o.UseEmptyTemplate, "use-empty-template", false, useEmptyTemplateFlag)
 	addDryRunFlag(cmd)

cmd/kosli/createFlow_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
 )
 
@@ -112,6 +113,23 @@ func (suite *CreateFlowCommandTestSuite) TestCreateFlowCmd() {
 	runTestCmd(suite.T(), tests)
 }
 
+// TestCreateFlowRejectsEmptyTemplateElement pins that an empty --template
+// element is refused rather than dropped. --template names the attestations a
+// flow requires, so an element lost on the way in weakens the flow's template
+// for every artifact that passes through it afterwards, long after the run that
+// caused it. `-t "$COVERAGE" -t unit-test` with COVERAGE unset is the shape
+// that does it.
+//
+// This is deliberately not part of CreateFlowCommandTestSuite: the rejection
+// happens while flags are parsed, before any request, so it needs no server.
+func TestCreateFlowRejectsEmptyTemplateElement(t *testing.T) {
+	_, _, _, _, err := executeCommandC(
+		`create flow myflow -t "" -t unit-test --org demo --api-token DRY_RUN --dry-run`)
+
+	require.Error(t, err)
+	require.ErrorContains(t, err, "template")
+}
+
 // In order for 'go test' to run this suite, we need to create
 // a normal test function and pass our suite to suite.Run
 func TestCreateFlowCommandTestSuite(t *testing.T) {

cmd/kosli/flags.go

@@ -110,7 +110,7 @@ func addAttestationFlags(cmd *cobra.Command, o *CommonAttestationOptions, payloa
 	cmd.Flags().StringVarP(&o.flowName, "flow", "f", "", flowNameFlag)
 	cmd.Flags().StringVarP(&o.trailName, "trail", "T", "", trailNameFlag)
 	cmd.Flags().StringVarP(&o.userDataFilePath, "user-data", "u", "", attestationUserDataFlag)
-	cmd.Flags().StringSliceVar(&o.attachments, "attachments", []string{}, attachmentsFlag)
+	cmd.Flags().Var(newNonEmptyStringSlice(&o.attachments), "attachments", attachmentsFlag)
 	cmd.Flags().StringVar(&o.srcRepoRoot, "repo-root", ".", attestationRepoRootFlag)
 	cmd.Flags().StringVar(&payload.Description, "description", "", attestationDescription)
 	cmd.Flags().StringVar(&o.repoID, "repo-id", DefaultValue(ci, "repo-id"), repoIDFlag)

cmd/kosli/nonEmptyStringSlice.go

@@ -0,0 +1,114 @@
+package main
+
+import (
+	"encoding/csv"
+	"fmt"
+	"strings"
+
+	"github.com/spf13/pflag"
+)
+
+// SliceValue is satisfied by type assertion at the point of use: bindFlags
+// applies a config file list through Replace, and a post-parse check reads
+// GetSlice. Nothing enforces it, so a drifting signature would silently fall
+// back to the string path instead of failing. This assertion makes that a
+// compile error. pflag.Value needs no such assertion - the Flags().VarP call
+// registering the flag already requires it.
+var _ pflag.SliceValue = (*nonEmptyStringSlice)(nil)
+
+// nonEmptyStringSlice holds the values of a multi-value flag and refuses an
+// empty one.
+//
+// pflag's own StringSlice splits each value with a CSV reader, which yields
+// nothing at all for an empty string. An empty element is therefore appended as
+// nothing and leaves no trace: after parsing, `--attachments "" --attachments
+// provenance.json` cannot be told apart from `--attachments provenance.json`.
+// Set is the last point at which the element still exists, so it is where the
+// refusal has to happen.
+//
+// The comma splitting is kept rather than switching to pflag's StringArray,
+// which stores values verbatim. An environment variable cannot be repeated, so
+// a comma list is the only way to give a multi-value flag more than one value
+// from the environment.
+type nonEmptyStringSlice struct {
+	values  *[]string
+	changed bool
+}
+
+// newNonEmptyStringSlice returns a flag value writing through to values.
+func newNonEmptyStringSlice(values *[]string) *nonEmptyStringSlice {
+	return &nonEmptyStringSlice{values: values}
+}
+
+// Append adds one value, refusing an empty one. It is part of pflag.SliceValue.
+func (s *nonEmptyStringSlice) Append(value string) error {
+	if value == "" {
+		return fmt.Errorf("empty values are not allowed")
+	}
+	*s.values = append(*s.values, value)
+	s.changed = true
+	return nil
+}
+
+// Replace overwrites every value, refusing an empty one. It is part of
+// pflag.SliceValue, and it is the path a config file list arrives by, so the
+// refusal has to hold here as well as in Set. Checking only Set would leave the
+// config file as a way in for the values the flag refuses on the command line.
+func (s *nonEmptyStringSlice) Replace(values []string) error {
+	for _, value := range values {
+		if value == "" {
+			return fmt.Errorf("empty values are not allowed")
+		}
+	}
+	*s.values = values
+	s.changed = true
+	return nil
+}
+
+// GetSlice returns the values as separate elements. It is part of
+// pflag.SliceValue, and it is what lets a post-parse check see the elements
+// rather than the bracketed String form.
+func (s *nonEmptyStringSlice) GetSlice() []string {
+	return *s.values
+}
+
+// String renders the values the way pflag's own StringSlice does, so help text
+// and defaults read identically for a flag that switches to this type.
+func (s *nonEmptyStringSlice) String() string {
+	return "[" + strings.Join(*s.values, ",") + "]"
+}
+
+// Type names the type shown in help text. It matches pflag's StringSlice so a
+// flag that switches to this type keeps the same help output.
+func (s *nonEmptyStringSlice) Type() string {
+	return "stringSlice"
+}
+
+// Set splits value on commas and stores the result, replacing whatever the flag
+// held on the first call and appending on later ones, which is how pflag
+// accumulates a repeated flag. It returns an error when value is empty or
+// contains an empty element. pflag prefixes that error with the flag name.
+func (s *nonEmptyStringSlice) Set(value string) error {
+	if value == "" {
+		return fmt.Errorf("empty values are not allowed")
+	}
+
+	elements, err := csv.NewReader(strings.NewReader(value)).Read()
+	if err != nil {
+		return err
+	}
+	for _, element := range elements {
+		if element == "" {
+			return fmt.Errorf("empty values are not allowed")
+		}
+	}
+
+	if s.changed {
+		*s.values = append(*s.values, elements...)
+	} else {
+		*s.values = elements
+		s.changed = true
+	}
+
+	return nil
+}

cmd/kosli/nonEmptyStringSlice_test.go

@@ -0,0 +1,91 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestNonEmptyStringSliceRejectsAnEmptyElement pins the whole point of the
+// type. pflag's own StringSlice runs each value through a CSV reader, and that
+// reader yields nothing for an empty string, so an empty element is appended as
+// nothing and leaves no trace behind. Refusing it at Set is the only place the
+// element still exists to be seen.
+func TestNonEmptyStringSliceRejectsAnEmptyElement(t *testing.T) {
+	for _, value := range []string{"", "a,,b", ",a", "a,"} {
+		t.Run(value, func(t *testing.T) {
+			var values []string
+			slice := newNonEmptyStringSlice(&values)
+
+			require.Error(t, slice.Set(value))
+		})
+	}
+}
+
+// TestNonEmptyStringSliceKeepsCommaSplitting pins the behaviour that stops this
+// being solved with pflag's StringArray instead. An environment variable cannot
+// be repeated, so a comma list is the only way to give a multi-value flag more
+// than one value from the environment.
+func TestNonEmptyStringSliceKeepsCommaSplitting(t *testing.T) {
+	var values []string
+	slice := newNonEmptyStringSlice(&values)
+
+	require.NoError(t, slice.Set("a,b"))
+
+	require.Equal(t, []string{"a", "b"}, values)
+}
+
+// TestNonEmptyStringSliceReplaceRejectsAnEmptyElement pins that the refusal
+// covers the pflag.SliceValue path as well as Set. bindFlags applies a config
+// file list through Replace, so a type that checked only Set would leave the
+// config file as a way in for exactly the values the flag refuses on the
+// command line.
+func TestNonEmptyStringSliceReplaceRejectsAnEmptyElement(t *testing.T) {
+	var values []string
+	slice := newNonEmptyStringSlice(&values)
+
+	require.Error(t, slice.Replace([]string{"a", "", "b"}))
+}
+
+// TestNonEmptyStringSliceReplaceOverwrites pins that Replace discards whatever
+// the flag held, which is what pflag documents it to do.
+func TestNonEmptyStringSliceReplaceOverwrites(t *testing.T) {
+	var values []string
+	slice := newNonEmptyStringSlice(&values)
+	require.NoError(t, slice.Set("a"))
+
+	require.NoError(t, slice.Replace([]string{"b", "c"}))
+
+	require.Equal(t, []string{"b", "c"}, values)
+}
+
+// TestNonEmptyStringSliceAppendRejectsAnEmptyElement pins the same refusal on
+// the remaining pflag.SliceValue method.
+func TestNonEmptyStringSliceAppendRejectsAnEmptyElement(t *testing.T) {
+	var values []string
+	slice := newNonEmptyStringSlice(&values)
+
+	require.Error(t, slice.Append(""))
+}
+
+// TestNonEmptyStringSliceGetSliceReturnsTheValues pins the accessor that lets a
+// post-parse check see the elements, rather than the bracketed String form.
+func TestNonEmptyStringSliceGetSliceReturnsTheValues(t *testing.T) {
+	var values []string
+	slice := newNonEmptyStringSlice(&values)
+	require.NoError(t, slice.Set("a,b"))
+
+	require.Equal(t, []string{"a", "b"}, slice.GetSlice())
+}
+
+// TestNonEmptyStringSliceAppendsOnRepeatedSet pins that a flag given more than
+// once accumulates its values, which is how pflag applies a repeated flag.
+func TestNonEmptyStringSliceAppendsOnRepeatedSet(t *testing.T) {
+	var values []string
+	slice := newNonEmptyStringSlice(&values)
+
+	require.NoError(t, slice.Set("a"))
+	require.NoError(t, slice.Set("b,c"))
+
+	require.Equal(t, []string{"a", "b", "c"}, values)
+}