Workaround Gitiles bug on All-Users visibility
Gitiles has special FilteredRepository wrapper that
allows to carefully hide refs based on the project's ACLs.
There is however an optimisation that skips the filtering
in case a user has READ permissions on every ACLs patterns.
When the target repository is All-Users, the optimisation
turns into a security issue because it allows seeing everything
that belongs to everyone:
- draft comments
- PII of all users
- external ids
- draft edits
Block Gitiles or any other part of Gerrit to abuse of this
power when the target repository is All-Users, where nobody
can be authorised to skip the ACLs evaluation.
Cover the additional special case of the All-Users project
access with two explicit positive and negative tests,
so that the security check is covered.
Bug: Issue 13621
Change-Id: Ia6ea1a9fd5473adff534204aea7d8f25324a45b7
(cherry picked from commit 45071d6977932bca5a1427c8abad24710fed2e33)
diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
index a0b1f67..2dd960f 100644
--- a/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
+++ b/gerrit-server/src/main/java/com/google/gerrit/server/project/ProjectControl.java
@@ -39,6 +39,7 @@
import com.google.gerrit.reviewdb.server.ReviewDb;
import com.google.gerrit.server.CurrentUser;
import com.google.gerrit.server.account.GroupMembership;
+import com.google.gerrit.server.config.AllUsersName;
import com.google.gerrit.server.config.GitReceivePackGroups;
import com.google.gerrit.server.config.GitUploadPackGroups;
import com.google.gerrit.server.git.GitRepositoryManager;
@@ -140,6 +141,7 @@
private final RefVisibilityControl refVisibilityControl;
private final VisibleRefFilter.Factory visibleRefFilterFactory;
private final GitRepositoryManager gitRepositoryManager;
+ private final AllUsersName allUsersName;
private List<SectionMatcher> allSections;
private Map<String, RefControl> refControls;
@@ -156,6 +158,7 @@
RefVisibilityControl refVisibilityControl,
GitRepositoryManager gitRepositoryManager,
VisibleRefFilter.Factory visibleRefFilterFactory,
+ AllUsersName allUsersName,
@Assisted CurrentUser who,
@Assisted ProjectState ps) {
this.changeControlFactory = changeControlFactory;
@@ -167,6 +170,7 @@
this.refVisibilityControl = refVisibilityControl;
this.gitRepositoryManager = gitRepositoryManager;
this.visibleRefFilterFactory = visibleRefFilterFactory;
+ this.allUsersName = allUsersName;
user = who;
state = ps;
}
@@ -262,7 +266,9 @@
}
private boolean allRefsAreVisible(Set<String> ignore) {
- return user.isInternalUser() || canPerformOnAllRefs(Permission.READ, ignore);
+ return user.isInternalUser()
+ || (!getProject().getNameKey().equals(allUsersName)
+ && canPerformOnAllRefs(Permission.READ, ignore));
}
/** Returns whether the project is hidden. */
diff --git a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
index dff5af0..364013f 100644
--- a/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
+++ b/gerrit-server/src/test/java/com/google/gerrit/server/project/RefControlTest.java
@@ -61,6 +61,7 @@
import com.google.gerrit.server.git.VisibleRefFilter;
import com.google.gerrit.server.index.SingleVersionModule.SingleVersionListener;
import com.google.gerrit.server.permissions.PermissionBackend;
+import com.google.gerrit.server.permissions.PermissionBackendException;
import com.google.gerrit.server.permissions.ProjectPermission;
import com.google.gerrit.server.permissions.RefPermission;
import com.google.gerrit.server.permissions.RefVisibilityControl;
@@ -110,6 +111,16 @@
assertThat(u.controlForRef(ref).isOwner()).named("NOT OWN " + ref).isFalse();
}
+ private void assertAllRefsAreVisible(ProjectControl u) throws PermissionBackendException {
+ assertThat(u.asForProject().test(ProjectPermission.READ)).named("all refs visible").isTrue();
+ }
+
+ private void assertAllRefsAreNotVisible(ProjectControl u) throws PermissionBackendException {
+ assertThat(u.asForProject().test(ProjectPermission.READ))
+ .named("all refs NOT visible")
+ .isFalse();
+ }
+
private void assertCanAccess(ProjectControl u) {
boolean access = u.asForProject().testOrFalse(ProjectPermission.ACCESS);
assertThat(access).named("can access").isTrue();
@@ -199,6 +210,7 @@
private final Map<Project.NameKey, ProjectState> all = new HashMap<>();
private Project.NameKey localKey = new Project.NameKey("local");
private ProjectConfig local;
+ private ProjectConfig allUsers;
private Project.NameKey parentKey = new Project.NameKey("parent");
private ProjectConfig parent;
private InMemoryRepositoryManager repoManager;
@@ -230,7 +242,7 @@
@Override
public ProjectState getAllUsers() {
- return null;
+ return get(allUsersName);
}
@Override
@@ -290,6 +302,11 @@
LabelType cr = Util.codeReview();
allProjects.getLabelSections().put(cr.getName(), cr);
add(allProjects);
+
+ Repository allUsersRepo = repoManager.createRepository(allUsersName);
+ allUsers = new ProjectConfig(new Project.NameKey(allUsersName.get()));
+ allUsers.load(allUsersRepo);
+ add(allUsers);
} catch (IOException | ConfigInvalidException e) {
throw new RuntimeException(e);
}
@@ -364,6 +381,24 @@
}
@Test
+ public void allRefsAreVisibleForRegularProject() throws Exception {
+ allow(local, READ, DEVS, "refs/*");
+ allow(local, READ, DEVS, "refs/groups/*");
+ allow(local, READ, DEVS, "refs/users/default");
+
+ assertAllRefsAreVisible(user(local, DEVS));
+ }
+
+ @Test
+ public void allRefsAreNotVisibleForAllUsers() throws Exception {
+ allow(allUsers, READ, DEVS, "refs/*");
+ allow(allUsers, READ, DEVS, "refs/groups/*");
+ allow(allUsers, READ, DEVS, "refs/users/default");
+
+ assertAllRefsAreNotVisible(user(allUsers, DEVS));
+ }
+
+ @Test
public void branchDelegation1() {
allow(local, OWNER, ADMIN, "refs/*");
allow(local, OWNER, DEVS, "refs/heads/x/*");
@@ -897,6 +932,7 @@
refVisibilityControl,
gitRepositoryManager,
visibleRefFilterFactory,
+ allUsersName,
new MockUser(name, memberOf),
newProjectState(local));
}
