Url encoding evaluated values when they are used in URL invocations #76

Author: tomasbjerre

CHANGELOG.md

@@ -2,6 +2,9 @@
 
 Changelog of Pull Request Notifier for Stash.
 
+## 1.34
+* Url encoding evaluated values when they are used in URL invocations.
+
 ## 1.33
 * New variables
  * ${PULL_REQUEST_MERGE_COMMIT} Hash of merged commit (only available for merged-event).

src/main/java/se/bjurr/prnfs/listener/PrnfsPullRequestEventListener.java

@@ -170,9 +170,9 @@ public void notify(final PrnfsNotification notification, PrnfsPullRequestAction
 
   Optional<String> postContent = absent();
   if (notification.getPostContent().isPresent()) {
-   postContent = of(renderer.render(notification.getPostContent().get()));
+   postContent = of(renderer.render(notification.getPostContent().get(), FALSE));
   }
-  String renderedUrl = renderer.render(notification.getUrl());
+  String renderedUrl = renderer.render(notification.getUrl(), TRUE);
   logger.info(notification.getName() + " > " //
     + pullRequest.getFromRef().getId() + "(" + pullRequest.getFromRef().getLatestChangeset() + ") -> " //
     + pullRequest.getToRef().getId() + "(" + pullRequest.getToRef().getLatestChangeset() + ")" + " " //
@@ -185,7 +185,7 @@ public void notify(final PrnfsNotification notification, PrnfsPullRequestAction
 
   for (Header header : notification.getHeaders()) {
    urlInvoker//
-     .withHeader(header.getName(), renderer.render(header.getValue()));
+     .withHeader(header.getName(), renderer.render(header.getValue(), FALSE));
   }
   invoker.invoke(urlInvoker//
     .withProxyServer(notification.getProxyServer()) //
@@ -201,8 +201,8 @@ public boolean notificationTriggeredByAction(PrnfsNotification notification, Prn
   }
   if (notification.getFilterRegexp().isPresent()
     && notification.getFilterString().isPresent()
-    && !compile(notification.getFilterRegexp().get()).matcher(renderer.render(notification.getFilterString().get()))
-      .find()) {
+    && !compile(notification.getFilterRegexp().get()).matcher(
+      renderer.render(notification.getFilterString().get(), FALSE)).find()) {
    return FALSE;
   }
 

src/main/java/se/bjurr/prnfs/listener/PrnfsRenderer.java

@@ -1,14 +1,18 @@
 package se.bjurr.prnfs.listener;
 
+import static com.google.common.base.Charsets.UTF_8;
+import static com.google.common.base.Throwables.propagate;
 import static com.google.common.collect.Iterables.filter;
 import static com.google.common.collect.Lists.newArrayList;
+import static java.net.URLEncoder.encode;
 import static java.util.logging.Logger.getLogger;
 import static java.util.regex.Pattern.compile;
 import static se.bjurr.prnfs.listener.PrnfsRenderer.REPO_PROTOCOL.http;
 import static se.bjurr.prnfs.listener.PrnfsRenderer.REPO_PROTOCOL.ssh;
 import static se.bjurr.prnfs.listener.UrlInvoker.urlInvoker;
 import static se.bjurr.prnfs.listener.UrlInvoker.HTTP_METHOD.GET;
 
+import java.io.UnsupportedEncodingException;
 import java.util.Map;
 import java.util.Set;
 import java.util.logging.Logger;
@@ -434,12 +438,17 @@ public PrnfsRenderer(PullRequest pullRequest, PrnfsPullRequestAction pullRequest
   this.variables = variables;
  }
 
- public String render(String string) {
+ public String render(String string, Boolean forUrl) {
   for (final PrnfsVariable variable : PrnfsVariable.values()) {
    final String regExpStr = "\\$\\{" + variable.name() + "\\}";
    if (string.contains(regExpStr.replaceAll("\\\\", ""))) {
-    string = string.replaceAll(regExpStr, variable.resolve(pullRequest, pullRequestAction, stashUser,
-      repositoryService, propertiesService, prnfsNotification, variables));
+    try {
+     String resolved = variable.resolve(pullRequest, pullRequestAction, stashUser, repositoryService,
+       propertiesService, prnfsNotification, variables);
+     string = string.replaceAll(regExpStr, forUrl ? encode(resolved, UTF_8.name()) : resolved);
+    } catch (UnsupportedEncodingException e) {
+     propagate(e);
+    }
    }
   }
   return string;

src/test/java/se/bjurr/prnfs/admin/PrnfsPullRequestEventListenerTest.java

@@ -292,7 +292,7 @@ public void testThatAUrlWithVariablesFromAndToCanBeInvoked() throws Exception {
          .getPullRequest() //
      ) //
      .triggerButton("The Button") //
-     .invokedUrl(1, "http://bjurr.se/10The%20Button%20Text");
+     .invokedUrl(1, "http://bjurr.se/10The+Button+Text");
   }
  }
 
@@ -336,7 +336,7 @@ public void testThatAUrlWithCommentVariableHasSpacesReplaced() throws Exception
         .withCommentText("a text with\nnewline") //
         .build() //
     ) //
-    .invokedUrl(0, "http://bjurr.se/a%20text%20with%20newline");
+    .invokedUrl(0, "http://bjurr.se/a+text+with%0Anewline");
  }
 
  @Test
@@ -361,7 +361,7 @@ public void testThatAUrlWithVariableFromBranchCanBeInvokedWhenBranchIdContainsSl
         .withPullRequestAction(OPENED) //
         .build() //
     ) //
-    .invokedUrl(0, "http://bjurr.se/feature/branchmodmerge");
+    .invokedUrl(0, "http://bjurr.se/feature%2Fbranchmodmerge");
  }
 
  @Test
@@ -429,7 +429,7 @@ public void testThatAUrlWithVariablesExceptFromAndToCanBeInvoked() throws Except
     ) //
     .invokedUrl(
       0,
-      "http://bjurr.se/id=10&action=OPENED&displayName=authorDisplayName&authorEmail=authorEmail&authorId=100&authorName=authorName&authorSlug=authorSlug&pullRequestUrl=http://stash.server/projects/theProject/repos/theRepoName/pull-requests/10" //
+      "http://bjurr.se/id=10&action=OPENED&displayName=authorDisplayName&authorEmail=authorEmail&authorId=100&authorName=authorName&authorSlug=authorSlug&pullRequestUrl=http%3A%2F%2Fstash.server%2Fprojects%2FtheProject%2Frepos%2FtheRepoName%2Fpull-requests%2F10" //
     );
  }
 
@@ -892,7 +892,7 @@ public void testThatFilterCanBeUsedWithComments() throws Exception {
         .withPullRequestAction(COMMENTED) //
         .build() //
     ) //
-    .invokedUrl(0, "http://bjurr.se/?comment=keyword%20A%20nice%20comment&version=0"); //
+    .invokedUrl(0, "http://bjurr.se/?comment=keyword+A+nice+comment&version=0"); //
  }
 
  @Test
@@ -925,7 +925,7 @@ public void testThatFilterCanBeUsedWithCommentsAndSpecialEscapedChars() throws E
         .withPullRequestAction(COMMENTED) //
         .build() //
     ) //
-    .invokedUrl(0, "http://bjurr.se/?comment=%20keyword%20&version=0");
+    .invokedUrl(0, "http://bjurr.se/?comment=+keyword+&version=0");
  }
 
  @Test
@@ -1433,7 +1433,7 @@ public void testThatButtonCanBeUsedForTriggeringEvent() throws Exception {
     ) //
     .store() //
     .triggerButton("Button Form") //
-    .invokedOnlyUrl("http://bjurr.se/Trigger%20notification") //
+    .invokedOnlyUrl("http://bjurr.se/Trigger+notification") //
     .hasButtonsEnabled("Button Form");
  }
 
@@ -1515,7 +1515,7 @@ public void testThatEventTriggeredByButtonCanBeFiltered() throws Exception {
     ) //
     .store() //
     .triggerButton("Button Form") //
-    .invokedOnlyUrl("http://bjurr.se/?123=button%20text%20123");
+    .invokedOnlyUrl("http://bjurr.se/?123=button+text+123");
  }
 
  @Test
@@ -1550,7 +1550,7 @@ public void testThatEventTriggeredByButtonCanBeIgnoredByFilterWhileAnotherIsNotI
     ) //
     .store() //
     .triggerButton("Button Form") //
-    .invokedOnlyUrl("http://bjurr.se/?123=button%20text%20123");
+    .invokedOnlyUrl("http://bjurr.se/?123=button+text+123");
  }
 
  @Test
@@ -1581,8 +1581,8 @@ public void testThatEventTriggeredByButtonCanResultInSeveralNotifications() thro
         .withFieldValue(button_visibility, EVERYONE.name()).build()) //
     .store() //
     .triggerButton("Button Form") //
-    .invokedUrl(0, "http://bjurr.se/?123=button%20text%20123") //
-    .invokedUrl(1, "http://bjurr.se/?456=button%20text%20123");
+    .invokedUrl(0, "http://bjurr.se/?123=button+text+123") //
+    .invokedUrl(1, "http://bjurr.se/?456=button+text+123");
  }
 
  @Test
@@ -1616,9 +1616,9 @@ public void testThatThereCanBeSeveralButtons() throws Exception {
     ) //
     .store() //
     .triggerButton("Button Form 1") //
-    .invokedUrl(0, "http://bjurr.se/?button%20text%201") //
+    .invokedUrl(0, "http://bjurr.se/?button+text+1") //
     .triggerButton("Button Form 2") //
-    .invokedUrl(1, "http://bjurr.se/?button%20text%202");
+    .invokedUrl(1, "http://bjurr.se/?button+text+2");
  }
 
  @Test
@@ -1640,7 +1640,7 @@ public void testThatValueFromUrlCanBeUsedInInvocation() throws Exception {
         .withPullRequestAction(OPENED) //
         .build() //
     ) //
-    .invokedOnlyUrl("http://bjurr.se/?some%20content");
+    .invokedOnlyUrl("http://bjurr.se/?some+content");
  }
 
  @Test