Merge branch 'po-backport-rev-injection-fix' into '1-58-stable'1-58-stable

Fix FindCommits flag injection exploit (cherry picked backport)

See merge request gitlab-org/gitaly!1419

37 files changed, 332 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..63f6176ba
--- /dev/null
+++ b/changelogs/unreleased/security-1799-flag-injection.yml
@@ -0,0 +1,5 @@
+---
+title: Fix FindCommits flag injection exploit
+merge_request: 31
+author:
+type: security
diff --git a/internal/git/proto.go b/internal/git/proto.go
index 5244e6bdb..17323c600 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 1388609d6..dc1877ad4 100644
--- a/internal/service/blob/get_blobs.go
+++ b/internal/service/blob/get_blobs.go
@@ -4,6 +4,7 @@ import (
 	"io"
 	"io/ioutil"
 
+	"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/proto/go/gitalypb"
@@ -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 b7fd6b9fa..da532e0df 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 1b4d3b87f..915a186dc 100644
--- a/internal/service/commit/commits_by_message.go
+++ b/internal/service/commit/commits_by_message.go
@@ -3,6 +3,7 @@ package commit
 import (
 	"fmt"
 
+	"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/proto/go/gitalypb"
@@ -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 e9a7f17c2..0a5986101 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 f96f0dca5..1f96bb2fc 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 f3ad92740..a4b07611e 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 5116dff30..8f717d90e 100644
--- a/internal/service/commit/find_all_commits.go
+++ b/internal/service/commit/find_all_commits.go
@@ -3,6 +3,7 @@ package commit
 import (
 	"fmt"
 
+	"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 7b1f6d028..43dd6fae7 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 e5d49a5ad..3e4709353 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 8e1dc4944..b68ac1def 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 63a104b2d..87c8ab0d5 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 a9faeae28..c23ba8a23 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/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
+	"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 60e3c3c7e..350b90b3b 100644
--- a/internal/service/commit/last_commit_for_path.go
+++ b/internal/service/commit/last_commit_for_path.go
@@ -2,8 +2,8 @@ package commit
 
 import (
 	"context"
-	"fmt"
 
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -44,8 +44,8 @@ func lastCommitForPath(ctx context.Context, in *gitalypb.LastCommitForPathReques
 }
 
 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 24c5c70a5..f9c58757c 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 615ab1534..547633dc2 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 a3d4132c9..8c80a4896 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/internal/service/ref"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -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 ada4b2915..cb379e420 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"
 
@@ -144,8 +143,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 b357c79c9..722964c58 100644
--- a/internal/service/commit/list_last_commits_for_tree_test.go
+++ b/internal/service/commit/list_last_commits_for_tree_test.go
@@ -278,6 +278,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 49be189e6..840d83ac6 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 8ecdbefcf..cde759ff2 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 373326622..2d9236754 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 ca99abc52..7e7fc23ee 100644
--- a/internal/service/commit/tree_entries.go
+++ b/internal/service/commit/tree_entries.go
@@ -5,6 +5,7 @@ import (
 
 	grpc_logrus "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus"
 	log "github.com/sirupsen/logrus"
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/chunk"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -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 4a86f7507..0cca60015 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 527edf654..8498b8436 100644
--- a/internal/service/commit/tree_entry.go
+++ b/internal/service/commit/tree_entry.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"strings"
 
+	"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/proto/go/gitalypb"
@@ -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 775e80517..5460cb336 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 f8c02be6a..e4d6977fb 100644
--- a/internal/service/diff/patch.go
+++ b/internal/service/diff/patch.go
@@ -1,11 +1,17 @@
 package diff
 
 import (
+	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
 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 d1a3396bf..4927f3083 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/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
+	"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 16485791a..e82968530 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 e836e03cc..905515a7e 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 6ecb62058..cf936b8de 100644
--- a/internal/service/wiki/find_file.go
+++ b/internal/service/wiki/find_file.go
@@ -1,6 +1,7 @@
 package wiki
 
 import (
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -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 e4735dfdd..9908686a0 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 3ba586544..239781cf8 100644
--- a/internal/service/wiki/find_page.go
+++ b/internal/service/wiki/find_page.go
@@ -1,6 +1,9 @@
 package wiki
 
 import (
+	"errors"
+
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -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 9cb8511a5..fd7e262f6 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 c4e24b7c3..27a49be6a 100644
--- a/internal/service/wiki/formatted_data.go
+++ b/internal/service/wiki/formatted_data.go
@@ -1,6 +1,7 @@
 package wiki
 
 import (
+	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -8,6 +9,10 @@ import (
 )
 
 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 3233c416a..d14513fa1 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)
+}