XWIKI-11528: Office viewer fails if the page where the file is attached has a...

XWIKI-11528: Office viewer fails if the page where the file is attached has a special character in the name (whitespace)
* Ensure the path to the temporary file is fully encoded (canonical form) even if the URL used to access the file is partially decoded (which can happen for instance when XWiki is behind Apache's mode_proxy with nocanon option disabled).

xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/web/TempResourceAction.java

@@ -25,11 +25,14 @@
 import java.net.URI;
 import java.net.URLDecoder;
 import java.net.URLEncoder;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 
 import org.apache.commons.io.FileUtils;
 import org.apache.commons.io.IOUtils;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.tika.Tika;
 import org.apache.tika.mime.MimeTypes;
 import org.slf4j.Logger;
@@ -64,6 +67,11 @@ public class TempResourceAction extends XWikiAction
      */
     public static final Pattern URI_PATTERN = Pattern.compile(".*?/temp/([^/]*+)/([^/]*+)/([^/]*+)/(.*+)");
 
+    /**
+     * The path separator.
+     */
+    private static final String PATH_SEPARATOR = "/";
+
     /**
      * The URL encoding.
      */
@@ -131,21 +139,25 @@ protected File getTemporaryFile(String uri, XWikiContext context)
         Matcher matcher = URI_PATTERN.matcher(uri);
         File result = null;
         if (matcher.find()) {
-            String wiki = context.getWikiId();
-            try {
-                wiki = URLEncoder.encode(wiki, URL_ENCODING);
-            } catch (UnsupportedEncodingException e) {
-                // This should never happen;
+            List<String> pathSegments = new ArrayList<String>();
+            // Add all the path segments.
+            pathSegments.add("temp");
+            // temp/module
+            pathSegments.add(withMinimalURLEncoding(matcher.group(3)));
+            // temp/module/wiki
+            pathSegments.add(encodeURLPathSegment(context.getWikiId()));
+            // temp/module/wiki/space
+            pathSegments.add(withMinimalURLEncoding(matcher.group(1)));
+            // temp/module/wiki/space/page
+            pathSegments.add(withMinimalURLEncoding(matcher.group(2)));
+            // Save the path prefix before adding the file path to be able to check if the file path tries to get out of
+            // the parent folder (e.g. using '/../').
+            String prefix = StringUtils.join(pathSegments, PATH_SEPARATOR);
+            // temp/module/wiki/space/page/path/to/file.tmp
+            for (String filePathSegment : matcher.group(4).split(PATH_SEPARATOR)) {
+                pathSegments.add(withMinimalURLEncoding(filePathSegment));
             }
-            String space = withMinimalURLEncoding(matcher.group(1));
-            String page = withMinimalURLEncoding(matcher.group(2));
-            String module = withMinimalURLEncoding(matcher.group(3));
-            // The file path is used as is, without any modifications (no decoding/encoding is performed). The modules
-            // that create the temporary files and the corresponding URLs used to access them are responsible for
-            // encoding the file path components so that they don't contain invalid characters.
-            String filePath = matcher.group(4);
-            String prefix = String.format("temp/%s/%s/%s/%s/", module, wiki, space, page);
-            String path = URI.create(prefix + filePath).normalize().toString();
+            String path = URI.create(StringUtils.join(pathSegments, PATH_SEPARATOR)).normalize().toString();
             if (path.startsWith(prefix)) {
                 result = new File(this.environment.getTemporaryDirectory(), path);
                 result = result.exists() ? result : null;
@@ -157,17 +169,36 @@ protected File getTemporaryFile(String uri, XWikiContext context)
     /**
      * Keeps only minimal URL encoding. Currently, XWiki's URL factory over encodes the URLs in order to protect them
      * from XWiki 1.0 syntax parser.
+     * <p>
+     * This method also ensures that the path to the temporary file is fully encoded (has the canonical form) even if
+     * the URL used to access the file is partially decoded (which can happen for instance when XWiki is behind Apache's
+     * {@code mode_proxy} with {@code nocanon} option disabled).
      * 
-     * @param component a URL component
+     * @param encodedPathSegment an encoded URL path segment
      * @return the given string with minimal URL encoding
      */
-    private String withMinimalURLEncoding(String component)
+    private String withMinimalURLEncoding(String encodedPathSegment)
+    {
+        return encodeURLPathSegment(decodeURLPathSegment(encodedPathSegment));
+    }
+
+    private String encodeURLPathSegment(String segment)
+    {
+        try {
+            return URLEncoder.encode(segment, URL_ENCODING);
+        } catch (UnsupportedEncodingException e) {
+            // This should never happen.
+            return segment;
+        }
+    }
+
+    private String decodeURLPathSegment(String encodedSegment)
     {
         try {
-            return URLEncoder.encode(URLDecoder.decode(component, URL_ENCODING), URL_ENCODING);
+            return URLDecoder.decode(encodedSegment, URL_ENCODING);
         } catch (UnsupportedEncodingException e) {
             // This should never happen.
-            return component;
+            return encodedSegment;
         }
     }
 }

xwiki-platform-core/xwiki-platform-oldcore/src/test/java/com/xpn/xwiki/web/TempResourceActionTest.java

@@ -22,9 +22,8 @@
 import java.io.File;
 import java.io.IOException;
 
-import org.junit.Assert;
-
 import org.jmock.Expectations;
+import org.junit.Assert;
 import org.junit.Test;
 import org.xwiki.context.Execution;
 import org.xwiki.context.ExecutionContext;
@@ -32,6 +31,7 @@
 import org.xwiki.environment.Environment;
 import org.xwiki.environment.internal.ServletEnvironment;
 
+import com.xpn.xwiki.XWikiContext;
 import com.xpn.xwiki.test.AbstractBridgedComponentTestCase;
 
 /**
@@ -101,6 +101,7 @@ private void createEmptyFile(String path) throws IOException
         File emptyFile = new File(base, path);
         emptyFile.getParentFile().mkdirs();
         emptyFile.createNewFile();
+        emptyFile.deleteOnExit();
     }
 
     /**
@@ -156,4 +157,17 @@ public void testGetTemporaryFileForOverEncodedURL() throws Exception
         Assert.assertNotNull(action.getTemporaryFile(
             "/xwiki/bin/temp/Sp%2Aace/Pa%2Dge/officeviewer/presentation.odp/presentation-slide0.jpg", getContext()));
     }
+
+    /**
+     * Tests {@link TempResourceAction#getTemporaryFile(String, XWikiContext)} when the URL is partially decoded. This
+     * can happen for instance when XWiki is behind Apache's {@code mode_proxy} with {@code nocanon} option disabled.
+     */
+    @Test
+    public void testGetTemporaryFileForPartiallyDecodedURL() throws Exception
+    {
+        createEmptyFile("temp/officeviewer/xwiki/Space/Page/"
+            + "attach%3Axwiki%3ASpace.Page%40pres%2Fentation.odp/13/presentation-slide0.jpg");
+        Assert.assertNotNull(action.getTemporaryFile("/xwiki/bin/temp/Space/Page/officeviewer/"
+            + "attach:xwiki:Space.Page@pres%2Fentation.odp/13/presentation-slide0.jpg", getContext()));
+    }
 }