operations: Fix UpdateSubmodule RPC confusing reference names

The `UpdateSubmodule()` RPC accepts as parameter an unqualified branch
name. We thus need to convert it to a fully qualified branch name in
order to not cause ambiguous reference lookups in the RPC's
implementation.

To convert the branch name, we currently use `NewBranchReferenceName()`,
which has dangerous semantics as it will treat a branch name starting
with "refs/heads/" as an already-qualified branch. As a result, we'd now
start to lookup the "master" branch instead of the "refs/heads/master"
branch. This is fixed by converting to `NewReferenceFromBranchName()`.

Naturally, our legacy Ruby implementation has similar bugs. First, it
uses `rev_parse()` on the unqualified branch name, which may potentially
look up a tag instead of a branch if both have the same name. And
second, it uses the `branches` collection to look up the branch. But
this function provided by Rugged has the same dangerous semantics as
`NewBranchReferenceName()`, as it will treat a fully-qualified reference
not as the branch name, but as the branch reference. Those bugs are
fixed by passing a fully-qualified reference to `rev_parse()` and using
`ref()` with a fully-qualified reference instead of the `branches`
collection.

5 files changed, 33 insertions, 9 deletions

diff --git a/internal/gitaly/service/operations/submodules.go b/internal/gitaly/service/operations/submodules.go
index 287dd4ba7..581045085 100644
--- a/internal/gitaly/service/operations/submodules.go
+++ b/internal/gitaly/service/operations/submodules.go
@@ -89,7 +89,7 @@ func (s *Server) userUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda
 		}, nil
 	}
 
-	branchRef, err := repo.GetReference(ctx, git.NewBranchReferenceName(string(req.GetBranch())))
+	branchRef, err := repo.GetReference(ctx, git.NewReferenceNameFromBranchName(string(req.GetBranch())))
 	if err != nil {
 		if errors.Is(err, git.ErrReferenceNotFound) {
 			return nil, helper.ErrInvalidArgumentf("Cannot find branch")
@@ -156,7 +156,7 @@ func (s *Server) userUpdateSubmodule(ctx context.Context, req *gitalypb.UserUpda
 		ctx,
 		req.GetRepository(),
 		req.GetUser(),
-		"refs/heads/"+string(req.GetBranch()),
+		branchRef.Name.String(),
 		result.CommitID,
 		branchRef.Target,
 	); err != nil {
diff --git a/internal/gitaly/service/operations/submodules_test.go b/internal/gitaly/service/operations/submodules_test.go
index f788c1f0f..7fd7b7ec6 100644
--- a/internal/gitaly/service/operations/submodules_test.go
+++ b/internal/gitaly/service/operations/submodules_test.go
@@ -9,6 +9,7 @@ import (
 	"github.com/golang/protobuf/ptypes/timestamp"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
+	"gitlab.com/gitlab-org/gitaly/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/git/lstree"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/config"
@@ -33,9 +34,24 @@ func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context)
 	client, conn := newOperationClient(t, serverSocketPath)
 	defer conn.Close()
 
-	testRepo, testRepoPath, cleanup := testhelper.NewTestRepo(t)
+	testRepoProto, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
+	testRepo := localrepo.New(testRepoProto, config.Config)
+
+	// This reference is created to check that we can correctly commit onto
+	// a branch which has a name starting with "refs/heads/".
+	currentOID, err := testRepo.ResolveRevision(ctx, "refs/heads/master")
+	require.NoError(t, err)
+	require.NoError(t, testRepo.UpdateRef(ctx, "refs/heads/refs/heads/master", currentOID, git.ZeroOID))
+
+	// If something uses the branch name as an unqualified reference, then
+	// git would return the tag instead of the branch. We thus create a tag
+	// with a different OID than the current master branch.
+	prevOID, err := testRepo.ResolveRevision(ctx, "refs/heads/master~")
+	require.NoError(t, err)
+	require.NoError(t, testRepo.UpdateRef(ctx, "refs/tags/master", prevOID, git.ZeroOID))
+
 	commitMessage := []byte("Update Submodule message")
 
 	testCases := []struct {
@@ -51,6 +67,12 @@ func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context)
 			branch:    "master",
 		},
 		{
+			desc:      "Update submodule on weird branch",
+			submodule: "gitlab-grack",
+			commitSha: "41fa1bc9e0f0630ced6a8a211d60c2af425ecc2d",
+			branch:    "refs/heads/master",
+		},
+		{
 			desc:      "Update submodule inside folder",
 			submodule: "test_inside_folder/another_folder/six",
 			commitSha: "e25eda1fece24ac7a03624ed1320f82396f35bd8",
@@ -61,7 +83,7 @@ func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context)
 	for _, testCase := range testCases {
 		t.Run(testCase.desc, func(t *testing.T) {
 			request := &gitalypb.UserUpdateSubmoduleRequest{
-				Repository:    testRepo,
+				Repository:    testRepoProto,
 				User:          testhelper.TestUser,
 				Submodule:     []byte(testCase.submodule),
 				CommitSha:     testCase.commitSha,
@@ -74,7 +96,7 @@ func testSuccessfulUserUpdateSubmoduleRequest(t *testing.T, ctx context.Context)
 			require.Empty(t, response.GetCommitError())
 			require.Empty(t, response.GetPreReceiveError())
 
-			commit, err := log.GetCommit(ctx, locator, testRepo, git.Revision(response.BranchUpdate.CommitId))
+			commit, err := log.GetCommit(ctx, locator, testRepoProto, git.Revision(response.BranchUpdate.CommitId))
 			require.NoError(t, err)
 			require.Equal(t, commit.Author.Email, testhelper.TestUser.Email)
 			require.Equal(t, commit.Committer.Email, testhelper.TestUser.Email)
diff --git a/proto/go/gitalypb/operations.pb.go b/proto/go/gitalypb/operations.pb.go
index 8938b24d7..27850cfe9 100644
--- a/proto/go/gitalypb/operations.pb.go
+++ b/proto/go/gitalypb/operations.pb.go
@@ -2715,7 +2715,8 @@ type UserUpdateSubmoduleRequest struct {
 	User *User `protobuf:"bytes,2,opt,name=user,proto3" json:"user,omitempty"`
 	// commit_sha is the object ID the submodule shall be updated to.
 	CommitSha string `protobuf:"bytes,3,opt,name=commit_sha,json=commitSha,proto3" json:"commit_sha,omitempty"`
-	// branch is the branch which shall be updated.
+	// branch is the branch which shall be updated. This is the unqualified name
+	// of the branch, it must not have a "refs/heads/" prefix.
 	Branch []byte `protobuf:"bytes,4,opt,name=branch,proto3" json:"branch,omitempty"`
 	// submodule is the path to the submodule which shall be updated.
 	Submodule []byte `protobuf:"bytes,5,opt,name=submodule,proto3" json:"submodule,omitempty"`
diff --git a/proto/operations.proto b/proto/operations.proto
index 70484113f..981e5864f 100644
--- a/proto/operations.proto
+++ b/proto/operations.proto
@@ -670,7 +670,8 @@ message UserUpdateSubmoduleRequest {
   User user = 2;
   // commit_sha is the object ID the submodule shall be updated to.
   string commit_sha = 3;
-  // branch is the branch which shall be updated.
+  // branch is the branch which shall be updated. This is the unqualified name
+  // of the branch, it must not have a "refs/heads/" prefix.
   bytes branch = 4;
   // submodule is the path to the submodule which shall be updated.
   bytes submodule = 5;
diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb
index d24ccb35c..e248ad774 100644
--- a/ruby/lib/gitlab/git/repository.rb
+++ b/ruby/lib/gitlab/git/repository.rb
@@ -406,7 +406,7 @@ module Gitlab
       end
 
       def update_submodule(submodule_path, commit_sha, branch, committer, message)
-        target = rugged.rev_parse(branch)
+        target = rugged.rev_parse("refs/heads/" + branch)
         raise CommitError, 'Invalid branch' unless target.is_a?(Rugged::Commit)
 
         current_entry = rugged_submodule_entry(target, submodule_path)
@@ -505,7 +505,7 @@ module Gitlab
       def find_branch(name, force_reload = false)
         reload_rugged if force_reload
 
-        rugged_ref = rugged.branches[name]
+        rugged_ref = rugged.ref("refs/heads/" + name)
         if rugged_ref
           target_commit = Gitlab::Git::Commit.find(self, rugged_ref.target)
           Gitlab::Git::Branch.new(self, rugged_ref.canonical_name, rugged_ref.target, target_commit)