many: fix snap set and unset panics on empty strings (#14958)

client/clientutil/config.go

@@ -20,6 +20,7 @@
 package clientutil
 
 import (
+	"errors"
 	"fmt"
 	"strings"
 
@@ -52,6 +53,10 @@ func ParseConfigValues(confValues []string, opts *ParseConfigOptions) (map[strin
 		parts := strings.SplitN(patchValue, "=", 2)
 		if len(parts) == 1 && strings.HasSuffix(patchValue, "!") {
 			key := strings.TrimSuffix(patchValue, "!")
+			if key == "" {
+				return nil, nil, errors.New(i18n.G("configuration keys cannot be empty (use key! to unset a key)"))
+			}
+
 			patchValues[key] = nil
 			keys = append(keys, key)
 			continue
@@ -61,6 +66,10 @@ func ParseConfigValues(confValues []string, opts *ParseConfigOptions) (map[strin
 			return nil, nil, fmt.Errorf(i18n.G("invalid configuration: %q (want key=value)"), patchValue)
 		}
 
+		if parts[0] == "" {
+			return nil, nil, errors.New(i18n.G("configuration keys cannot be empty"))
+		}
+
 		if opts.String {
 			patchValues[parts[0]] = parts[1]
 		} else {

client/clientutil/config_test.go

@@ -80,3 +80,14 @@ func (s *parseSuite) TestParseConfigValues(c *C) {
 	})
 	c.Assert(keys, DeepEquals, []string{"foo"})
 }
+
+func (s *parseSuite) TestParseConfigValuesEmptyKey(c *C) {
+	_, _, err := clientutil.ParseConfigValues([]string{""}, nil)
+	c.Assert(err, ErrorMatches, `invalid configuration: "" \(want key=value\)`)
+
+	_, _, err = clientutil.ParseConfigValues([]string{"=value"}, nil)
+	c.Assert(err, ErrorMatches, `configuration keys cannot be empty`)
+
+	_, _, err = clientutil.ParseConfigValues([]string{"!"}, nil)
+	c.Assert(err, ErrorMatches, `configuration keys cannot be empty \(use key! to unset a key\)`)
+}

cmd/snap/cmd_set_test.go

@@ -311,3 +311,14 @@ func (s *confdbSuite) TestConfdbSetExclamationMark(c *check.C) {
 	_, err := snapset.Parser(snapset.Client()).ParseArgs([]string{"set", "foo/bar/baz", "abc!"})
 	c.Assert(err, check.IsNil)
 }
+
+func (s *confdbSuite) TestSetEmptyKey(c *check.C) {
+	_, err := snapset.Parser(snapset.Client()).ParseArgs([]string{"set", "some-snap", "!"})
+	c.Assert(err, check.ErrorMatches, "configuration keys cannot be empty \\(use key! to unset a key\\)")
+
+	_, err = snapset.Parser(snapset.Client()).ParseArgs([]string{"set", "some-snap", "=value"})
+	c.Assert(err, check.ErrorMatches, "configuration keys cannot be empty")
+
+	_, err = snapset.Parser(snapset.Client()).ParseArgs([]string{"set", "some-snap", "="})
+	c.Assert(err, check.ErrorMatches, "configuration keys cannot be empty")
+}

cmd/snap/cmd_unset.go

@@ -20,6 +20,8 @@
 package main
 
 import (
+	"errors"
+
 	"github.com/jessevdk/go-flags"
 
 	"github.com/snapcore/snapd/i18n"
@@ -76,6 +78,10 @@ func init() {
 func (x *cmdUnset) Execute(args []string) error {
 	patchValues := make(map[string]interface{})
 	for _, confKey := range x.Positional.ConfKeys {
+		if confKey == "" {
+			return errors.New(i18n.G("configuration keys cannot be empty"))
+		}
+
 		patchValues[confKey] = nil
 	}
 

cmd/snap/cmd_unset_test.go

@@ -99,3 +99,9 @@ func (s *confdbSuite) TestConfdbUnsetInvalidConfdbID(c *check.C) {
 	c.Assert(err, check.NotNil)
 	c.Check(err.Error(), check.Equals, "confdb identifier must conform to format: <account-id>/<confdb>/<view>")
 }
+
+func (s *confdbSuite) TestUnsetEmptyKey(c *check.C) {
+	_, err := snapunset.Parser(snapunset.Client()).ParseArgs([]string{"unset", "some-snap", ""})
+	c.Assert(err, check.NotNil)
+	c.Check(err, check.ErrorMatches, "configuration keys cannot be empty")
+}

overlord/configstate/config/transaction.go

@@ -146,6 +146,10 @@ func (t *Transaction) Set(instanceName, key string, value interface{}) error {
 	t.mu.Lock()
 	defer t.mu.Unlock()
 
+	if key == "" {
+		return errors.New("internal error: key cannot be an empty string")
+	}
+
 	config := t.changes[instanceName]
 	if config == nil {
 		config = make(map[string]interface{})

overlord/configstate/config/transaction_test.go

@@ -890,3 +890,17 @@ func (s *transactionSuite) TestOverlapsWithExternalConfig(c *C) {
 		c.Check(overlap, Equals, tc.overlap, Commentf("%v", tc))
 	}
 }
+
+func (s *transactionSuite) TestEmptyKeyValue(c *C) {
+	s.state.Lock()
+	defer s.state.Unlock()
+
+	tr := config.NewTransaction(s.state)
+
+	var res interface{}
+	err := tr.Set("some-snap", "", &res)
+	c.Assert(err, ErrorMatches, "internal error: key cannot be an empty string")
+
+	err = tr.Set("some-snap", "", nil)
+	c.Assert(err, ErrorMatches, "internal error: key cannot be an empty string")
+}

tests/main/snap-set/task.yaml

@@ -101,3 +101,8 @@ execute: |
         snap get snapctl-hooks -l one.two | MATCH "one.two[ ]*2"
         snap get snapctl-hooks -l one.three | MATCH "one.three[ ]*3"
     done
+
+    echo "Test that 'snap set' correctly handles empty strings"
+    snap set snapctl-hooks '' 2>&1 | MATCH 'invalid configuration: "" \(want key=value\)'
+    snap set snapctl-hooks '!' 2>&1 | MATCH 'configuration keys cannot be empty \(use key! to unset a key\)'
+    snap set snapctl-hooks '=value' 2>&1 | MATCH 'configuration keys cannot be empty'

tests/main/snap-unset/task.yaml

@@ -28,3 +28,7 @@ execute: |
     snap unset basic-hooks bar baz
     snap get basic-hooks bar 2>&1 | MATCH 'snap "basic-hooks" has no "bar" configuration option'
     snap get basic-hooks baz 2>&1 | MATCH 'snap "basic-hooks" has no "baz" configuration option'
+
+
+    echo "Test that 'snap unset' correctly handles empty strings"
+    snap unset basic-hooks '' 2>&1 | MATCH 'configuration keys cannot be empty'