Merge pull request #788 from marklogic/feature/polaris

MLE-24375 Fixing URI construction

Author: rjrudin

ml-app-deployer/src/main/java/com/marklogic/mgmt/resource/AbstractResourceManager.java

@@ -36,13 +36,14 @@ public String getResourcesPath() {
     }
 
     public String getResourcePath(String resourceNameOrId, String... resourceUrlParams) {
-        resourceNameOrId = encodeResourceId(resourceNameOrId);
+        resourceNameOrId = encodeResourceIdBeforeAddingToPath(resourceNameOrId);
         return appendParamsAndValuesToPath(format("%s/%s", getResourcesPath(), resourceNameOrId), resourceUrlParams);
     }
 
     public String getPropertiesPath(String resourceNameOrId, String... resourceUrlParams) {
-        return appendParamsAndValuesToPath(format("%s/properties", getResourcePath(resourceNameOrId)),
-                resourceUrlParams);
+		String resourcePath = getResourcePath(resourceNameOrId);
+		String propertiesPath = format("%s/properties", resourcePath);
+        return appendParamsAndValuesToPath(propertiesPath,  resourceUrlParams);
     }
 
     /**
@@ -62,27 +63,27 @@ public ResourcesFragment getAsXml() {
     }
 
     public Fragment getAsXml(String resourceNameOrId, String... resourceUrlParams) {
-        String path = appendParamsAndValuesToPath(getResourcePath(resourceNameOrId, resourceUrlParams));
+        String path = getResourcePath(resourceNameOrId, resourceUrlParams);
 	    return useSecurityUser() ? manageClient.getXmlAsSecurityUser(path) : manageClient.getXml(path);
     }
 
     public Fragment getPropertiesAsXml(String resourceNameOrId, String... resourceUrlParams) {
-        String path = appendParamsAndValuesToPath(getPropertiesPath(resourceNameOrId, resourceUrlParams));
+        String path = getPropertiesPath(resourceNameOrId, resourceUrlParams);
 	    return useSecurityUser() ? manageClient.getXmlAsSecurityUser(path) : manageClient.getXml(path);
     }
 
     public String getPropertiesAsXmlString(String resourceNameOrId, String... resourceUrlParams) {
-        String path = appendParamsAndValuesToPath(getPropertiesPath(resourceNameOrId, resourceUrlParams));
+        String path = getPropertiesPath(resourceNameOrId, resourceUrlParams);
 	    return useSecurityUser() ? manageClient.getXmlStringAsSecurityUser(path) : manageClient.getXmlString(path);
     }
 
     public String getAsJson(String resourceNameOrId, String... resourceUrlParams) {
-        String path = appendParamsAndValuesToPath(getPropertiesPath(resourceNameOrId, resourceUrlParams));
+        String path = getPropertiesPath(resourceNameOrId, resourceUrlParams);
 	    return useSecurityUser() ? manageClient.getJsonAsSecurityUser(path) : manageClient.getJson(path);
     }
 
     public String getPropertiesAsJson(String resourceNameOrId, String... resourceUrlParams) {
-        String path = appendParamsAndValuesToPath(getPropertiesPath(resourceNameOrId, resourceUrlParams));
+        String path = getPropertiesPath(resourceNameOrId, resourceUrlParams);
 	    return useSecurityUser() ? manageClient.getJsonAsSecurityUser(path) : manageClient.getJson(path);
     }
 
@@ -123,18 +124,17 @@ protected SaveReceipt createNewResource(String payload, String resourceId) {
     }
 
     /**
-     * Mimetypes are likely to have a "+" in them, which the Management REST API won't support in a path - it needs to
-     * be encoded. Other resources could have a "+" in their ID value as well. However, doing a full encoding doesn't
-     * seem to be a great idea, as that will e.g. encode a forward slash in a mimetype as well, which will result in a
-     * 404.
+	 * The resource ID needs to be encoded for one known reason, which was first discovered in May 2016 - mimetypes
+	 * can have "+" in them, but the server erroneously does not support a "+" in the path part of a URI. So the "+"
+	 * needs to be encoded. For example, "http://localhost:8002/manage/v2/mimetypes/text/foo+bar" doesn't work,
+	 * but "http://localhost:8002/manage/v2/mimetypes/text/foo%2Bbar" does work.
+	 *
+	 * See MLE-24380 for the internal server bug.
      *
      * @param idValue
      * @return
      */
-    protected String encodeResourceId(String idValue) {
-		// Polaris effectively complains that this is allowing server-side request forgery. However, we cannot perform
-		// a full URL encoding here as "/" is allowed in some resource names, such as MIME types. Polaris also is
-		// seemingly only seeing the path and not the fact that the ManageClient will always build an http or https URL.
+    protected final String encodeResourceIdBeforeAddingToPath(String idValue) {
         return idValue != null ? idValue.replace("+", "%2B") : idValue;
     }
 

ml-app-deployer/src/main/java/com/marklogic/mgmt/resource/security/ProtectedCollectionsManager.java

@@ -3,11 +3,8 @@
  */
 package com.marklogic.mgmt.resource.security;
 
-import com.marklogic.mgmt.resource.AbstractResourceManager;
 import com.marklogic.mgmt.ManageClient;
-
-import java.io.UnsupportedEncodingException;
-import java.net.URLEncoder;
+import com.marklogic.mgmt.resource.AbstractResourceManager;
 
 public class ProtectedCollectionsManager extends AbstractResourceManager {
 
@@ -32,28 +29,11 @@ protected String getIdFieldName() {
 
     @Override
     public String getPropertiesPath(String resourceNameOrId, String... resourceUrlParams) {
-        return getResourcesPath() + "/properties?collection=" + encodeCollectionName(resourceNameOrId);
+        return getResourcesPath() + "/properties?collection=" + resourceNameOrId;
     }
 
     @Override
     public String getResourcePath(String resourceNameOrId, String... resourceUrlParams) {
-        return getResourcesPath() + "?collection=" + encodeCollectionName(resourceNameOrId);
+        return getResourcesPath() + "?collection=" + resourceNameOrId;
     }
-
-	/**
-	 * For most Manage API resources, MarkLogic prohibits characters that require URL encoding. Collection names are
-	 * an exception though and thus require URL encoding so that their names can be used in a querystring.
-	 *
-	 * @param collectionName
-	 * @return
-	 */
-	private String encodeCollectionName(String collectionName) {
-		try {
-			return URLEncoder.encode(collectionName, "utf-8");
-		} catch (UnsupportedEncodingException e) {
-			logger.warn(format("Unable to encode collection: %s; will include un-encoded collection name in " +
-				"querystring; cause: %s", collectionName, e.getMessage()));
-			return collectionName;
-		}
-	}
 }

ml-app-deployer/src/main/java/com/marklogic/mgmt/resource/temporal/TemporalCollectionManager.java

@@ -22,7 +22,7 @@ public String getResourcesPath() {
 
 	@Override
 	public String getResourcePath(String resourceNameOrId, String... resourceUrlParams) {
-		resourceNameOrId = encodeResourceId(resourceNameOrId);
+		resourceNameOrId = encodeResourceIdBeforeAddingToPath(resourceNameOrId);
 		return appendParamsAndValuesToPath(format("%s?collection=%s", getResourcesPath(), resourceNameOrId), resourceUrlParams);
 	}
 

ml-app-deployer/src/main/java/com/marklogic/rest/util/RestConfig.java

@@ -11,10 +11,10 @@
 import com.marklogic.client.ext.ssl.SslUtil;
 import okhttp3.OkHttpClient;
 import org.springframework.util.StringUtils;
+import org.springframework.web.util.UriComponentsBuilder;
 
 import javax.net.ssl.SSLContext;
 import java.net.URI;
-import java.net.URISyntaxException;
 
 public class RestConfig {
 
@@ -158,30 +158,94 @@ public String toString() {
 	}
 
 	/**
-	 * Using the java.net.URI constructor that takes a string. Using any other constructor runs into encoding problems,
-	 * e.g. when a mimetype has a plus in it, that plus needs to be encoded, but doing as %2B will result in the % being
-	 * double encoded. Unfortunately, it seems some encoding is still needed - e.g. for a pipeline like "Flexible Replication"
-	 * with a space in its name, the space must be encoded properly as a "+".
+	 * Builds a URI using Spring's UriComponentsBuilder to prevent URL manipulation attacks.
+	 * This replaces the previous vulnerable implementation that allowed user-controllable URL construction.
+	 * Handles query parameters that may be included in the path for backwards compatibility.
 	 *
 	 * @param path
 	 * @return
 	 */
 	public URI buildUri(String path) {
-		String basePathToAppend = "";
+		try {
+			UriComponentsBuilder builder = UriComponentsBuilder.newInstance()
+				.scheme(getScheme())
+				.host(getHost())
+				.port(getPort());
+
+			final String fullPath = determineFullPath(path);
+
+			int queryIndex = fullPath.indexOf('?');
+			if (queryIndex != -1) {
+				String pathPart = fullPath.substring(0, queryIndex);
+				builder.path(pathPart);
+				String queryPart = fullPath.substring(queryIndex + 1);
+				applyQueryParams(builder, queryPart);
+			} else {
+				builder.path(fullPath);
+			}
+
+			URI uri = builder.build().toUri();
+			uri = fixDoubleEncodingOfPlusSign(uri);
+			return uri;
+		} catch (Exception ex) {
+			throw new RuntimeException("Unable to build URI for path: " + path + "; cause: " + ex.getMessage(), ex);
+		}
+	}
+
+	private String determineFullPath(String path) {
+		String fullPath = path;
 		if (basePath != null) {
-			if (!basePath.startsWith("/")) {
-				basePathToAppend = "/";
+			String normalizedBasePath = basePath.startsWith("/") ? basePath : "/" + basePath;
+			if (path.startsWith("/") && normalizedBasePath.endsWith("/")) {
+				normalizedBasePath = normalizedBasePath.substring(0, normalizedBasePath.length() - 1);
 			}
-			basePathToAppend += basePath;
-			if (path.startsWith("/") && basePathToAppend.endsWith("/")) {
-				basePathToAppend = basePathToAppend.substring(0, basePathToAppend.length() - 1);
+			fullPath = normalizedBasePath + path;
+		}
+		return fullPath;
+	}
+
+	private UriComponentsBuilder applyQueryParams(UriComponentsBuilder builder, String queryPart) {
+		// Parse and add query parameters
+		if (!queryPart.isEmpty()) {
+			String[] params = queryPart.split("&");
+			for (String param : params) {
+				int equalIndex = param.indexOf('=');
+				if (equalIndex != -1) {
+					String key = param.substring(0, equalIndex);
+					String value = param.substring(equalIndex + 1);
+					builder.queryParam(key, value);
+				} else {
+					// Handle parameters without values
+					builder.queryParam(param, "");
+				}
 			}
 		}
-		try {
-			return new URI(String.format("%s://%s:%d%s%s", getScheme(), getHost(), getPort(), basePathToAppend, path.replace(" ", "+")));
-		} catch (URISyntaxException ex) {
-			throw new RuntimeException("Unable to build URI for path: " + path + "; cause: " + ex.getMessage(), ex);
+		return builder;
+	}
+
+	private URI fixDoubleEncodingOfPlusSign(final URI uri) {
+		// Fix double-encoding issue: MarkLogic requires "+" to be encoded as "%2B" in paths (at least for mimetypes),
+		// but UriComponentsBuilder double-encodes it to "%252B".
+		String uriString = uri.toString();
+		if (uriString.contains("%252B")) {
+			try {
+				// Split the URI to handle path and query separately
+				String[] parts = uriString.split("\\?", 2);
+				String pathPart = parts[0];
+				String queryPart = parts.length > 1 ? parts[1] : null;
+
+				// Only fix double-encoding in the path part
+				if (pathPart.contains("%252B")) {
+					pathPart = pathPart.replace("%252B", "%2B");
+					String fixedUriString = queryPart != null ? pathPart + "?" + queryPart : pathPart;
+					return new URI(fixedUriString);
+				}
+			} catch (Exception e) {
+				// If URI reconstruction fails, return the original URI
+				// This preserves existing behavior in case of unexpected issues
+			}
 		}
+		return uri;
 	}
 
 	public String getBaseUrl() {

ml-app-deployer/src/test/java/com/marklogic/appdeployer/command/mimetypes/ManageMimetypesTest.java

@@ -8,7 +8,7 @@
 import com.marklogic.mgmt.resource.ResourceManager;
 import com.marklogic.mgmt.resource.mimetypes.MimetypeManager;
 
-public class ManageMimetypesTest extends AbstractManageResourceTest {
+class ManageMimetypesTest extends AbstractManageResourceTest {
 
 	@Override
 	protected ResourceManager newResourceManager() {