Merge branch 'delete-config-validation' into 'master'

Add validation for config keys

See merge request gitlab-org/gitaly!788

3 files changed, 75 insertions, 2 deletions

diff --git a/changelogs/unreleased/delete-config-validation.yml b/changelogs/unreleased/delete-config-validation.yml
new file mode 100644
index 000000000..d06db8c2d
--- /dev/null
+++ b/changelogs/unreleased/delete-config-validation.yml
@@ -0,0 +1,5 @@
+---
+title: Add validation for config keys
+merge_request: 788
+author:
+type: other
diff --git a/internal/service/repository/config.go b/internal/service/repository/config.go
index 23dad7422..023bb0c1c 100644
--- a/internal/service/repository/config.go
+++ b/internal/service/repository/config.go
@@ -1,6 +1,8 @@
 package repository
 
 import (
+	"regexp"
+
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
@@ -11,8 +13,19 @@ import (
 	"google.golang.org/grpc/status"
 )
 
+var (
+	// validConfigKey is currently not an exhaustive validation, we can
+	// improve it over time. It should reject no valid keys. It may fail to
+	// reject some invalid keys.
+	validConfigKey = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9\-.]*$`)
+)
+
 func (*server) DeleteConfig(ctx context.Context, req *pb.DeleteConfigRequest) (*pb.DeleteConfigResponse, error) {
 	for _, k := range req.Keys {
+		if !validConfigKey.MatchString(k) {
+			return nil, status.Errorf(codes.InvalidArgument, "invalid config key: %q", k)
+		}
+
 		// We assume k does not contain any secrets; it is leaked via 'ps'.
 		cmd, err := git.Command(ctx, req.Repository, "config", "--unset-all", k)
 		if err != nil {
@@ -33,6 +46,12 @@ func (*server) DeleteConfig(ctx context.Context, req *pb.DeleteConfigRequest) (*
 }
 
 func (s *server) SetConfig(ctx context.Context, req *pb.SetConfigRequest) (*pb.SetConfigResponse, error) {
+	for _, entry := range req.Entries {
+		if !validConfigKey.MatchString(entry.Key) {
+			return nil, status.Errorf(codes.InvalidArgument, "invalid config key: %q", entry.Key)
+		}
+	}
+
 	// We use gitaly-ruby here because in gitaly-ruby we can use Rugged, and
 	// Rugged lets us set config values without leaking secrets via 'ps'. We
 	// can't use `git config foo.bar secret` because that leaks secrets.
diff --git a/internal/service/repository/config_test.go b/internal/service/repository/config_test.go
index 1c824f0d7..a35ab8728 100644
--- a/internal/service/repository/config_test.go
+++ b/internal/service/repository/config_test.go
@@ -9,8 +9,33 @@ import (
 	"github.com/stretchr/testify/require"
 	pb "gitlab.com/gitlab-org/gitaly-proto/go"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
 )
 
+func TestInvalidConfigKey(t *testing.T) {
+	testCases := []struct {
+		key string
+		ok  bool
+	}{
+		{key: "foo.abC-123", ok: true},
+		{key: "foo.abC 123"},
+		{key: "foo.abC,123"},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.key, func(t *testing.T) {
+			match := validConfigKey.MatchString(tc.key)
+
+			if tc.ok {
+				require.True(t, match, "key %q must be valid", tc.key)
+			} else {
+				require.False(t, match, "key %q must be invalid", tc.key)
+			}
+		})
+	}
+}
+
 func TestDeleteConfig(t *testing.T) {
 	server, serverSocketPath := runRepoServer(t)
 	defer server.Stop()
@@ -22,6 +47,7 @@ func TestDeleteConfig(t *testing.T) {
 		desc    string
 		addKeys []string
 		reqKeys []string
+		code    codes.Code
 	}{
 		{
 			desc: "empty request",
@@ -35,6 +61,11 @@ func TestDeleteConfig(t *testing.T) {
 			addKeys: []string{"test.bar"},
 			reqKeys: []string{"test.foo", "test.bar", "test.baz"},
 		},
+		{
+			desc:    "key with comma",
+			reqKeys: []string{"test.foo,"},
+			code:    codes.InvalidArgument,
+		},
 	}
 
 	for _, tc := range testcases {
@@ -50,7 +81,12 @@ func TestDeleteConfig(t *testing.T) {
 			}
 
 			_, err := client.DeleteConfig(ctx, &pb.DeleteConfigRequest{Repository: testRepo, Keys: tc.reqKeys})
-			require.NoError(t, err)
+			if tc.code == codes.OK {
+				require.NoError(t, err)
+			} else {
+				require.Equal(t, tc.code, status.Code(err), "expected grpc error code")
+				return
+			}
 
 			actualConfig := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "-l")
 			scanner := bufio.NewScanner(bytes.NewReader(actualConfig))
@@ -76,6 +112,7 @@ func TestSetConfig(t *testing.T) {
 		desc     string
 		entries  []*pb.SetConfigRequest_Entry
 		expected []string
+		code     codes.Code
 	}{
 		{
 			desc: "empty request",
@@ -93,6 +130,13 @@ func TestSetConfig(t *testing.T) {
 				"test.foo3=true",
 			},
 		},
+		{
+			desc: "invalid key",
+			entries: []*pb.SetConfigRequest_Entry{
+				&pb.SetConfigRequest_Entry{Key: "test.foo1,", Value: &pb.SetConfigRequest_Entry_ValueStr{"hello world"}},
+			},
+			code: codes.InvalidArgument,
+		},
 	}
 
 	for _, tc := range testcases {
@@ -104,7 +148,12 @@ func TestSetConfig(t *testing.T) {
 			defer cleanupFn()
 
 			_, err := client.SetConfig(ctx, &pb.SetConfigRequest{Repository: testRepo, Entries: tc.entries})
-			require.NoError(t, err)
+			if tc.code == codes.OK {
+				require.NoError(t, err)
+			} else {
+				require.Equal(t, tc.code, status.Code(err), "expected grpc error code")
+				return
+			}
 
 			actualConfigBytes := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "--local", "-l")
 			scanner := bufio.NewScanner(bytes.NewReader(actualConfigBytes))