Merge branch 'stable-3.5'
* stable-3.5:
Destination: skip unnecessary String.format()
Fix URI double escaping
Change-Id: If0850f76c51ad2b769464b1ba49015d75c819959
Release-Notes: skip
diff --git a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
index 53bc9cf..a7235d5 100644
--- a/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
+++ b/src/main/java/com/googlesource/gerrit/plugins/replication/Destination.java
@@ -19,12 +19,14 @@
import static org.eclipse.jgit.transport.RemoteRefUpdate.Status.NON_EXISTING;
import static org.eclipse.jgit.transport.RemoteRefUpdate.Status.REJECTED_OTHER_REASON;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.io.Files;
+import com.google.common.net.UrlEscapers;
import com.google.gerrit.common.Nullable;
import com.google.gerrit.entities.AccountGroup;
import com.google.gerrit.entities.BranchNameKey;
@@ -67,8 +69,7 @@
import com.googlesource.gerrit.plugins.replication.events.RefReplicatedEvent;
import com.googlesource.gerrit.plugins.replication.events.ReplicationScheduledEvent;
import java.io.IOException;
-import java.io.UnsupportedEncodingException;
-import java.net.URLEncoder;
+import java.net.URISyntaxException;
import java.util.Collection;
import java.util.HashMap;
import java.util.HashSet;
@@ -687,9 +688,14 @@
private boolean matches(URIish uri, Project.NameKey project) {
for (URIish configUri : config.getRemoteConfig().getURIs()) {
- URIish projectUri = getURI(configUri, project);
- if (uri.equals(projectUri)) {
- return true;
+ try {
+ URIish projectUri = getURI(configUri, project);
+ if (uri.equals(projectUri)) {
+ return true;
+ }
+ } catch (URISyntaxException e) {
+ repLog.atSevere().withCause(e).log(
+ "remote config uri %s has invalid syntax with project %s", configUri, project);
}
}
return false;
@@ -698,20 +704,35 @@
List<URIish> getURIs(Project.NameKey project, String urlMatch) {
List<URIish> r = Lists.newArrayListWithCapacity(config.getRemoteConfig().getURIs().size());
for (URIish configUri : config.getRemoteConfig().getURIs()) {
- URIish uri = getURI(configUri, project);
- if (matches(configUri, urlMatch) || matches(uri, urlMatch)) {
- r.add(uri);
+ try {
+ URIish uri = getURI(configUri, project);
+ if (matches(configUri, urlMatch) || matches(uri, urlMatch)) {
+ r.add(uri);
+ }
+ } catch (URISyntaxException e) {
+ repLog.atSevere().withCause(e).log(
+ "remote config uri %s has invalid syntax with project %s", configUri, project);
}
}
return r;
}
- URIish getURI(URIish template, Project.NameKey project) {
+ URIish getURI(URIish template, Project.NameKey project) throws URISyntaxException {
+ return getURI(template, project, config.getRemoteNameStyle(), config.isSingleProjectMatch());
+ }
+
+ @VisibleForTesting
+ static URIish getURI(
+ URIish template,
+ Project.NameKey project,
+ String remoteNameStyle,
+ boolean isSingleProjectMatch)
+ throws URISyntaxException {
String name = project.get();
- if (needsUrlEncoding(template)) {
- name = encode(name);
+ if (needsUrlEscaping(template)) {
+ name = escape(name);
}
- String remoteNameStyle = config.getRemoteNameStyle();
+
if (remoteNameStyle.equals("dash")) {
name = name.replace("/", "-");
} else if (remoteNameStyle.equals("underscore")) {
@@ -721,27 +742,21 @@
} else if (!remoteNameStyle.equals("slash")) {
repLog.atFine().log("Unknown remoteNameStyle: %s, falling back to slash", remoteNameStyle);
}
- String replacedPath = replaceName(template.getPath(), name, isSingleProjectMatch());
- return (replacedPath != null) ? template.setPath(replacedPath) : template;
+ String replacedPath = replaceName(template.getPath(), name, isSingleProjectMatch);
+ return (replacedPath != null) ? template.setRawPath(replacedPath) : template;
}
- static boolean needsUrlEncoding(URIish uri) {
+ static boolean needsUrlEscaping(URIish uri) {
return "http".equalsIgnoreCase(uri.getScheme())
|| "https".equalsIgnoreCase(uri.getScheme())
|| "amazon-s3".equalsIgnoreCase(uri.getScheme());
}
- static String encode(String str) {
- try {
- // Some cleanup is required. The '/' character is always encoded as %2F
- // however remote servers will expect it to be not encoded as part of the
- // path used to the repository. Space is incorrectly encoded as '+' for this
- // context. In the path part of a URI space should be %20, but in form data
- // space is '+'. Our cleanup replace fixes these two issues.
- return URLEncoder.encode(str, "UTF-8").replaceAll("%2[fF]", "/").replace("+", "%20");
- } catch (UnsupportedEncodingException e) {
- throw new RuntimeException(e);
- }
+ static String escape(String str) {
+ // The '/' character is always encoded as %2F however, remote servers will expect it to be not
+ // encoded as part of the path used to the repository. The fix to avoid this would be to build
+ // up the path and escape each segment separately.
+ return UrlEscapers.urlPathSegmentEscaper().escape(str).replaceAll("%2[fF]", "/");
}
ImmutableList<String> getAdminUrls() {
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/PushReplicationTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/PushReplicationTest.java
index 8480cbe..122465b 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/PushReplicationTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/PushReplicationTest.java
@@ -15,9 +15,10 @@
package com.googlesource.gerrit.plugins.replication;
import static com.google.common.truth.Truth.assertThat;
-import static com.googlesource.gerrit.plugins.replication.Destination.encode;
-import static com.googlesource.gerrit.plugins.replication.Destination.needsUrlEncoding;
+import static com.googlesource.gerrit.plugins.replication.Destination.escape;
+import static com.googlesource.gerrit.plugins.replication.Destination.needsUrlEscaping;
+import com.google.gerrit.entities.Project;
import java.net.URISyntaxException;
import org.eclipse.jgit.transport.URIish;
import org.junit.Test;
@@ -25,22 +26,46 @@
public class PushReplicationTest {
@Test
- public void testNeedsUrlEncoding() throws URISyntaxException {
- assertThat(needsUrlEncoding(new URIish("http://host/path"))).isTrue();
- assertThat(needsUrlEncoding(new URIish("https://host/path"))).isTrue();
- assertThat(needsUrlEncoding(new URIish("amazon-s3://config/bucket/path"))).isTrue();
+ public void testNeedsUrlEscaping() throws URISyntaxException {
+ assertThat(needsUrlEscaping(new URIish("http://host/path"))).isTrue();
+ assertThat(needsUrlEscaping(new URIish("https://host/path"))).isTrue();
+ assertThat(needsUrlEscaping(new URIish("amazon-s3://config/bucket/path"))).isTrue();
- assertThat(needsUrlEncoding(new URIish("host:path"))).isFalse();
- assertThat(needsUrlEncoding(new URIish("user@host:path"))).isFalse();
- assertThat(needsUrlEncoding(new URIish("git://host/path"))).isFalse();
- assertThat(needsUrlEncoding(new URIish("ssh://host/path"))).isFalse();
+ assertThat(needsUrlEscaping(new URIish("host:path"))).isFalse();
+ assertThat(needsUrlEscaping(new URIish("user@host:path"))).isFalse();
+ assertThat(needsUrlEscaping(new URIish("git://host/path"))).isFalse();
+ assertThat(needsUrlEscaping(new URIish("ssh://host/path"))).isFalse();
}
@Test
- public void urlEncoding() {
- assertThat(encode("foo/bar/thing")).isEqualTo("foo/bar/thing");
- assertThat(encode("-- All Projects --")).isEqualTo("--%20All%20Projects%20--");
- assertThat(encode("name/with a space")).isEqualTo("name/with%20a%20space");
- assertThat(encode("name\nwith-LF")).isEqualTo("name%0Awith-LF");
+ public void urlEscaping() {
+ assertThat(escape("foo/bar/thing")).isEqualTo("foo/bar/thing");
+ assertThat(escape("-- All Projects --")).isEqualTo("--%20All%20Projects%20--");
+ assertThat(escape("name/with a space")).isEqualTo("name/with%20a%20space");
+ assertThat(escape("name\nwith-LF")).isEqualTo("name%0Awith-LF");
+ assertThat(
+ escape(
+ "key=a-value=1, --option1 \"OPTION_VALUE_1\" --option-2 <option_VALUE-2> --option-without-value"))
+ .isEqualTo(
+ "key=a-value=1,%20--option1%20%22OPTION_VALUE_1%22%20--option-2%20%3Coption_VALUE-2%3E%20--option-without-value");
+ }
+
+ @Test
+ public void getHttpsURIWithStrangeProjectName() throws Exception {
+ String urlBase = "https://git.server.example.com";
+ String url = urlBase + "/${name}.git";
+ URIish template = new URIish(url);
+ String name =
+ "key=a-value=1, --option1 \"OPTION_VALUE_1\" --option-2 <option_VALUE-2> --option-without-value";
+ String expectedAsciiName =
+ "key=a-value=1,%20--option1%20\"OPTION_VALUE_1\"%20--option-2%20<option_VALUE-2>%20--option-without-value";
+ String expectedEscapedName =
+ "key=a-value=1,%20--option1%20%22OPTION_VALUE_1%22%20--option-2%20%3Coption_VALUE-2%3E%20--option-without-value";
+ Project.NameKey project = Project.nameKey(name);
+ URIish expanded = Destination.getURI(template, project, "slash", false);
+
+ assertThat(expanded.getPath()).isEqualTo("/" + name + ".git");
+ assertThat(expanded.toString()).isEqualTo(urlBase + "/" + expectedEscapedName + ".git");
+ assertThat(expanded.toASCIIString()).isEqualTo(urlBase + "/" + expectedAsciiName + ".git");
}
}
diff --git a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
index 7061c79..6e02573 100644
--- a/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
+++ b/src/test/java/com/googlesource/gerrit/plugins/replication/ReplicationTasksStorageTest.java
@@ -23,6 +23,7 @@
import com.google.common.jimfs.Configuration;
import com.google.common.jimfs.Jimfs;
import com.google.common.truth.IterableSubject;
+import com.google.gerrit.entities.Project;
import com.googlesource.gerrit.plugins.replication.ReplicationTasksStorage.ReplicateRefUpdate;
import java.net.URISyntaxException;
import java.nio.file.FileSystem;
@@ -164,6 +165,26 @@
}
@Test
+ public void canStoreAndReadUrisWithEncodedChars() throws Exception {
+ String urlBase = "https://git.server.example.com";
+ String url = urlBase + "/${name}.git";
+ URIish template = new URIish(url);
+ String strangeValidName =
+ "project/with/a/strange/name key=a-value=1, --option1 \"OPTION_VALUE_1\" --option-2 <option_VALUE-2> --option-without-value";
+ Project.NameKey project = Project.nameKey(strangeValidName);
+ URIish expanded = Destination.getURI(template, project, "slash", false);
+ ReplicateRefUpdate update =
+ ReplicateRefUpdate.create(strangeValidName, Set.of(REF), expanded, REMOTE);
+ storage.create(update);
+
+ assertThat(new URIish(update.uri())).isEqualTo(expanded);
+ assertThatStream(storage.streamWaiting()).hasSize(1);
+ for (ReplicateRefUpdate rru : storage.streamWaiting().collect(Collectors.toList())) {
+ assertThat(new URIish(rru.uri())).isEqualTo(expanded);
+ }
+ }
+
+ @Test
public void canStartDifferentUris() throws Exception {
ReplicateRefUpdate updateB =
ReplicateRefUpdate.create(
