Fix URI double escaping
There were two problems here that had to be solved at the same time:
1) Escaping for a URI path uses different rules than encoding for a URL
form. URLEncoder is intended for forms, so switch to using Guava's
UrlEscapers.urlPathSegmentEscaper().
2) When we replace the ${name} token in the JGit URIish, we need to call
uri.setRawPath() so that uri.path is set to the unescaped URI.
Add tests showing the now fixed URI escaping in the context of
ReplicationTaskStorage. Also refactor getURI() to make it more testable
and add tests for the correct behavior.
Change-Id: I9be55fafd78f546aecdeffebedd1d0d5e136022f
Release-Notes: https remote URIs correctly escape project names
Release-Notes: Stored tasks with (broken) double escaped URIs must be manually removed
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 ac4b671..a9ff233 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.entities.AccountGroup;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.GroupReference;
@@ -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.HashMap;
import java.util.List;
import java.util.Map;
@@ -678,9 +679,15 @@
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(
+ String.format(
+ "remote config uri %s has invalid syntax with project %s", configUri, project));
}
}
return false;
@@ -689,20 +696,36 @@
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(
+ String.format(
+ "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")) {
@@ -712,27 +735,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 16a0363..bb47ef8 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;
@@ -146,6 +147,25 @@
}
@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, 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(
