Fix Access when the only readable ref is a RegExp with a gerrit pattern
When no other ref than a RegExp with a gerrit username or shardeduserid
pattern, the permission is not properly validated and repository access
is denied.
With change Iedf3022f6c, calling shortestExample on regexp+parameters
refs produces a reference that won't match the username or the
shardeduserid.
For a reference like : ^refs/heads/users/${username}/.+
we will have the following:
'refs/heads/users/user1/my_branch' vs
'refs/heads/users/_PLACEHOLDER_/my_branch'
When we remove shortestExample, we have to modify Regexp and
ExpandParameters to skip the regexp start anchor caracter when comparing
against the prefix, or to compare regexps themselves.
Bug: Issue 3340
Change-Id: Id6a890a91c5d6d954dab74694c2d3fd53c15897c
This commit is contained in:
committed by
David Pursehouse
parent
1b4b264adf
commit
80f75734e7
@@ -15,6 +15,7 @@
|
||||
package com.google.gerrit.server.permissions;
|
||||
|
||||
import static com.google.gerrit.common.data.PermissionRule.Action.BLOCK;
|
||||
import static com.google.gerrit.server.project.RefPattern.containsParameters;
|
||||
import static com.google.gerrit.server.project.RefPattern.isRE;
|
||||
import static java.util.stream.Collectors.mapping;
|
||||
import static java.util.stream.Collectors.toList;
|
||||
@@ -131,7 +132,9 @@ public class PermissionCollection {
|
||||
Iterable<SectionMatcher> matcherList, String ref, CurrentUser user) {
|
||||
try (Timer0.Context ignored = filterLatency.start()) {
|
||||
if (isRE(ref)) {
|
||||
ref = RefPattern.shortestExample(ref);
|
||||
if (!containsParameters(ref)) {
|
||||
ref = RefPattern.shortestExample(ref);
|
||||
}
|
||||
} else if (ref.endsWith("/*")) {
|
||||
ref = ref.substring(0, ref.length() - 1);
|
||||
}
|
||||
|
||||
@@ -73,6 +73,10 @@ public class RefPattern {
|
||||
return refPattern.startsWith(AccessSection.REGEX_PREFIX);
|
||||
}
|
||||
|
||||
public static boolean containsParameters(String refPattern) {
|
||||
return refPattern.contains("${");
|
||||
}
|
||||
|
||||
public static RegExp toRegExp(String refPattern) {
|
||||
if (isRE(refPattern)) {
|
||||
refPattern = refPattern.substring(1);
|
||||
|
||||
@@ -15,6 +15,7 @@
|
||||
package com.google.gerrit.server.project;
|
||||
|
||||
import static com.google.common.collect.ImmutableSet.toImmutableSet;
|
||||
import static com.google.gerrit.server.project.RefPattern.containsParameters;
|
||||
import static com.google.gerrit.server.project.RefPattern.isRE;
|
||||
|
||||
import com.google.common.collect.ImmutableMap;
|
||||
@@ -32,7 +33,7 @@ import java.util.stream.Stream;
|
||||
|
||||
public abstract class RefPatternMatcher {
|
||||
public static RefPatternMatcher getMatcher(String pattern) {
|
||||
if (pattern.contains("${")) {
|
||||
if (containsParameters(pattern)) {
|
||||
return new ExpandParameters(pattern);
|
||||
} else if (isRE(pattern)) {
|
||||
return new Regexp(pattern);
|
||||
@@ -80,7 +81,7 @@ public abstract class RefPatternMatcher {
|
||||
|
||||
@Override
|
||||
public boolean match(String ref, CurrentUser user) {
|
||||
return pattern.matcher(ref).matches();
|
||||
return pattern.matcher(ref).matches() || (isRE(ref) && pattern.pattern().equals(ref));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -112,7 +113,11 @@ public abstract class RefPatternMatcher {
|
||||
|
||||
@Override
|
||||
public boolean match(String ref, CurrentUser user) {
|
||||
if (!ref.startsWith(prefix)) {
|
||||
if (isRE(ref)) {
|
||||
if (!ref.substring(1).startsWith(prefix)) {
|
||||
return false;
|
||||
}
|
||||
} else if (!ref.startsWith(prefix)) {
|
||||
return false;
|
||||
}
|
||||
|
||||
@@ -142,6 +147,9 @@ public abstract class RefPatternMatcher {
|
||||
}
|
||||
|
||||
public boolean matchPrefix(String ref) {
|
||||
if (isRE(ref)) {
|
||||
return ref.substring(1).startsWith(prefix);
|
||||
}
|
||||
return ref.startsWith(prefix);
|
||||
}
|
||||
|
||||
|
||||
@@ -529,6 +529,7 @@ public class RefControlTest {
|
||||
public void usernamePatternRegExpCanUploadToAnyRef() throws Exception {
|
||||
allow(local, PUSH, REGISTERED_USERS, "^refs/heads/users/${username}/(public|private)/.+");
|
||||
ProjectControl u = user(local, "a-registered-user");
|
||||
assertCanUpload(u);
|
||||
assertCanUpdate("refs/heads/users/a-registered-user/private/a", u);
|
||||
}
|
||||
|
||||
@@ -548,6 +549,8 @@ public class RefControlTest {
|
||||
|
||||
ProjectControl u = user(local, "d.v", DEVS);
|
||||
ProjectControl d = user(local, "dev", DEVS);
|
||||
assertCanAccess(u);
|
||||
assertCanAccess(d);
|
||||
assertCannotRead("refs/sb/dev/heads/foobar", u);
|
||||
assertCanRead("refs/sb/dev/heads/foobar", d);
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user