diff options
3 files changed, 103 insertions, 42 deletions
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 com.googlesource.gerrit.plugins.replication.ReplicationFileBasedCo 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.ProjectDeletionState; 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 @@ public class Destination { 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 @@ public class Destination { 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 @@ public class Destination { } 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 @@ import org.junit.Test; 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(); - - 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(); + 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(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 static org.junit.Assert.assertTrue; 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 @@ public class ReplicationTasksStorageTest { } @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( |