Fix IllegalArgumentException for % that is not part of an encoded char
URLs use percentage encoding which replaces unsafe ASCII characters with
a '%' followed by two hexadecimal digits. If there is '%' that is not
followed by two hexadecimal digits Url.decode(String) and the code in
KeyUtil.decode fail with an IllegalArgumentException. To prevent this
replace any '%' that is not followed by two hexadecimal digits by "%25",
which is the URL encoding for '%', before decoding.
Without this fix users can trigger 500 internal server errors in Gerrit
by providing invalid strings for URL parameters (e.g. an invalid project
name for the ListAccess REST endpoint or using an invalid name in a
change identifier).
For context see:
https://stackoverflow.com/questions/6067673/urldecoder-illegal-hex-characters-in-escape-pattern-for-input-string
Release-Notes: Fixed parsing of URL parameters that contain a '%' that is not part of an encoded character
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I78ba7f34ea1ca511b55889a8d22345c6fb1091ea
Forward-Compatible: yes
diff --git a/java/com/google/gerrit/entities/KeyUtil.java b/java/com/google/gerrit/entities/KeyUtil.java
index 0f14cd9..4aec7ac 100644
--- a/java/com/google/gerrit/entities/KeyUtil.java
+++ b/java/com/google/gerrit/entities/KeyUtil.java
@@ -66,7 +66,13 @@
return r.toString();
}
- public static String decode(final String key) {
+ public static String decode(String key) {
+ // URLs use percentage encoding which replaces unsafe ASCII characters with a '%' followed by
+ // two hexadecimal digits. If there is '%' that is not followed by two hexadecimal digits
+ // the code below fails with an IllegalArgumentException. To prevent this replace any '%'
+ // that is not followed by two hexadecimal digits by "%25", which is the URL encoding for '%'.
+ key = key.replaceAll("%(?![0-9a-fA-F]{2})", "%25");
+
if (key.indexOf('%') < 0) {
return key.replace('+', ' ');
}
diff --git a/java/com/google/gerrit/extensions/restapi/IdString.java b/java/com/google/gerrit/extensions/restapi/IdString.java
index b2538fa..a69919f 100644
--- a/java/com/google/gerrit/extensions/restapi/IdString.java
+++ b/java/com/google/gerrit/extensions/restapi/IdString.java
@@ -38,7 +38,16 @@
/** Returns the decoded value of the string. */
public String get() {
- return Url.decode(urlEncoded);
+ String data = urlEncoded;
+
+ // URLs use percentage encoding which replaces unsafe ASCII characters with a '%' followed by
+ // two hexadecimal digits. If there is '%' that is not followed by two hexadecimal digits
+ // Url.decode(String) fails with an IllegalArgumentException. To prevent this replace any '%'
+ // that is not followed by two hexadecimal digits by "%25", which is the URL encoding for '%',
+ // before calling Url.decode(String).
+ data = data.replaceAll("%(?![0-9a-fA-F]{2})", "%25");
+
+ return Url.decode(data);
}
/** Returns true if the string is the empty string. */
diff --git a/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java b/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java
index 06e24ab..779d8eb 100644
--- a/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/change/ChangeIdIT.java
@@ -19,6 +19,7 @@
import com.google.gerrit.acceptance.RestResponse;
import com.google.gerrit.entities.Change;
import com.google.gerrit.extensions.api.changes.ChangeApi;
+import com.google.gerrit.extensions.restapi.IdString;
import org.junit.Test;
public class ChangeIdIT extends AbstractDaemonTest {
@@ -47,6 +48,13 @@
}
@Test
+ public void invalidProjectChangeNumberReturnsNotFound() throws Exception {
+ RestResponse res =
+ adminRestSession.get(changeDetail(IdString.fromDecoded("<%=FOO%>~1").encoded()));
+ res.assertNotFound();
+ }
+
+ @Test
public void changeNumberReturnsChange() throws Exception {
PushOneCommit.Result c = createChange();
RestResponse res = adminRestSession.get(changeDetail(getNumericChangeId(c.getChangeId())));
diff --git a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
index b94ea37..ca7c3c5 100644
--- a/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
+++ b/javatests/com/google/gerrit/acceptance/rest/project/AccessIT.java
@@ -109,4 +109,13 @@
.fromJson(r.getReader(), new TypeToken<Map<String, ProjectAccessInfo>>() {}.getType());
assertThat(infoByProject.keySet()).containsExactly(project.get());
}
+
+ @Test
+ public void listAccess_invalidProject() throws Exception {
+ String invalidProject = "<%=FOO%>";
+ RestResponse r =
+ adminRestSession.get("/access/?project=" + IdString.fromDecoded(invalidProject));
+ r.assertNotFound();
+ assertThat(r.getEntityContent()).isEqualTo(invalidProject);
+ }
}
diff --git a/javatests/com/google/gerrit/extensions/restapi/IdStringTest.java b/javatests/com/google/gerrit/extensions/restapi/IdStringTest.java
new file mode 100644
index 0000000..3a92864
--- /dev/null
+++ b/javatests/com/google/gerrit/extensions/restapi/IdStringTest.java
@@ -0,0 +1,28 @@
+// Copyright (C) 2023 The Android Open Source Project
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.gerrit.extensions.restapi;
+
+import static com.google.common.truth.Truth.assertThat;
+
+import org.junit.Test;
+
+/** Unit tests for {@link IdString}. */
+public class IdStringTest {
+ @Test
+ public void decodeStringWithPercentageThatIsNotFollowedByTwoHexadecimalDigits() throws Exception {
+ String s = "<%=FOO%>";
+ assertThat(IdString.fromUrl(s).get()).isEqualTo(s);
+ }
+}
