From 8b0c25b57dfeb132555cbd77714e54ae4a15e8ba Mon Sep 17 00:00:00 2001 From: Youssef Elghareeb Date: Fri, 15 Jan 2021 12:13:32 +0100 Subject: [PATCH] Disallow creating branches in Gerrit internal or tag refs namespaces This change explicitly bans creating branches using the CREATE Branch rest endpoint to any Gerrit internal or tags refs. Before this change, the user could have done that given that they have enough permissions to create references under these namespaces. Change-Id: I952ee9c5bffc879613c8808e98ba6d80e2bc22d8 --- .../server/restapi/project/CreateBranch.java | 11 ++++++++ .../rest/change/CreateChangeIT.java | 14 ++-------- .../rest/project/CreateBranchIT.java | 28 +++++++++++++++---- 3 files changed, 35 insertions(+), 18 deletions(-) diff --git a/java/com/google/gerrit/server/restapi/project/CreateBranch.java b/java/com/google/gerrit/server/restapi/project/CreateBranch.java index b901057a86..2fd2d6530f 100644 --- a/java/com/google/gerrit/server/restapi/project/CreateBranch.java +++ b/java/com/google/gerrit/server/restapi/project/CreateBranch.java @@ -109,6 +109,12 @@ public class CreateBranch + MagicBranch.getMagicRefNamePrefix(ref) + "\""); } + if (!isBranchAllowed(ref)) { + throw new BadRequestException( + "Cannot create a branch with name \"" + + ref + + "\". Not allowed to create branches under Gerrit internal or tags refs."); + } BranchNameKey name = BranchNameKey.create(rsrc.getNameKey(), ref); try (Repository repo = repoManager.openRepository(rsrc.getNameKey())) { @@ -187,4 +193,9 @@ public class CreateBranch throw new BadRequestException("invalid revision \"" + input.revision + "\"", e); } } + + /** Branches cannot be created under any Gerrit internal or tags refs. */ + private boolean isBranchAllowed(String branch) { + return !RefNames.isGerritRef(branch) && !branch.startsWith(RefNames.REFS_TAGS); + } } diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index a539bd5436..129b546a6c 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -190,15 +190,10 @@ public class CreateChangeIT extends AbstractDaemonTest { .add(allow(CREATE).ref("refs/*").group(REGISTERED_USERS)) .update(); - String disallowedRef = "refs/changes/00/1000"; // All Gerrit internal refs behave the same way - requestScopeOperations.setApiUser(admin.id()); - BranchNameKey branchNameKey = BranchNameKey.create(project, disallowedRef); - createBranch(branchNameKey); - requestScopeOperations.setApiUser(user.id()); ChangeInput ci = newChangeInput(ChangeStatus.NEW); ci.subject = "Subject"; - ci.branch = disallowedRef; + ci.branch = "refs/changes/00/1000"; // disallowedRef Throwable thrown = assertThrows(RestApiException.class, () -> gApi.changes().create(ci)); assertThat(thrown).hasMessageThat().contains("Cannot create a change on ref " + ci.branch); @@ -213,15 +208,10 @@ public class CreateChangeIT extends AbstractDaemonTest { .add(allow(CREATE).ref("refs/*").group(REGISTERED_USERS)) .update(); - String branchName = "refs/tags/v1.0"; - requestScopeOperations.setApiUser(admin.id()); - BranchNameKey branchNameKey = BranchNameKey.create(project, branchName); - createBranch(branchNameKey); - requestScopeOperations.setApiUser(user.id()); ChangeInput ci = newChangeInput(ChangeStatus.NEW); ci.subject = "Subject"; - ci.branch = branchName; + ci.branch = "refs/tags/v1.0"; // disallowed ref Throwable thrown = assertThrows(RestApiException.class, () -> gApi.changes().create(ci)); assertThat(thrown).hasMessageThat().contains("Cannot create a change on ref " + ci.branch); diff --git a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java index 096c72b0fd..93ce255f62 100644 --- a/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/project/CreateBranchIT.java @@ -207,7 +207,7 @@ public class CreateBranchIT extends AbstractDaemonTest { } @Test - public void createUserBranch_Conflict() throws Exception { + public void createUserBranch_NotAllowed() throws Exception { projectOperations .project(allUsers) .forUpdate() @@ -217,12 +217,12 @@ public class CreateBranchIT extends AbstractDaemonTest { assertCreateFails( BranchNameKey.create(allUsers, RefNames.refsUsers(Account.id(1))), RefNames.refsUsers(admin.id()), - ResourceConflictException.class, - "Not allowed to create user branch."); + BadRequestException.class, + "Not allowed to create branches under Gerrit internal or tags refs."); } @Test - public void createGroupBranch_Conflict() throws Exception { + public void createGroupBranch_NotAllowed() throws Exception { projectOperations .project(allUsers) .forUpdate() @@ -232,8 +232,8 @@ public class CreateBranchIT extends AbstractDaemonTest { assertCreateFails( BranchNameKey.create(allUsers, RefNames.refsGroups(AccountGroup.uuid("foo"))), RefNames.refsGroups(adminGroupUuid()), - ResourceConflictException.class, - "Not allowed to create group branch."); + BadRequestException.class, + "Not allowed to create branches under Gerrit internal or tags refs."); } @Test @@ -354,6 +354,22 @@ public class CreateBranchIT extends AbstractDaemonTest { "not allowed to create branches under \"" + MagicBranch.NEW_CHANGE + "\""); } + @Test + public void cannotCreateBranchInGerritInternalRefsNamespace() throws Exception { + assertCreateFails( + BranchNameKey.create(project, RefNames.REFS_CHANGES + "00/1000"), + BadRequestException.class, + "Not allowed to create branches under Gerrit internal or tags refs."); + } + + @Test + public void cannotCreateBranchInTagsNamespace() throws Exception { + assertCreateFails( + BranchNameKey.create(project, RefNames.REFS_TAGS + "v1.0"), + BadRequestException.class, + "Not allowed to create branches under Gerrit internal or tags refs."); + } + @Test public void cannotCreateBranchWithInvalidName() throws Exception { assertCreateFails(