org.xwiki.platform:xwiki-platform-wysiwyg-api Open Redirect vulnerability

@@ -97,4 +97,28 @@ default URI parseToSafeURI(String serializedURI) throws URISyntaxException, Secu
     {
         throw new SecurityException("Cannot guarantee safeness of " + serializedURI);
     }
+
+    /**
+     * Parse the given string to create a URI that is safe to use.
+     * This method throws a {@link SecurityException} if the parsed URI is not safe to use according to
+     * {@link #isURITrusted(URI)}. It might also throw a {@link URISyntaxException} if the parameter cannot be properly
+     * parsed.
+     * Note that this method might try to "repair" URI that are not parsed correctly by {@link URI#URI(String)}
+     * (e.g. serialized uri containing spaces).
+     *
+     * @param serializedURI a string representing a URI that needs to be parsed.
+     * @param requestHost the host the current request, this host will be added to the safe domains, when omitted this
+     * host is extracted from the context, this parameter exists for cases where the context is not available
+     * @return a URI safe to use
+     * @throws URISyntaxException if the given parameter cannot be properly parsed
+     * @throws SecurityException if the parsed URI is not safe according to {@link #isURITrusted(URI)}
+     * @since 16.8.0
+     * @since 16.4.4
+     * @since 15.10.13
+     */
+    @Unstable
+    default URI parseToSafeURI(String serializedURI, String requestHost) throws URISyntaxException, SecurityException
+    {
+        throw new SecurityException("Cannot guarantee that " + serializedURI + " is safe.");
+    }
 }

@@ -139,11 +139,7 @@ private String getCurrentDomain()
     public boolean isDomainTrusted(URL urlToCheck)
     {
         if (this.urlConfiguration.isTrustedDomainsEnabled()) {
-            if (this.trustedDomains == null) {
-                computeTrustedDomains();
-            }
-
-            this.trustedDomains.add(this.getCurrentDomain());
+            maybeInitializeWithDomain(this.getCurrentDomain());
             String host = urlToCheck.getHost();
 
             do {
@@ -171,6 +167,23 @@ public boolean isDomainTrusted(URL urlToCheck)
         }
     }
 
+    /**
+     * Initialize the trusted domains with the given domain as additional trusted domains if trusted domains are
+     * enabled.
+     *
+     * @param domain the domain to add to the trusted domains
+     */
+    private void maybeInitializeWithDomain(String domain)
+    {
+        if (this.urlConfiguration.isTrustedDomainsEnabled()) {
+            if (this.trustedDomains == null) {
+                computeTrustedDomains();
+            }
+
+            this.trustedDomains.add(domain);
+        }
+    }
+
     /**
      * Invalidate the set of trusted domains: this should mainly be used when a subwiki is added/edited/deleted.
      */
@@ -271,6 +284,14 @@ public URI parseToSafeURI(String serializedURI) throws URISyntaxException, Secur
         }
     }
 
+    @Override
+    public URI parseToSafeURI(String serializedURI, String requestHost) throws URISyntaxException, SecurityException
+    {
+        maybeInitializeWithDomain(requestHost);
+
+        return parseToSafeURI(serializedURI);
+    }
+
     /**
      * The goal of this method is to parse the given String and to replace all {@code %} character by an internal
      * replacement chain if and only if this {@code %} character belongs to a percent encoded byte.

@@ -513,4 +513,19 @@ void parseToSafeURI() throws URISyntaxException
             + "or try to use a different marker.", illegalArgumentException.getMessage());
         assertTrue(illegalArgumentException.getCause() instanceof URISyntaxException);
     }
+
+    @Test
+    void parseToSafeURIWithDomain() throws URISyntaxException
+    {
+        when(this.urlConfiguration.getTrustedSchemes()).thenReturn(List.of("https"));
+
+        String url = "https://www.example.com/path";
+        assertEquals(url, this.urlSecurityManager.parseToSafeURI(url, "www.example.com").toString());
+
+        SecurityException securityException = assertThrows(SecurityException.class,
+            () -> this.urlSecurityManager.parseToSafeURI("https://example.com",
+                "www.example.com"));
+
+        assertEquals("The given URI [https://example.com] is not safe on this server.", securityException.getMessage());
+    }
 }

@@ -20,6 +20,8 @@
 package org.xwiki.wysiwyg.internal.converter;
 
 import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.HashMap;
 import java.util.Map;
 import java.util.Optional;
@@ -35,6 +37,7 @@
 import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.xwiki.component.annotation.Component;
+import org.xwiki.url.URLSecurityManager;
 import org.xwiki.wysiwyg.converter.HTMLConverter;
 import org.xwiki.wysiwyg.converter.RequestParameterConversionResult;
 import org.xwiki.wysiwyg.converter.RequestParameterConverter;
@@ -81,6 +84,9 @@ public class DefaultRequestParameterConverter implements RequestParameterConvert
     @Inject
     private HTMLConverter htmlConverter;
 
+    @Inject
+    private URLSecurityManager urlSecurityManager;
+
     @Inject
     private Logger logger;
 
@@ -172,7 +178,23 @@ private void handleConversionErrors(RequestParameterConversionResult conversionR
         }
         // Save the output and the caught exceptions on the session.
         queryString += "key=" + save(conversionResult);
-        mutableRequest.sendRedirect(res, redirectURL + '?' + queryString);
+        String unsafeURL = redirectURL + '?' + queryString;
+        if (originalRequest instanceof HttpServletRequest) {
+            HttpServletRequest httpRequest = (HttpServletRequest) originalRequest;
+            try {
+                URI safeURI = this.urlSecurityManager.parseToSafeURI(unsafeURL, httpRequest.getServerName());
+                mutableRequest.sendRedirect(res, safeURI.toString());
+            } catch (URISyntaxException | SecurityException e) {
+                this.logger.warn(
+                    "Possible phishing attack, attempting to redirect to [{}], this request has been blocked. "
+                        + "If the request was legitimate, please check the URL security configuration. You "
+                        + "might need to add the domain related to this request in the list of trusted domains in "
+                        + "the configuration: it can be configured in xwiki.properties in url.trustedDomains.",
+                    unsafeURL);
+                this.logger.debug("Original error preventing the redirect: ", e);
+                ((HttpServletResponse) res).sendError(400, "The error redirect URI isn't considered safe.");
+            }
+        }
     }
 
     /**

@@ -0,0 +1,112 @@
+/*
+ * See the NOTICE file distributed with this work for additional
+ * information regarding copyright ownership.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU Lesser General Public License as
+ * published by the Free Software Foundation; either version 2.1 of
+ * the License, or (at your option) any later version.
+ *
+ * This software is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this software; if not, write to the Free
+ * Software Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA, or see the FSF site: http://www.fsf.org.
+ */
+package org.xwiki.wysiwyg.internal.converter;
+
+import java.io.IOException;
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.Map;
+import java.util.Optional;
+
+import javax.servlet.ServletRequest;
+import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletResponse;
+
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.RegisterExtension;
+import org.xwiki.test.LogLevel;
+import org.xwiki.test.annotation.ComponentList;
+import org.xwiki.test.junit5.LogCaptureExtension;
+import org.xwiki.test.junit5.mockito.ComponentTest;
+import org.xwiki.test.junit5.mockito.InjectMockComponents;
+import org.xwiki.test.junit5.mockito.MockComponent;
+import org.xwiki.url.URLSecurityManager;
+import org.xwiki.wysiwyg.converter.HTMLConverter;
+import org.xwiki.wysiwyg.internal.filter.http.MutableHttpServletRequestFactory;
+
+import ch.qos.logback.classic.spi.ILoggingEvent;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.startsWith;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+/**
+ * Unit test for {@link DefaultRequestParameterConverter}.
+ *
+ * @version $Id$
+ */
+@ComponentTest
+@ComponentList({ MutableHttpServletRequestFactory.class })
+class DefaultRequestParameterConverterTest
+{
+    @InjectMockComponents
+    private DefaultRequestParameterConverter converter;
+
+    @MockComponent
+    private HTMLConverter htmlConverter;
+
+    @MockComponent
+    private URLSecurityManager urlSecurityManager;
+
+    @RegisterExtension
+    private LogCaptureExtension logCapture = new LogCaptureExtension(LogLevel.WARN);
+
+    @Test
+    void convertWithError() throws URISyntaxException, IOException
+    {
+        HttpServletRequest servletRequest = mock();
+        String domain = "domain";
+        when(servletRequest.getServerName()).thenReturn(domain);
+        when(servletRequest.getSession()).thenReturn(mock());
+        HttpServletResponse servletResponse = mock();
+
+        String parameterName = "test";
+        when(servletRequest.getParameterValues("RequiresHTMLConversion")).thenReturn(new String[] { parameterName });
+        String testContent = "testContent";
+        String testSyntax = "testSyntax";
+        String errorURL = "errorURL";
+        when(servletRequest.getParameterMap()).thenReturn(
+            Map.of(parameterName, new String[] { testContent },
+                parameterName + "_syntax", new String[] { testSyntax },
+                "xerror", new String[] { errorURL }));
+
+        String testMessage = "TestException";
+        IllegalArgumentException testException = new IllegalArgumentException(testMessage);
+        when(this.htmlConverter.fromHTML(testContent, testSyntax)).thenThrow(testException);
+
+        String safeURL = "https://www.xwiki.org";
+        when(this.urlSecurityManager.parseToSafeURI(startsWith(errorURL), eq(domain)))
+            .thenReturn(new URI(safeURL));
+
+        Optional<ServletRequest> result = this.converter.convert(servletRequest, servletResponse);
+
+        assertTrue(result.isEmpty());
+
+        verify(servletResponse).sendRedirect(safeURL);
+
+        assertEquals(1, this.logCapture.size());
+        ILoggingEvent logEvent = this.logCapture.getLogEvent(0);
+        assertEquals(testMessage, logEvent.getMessage());
+    }
+}