SLING-13144: PostServlet with space in :redirect (#34)

Co-authored-by: dirk.tschentscher <orx0815@users.noreply.github.com>

Author: orx0815

src/main/java/org/apache/sling/servlets/post/impl/SlingPostServlet.java

@@ -343,7 +343,19 @@ boolean redirectIfNeeded(
             final JakartaPostResponse htmlResponse,
             final SlingJakartaHttpServletResponse response)
             throws IOException {
-        final String redirectURL = getRedirectUrl(request, htmlResponse);
+        final String redirectURL = getRedirectUrl(request, htmlResponse, response);
+        if (redirectURL != null) {
+            log.debug("redirecting to URL [{}]", redirectURL);
+            response.sendRedirect(redirectURL);
+            return true;
+        }
+        return false;
+    }
+
+    private String encodeRedirectUrl(
+            final String redirectURL,
+            final SlingJakartaHttpServletResponse response,
+            final SlingJakartaHttpServletRequest request) {
         if (redirectURL != null) {
             final Matcher m = REDIRECT_WITH_SCHEME_PATTERN.matcher(redirectURL);
             final boolean hasScheme = m.matches();
@@ -356,11 +368,9 @@ boolean redirectIfNeeded(
                 log.debug("Request path is [{}]", request.getPathInfo());
                 encodedURL = response.encodeRedirectURL(redirectURL);
             }
-            log.debug("redirecting to URL [{}] - encoded as [{}]", redirectURL, encodedURL);
-            response.sendRedirect(encodedURL);
-            return true;
+            return encodedURL;
         }
-        return false;
+        return null;
     }
 
     private static final Pattern REDIRECT_WITH_SCHEME_PATTERN = Pattern.compile("^(https?://[^/]+)(.*)$");
@@ -441,12 +451,16 @@ private JakartaPostOperation getSlingPostOperation(final SlingJakartaHttpServlet
      * @param ctx the post processor
      * @return the redirect location or <code>null</code>
      */
-    protected String getRedirectUrl(final SlingJakartaHttpServletRequest request, final JakartaPostResponse ctx) {
+    private String getRedirectUrl(
+            final SlingJakartaHttpServletRequest request,
+            final JakartaPostResponse ctx,
+            SlingJakartaHttpServletResponse response) {
         // redirect param has priority (but see below, magic star)
         String result = request.getParameter(SlingPostConstants.RP_REDIRECT_TO);
         if (result != null) {
             try {
-                URI redirectUri = new URI(result);
+                String encodedURL = encodeRedirectUrl(result, response, request);
+                URI redirectUri = new URI(encodedURL);
                 if (redirectUri.getAuthority() != null) {
                     // if it has a host information
                     log.warn(
@@ -462,42 +476,48 @@ protected String getRedirectUrl(final SlingJakartaHttpServletRequest request, fi
 
             log.debug("redirect requested as [{}] for path [{}]", result, ctx.getPath());
 
-            // redirect to created/modified Resource
-            final int star = result.indexOf('*');
-            if (star >= 0 && ctx.getPath() != null) {
-                final StringBuilder buf = new StringBuilder();
+            result = handleStarResource(result, ctx, request);
 
-                // anything before the star
-                if (star > 0) {
-                    buf.append(result.substring(0, star));
-                }
+            log.debug("Will redirect to {}", result);
+        }
+        return encodeRedirectUrl(result, response, request);
+    }
 
-                // append the name of the manipulated node
-                buf.append(ResourceUtil.getName(ctx.getPath()));
+    private String handleStarResource(
+            String result, final JakartaPostResponse ctx, final SlingJakartaHttpServletRequest request) {
+        // redirect to created/modified Resource
+        final int star = result.indexOf('*');
+        if (star >= 0 && ctx.getPath() != null) {
+            final StringBuilder buf = new StringBuilder();
 
-                // anything after the star
-                if (star < result.length() - 1) {
-                    buf.append(result.substring(star + 1));
-                }
+            // anything before the star
+            if (star > 0) {
+                buf.append(result.substring(0, star));
+            }
 
-                // Prepend request path if it ends with create suffix and result isn't absolute
-                final String requestPath = request.getPathInfo();
-                if (requestPath.endsWith(SlingPostConstants.DEFAULT_CREATE_SUFFIX)
-                        && buf.charAt(0) != '/'
-                        && !REDIRECT_WITH_SCHEME_PATTERN.matcher(buf).matches()) {
-                    buf.insert(0, requestPath);
-                }
+            // append the name of the manipulated node
+            buf.append(ResourceUtil.getName(ctx.getPath()));
 
-                // use the created path as the redirect result
-                result = buf.toString();
+            // anything after the star
+            if (star < result.length() - 1) {
+                buf.append(result.substring(star + 1));
+            }
 
-            } else if (result.endsWith(SlingPostConstants.DEFAULT_CREATE_SUFFIX)) {
-                // if the redirect has a trailing slash, append modified node
-                // name
-                result = result.concat(ResourceUtil.getName(ctx.getPath()));
+            // Prepend request path if it ends with create suffix and result isn't absolute
+            final String requestPath = request.getPathInfo();
+            if (requestPath.endsWith(SlingPostConstants.DEFAULT_CREATE_SUFFIX)
+                    && buf.charAt(0) != '/'
+                    && !REDIRECT_WITH_SCHEME_PATTERN.matcher(buf).matches()) {
+                buf.insert(0, requestPath);
             }
 
-            log.debug("Will redirect to {}", result);
+            // use the created path as the redirect result
+            result = buf.toString();
+
+        } else if (result.endsWith(SlingPostConstants.DEFAULT_CREATE_SUFFIX)) {
+            // if the redirect has a trailing slash, append modified node
+            // name
+            result = result.concat(ResourceUtil.getName(ctx.getPath()));
         }
         return result;
     }

src/test/java/org/apache/sling/servlets/post/impl/SlingPostServletTest.java

@@ -19,11 +19,12 @@
 package org.apache.sling.servlets.post.impl;
 
 import java.io.IOException;
-import java.io.UnsupportedEncodingException;
-import java.net.URLEncoder;
 import java.util.StringTokenizer;
 
 import junit.framework.TestCase;
+import org.apache.commons.httpclient.URIException;
+import org.apache.commons.httpclient.util.URIUtil;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.sling.api.SlingJakartaHttpServletRequest;
 import org.apache.sling.api.request.builder.Builders;
 import org.apache.sling.api.request.header.JakartaMediaRangeList;
@@ -190,6 +191,9 @@ public void testRedirection() throws Exception {
         testRedirection("/", "/fred/abc", "https://forced.com/test", null);
         // invalid URI
         testRedirection("/", "/fred/abc", "file://c:\\Users\\workspace\\test.java", null);
+
+        // test redirect with spaces in path
+        testRedirection("/", "/", "/my space.html?q=hello+space", "/my%20space.html?q=hello+space");
     }
 
     private void testRedirection(String requestPath, String resourcePath, String redirect, String expected)
@@ -236,21 +240,28 @@ public RedirectServletResponse() {
 
         @Override
         public String encodeRedirectURL(String s) {
-            StringTokenizer st = new StringTokenizer(s, "/", true);
+
+            String pathPart = StringUtils.substringBefore(s, "?");
+            StringTokenizer st = new StringTokenizer(pathPart, "/", true);
             StringBuilder sb = new StringBuilder();
             try {
                 while (st.hasMoreTokens()) {
                     String token = st.nextToken();
                     if ("/".equals(token)) {
                         sb.append(token);
                     } else {
-                        sb.append(URLEncoder.encode(token, "UTF-8"));
+                        // URLEncoder would replace ' ' with '+'. Needs to be '%20' as the real wrapper does too
+                        sb.append(URIUtil.encodeWithinPath(token, "UTF-8"));
                     }
                 }
-            } catch (UnsupportedEncodingException e) {
+            } catch (URIException e) {
                 fail("Should have UTF-8?? " + e);
                 return null;
             }
+            String queryPart = StringUtils.substringAfter(s, "?");
+            if (StringUtils.isNotEmpty(queryPart)) {
+                sb.append("?").append(queryPart);
+            }
             return sb.toString();
         }