From dd4f061945e16f3d6fa886a151b8d1119a50f6e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 24 Nov 2020 17:04:36 +0100 Subject: Branch: fix API use bug in User(Create|Delete)Branch Change code added in [1] and [2] to use GetReference() instead of GetBranch(). This solves a bug that could theoretically occur, but is mainly a trivial API code cleanup. The GetBranch() function can take refs/heads/master or heads/master or whatever to resolve "master", but the underlying updateReferenceWithHooks() we're calling will only accept a refs/heads/master. Since we'd accept a request of "master" which we'd always normalize to "refs/heads/master" this shouldn't change anything in practice. It does solve an obscure bug though. If you had both a refs/heads/master and a refs/heads/heads/master we'd get the old revision from the latter, but operate on the former. Thus hooks would get an invalid old revision in such a case, and furthermore we would unexpectedly allow deletion of refs/heads/heads/master in the obscure edge case where we raced between GetReference() and s.updateReferenceWithHooks() while another process updated refs/heads/heads/master. 1. c3b327226 (Initial go implementation of UserDeleteBranch, 2020-09-29) 2. c1e3ccca6 (Initial go implementation of UserCreateBranch, 2020-09-30) --- ...-and-create-branch-GetBranch-vs-GetReference-bugfix.yml | 5 +++++ internal/gitaly/service/operations/branches.go | 14 ++++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) create mode 100644 changelogs/unreleased/avar-user-delete-and-create-branch-GetBranch-vs-GetReference-bugfix.yml diff --git a/changelogs/unreleased/avar-user-delete-and-create-branch-GetBranch-vs-GetReference-bugfix.yml b/changelogs/unreleased/avar-user-delete-and-create-branch-GetBranch-vs-GetReference-bugfix.yml new file mode 100644 index 000000000..de315c9bb --- /dev/null +++ b/changelogs/unreleased/avar-user-delete-and-create-branch-GetBranch-vs-GetReference-bugfix.yml @@ -0,0 +1,5 @@ +--- +title: 'Branch: fix API use bug in User(Create|Delete)Branch' +merge_request: 2838 +author: +type: fixed diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index f1cb37c04..b2697451b 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -39,16 +39,15 @@ func (s *server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB return nil, helper.ErrPreconditionFailed(err) } - _, err = git.NewRepository(req.Repository).GetBranch(ctx, string(req.BranchName)) + referenceName := fmt.Sprintf("refs/heads/%s", req.BranchName) + _, err = git.NewRepository(req.Repository).GetReference(ctx, referenceName) if err == nil { return nil, status.Errorf(codes.FailedPrecondition, "Bad Request (branch exists)") } else if !errors.Is(err, git.ErrReferenceNotFound) { return nil, status.Error(codes.Internal, err.Error()) } - branch := fmt.Sprintf("refs/heads/%s", req.BranchName) - - if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, string(req.StartPoint), git.NullSHA); err != nil { + if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, referenceName, string(req.StartPoint), git.NullSHA); err != nil { var preReceiveError preReceiveError if errors.As(err, &preReceiveError) { return &gitalypb.UserCreateBranchResponse{ @@ -118,14 +117,13 @@ func (s *server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB // Implement UserDeleteBranch in Go - revision, err := git.NewRepository(req.Repository).GetBranch(ctx, string(req.BranchName)) + referenceName := fmt.Sprintf("refs/heads/%s", req.BranchName) + revision, err := git.NewRepository(req.Repository).GetReference(ctx, referenceName) if err != nil { return nil, helper.ErrPreconditionFailed(err) } - branch := fmt.Sprintf("refs/heads/%s", req.BranchName) - - if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, branch, git.NullSHA, revision.Name); err != nil { + if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, referenceName, git.NullSHA, revision.Name); err != nil { var preReceiveError preReceiveError if errors.As(err, &preReceiveError) { return &gitalypb.UserDeleteBranchResponse{ -- cgit v1.2.3