opendev/gerrit
Code Issues Proposed changes

Do not fail with ISE if default change query contains quoted invalid regex

Browse Source 
If a default query contains an invalid regex, e.g. "^[A", parsing the
query fails and the request is rejected as '400 Bad Request' (the
parsing fails because '[' is a syntax character in our Antlr syntax
specification). If the invalid regex is quoted the parsing succeeded,
but then creating the Automaton failed with an IllegalArgumentException
which caused a '500 Internal Sever Error'. We now catch the
IllegalArgumentException and wrap it into a QueryParseException so that
the user gets a '400 Bad Request'.  This is better than failing, but not
ideal yet. Ideally we would ignore the QueryParseException for the
default query and just have no match, but if we do that executing the
query fails in Lucene which would again be an internal server error.

Change-Id: Ic1d18919ca8a6a8d21156b7b3bf3b96aeb382624
Signed-off-by: Edwin Kempin <ekempin@google.com>
This commit is contained in:
Edwin Kempin
2020-02-26 12:34:38 +01:00
parent b4cddf0227
commit f9ee2e4641
3 changed files with 31 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/server/query/change/ChangeQueryBuilder.java
View File

@@ -703,7 +703,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
}
@Operator
public Predicate<ChangeData> branch(String name) {
public Predicate<ChangeData> branch(String name) throws QueryParseException {
if (name.startsWith("^")) {
return ref("^" + RefNames.fullName(name.substring(1)));
}
@@ -732,7 +732,7 @@ public class ChangeQueryBuilder extends QueryBuilder<ChangeData, ChangeQueryBuil
}
@Operator
public Predicate<ChangeData> ref(String ref) {
public Predicate<ChangeData> ref(String ref) throws QueryParseException {
if (ref.startsWith("^")) {
return new RegexRefPredicate(ref);
}

9
java/com/google/gerrit/server/query/change/RegexRefPredicate.java
View File

@@ -15,6 +15,7 @@
package com.google.gerrit.server.query.change;
import com.google.gerrit.entities.Change;
import com.google.gerrit.index.query.QueryParseException;
import com.google.gerrit.server.index.change.ChangeField;
import dk.brics.automaton.RegExp;
import dk.brics.automaton.RunAutomaton;
@@ -22,7 +23,7 @@ import dk.brics.automaton.RunAutomaton;
public class RegexRefPredicate extends ChangeRegexPredicate {
protected final RunAutomaton pattern;
public RegexRefPredicate(String re) {
public RegexRefPredicate(String re) throws QueryParseException {
super(ChangeField.REF, re);
if (re.startsWith("^")) {
@@ -33,7 +34,11 @@ public class RegexRefPredicate extends ChangeRegexPredicate {
re = re.substring(0, re.length() - 1);
}
this.pattern = new RunAutomaton(new RegExp(re).toAutomaton());
try {
this.pattern = new RunAutomaton(new RegExp(re).toAutomaton());
} catch (IllegalArgumentException e) {
throw new QueryParseException(String.format("invalid regular expression: %s", re), e);
}
}
@Override