[SECURITY-1815]

Author: PierreBtz

src/main/java/hudson/plugins/audit_trail/AuditTrailFilter.java

@@ -34,6 +34,9 @@
 import javax.servlet.*;
 import javax.servlet.http.HttpServletRequest;
 import java.io.IOException;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
 import java.util.logging.Level;
 import java.util.logging.Logger;
 import java.util.regex.Pattern;
@@ -79,14 +82,7 @@ static void setPattern(String pattern) throws PatternSyntaxException {
     public void doFilter(ServletRequest request, ServletResponse res, FilterChain chain)
           throws IOException, ServletException {
         HttpServletRequest req = (HttpServletRequest) request;
-        String uri;
-        if (req.getPathInfo() == null) {
-            // workaround: on some containers such as CloudBees DEV@cloud, req.getPathInfo() is unexpectedly null,
-            // construct pathInfo based on contextPath and requestUri
-            uri = req.getRequestURI().substring(req.getContextPath().length());
-        } else {
-            uri = req.getPathInfo();
-        }
+        String uri = getPathInfo(req);
         if (uriPattern != null && uriPattern.matcher(uri).matches()) {
             User user = User.current();
             String username = user != null ? user.getId() : req.getRemoteAddr();
@@ -151,4 +147,45 @@ private void onRequest(String uri, String extra, String username) {
             }
         }
     }
+
+    // See SECURITY-1815
+    private static String getPathInfo(HttpServletRequest request) {
+        return canonicalPath(request.getRequestURI().substring(request.getContextPath().length()));
+    }
+
+    // Copied from Stapler#canonicalPath
+    private static String canonicalPath(String path) {
+        List<String> r = new ArrayList<>(Arrays.asList(path.split("/+")));
+        for (int i = 0; i < r.size(); ) {
+            if (r.get(i).length() == 0 || r.get(i).equals(".")) {
+                // empty token occurs for example, "".split("/+") is [""]
+                r.remove(i);
+            } else if (r.get(i).equals("..")) {
+                // i==0 means this is a broken URI.
+                r.remove(i);
+                if (i > 0) {
+                    r.remove(i - 1);
+                    i--;
+                }
+            } else {
+                i++;
+            }
+        }
+
+        StringBuilder buf = new StringBuilder();
+        if (path.startsWith("/")) {
+            buf.append('/');
+        }
+        boolean first = true;
+        for (String token : r) {
+            if (!first) buf.append('/');
+            else first = false;
+            buf.append(token);
+        }
+        // translation: if (path.endsWith("/") && !buf.endsWith("/"))
+        if (path.endsWith("/") && (buf.length() == 0 || buf.charAt(buf.length() - 1) != '/')) {
+            buf.append('/');
+        }
+        return buf.toString();
+    }
 }

src/test/java/hudson/plugins/audit_trail/AuditTrailFilterTest.java

@@ -6,9 +6,11 @@
 import hudson.Util;
 import hudson.model.Cause;
 import hudson.model.FreeStyleProject;
+import org.apache.http.HttpStatus;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 
 import java.io.File;
@@ -49,4 +51,30 @@ public void cancelItemLogsTheQueryStringAndTheUser() throws Exception {
         String log = Util.loadFile(new File(tmpDir.getRoot(), "test.log.0"), StandardCharsets.UTF_8);
         assertTrue("logged actions: " + log, Pattern.compile(".*id=1.*job/test-job.*by \\Q127.0.0.1\\E.*", Pattern.DOTALL).matcher(log).matches());
     }
+
+    @Test
+    @Issue("SECURITY-1815")
+    public void requestWithSemiColumnIsProperlyLogged() throws Exception {
+        String logFileName = "security-1815.log";
+        File logFile = new File(tmpDir.getRoot(), logFileName);
+        JenkinsRule.WebClient wc = j.createWebClient();
+        new SimpleAuditTrailPluginConfiguratorHelper(logFile).sendConfiguration(j, wc);
+
+        WebRequest request = new WebRequest(new URL(wc.getContextPath()+ "quietDown/..;/") , HttpMethod.POST);
+        wc.addCrumb(request);
+
+        try {
+            wc.getPage(request);
+        } catch (FailingHttpStatusCodeException e) {
+            if(e.getStatusCode() != HttpStatus.SC_METHOD_NOT_ALLOWED) {
+                // when the plugin is moved to a Core implementing SECURITY-1815 this request will start returning with
+                // a 400 error code. Voluntarily rethrowing to have this fail,
+                // because this failing test will be the time to reconsider removing this specific dev
+                throw e;
+            }
+            // otherwise silently ignore, the endpoint returns a 405
+        }
+        String log = Util.loadFile(new File(tmpDir.getRoot(), logFileName + ".0"), StandardCharsets.UTF_8);
+        assertTrue("logged actions: " + log, log.contains("quietDown"));
+    }
 }

src/test/java/hudson/plugins/audit_trail/SimpleAuditTrailPluginConfiguratorHelper.java

@@ -25,7 +25,7 @@ public class SimpleAuditTrailPluginConfiguratorHelper {
     private final File logFile;
 
     private boolean logBuildCause =true;
-    private String pattern = ".*/(?:enable|cancelItem)";
+    private String pattern = ".*/(?:enable|cancelItem|quietDown)/?.*";
 
     public SimpleAuditTrailPluginConfiguratorHelper(File logFile) {
         this.logFile = logFile;