Merge pull request #4302 from daniel-beck/JENKINS-59849

[JENKINS-59849] Don't fail to serve resource files with nontrivial names

Author: daniel-beck

core/src/main/java/jenkins/security/ResourceDomainRootAction.java

@@ -23,6 +23,7 @@
  */
 package jenkins.security;
 
+import com.google.common.annotations.VisibleForTesting;
 import hudson.Extension;
 import hudson.ExtensionList;
 import hudson.Util;
@@ -51,6 +52,7 @@
 import java.util.Base64;
 import java.util.logging.Level;
 import java.util.logging.Logger;
+import java.util.stream.Collectors;
 
 import static java.time.Instant.*;
 import static java.time.temporal.ChronoUnit.MINUTES;
@@ -147,7 +149,7 @@ public String getRedirectUrl(@Nonnull Token token, @Nonnull String restOfPath) {
             // Unsure whether this can happen -- just be safe here
             restOfPath = "/" + restOfPath;
         }
-        return resourceRootUrl + getUrlName() + "/" + token.encode() + restOfPath;
+        return resourceRootUrl + getUrlName() + "/" + token.encode() + Arrays.stream(restOfPath.split("[/]")).map(Util::rawEncode).collect(Collectors.joining("/"));
     }
 
     private static String getResourceRootUrl() {
@@ -165,7 +167,7 @@ private static String getResourceRootUrl() {
     @CheckForNull
     public Token getToken(@Nonnull DirectoryBrowserSupport dbs, @Nonnull StaplerRequest req) {
         // This is the "restOfPath" of the DirectoryBrowserSupport, i.e. the directory/file/pattern "inside" the DBS.
-        final String dbsFile = req.getRestOfPath();
+        final String dbsFile = req.getOriginalRestOfPath();
 
         // Now get the 'restOfUrl' after the top-level ancestor (which is the Jenkins singleton).
         // In other words, this is the complete URL after Jenkins handled the top-level request.
@@ -222,7 +224,8 @@ public void doDynamic(StaplerRequest req, StaplerResponse rsp) throws IOExceptio
 
             try (ACLContext ignored = ACL.as(auth)) {
                 try {
-                    Stapler.getCurrent().invoke(req, rsp, Jenkins.get(), requestUrlSuffix + restOfPath);
+                    String path = requestUrlSuffix + Arrays.stream(restOfPath.split("[/]")).map(Util::rawEncode).collect(Collectors.joining("/"));
+                    Stapler.getCurrent().invoke(req, rsp, Jenkins.get(), path);
                 } catch (Exception ex) {
                     // cf. UnwrapSecurityExceptionFilter
                     Throwable cause = ex.getCause();
@@ -263,7 +266,9 @@ public static class Token {
         private String path;
         private String username;
         private Instant timestamp;
-        private Token (String path, @Nullable String username, Instant timestamp) {
+
+        @VisibleForTesting
+        Token (String path, @Nullable String username, Instant timestamp) {
             this.path = path;
             this.username = Util.fixNull(username);
             this.timestamp = timestamp;
@@ -277,8 +282,8 @@ private String encode() {
         }
 
         private static Token decode(String value) {
-            byte[] byteValue = Base64.getUrlDecoder().decode(value);
             try {
+                byte[] byteValue = Base64.getUrlDecoder().decode(value);
                 byte[] mac = Arrays.copyOf(byteValue, 32);
                 byte[] restBytes = Arrays.copyOfRange(byteValue, 32, byteValue.length);
                 String rest = new String(restBytes, StandardCharsets.UTF_8);

test/src/test/java/jenkins/security/ResourceDomainTest.java

@@ -3,9 +3,11 @@
 import com.gargoylesoftware.htmlunit.Page;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
 import hudson.ExtensionList;
+import hudson.FilePath;
 import hudson.model.DirectoryBrowserSupport;
 import hudson.model.FreeStyleProject;
 import hudson.model.Item;
+import hudson.model.UnprotectedRootAction;
 import jenkins.model.Jenkins;
 import jenkins.model.JenkinsLocationConfiguration;
 import org.junit.Assert;
@@ -17,8 +19,12 @@
 import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.MockAuthorizationStrategy;
+import org.jvnet.hudson.test.TestExtension;
+import org.kohsuke.stapler.HttpResponse;
 
+import javax.annotation.CheckForNull;
 import java.net.URL;
+import java.time.Instant;
 import java.util.UUID;
 
 @Issue("JENKINS-41891")
@@ -279,4 +285,88 @@ public void adminMonitorShowsUpWithOverriddenCSP() throws Exception {
         }
         Assert.assertFalse(monitor.isActivated());
     }
+
+    @Test
+    public void testRedirectUrls() throws Exception {
+        ResourceDomainRootAction rootAction = ResourceDomainRootAction.get();
+        String url = rootAction.getRedirectUrl(new ResourceDomainRootAction.Token("foo", "bar", Instant.now()), "foo bar baz");
+        Assert.assertFalse("urlencoded", url.contains(" "));
+    }
+
+    @Test
+    @Issue("JENKINS-59849")
+    public void testUrlEncoding() throws Exception {
+        FreeStyleProject project = j.createFreeStyleProject();
+        project.getBuildersList().add(new CreateFileBuilder("This has spaces and is 100% evil.html", "<html><body>the content</body></html>"));
+        project.save();
+
+        j.buildAndAssertSuccess(project);
+
+        JenkinsRule.WebClient webClient = j.createWebClient();
+        webClient.setThrowExceptionOnFailingStatusCode(false);
+        webClient.setRedirectEnabled(true);
+
+        HtmlPage page = webClient.getPage(project, "ws/This%20has%20spaces%20and%20is%20100%25%20evil.html");
+        Assert.assertEquals("page is found", 200, page.getWebResponse().getStatusCode());
+        Assert.assertTrue("page content is as expected", page.getWebResponse().getContentAsString().contains("the content"));
+
+        URL url = page.getUrl();
+        Assert.assertTrue("page is served by resource domain", url.toString().contains("/static-files/"));
+    }
+
+    @Test
+    @Issue("JENKINS-59849")
+    public void testMoreUrlEncoding() throws Exception {
+        JenkinsRule.WebClient webClient = j.createWebClient();
+        webClient.setThrowExceptionOnFailingStatusCode(false);
+        webClient.setRedirectEnabled(true);
+
+        Page page = webClient.goTo("100%25%20evil/%20100%25%20evil%20dir%20name%20%20%20/%20100%25%20evil%20content%20.html");
+        Assert.assertEquals("page is found", 200, page.getWebResponse().getStatusCode());
+        Assert.assertTrue("page content is as expected", page.getWebResponse().getContentAsString().contains("this is the content"));
+
+        URL url = page.getUrl();
+        Assert.assertTrue("page is served by resource domain", url.toString().contains("/static-files/"));
+
+        URL dirUrl = new URL(url.toString().replace("%20100%25%20evil%20content%20.html", ""));
+        Page dirPage = webClient.getPage(dirUrl);
+        Assert.assertEquals("page is found", 200, dirPage.getWebResponse().getStatusCode());
+        Assert.assertTrue("page content is HTML", dirPage.getWebResponse().getContentAsString().contains("href"));
+        Assert.assertTrue("page content references file", dirPage.getWebResponse().getContentAsString().contains("evil content"));
+
+        URL topDirUrl = new URL(url.toString().replace("%20100%25%20evil%20dir%20name%20%20%20/%20100%25%20evil%20content%20.html", ""));
+        Page topDirPage = webClient.getPage(topDirUrl);
+        Assert.assertEquals("page is found", 200, topDirPage.getWebResponse().getStatusCode());
+        Assert.assertTrue("page content is HTML", topDirPage.getWebResponse().getContentAsString().contains("href"));
+        Assert.assertTrue("page content references directory", topDirPage.getWebResponse().getContentAsString().contains("evil dir name"));
+    }
+
+    @TestExtension
+    public static class RootActionImpl implements UnprotectedRootAction {
+
+        @CheckForNull
+        @Override
+        public String getIconFileName() {
+            return null;
+        }
+
+        @CheckForNull
+        @Override
+        public String getDisplayName() {
+            return null;
+        }
+
+        @CheckForNull
+        @Override
+        public String getUrlName() {
+            return "100% evil";
+        }
+
+        public HttpResponse doDynamic() throws Exception {
+            Jenkins jenkins = Jenkins.get();
+            FilePath tempDir = jenkins.getRootPath().createTempDir("root", "tmp");
+            tempDir.child(" 100% evil dir name   ").child(" 100% evil content .html").write("this is the content", "UTF-8");
+            return new DirectoryBrowserSupport(jenkins, tempDir, "title", "", true);
+        }
+    }
 }