Merge pull request #433 from amvanbaren/bugfix/issue-426

URI encode FileResource urls
eclipse · Mar 16, 2022 · a77dc99 · a77dc99
2 parents f475efa + 56b1eb4
commit a77dc99
Show file tree

Hide file tree

Showing 10 changed files with 131 additions and 71 deletions.
diff --git a/server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java b/server/src/main/java/org/eclipse/openvsx/LocalRegistryService.java
@@ -616,9 +616,9 @@ public ExtensionJson toExtensionVersionJson(
         }
 
         json.files = Maps.newLinkedHashMapWithExpectedSize(6);
-        var versionUrl = UrlUtil.createApiUrl(versionBaseUrl, json.version);
+        var fileBaseUrl = UrlUtil.createApiUrl(versionBaseUrl, json.version, "file");
         for (var resource : resources) {
-            var fileUrl = UrlUtil.createApiUrl(versionUrl, "file", resource.getName());
+            var fileUrl = UrlUtil.createApiUrl(fileBaseUrl, resource.getName().split("/"));
             json.files.put(resource.getType(), fileUrl);
         }
 

diff --git a/server/src/main/java/org/eclipse/openvsx/RegistryAPI.java b/server/src/main/java/org/eclipse/openvsx/RegistryAPI.java
@@ -47,9 +47,10 @@
 
 import springfox.documentation.annotations.ApiIgnore;
 
+import javax.servlet.http.HttpServletRequest;
+
 @RestController
 public class RegistryAPI {
-
     private final static int REVIEW_TITLE_SIZE = 255;
     private final static int REVIEW_COMMENT_SIZE = 2048;
 
@@ -172,7 +173,7 @@ public ResponseEntity<ExtensionJson> getExtension(
         return new ResponseEntity<>(json, HttpStatus.NOT_FOUND);
     }
 
-    @GetMapping("/api/{namespace}/{extension}/{version}/file/{fileName:.+}")
+    @GetMapping("/api/{namespace}/{extension}/{version}/file/**")
     @CrossOrigin
     @ApiOperation("Access a file packaged by an extension")
     @ApiResponses({
@@ -195,15 +196,15 @@ public ResponseEntity<ExtensionJson> getExtension(
         )
     })
     public ResponseEntity<byte[]> getFile(
+            HttpServletRequest request,
             @PathVariable @ApiParam(value = "Extension namespace", example = "redhat")
             String namespace,
             @PathVariable @ApiParam(value = "Extension name", example = "java")
             String extension,
             @PathVariable @ApiParam(value = "Extension version", example = "0.65.0")
-            String version,
-            @PathVariable @ApiParam(value = "Name of the file to access", example = "LICENSE.txt")
-            String fileName
+            String version
         ) {
+        var fileName = UrlUtil.extractWildcardPath(request);
         for (var registry : getRegistries()) {
             try {
                 return registry.getFile(namespace, extension, version, fileName);

diff --git a/server/src/main/java/org/eclipse/openvsx/adapter/VSCodeAdapter.java b/server/src/main/java/org/eclipse/openvsx/adapter/VSCodeAdapter.java
@@ -10,6 +10,7 @@
 package org.eclipse.openvsx.adapter;
 
 import com.google.common.collect.Lists;
+import org.apache.commons.lang3.ArrayUtils;
 import org.eclipse.openvsx.dto.ExtensionDTO;
 import org.eclipse.openvsx.dto.ExtensionVersionDTO;
 import org.eclipse.openvsx.dto.FileResourceDTO;
@@ -224,7 +225,8 @@ private String createFileUrl(List<FileResourceDTO> singleResource, String versio
     }
 
     private String createFileUrl(FileResourceDTO resource, String versionUrl) {
-        return resource != null ? UrlUtil.createApiUrl(versionUrl, "file", resource.getName()) : null;
+        var segments = ArrayUtils.addAll(new String[]{"file"}, resource.getName().split("/"));
+        return resource != null ? UrlUtil.createApiUrl(versionUrl, segments) : null;
     }
 
     private ExtensionQueryResult toQueryResult(List<ExtensionQueryResult.Extension> extensions, long totalCount) {

diff --git a/server/src/main/java/org/eclipse/openvsx/storage/AzureBlobStorageService.java b/server/src/main/java/org/eclipse/openvsx/storage/AzureBlobStorageService.java
@@ -11,9 +11,7 @@
 
 import java.io.ByteArrayInputStream;
 import java.io.IOException;
-import java.io.UnsupportedEncodingException;
 import java.net.URI;
-import java.net.URLEncoder;
 
 import javax.transaction.Transactional;
 import javax.transaction.Transactional.TxType;
@@ -22,11 +20,11 @@
 import com.azure.storage.blob.BlobContainerClientBuilder;
 import com.azure.storage.blob.models.BlobHttpHeaders;
 import com.azure.storage.blob.models.BlobStorageException;
-import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 
-import org.eclipse.openvsx.entities.ExtensionVersion;
+import org.apache.commons.lang3.ArrayUtils;
 import org.eclipse.openvsx.entities.FileResource;
+import org.eclipse.openvsx.util.UrlUtil;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.annotation.Value;
 import org.springframework.http.HttpStatus;
@@ -68,7 +66,7 @@ protected BlobContainerClient getContainerClient() {
 	@Override
 	@Transactional(TxType.MANDATORY)
     public void uploadFile(FileResource resource) {
-        var blobName = getBlobName(resource.getName(), resource.getExtension());
+        var blobName = getBlobName(resource);
         if (Strings.isNullOrEmpty(serviceEndpoint)) {
             throw new IllegalStateException("Cannot upload file "
                     + blobName + ": missing Azure blob service endpoint");
@@ -98,7 +96,7 @@ protected void uploadFile(byte[] content, String fileName, String blobName) {
 
 	@Override
 	public void removeFile(FileResource resource) {
-		var blobName = getBlobName(resource.getName(), resource.getExtension());
+		var blobName = getBlobName(resource);
         if (Strings.isNullOrEmpty(serviceEndpoint)) {
             throw new IllegalStateException("Cannot remove file "
                     + blobName + ": missing Azure blob service endpoint");
@@ -117,7 +115,7 @@ public void removeFile(FileResource resource) {
 
 	@Override
 	public URI getLocation(FileResource resource) {
-		var blobName = getBlobName(resource.getName(), resource.getExtension());
+        var blobName = getBlobName(resource);
         if (Strings.isNullOrEmpty(serviceEndpoint)) {
             throw new IllegalStateException("Cannot determine location of file "
                     + blobName + ": missing Azure blob service endpoint");
@@ -128,18 +126,13 @@ public URI getLocation(FileResource resource) {
         return URI.create(serviceEndpoint + blobContainer + "/" + blobName);
 	}
 
-    protected String getBlobName(String name, ExtensionVersion extVersion) {
-        Preconditions.checkNotNull(name);
-        try {
-            var extension = extVersion.getExtension();
-            var namespace = extension.getNamespace();
-			return namespace.getName()
-			        + "/" + extension.getName()
-			        + "/" + URLEncoder.encode(extVersion.getVersion(), "UTF-8")
-			        + "/" + name;
-		} catch (UnsupportedEncodingException exc) {
-			throw new RuntimeException(exc);
-		}
+    protected String getBlobName(FileResource resource) {
+        var extVersion = resource.getExtension();
+        var extension = extVersion.getExtension();
+        var namespace = extension.getNamespace();
+        var segments = new String[]{namespace.getName(), extension.getName(), extVersion.getVersion()};
+        segments = ArrayUtils.addAll(segments, resource.getName().split("/"));
+        return UrlUtil.createApiUrl("", segments).substring(1); // remove first '/'
     }
 
 }
diff --git a/server/src/main/java/org/eclipse/openvsx/storage/AzureDownloadCountService.java b/server/src/main/java/org/eclipse/openvsx/storage/AzureDownloadCountService.java
@@ -31,11 +31,13 @@
 import org.springframework.transaction.TransactionException;
 import org.springframework.transaction.support.TransactionTemplate;
 import org.springframework.util.StopWatch;
+import org.springframework.web.util.UriUtils;
 
 import javax.persistence.EntityManager;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.net.URI;
+import java.nio.charset.StandardCharsets;
 import java.time.Duration;
 import java.time.LocalDateTime;
 import java.time.format.DateTimeFormatter;
@@ -177,7 +179,7 @@ private void processBlobItem(String blobName) {
                     matchesStorageBlobContainer = storageBlobContainer.equals(container);
                 }
                 if(matchesStorageBlobContainer) {
-                    var fileName = pathParams[pathParams.length - 1].toUpperCase();
+                    var fileName = UriUtils.decode(pathParams[pathParams.length - 1], StandardCharsets.UTF_8).toUpperCase();
                     var timestamps = files.getOrDefault(fileName, new ArrayList<>());
                     timestamps.add(LocalDateTime.parse(node.get("time").asText(), DateTimeFormatter.ISO_ZONED_DATE_TIME));
                     files.put(fileName, timestamps);

diff --git a/server/src/main/java/org/eclipse/openvsx/storage/GoogleCloudStorageService.java b/server/src/main/java/org/eclipse/openvsx/storage/GoogleCloudStorageService.java
@@ -21,6 +21,7 @@
 import com.google.common.base.Preconditions;
 import com.google.common.base.Strings;
 
+import org.apache.commons.lang3.ArrayUtils;
 import org.eclipse.openvsx.entities.ExtensionVersion;
 import org.eclipse.openvsx.entities.FileResource;
 import org.eclipse.openvsx.util.UrlUtil;
@@ -67,7 +68,7 @@ protected Storage getStorage() {
     @Override
     @Transactional(TxType.MANDATORY)
     public void uploadFile(FileResource resource) {
-        var objectId = getObjectId(resource.getName(), resource.getExtension());
+        var objectId = getObjectId(resource);
         if (Strings.isNullOrEmpty(bucketId)) {
             throw new IllegalStateException("Cannot upload file "
                     + objectId + ": missing Google bucket id");
@@ -91,7 +92,7 @@ protected void uploadFile(byte[] content, String fileName, String objectId) {
 
     @Override
     public void removeFile(FileResource resource) {
-        var objectId = getObjectId(resource.getName(), resource.getExtension());
+        var objectId = getObjectId(resource);
         if (Strings.isNullOrEmpty(bucketId)) {
             throw new IllegalStateException("Cannot remove file "
                     + objectId + ": missing Google bucket id");
@@ -105,21 +106,16 @@ public URI getLocation(FileResource resource) {
             throw new IllegalStateException("Cannot determine location of file "
                     + resource.getName() + ": missing Google bucket id");
         }
-        var extVersion = resource.getExtension();
-        var extension = extVersion.getExtension();
-        var namespace = extension.getNamespace();
-        return URI.create(UrlUtil.createApiUrl(BASE_URL, bucketId, namespace.getName(),
-                extension.getName(), extVersion.getVersion(), resource.getName()));
+        return URI.create(BASE_URL + bucketId + "/" + getObjectId(resource));
     }
 
-    protected String getObjectId(String name, ExtensionVersion extVersion) {
-        Preconditions.checkNotNull(name);
+    protected String getObjectId(FileResource resource) {
+        var extVersion = resource.getExtension();
         var extension = extVersion.getExtension();
         var namespace = extension.getNamespace();
-        return namespace.getName()
-                + "/" + extension.getName()
-                + "/" + extVersion.getVersion()
-                + "/" + name;
+        var segments = new String[]{namespace.getName(), extension.getName(), extVersion.getVersion()};
+        segments = ArrayUtils.addAll(segments, resource.getName().split("/"));
+        return UrlUtil.createApiUrl("", segments).substring(1); // remove first '/'
     }
 
 }
diff --git a/server/src/main/java/org/eclipse/openvsx/storage/StorageUtilService.java b/server/src/main/java/org/eclipse/openvsx/storage/StorageUtilService.java
@@ -22,6 +22,7 @@
 
 import com.google.common.base.Strings;
 
+import org.apache.commons.lang3.ArrayUtils;
 import org.eclipse.openvsx.entities.Download;
 import org.eclipse.openvsx.entities.ExtensionVersion;
 import org.eclipse.openvsx.entities.FileResource;
@@ -145,8 +146,9 @@ public URI getLocation(FileResource resource) {
     private String getFileUrl(String name, ExtensionVersion extVersion, String serverUrl) {
         var extension = extVersion.getExtension();
         var namespace = extension.getNamespace();
-        return UrlUtil.createApiUrl(serverUrl, "api", namespace.getName(), extension.getName(), extVersion.getVersion(),
-                "file", name);
+        var segments = new String[]{"api", namespace.getName(), extension.getName(), extVersion.getVersion(), "file"};
+        segments = ArrayUtils.addAll(segments, name.split("/"));
+        return UrlUtil.createApiUrl(serverUrl, segments);
     }
 
     /**
@@ -155,10 +157,10 @@ private String getFileUrl(String name, ExtensionVersion extVersion, String serve
     public void addFileUrls(ExtensionVersion extVersion, String serverUrl, Map<String, String> type2Url, String... types) {
         var extension = extVersion.getExtension();
         var namespace = extension.getNamespace();
-        var versionUrl = UrlUtil.createApiUrl(serverUrl, "api", namespace.getName(), extension.getName(), extVersion.getVersion());
+        var baseUrl = UrlUtil.createApiUrl(serverUrl, "api", namespace.getName(), extension.getName(), extVersion.getVersion(), "file");
         var resources = repositories.findFilesByType(extVersion, Arrays.asList(types));
         for (var resource : resources) {
-            var fileUrl = UrlUtil.createApiUrl(versionUrl, "file", resource.getName());
+            var fileUrl = UrlUtil.createApiUrl(baseUrl, resource.getName().split("/"));
             type2Url.put(resource.getType(), fileUrl);
         }
     }
@@ -167,11 +169,11 @@ public Map<String, String> getWebResourceUrls(ExtensionVersion extVersion, Strin
         var name2Url = new HashMap<String, String>();
         var extension = extVersion.getExtension();
         var namespace = extension.getNamespace();
-        var versionUrl = UrlUtil.createApiUrl(serverUrl, "api", namespace.getName(), extension.getName(), extVersion.getVersion());
+        var baseUrl = UrlUtil.createApiUrl(serverUrl, "api", namespace.getName(), extension.getName(), extVersion.getVersion(), "file");
         var resources = repositories.findFilesByType(extVersion, Arrays.asList(WEB_RESOURCE));
         if (resources != null) {
             for (var resource : resources) {
-                var fileUrl = UrlUtil.createApiUrl(versionUrl, "file", resource.getName());
+                var fileUrl = UrlUtil.createApiUrl(baseUrl, resource.getName().split("/"));
                 name2Url.put(resource.getName(), fileUrl);
             }
         }

diff --git a/server/src/main/java/org/eclipse/openvsx/util/UrlUtil.java b/server/src/main/java/org/eclipse/openvsx/util/UrlUtil.java
@@ -9,16 +9,16 @@
  ********************************************************************************/
 package org.eclipse.openvsx.util;
 
-import java.io.UnsupportedEncodingException;
-import java.net.URLEncoder;
 import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 
 import javax.servlet.http.HttpServletRequest;
 
 import org.springframework.util.AntPathMatcher;
 import org.springframework.web.context.request.RequestContextHolder;
 import org.springframework.web.context.request.ServletRequestAttributes;
 import org.springframework.web.servlet.HandlerMapping;
+import org.springframework.web.util.UriUtils;
 
 public final class UrlUtil {
 
@@ -33,7 +33,6 @@ public static String createApiUrl(String baseUrl, String... segments) {
         for (var segment : segments) {
             initialCapacity += segment.length() + 1;
         }
-        var charset = Charset.forName("UTF-8");
         var result = new StringBuilder(initialCapacity);
         result.append(baseUrl);
         for (var segment : segments) {
@@ -43,7 +42,7 @@ public static String createApiUrl(String baseUrl, String... segments) {
                 continue;
             if (result.length() == 0 || result.charAt(result.length() - 1) != '/')
                 result.append('/');
-            result.append(URLEncoder.encode(segment, charset));
+            result.append(UriUtils.encodePathSegment(segment, StandardCharsets.UTF_8));
         }
         return result.toString();
     }
@@ -55,27 +54,24 @@ public static String createApiUrl(String baseUrl, String... segments) {
     public static String addQuery(String url, String... parameters) {
         if (parameters.length % 2 != 0)
             throw new IllegalArgumentException("Expected an even number of parameters.");
-        try {
-            var result = new StringBuilder(url);
-            var printedParams = 0;
-            for (var i = 0; i < parameters.length; i += 2) {
-                var key = parameters[i];
-                var value = parameters[i + 1];
-                if (key == null)
-                    throw new NullPointerException("Parameter key must not be null");
-                if (value != null) {
-                    if (printedParams == 0)
-                        result.append('?');
-                    else
-                        result.append('&');
-                    result.append(key).append('=').append(URLEncoder.encode(value, "UTF-8"));
-                    printedParams++;
-                }
+
+        var result = new StringBuilder(url);
+        var printedParams = 0;
+        for (var i = 0; i < parameters.length; i += 2) {
+            var key = parameters[i];
+            var value = parameters[i + 1];
+            if (key == null)
+                throw new NullPointerException("Parameter key must not be null");
+            if (value != null) {
+                if (printedParams == 0)
+                    result.append('?');
+                else
+                    result.append('&');
+                result.append(key).append('=').append(UriUtils.encodeQueryParam(value, StandardCharsets.UTF_8));
+                printedParams++;
             }
-            return result.toString();
-        } catch (UnsupportedEncodingException exc) {
-            throw new RuntimeException(exc);
         }
+        return result.toString();
     }
 
     /**