Merge branch 'security-1799-1-42-stable' into '1-42-stable'

Cherry pick commits into v1.42.7

See merge request gitlab/gitaly!35

37 files changed, 333 insertions, 25 deletions

diff --git a/changelogs/unreleased/security-1799-flag-injection.yml b/changelogs/unreleased/security-1799-flag-injection.yml
new file mode 100644
index 000000000..df1467f7c
--- /dev/null
+++ b/changelogs/unreleased/security-1799-flag-injection.yml
@@ -0,0 +1,5 @@
+---
+title: Fix FindCommits flag injection exploit
+merge_request: 35
+author:
+type: security
diff --git a/internal/git/proto.go b/internal/git/proto.go
index dfe112311..a5f771762 100644
--- a/internal/git/proto.go
+++ b/internal/git/proto.go
@@ -18,9 +18,8 @@ import (
 // See https://gitlab.com/gitlab-org/gitaly/issues/556#note_40289573
 var FallbackTimeValue = time.Unix(1<<63-62135596801, 999999999)
 
-// ValidateRevision checks if a revision looks valid
-func ValidateRevision(revision []byte) error {
-	if len(revision) == 0 {
+func validateRevision(revision []byte, allowEmpty bool) error {
+	if !allowEmpty && len(revision) == 0 {
 		return fmt.Errorf("empty revision")
 	}
 	if bytes.HasPrefix(revision, []byte("-")) {
@@ -38,6 +37,17 @@ func ValidateRevision(revision []byte) error {
 	return nil
 }
 
+// ValidateRevisionAllowEmpty checks if a revision looks valid, but allows
+// empty strings
+func ValidateRevisionAllowEmpty(revision []byte) error {
+	return validateRevision(revision, true)
+}
+
+// ValidateRevision checks if a revision looks valid
+func ValidateRevision(revision []byte) error {
+	return validateRevision(revision, false)
+}
+
 // Version returns the used git version.
 func Version() (string, error) {
 	ctx, cancel := context.WithCancel(context.Background())
diff --git a/internal/service/blob/get_blobs.go b/internal/service/blob/get_blobs.go
index f0fad5a81..e7fdd9c7c 100644
--- a/internal/service/blob/get_blobs.go
+++ b/internal/service/blob/get_blobs.go
@@ -5,6 +5,7 @@ import (
 	"io/ioutil"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/service/commit"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -105,6 +106,19 @@ func sendGetBlobsResponse(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobSer
 }
 
 func (*server) GetBlobs(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobService_GetBlobsServer) error {
+	if err := validateGetBlobsRequest(req); err != nil {
+		return err
+	}
+
+	c, err := catfile.New(stream.Context(), req.Repository)
+	if err != nil {
+		return err
+	}
+
+	return sendGetBlobsResponse(req, stream, c)
+}
+
+func validateGetBlobsRequest(req *gitalypb.GetBlobsRequest) error {
 	if req.Repository == nil {
 		return status.Errorf(codes.InvalidArgument, "GetBlobs: empty Repository")
 	}
@@ -112,10 +126,12 @@ func (*server) GetBlobs(req *gitalypb.GetBlobsRequest, stream gitalypb.BlobServi
 	if len(req.RevisionPaths) == 0 {
 		return status.Errorf(codes.InvalidArgument, "GetBlobs: empty RevisionPaths")
 	}
-	c, err := catfile.New(stream.Context(), req.Repository)
-	if err != nil {
-		return err
+
+	for _, rp := range req.RevisionPaths {
+		if err := git.ValidateRevision([]byte(rp.Revision)); err != nil {
+			return status.Errorf(codes.InvalidArgument, "GetBlobs: %v", err)
+		}
 	}
 
-	return sendGetBlobsResponse(req, stream, c)
+	return nil
 }
diff --git a/internal/service/blob/get_blobs_test.go b/internal/service/blob/get_blobs_test.go
index 85b0ca23d..a04385aa3 100644
--- a/internal/service/blob/get_blobs_test.go
+++ b/internal/service/blob/get_blobs_test.go
@@ -151,6 +151,19 @@ func TestFailedGetBlobsRequestDueToValidation(t *testing.T) {
 			},
 			code: codes.InvalidArgument,
 		},
+		{
+			desc: "invalid Revision",
+			request: &gitalypb.GetBlobsRequest{
+				Repository: testRepo,
+				RevisionPaths: []*gitalypb.GetBlobsRequest_RevisionPath{
+					{
+						Path:     []byte("CHANGELOG"),
+						Revision: "--output=/meow",
+					},
+				},
+			},
+			code: codes.InvalidArgument,
+		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/commit/commits_by_message.go b/internal/service/commit/commits_by_message.go
index 87ca6622f..36f93db61 100644
--- a/internal/service/commit/commits_by_message.go
+++ b/internal/service/commit/commits_by_message.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/chunk"
 )
@@ -67,6 +68,10 @@ func commitsByMessage(in *gitalypb.CommitsByMessageRequest, stream gitalypb.Comm
 }
 
 func validateCommitsByMessageRequest(in *gitalypb.CommitsByMessageRequest) error {
+	if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil {
+		return err
+	}
+
 	if in.GetQuery() == "" {
 		return fmt.Errorf("empty Query")
 	}
diff --git a/internal/service/commit/commits_by_message_test.go b/internal/service/commit/commits_by_message_test.go
index 1801eb1cf..e9ad271ee 100644
--- a/internal/service/commit/commits_by_message_test.go
+++ b/internal/service/commit/commits_by_message_test.go
@@ -184,6 +184,11 @@ func TestFailedCommitsByMessageRequest(t *testing.T) {
 			request: &gitalypb.CommitsByMessageRequest{Repository: testRepo},
 			code:    codes.InvalidArgument,
 		},
+		{
+			desc:    "Revision is invalid",
+			request: &gitalypb.CommitsByMessageRequest{Repository: testRepo, Revision: []byte("--output=/meow"), Query: "not empty"},
+			code:    codes.InvalidArgument,
+		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/commit/count_commits.go b/internal/service/commit/count_commits.go
index 55d82d83e..88774cf09 100644
--- a/internal/service/commit/count_commits.go
+++ b/internal/service/commit/count_commits.go
@@ -72,6 +72,10 @@ func (s *server) CountCommits(ctx context.Context, in *gitalypb.CountCommitsRequ
 }
 
 func validateCountCommitsRequest(in *gitalypb.CountCommitsRequest) error {
+	if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil {
+		return err
+	}
+
 	if len(in.GetRevision()) == 0 && !in.GetAll() {
 		return fmt.Errorf("empty Revision and false All")
 	}
diff --git a/internal/service/commit/count_commits_test.go b/internal/service/commit/count_commits_test.go
index 8c41f0755..499578f06 100644
--- a/internal/service/commit/count_commits_test.go
+++ b/internal/service/commit/count_commits_test.go
@@ -177,8 +177,9 @@ func TestFailedCountCommitsRequestDueToValidationError(t *testing.T) {
 
 	rpcRequests := []gitalypb.CountCommitsRequest{
 		{Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}, Revision: revision}, // Repository doesn't exist
-		{Repository: nil, Revision: revision},             // Repository is nil
-		{Repository: testRepo, Revision: nil, All: false}, // Revision is empty and All is false
+		{Repository: nil, Revision: revision},                                  // Repository is nil
+		{Repository: testRepo, Revision: nil, All: false},                      // Revision is empty and All is false
+		{Repository: testRepo, Revision: []byte("--output=/meow"), All: false}, // Revision is invalid
 	}
 
 	for _, rpcRequest := range rpcRequests {
diff --git a/internal/service/commit/find_all_commits.go b/internal/service/commit/find_all_commits.go
index 84b0639f7..3bd5369ab 100644
--- a/internal/service/commit/find_all_commits.go
+++ b/internal/service/commit/find_all_commits.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/chunk"
 	"gitlab.com/gitlab-org/gitaly/internal/service/ref"
@@ -26,6 +27,10 @@ func (sender *findAllCommitsSender) Send() error {
 }
 
 func (s *server) FindAllCommits(in *gitalypb.FindAllCommitsRequest, stream gitalypb.CommitService_FindAllCommitsServer) error {
+	if err := validateFindAllCommitsRequest(in); err != nil {
+		return err
+	}
+
 	var revisions []string
 	if len(in.GetRevision()) == 0 {
 		branchNames, err := _findBranchNamesFunc(stream.Context(), in.Repository)
@@ -47,6 +52,14 @@ func (s *server) FindAllCommits(in *gitalypb.FindAllCommitsRequest, stream gital
 	return nil
 }
 
+func validateFindAllCommitsRequest(in *gitalypb.FindAllCommitsRequest) error {
+	if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil {
+		return helper.ErrInvalidArgument(err)
+	}
+
+	return nil
+}
+
 func findAllCommits(in *gitalypb.FindAllCommitsRequest, stream gitalypb.CommitService_FindAllCommitsServer, revisions []string) error {
 	sender := &findAllCommitsSender{stream: stream}
 
diff --git a/internal/service/commit/find_all_commits_test.go b/internal/service/commit/find_all_commits_test.go
index 95a8082f1..47131f9db 100644
--- a/internal/service/commit/find_all_commits_test.go
+++ b/internal/service/commit/find_all_commits_test.go
@@ -292,6 +292,9 @@ func TestFailedFindAllCommitsRequest(t *testing.T) {
 
 	invalidRepo := &gitalypb.Repository{StorageName: "fake", RelativePath: "path"}
 
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
 	testCases := []struct {
 		desc    string
 		request *gitalypb.FindAllCommitsRequest
@@ -307,6 +310,14 @@ func TestFailedFindAllCommitsRequest(t *testing.T) {
 			request: &gitalypb.FindAllCommitsRequest{},
 			code:    codes.InvalidArgument,
 		},
+		{
+			desc: "Revision is invalid",
+			request: &gitalypb.FindAllCommitsRequest{
+				Repository: testRepo,
+				Revision:   []byte("--output=/meow"),
+			},
+			code: codes.InvalidArgument,
+		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/commit/find_commits.go b/internal/service/commit/find_commits.go
index 489f11240..a57b5ce5c 100644
--- a/internal/service/commit/find_commits.go
+++ b/internal/service/commit/find_commits.go
@@ -21,6 +21,10 @@ const commitsPerPage int = 20
 func (s *server) FindCommits(req *gitalypb.FindCommitsRequest, stream gitalypb.CommitService_FindCommitsServer) error {
 	ctx := stream.Context()
 
+	if err := git.ValidateRevisionAllowEmpty(req.Revision); err != nil {
+		return helper.ErrInvalidArgument(err)
+	}
+
 	// Use Gitaly's default branch lookup function because that is already
 	// migrated.
 	if revision := req.Revision; len(revision) == 0 && !req.GetAll() {
diff --git a/internal/service/commit/find_commits_test.go b/internal/service/commit/find_commits_test.go
index f7b7803f1..85add0e86 100644
--- a/internal/service/commit/find_commits_test.go
+++ b/internal/service/commit/find_commits_test.go
@@ -438,6 +438,15 @@ func TestFailureFindCommitsRequest(t *testing.T) {
 			},
 			code: codes.InvalidArgument,
 		},
+		{
+			desc: "invalid revision",
+			request: &gitalypb.FindCommitsRequest{
+				Repository: testRepo,
+				Revision:   []byte("--output=/meow"),
+				Limit:      1,
+			},
+			code: codes.InvalidArgument,
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/internal/service/commit/languages.go b/internal/service/commit/languages.go
index 73ef24d6e..9ae24bfb5 100644
--- a/internal/service/commit/languages.go
+++ b/internal/service/commit/languages.go
@@ -18,6 +18,10 @@ import (
 func (*server) CommitLanguages(ctx context.Context, req *gitalypb.CommitLanguagesRequest) (*gitalypb.CommitLanguagesResponse, error) {
 	repo := req.Repository
 
+	if err := git.ValidateRevisionAllowEmpty(req.Revision); err != nil {
+		return nil, helper.ErrInvalidArgument(err)
+	}
+
 	revision := string(req.Revision)
 	if revision == "" {
 		defaultBranch, err := ref.DefaultBranchName(ctx, req.Repository)
diff --git a/internal/service/commit/languages_test.go b/internal/service/commit/languages_test.go
index 2d82a7712..c4f41937f 100644
--- a/internal/service/commit/languages_test.go
+++ b/internal/service/commit/languages_test.go
@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	"google.golang.org/grpc/codes"
 )
 
 func TestLanguages(t *testing.T) {
@@ -91,3 +92,23 @@ func TestLanguagesEmptyRevision(t *testing.T) {
 	}
 	require.True(t, foundRuby, "expected to find Ruby as a language on HEAD")
 }
+
+func TestInvalidCommitLanguagesRequestRevision(t *testing.T) {
+	server, serverSocketPath := startTestServices(t)
+	defer server.Stop()
+
+	client, conn := newCommitServiceClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	_, err := client.CommitLanguages(ctx, &gitalypb.CommitLanguagesRequest{
+		Repository: testRepo,
+		Revision:   []byte("--output=/meow"),
+	})
+	testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
+}
diff --git a/internal/service/commit/last_commit_for_path.go b/internal/service/commit/last_commit_for_path.go
index fc4b896f3..f76b9370c 100644
--- a/internal/service/commit/last_commit_for_path.go
+++ b/internal/service/commit/last_commit_for_path.go
@@ -2,10 +2,11 @@ package commit
 
 import (
 	"context"
-	"fmt"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
@@ -29,8 +30,8 @@ func (s *server) LastCommitForPath(ctx context.Context, in *gitalypb.LastCommitF
 }
 
 func validateLastCommitForPathRequest(in *gitalypb.LastCommitForPathRequest) error {
-	if len(in.Revision) == 0 {
-		return fmt.Errorf("empty Revision")
+	if err := git.ValidateRevision(in.Revision); err != nil {
+		return helper.ErrInvalidArgument(err)
 	}
 	return nil
 }
diff --git a/internal/service/commit/last_commit_for_path_test.go b/internal/service/commit/last_commit_for_path_test.go
index 3552fc701..bbf145552 100644
--- a/internal/service/commit/last_commit_for_path_test.go
+++ b/internal/service/commit/last_commit_for_path_test.go
@@ -121,6 +121,13 @@ func TestFailedLastCommitForPathRequest(t *testing.T) {
 			request: &gitalypb.LastCommitForPathRequest{Repository: testRepo, Path: []byte("foo/bar")},
 			code:    codes.InvalidArgument,
 		},
+		{
+			desc: "Revision is invalid",
+			request: &gitalypb.LastCommitForPathRequest{
+				Repository: testRepo, Path: []byte("foo/bar"), Revision: []byte("--output=/meow"),
+			},
+			code: codes.InvalidArgument,
+		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/commit/list_files.go b/internal/service/commit/list_files.go
index bab30138c..5ff2e1673 100644
--- a/internal/service/commit/list_files.go
+++ b/internal/service/commit/list_files.go
@@ -19,6 +19,10 @@ func (s *server) ListFiles(in *gitalypb.ListFilesRequest, stream gitalypb.Commit
 		"Revision": in.GetRevision(),
 	}).Debug("ListFiles")
 
+	if err := validateListFilesRequest(in); err != nil {
+		return err
+	}
+
 	repo := in.Repository
 	if _, err := helper.GetRepoPath(repo); err != nil {
 		return err
@@ -45,6 +49,13 @@ func (s *server) ListFiles(in *gitalypb.ListFilesRequest, stream gitalypb.Commit
 	return nil
 }
 
+func validateListFilesRequest(in *gitalypb.ListFilesRequest) error {
+	if err := git.ValidateRevisionAllowEmpty(in.Revision); err != nil {
+		return helper.ErrInvalidArgument(err)
+	}
+	return nil
+}
+
 func listFiles(repo *gitalypb.Repository, revision string, stream gitalypb.CommitService_ListFilesServer) error {
 	args := []string{"ls-tree", "-z", "-r", "--full-tree", "--full-name", "--", revision}
 	cmd, err := git.Command(stream.Context(), repo, args...)
diff --git a/internal/service/commit/list_files_test.go b/internal/service/commit/list_files_test.go
index c2a702368..90c8c3cc7 100644
--- a/internal/service/commit/list_files_test.go
+++ b/internal/service/commit/list_files_test.go
@@ -7,6 +7,7 @@ import (
 	"io"
 	"testing"
 
+	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/service/ref"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -184,3 +185,26 @@ func drainListFilesResponse(c gitalypb.CommitService_ListFilesClient) error {
 	}
 	return err
 }
+
+func TestInvalidListFilesRequestRevision(t *testing.T) {
+	server, serverSocketPath := startTestServices(t)
+	defer server.Stop()
+
+	client, conn := newCommitServiceClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	stream, err := client.ListFiles(ctx, &gitalypb.ListFilesRequest{
+		Repository: testRepo,
+		Revision:   []byte("--output=/meow"),
+	})
+	require.NoError(t, err)
+
+	_, err = stream.Recv()
+	testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
+}
diff --git a/internal/service/commit/list_last_commits_for_tree.go b/internal/service/commit/list_last_commits_for_tree.go
index fa906fa63..24ac1f961 100644
--- a/internal/service/commit/list_last_commits_for_tree.go
+++ b/internal/service/commit/list_last_commits_for_tree.go
@@ -1,7 +1,6 @@
 package commit
 
 import (
-	"fmt"
 	"io"
 	"sort"
 	"unicode/utf8"
@@ -140,8 +139,5 @@ func sendCommitsForTree(batch []*gitalypb.ListLastCommitsForTreeResponse_CommitF
 }
 
 func validateListLastCommitsForTreeRequest(in *gitalypb.ListLastCommitsForTreeRequest) error {
-	if in.Revision == "" {
-		return fmt.Errorf("empty Revision")
-	}
-	return nil
+	return git.ValidateRevision([]byte(in.Revision))
 }
diff --git a/internal/service/commit/list_last_commits_for_tree_test.go b/internal/service/commit/list_last_commits_for_tree_test.go
index 9d2ac116c..c58c8fc8c 100644
--- a/internal/service/commit/list_last_commits_for_tree_test.go
+++ b/internal/service/commit/list_last_commits_for_tree_test.go
@@ -282,6 +282,16 @@ func TestFailedListLastCommitsForTreeRequest(t *testing.T) {
 			},
 			code: codes.Internal,
 		},
+		{
+			desc: "Invalid revision",
+			request: &gitalypb.ListLastCommitsForTreeRequest{
+				Repository: testRepo,
+				Revision:   "--output=/meow",
+				Offset:     0,
+				Limit:      25,
+			},
+			code: codes.InvalidArgument,
+		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/commit/raw_blame.go b/internal/service/commit/raw_blame.go
index 0e7c40483..a31212909 100644
--- a/internal/service/commit/raw_blame.go
+++ b/internal/service/commit/raw_blame.go
@@ -46,8 +46,8 @@ func (s *server) RawBlame(in *gitalypb.RawBlameRequest, stream gitalypb.CommitSe
 }
 
 func validateRawBlameRequest(in *gitalypb.RawBlameRequest) error {
-	if len(in.GetRevision()) == 0 {
-		return fmt.Errorf("empty Revision")
+	if err := git.ValidateRevision(in.Revision); err != nil {
+		return err
 	}
 
 	if len(in.GetPath()) == 0 {
diff --git a/internal/service/commit/raw_blame_test.go b/internal/service/commit/raw_blame_test.go
index 932b34e1c..b9d40bb81 100644
--- a/internal/service/commit/raw_blame_test.go
+++ b/internal/service/commit/raw_blame_test.go
@@ -113,6 +113,13 @@ func TestFailedRawBlameRequest(t *testing.T) {
 			path:        []byte(""),
 			code:        codes.InvalidArgument,
 		},
+		{
+			description: "Invalid revision",
+			repo:        testRepo,
+			revision:    []byte("--output=/meow"),
+			path:        []byte("a/b/c"),
+			code:        codes.InvalidArgument,
+		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/commit/stats_test.go b/internal/service/commit/stats_test.go
index f39534730..a64505672 100644
--- a/internal/service/commit/stats_test.go
+++ b/internal/service/commit/stats_test.go
@@ -119,6 +119,12 @@ func TestCommitStatsFailure(t *testing.T) {
 			revision: []byte("non/existing"),
 			err:      codes.Internal,
 		},
+		{
+			desc:     "invalid revision",
+			repo:     testRepo,
+			revision: []byte("--outpu=/meow"),
+			err:      codes.InvalidArgument,
+		},
 	}
 
 	for _, tc := range tests {
diff --git a/internal/service/commit/tree_entries.go b/internal/service/commit/tree_entries.go
index f46bfd910..da7f8574e 100644
--- a/internal/service/commit/tree_entries.go
+++ b/internal/service/commit/tree_entries.go
@@ -6,6 +6,7 @@ import (
 	grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	log "github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/chunk"
 	"google.golang.org/grpc/codes"
@@ -15,8 +16,8 @@ import (
 var maxTreeEntries = 1000
 
 func validateGetTreeEntriesRequest(in *gitalypb.GetTreeEntriesRequest) error {
-	if len(in.GetRevision()) == 0 {
-		return fmt.Errorf("empty Revision")
+	if err := git.ValidateRevision(in.Revision); err != nil {
+		return err
 	}
 
 	if len(in.GetPath()) == 0 {
diff --git a/internal/service/commit/tree_entries_test.go b/internal/service/commit/tree_entries_test.go
index 5eb422499..fa6ad090d 100644
--- a/internal/service/commit/tree_entries_test.go
+++ b/internal/service/commit/tree_entries_test.go
@@ -432,6 +432,7 @@ func TestFailedGetTreeEntriesRequestDueToValidationError(t *testing.T) {
 		{Repository: nil, Revision: revision, Path: path},                                                             // Repository is nil
 		{Repository: testRepo, Revision: nil, Path: path},                                                             // Revision is empty
 		{Repository: testRepo, Revision: revision},                                                                    // Path is empty
+		{Repository: testRepo, Revision: []byte("--output=/meow"), Path: path},                                        // Revision is invalid
 	}
 
 	for _, rpcRequest := range rpcRequests {
diff --git a/internal/service/commit/tree_entry.go b/internal/service/commit/tree_entry.go
index f00c4cf44..9acbe5ea0 100644
--- a/internal/service/commit/tree_entry.go
+++ b/internal/service/commit/tree_entry.go
@@ -6,6 +6,7 @@ import (
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -122,8 +123,8 @@ func (s *server) TreeEntry(in *gitalypb.TreeEntryRequest, stream gitalypb.Commit
 }
 
 func validateRequest(in *gitalypb.TreeEntryRequest) error {
-	if len(in.GetRevision()) == 0 {
-		return fmt.Errorf("empty Revision")
+	if err := git.ValidateRevision(in.Revision); err != nil {
+		return err
 	}
 
 	if len(in.GetPath()) == 0 {
diff --git a/internal/service/commit/tree_entry_test.go b/internal/service/commit/tree_entry_test.go
index d7a75d592..48e15ee4b 100644
--- a/internal/service/commit/tree_entry_test.go
+++ b/internal/service/commit/tree_entry_test.go
@@ -178,6 +178,7 @@ func TestFailedTreeEntryRequestDueToValidationError(t *testing.T) {
 		{Repository: nil, Revision: revision, Path: path},                                                             // Repository is nil
 		{Repository: testRepo, Revision: nil, Path: path},                                                             // Revision is empty
 		{Repository: testRepo, Revision: revision},                                                                    // Path is empty
+		{Repository: testRepo, Revision: []byte("--output=/meow"), Path: path},                                        // Revision is invalid
 	}
 
 	for _, rpcRequest := range rpcRequests {
diff --git a/internal/service/diff/patch.go b/internal/service/diff/patch.go
index 51718088a..6ea593915 100644
--- a/internal/service/diff/patch.go
+++ b/internal/service/diff/patch.go
@@ -2,10 +2,16 @@ package diff
 
 import (
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 )
 
 func (s *server) CommitPatch(in *gitalypb.CommitPatchRequest, stream gitalypb.DiffService_CommitPatchServer) error {
+	if err := git.ValidateRevision(in.Revision); err != nil {
+		return helper.ErrInvalidArgument(err)
+	}
+
 	ctx := stream.Context()
 
 	client, err := s.DiffServiceClient(ctx)
diff --git a/internal/service/diff/patch_test.go b/internal/service/diff/patch_test.go
index dbe75c4f5..15711e4c5 100644
--- a/internal/service/diff/patch_test.go
+++ b/internal/service/diff/patch_test.go
@@ -5,8 +5,10 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	"google.golang.org/grpc/codes"
 )
 
 func TestSuccessfulCommitPatchRequest(t *testing.T) {
@@ -67,3 +69,26 @@ func TestSuccessfulCommitPatchRequest(t *testing.T) {
 		})
 	}
 }
+
+func TestInvalidCommitPatchRequestRevision(t *testing.T) {
+	server, serverSocketPath := runDiffServer(t)
+	defer server.Stop()
+
+	client, conn := newDiffClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	stream, err := client.CommitPatch(ctx, &gitalypb.CommitPatchRequest{
+		Repository: testRepo,
+		Revision:   []byte("--output=/meow"),
+	})
+	require.NoError(t, err)
+
+	_, err = stream.Recv()
+	testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
+}
diff --git a/internal/service/repository/apply_gitattributes_test.go b/internal/service/repository/apply_gitattributes_test.go
index c51889d92..3c3ab7ea7 100644
--- a/internal/service/repository/apply_gitattributes_test.go
+++ b/internal/service/repository/apply_gitattributes_test.go
@@ -110,6 +110,11 @@ func TestApplyGitattributesFailure(t *testing.T) {
 			revision: []byte("not-existing-ref"),
 			code:     codes.InvalidArgument,
 		},
+		{
+			repo:     testRepo,
+			revision: []byte("--output=/meow"),
+			code:     codes.InvalidArgument,
+		},
 	}
 
 	for _, test := range tests {
diff --git a/internal/service/repository/write_ref_test.go b/internal/service/repository/write_ref_test.go
index f000208b1..8e0ac6df2 100644
--- a/internal/service/repository/write_ref_test.go
+++ b/internal/service/repository/write_ref_test.go
@@ -137,6 +137,14 @@ func TestWriteRefValidationError(t *testing.T) {
 				Revision:   []byte("0123012301231243"),
 			},
 		},
+		{
+			desc: "invalid revision",
+			req: &gitalypb.WriteRefRequest{
+				Repository: testRepo,
+				Ref:        []byte("refs/heads/master"),
+				Revision:   []byte("--output=/meow"),
+			},
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/internal/service/wiki/find_file.go b/internal/service/wiki/find_file.go
index c05598c92..e0528289c 100644
--- a/internal/service/wiki/find_file.go
+++ b/internal/service/wiki/find_file.go
@@ -2,6 +2,7 @@ package wiki
 
 import (
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -10,6 +11,10 @@ import (
 func (s *server) WikiFindFile(request *gitalypb.WikiFindFileRequest, stream gitalypb.WikiService_WikiFindFileServer) error {
 	ctx := stream.Context()
 
+	if err := git.ValidateRevisionAllowEmpty(request.Revision); err != nil {
+		return status.Errorf(codes.InvalidArgument, "WikiFindFile: %s", err)
+	}
+
 	if len(request.GetName()) == 0 {
 		return status.Errorf(codes.InvalidArgument, "WikiFindFile: Empty Name")
 	}
diff --git a/internal/service/wiki/find_file_test.go b/internal/service/wiki/find_file_test.go
index 9e750c916..5716ada89 100644
--- a/internal/service/wiki/find_file_test.go
+++ b/internal/service/wiki/find_file_test.go
@@ -180,6 +180,12 @@ func TestFailedWikiFindFileDueToValidation(t *testing.T) {
 			revision: "deadfacedeadfacedeadfacedeadfacedeadface",
 			code:     codes.Unknown,
 		},
+		{
+			desc:     "dangerously invalid revision",
+			name:     "image.jpg",
+			revision: "--output=/meow",
+			code:     codes.InvalidArgument,
+		},
 	}
 
 	for _, testCase := range testCases {
diff --git a/internal/service/wiki/find_page.go b/internal/service/wiki/find_page.go
index 290e92ce5..e02214859 100644
--- a/internal/service/wiki/find_page.go
+++ b/internal/service/wiki/find_page.go
@@ -1,7 +1,10 @@
 package wiki
 
 import (
+	"errors"
+
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -10,8 +13,8 @@ import (
 func (s *server) WikiFindPage(request *gitalypb.WikiFindPageRequest, stream gitalypb.WikiService_WikiFindPageServer) error {
 	ctx := stream.Context()
 
-	if len(request.GetTitle()) == 0 {
-		return status.Errorf(codes.InvalidArgument, "WikiFindPage: Empty Title")
+	if err := validateWikiFindPage(request); err != nil {
+		return status.Errorf(codes.InvalidArgument, "WikiFindPage: %v", err)
 	}
 
 	client, err := s.WikiServiceClient(ctx)
@@ -39,3 +42,13 @@ func (s *server) WikiFindPage(request *gitalypb.WikiFindPageRequest, stream gita
 		return stream.Send(resp)
 	})
 }
+
+func validateWikiFindPage(request *gitalypb.WikiFindPageRequest) error {
+	if err := git.ValidateRevisionAllowEmpty(request.Revision); err != nil {
+		return err
+	}
+	if len(request.GetTitle()) == 0 {
+		return errors.New("empty Title")
+	}
+	return nil
+}
diff --git a/internal/service/wiki/find_page_test.go b/internal/service/wiki/find_page_test.go
index 2db25e290..31eb96945 100644
--- a/internal/service/wiki/find_page_test.go
+++ b/internal/service/wiki/find_page_test.go
@@ -370,3 +370,27 @@ func readFullWikiPageFromWikiFindPageClient(t *testing.T, c gitalypb.WikiService
 
 	return wikiPage
 }
+
+func TestInvalidWikiFindPageRequestRevision(t *testing.T) {
+	server, serverSocketPath := runWikiServiceServer(t)
+	defer server.Stop()
+
+	client, conn := newWikiClient(t, serverSocketPath)
+	defer conn.Close()
+
+	wikiRepo, _, cleanupFunc := setupWikiRepo(t)
+	defer cleanupFunc()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	stream, err := client.WikiFindPage(ctx, &gitalypb.WikiFindPageRequest{
+		Repository: wikiRepo,
+		Title:      []byte("non-empty title"),
+		Revision:   []byte("--output=/meow"),
+	})
+	require.NoError(t, err)
+
+	_, err = stream.Recv()
+	testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
+}
diff --git a/internal/service/wiki/formatted_data.go b/internal/service/wiki/formatted_data.go
index 12b2f11f6..f767bd8d6 100644
--- a/internal/service/wiki/formatted_data.go
+++ b/internal/service/wiki/formatted_data.go
@@ -2,12 +2,17 @@ package wiki
 
 import (
 	"gitlab.com/gitlab-org/gitaly-proto/go/gitalypb"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 )
 
 func (s *server) WikiGetFormattedData(request *gitalypb.WikiGetFormattedDataRequest, stream gitalypb.WikiService_WikiGetFormattedDataServer) error {
+	if err := git.ValidateRevisionAllowEmpty(request.Revision); err != nil {
+		return status.Errorf(codes.InvalidArgument, "WikiGetFormattedData: %s", err)
+	}
+
 	ctx := stream.Context()
 
 	if len(request.GetTitle()) == 0 {
diff --git a/internal/service/wiki/formatted_data_test.go b/internal/service/wiki/formatted_data_test.go
index 90263dcbb..45f16d404 100644
--- a/internal/service/wiki/formatted_data_test.go
+++ b/internal/service/wiki/formatted_data_test.go
@@ -157,3 +157,27 @@ func readFullDataFromWikiGetFormattedDataClient(t *testing.T, c gitalypb.WikiSer
 
 	return
 }
+
+func TestInvalidWikiGetFormattedDataRequestRevision(t *testing.T) {
+	wikiRepo, _, cleanupFunc := setupWikiRepo(t)
+	defer cleanupFunc()
+
+	server, serverSocketPath := runWikiServiceServer(t)
+	defer server.Stop()
+
+	client, conn := newWikiClient(t, serverSocketPath)
+	defer conn.Close()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	stream, err := client.WikiGetFormattedData(ctx, &gitalypb.WikiGetFormattedDataRequest{
+		Repository: wikiRepo,
+		Title:      []byte("Home Pagé"),
+		Revision:   []byte("--output=/meow"),
+	})
+	require.NoError(t, err)
+
+	_, err = stream.Recv()
+	testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
+}