Merge pull request #656 from davidjgonzalez/fix/652-653-654-655

Fix/652 653 654 655
adobe · Sep 15, 2021 · c1bd43c · c1bd43c
2 parents 8d7aa22 + d6b202b
commit c1bd43c
Show file tree

Hide file tree

Showing 10 changed files with 78 additions and 35 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -6,6 +6,13 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 ## [Unreleased]
 
+### Fixed
+
+- 0652 - Fixed over-escaping of static rendition paths.
+- 0653 - 6.5 only - Bundle does not start on 6.5.9
+- 0654 - 6.5 only - ExternalRenditionDispatcher expression evaluation does not resolve to correct asset resource
+- 0655 - 6.5 only - Erring HTTP POST is made on all page loads
+
 ## [v2.1.8]
 
 ### Fixed 

diff --git a/core/pom.xml b/core/pom.xml
@@ -57,6 +57,8 @@ Import-Package: javax.annotation;version=0.0.0, \
                 com.day.cq.search.eval;version="[1.4.0,2)", \
                 org.apache.commons.io;version="[2.6.0,4)", \
                 org.apache.commons.lang3;version="[3.9.0,4)", \
+                org.osgi.framework;version="[1.9.0,2)", \
+                org.apache.sling.api.wrappers;version="[2.6.4,3)", \
                 *
 -conditionalpackage: com.adobe.acs.commons.util.*,
 -exportcontents: ${packages;VERSIONED}

diff --git a/core/src/main/java/com/adobe/aem/commons/assetshare/content/renditions/AssetRendition.java b/core/src/main/java/com/adobe/aem/commons/assetshare/content/renditions/AssetRendition.java
@@ -21,11 +21,13 @@
 
 import com.adobe.aem.commons.assetshare.util.UrlUtil;
 import com.day.text.Text;
+import org.apache.commons.lang3.StringUtils;
 import org.apache.http.client.utils.URIUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import java.net.URI;
+import java.net.URISyntaxException;
 import java.util.Optional;
 
 /**
@@ -40,16 +42,21 @@ public class AssetRendition {
     private String mimeType;
 
     public AssetRendition(URI uri, Long size, String mimeType) {
-        setBinaryUri(uri);
-        setSize(size);
-        setMimeType(mimeType);
+        this(uri.toString(), size, mimeType);
+
     }
 
     public AssetRendition(String uri, Long size, String mimeType) {
+        URI cleanURI = null;
 
-        uri = UrlUtil.escape(uri, true);
+        try {
+            cleanURI = cleanURI(uri.toString());
+        } catch (URISyntaxException e) {
+            log.warn("Unable to clean the URI [ {} ], using it as is.", uri, e);
+            cleanURI = URI.create(uri);
+        }
 
-        setBinaryUri(URI.create(uri));
+        setBinaryUri(cleanURI);
         setSize(size);
         setMimeType(mimeType);
     }
@@ -84,6 +91,12 @@ public void setMimeType(String mimeType) {
         this.mimeType = mimeType;
     }
 
+    private URI cleanURI(String uri) throws URISyntaxException {
+        uri  = StringUtils.replace(uri, " ", "%20");
+        uri  = StringUtils.replace(uri, "/_jcr_content", "/jcr:content");
+
+        return new URI(uri);
+    }
 }
 
 

diff --git a/...in/java/com/adobe/aem/commons/assetshare/content/renditions/impl/AssetRenditionsImpl.java b/...in/java/com/adobe/aem/commons/assetshare/content/renditions/impl/AssetRenditionsImpl.java
@@ -78,7 +78,7 @@ public Map<String, String> getOptions(final Map<String, ? extends Object> mappin
 
     @Override
     public String evaluateExpression(final SlingHttpServletRequest request, String expression) {
-        final AssetModel assetModel = request.adaptTo(AssetModel.class);
+        final AssetModel assetModel = modelFactory.createModel(request.getResource(), AssetModel.class);
         return evaluateExpression(assetModel, new AssetRenditionParameters(request).getRenditionName(), expression);
     }
 

diff --git a/...mmons/assetshare/content/renditions/impl/dispatchers/AbstractRenditionDispatcherImpl.java b/...mmons/assetshare/content/renditions/impl/dispatchers/AbstractRenditionDispatcherImpl.java
@@ -92,7 +92,8 @@ public String cleanURI(String uri) throws URISyntaxException {
                 .filter(nvp -> !QUERY_PARAM_SUGGESTED_EXTENSION.equals(nvp.getName()))
                 .forEach(nvp -> newURI.addParameter(nvp.getName(), nvp.getValue()));;
 
+        // Common pattern in DM URLS; This escaping probably is not necessary, but keeping for
+        // now as it would take a closer look to remove (and it breaks tests)
         return StringUtils.replace(newURI.toString(), "%24", "$");
     }
-
 }
diff --git a/core/src/main/java/com/adobe/aem/commons/assetshare/util/UrlUtil.java b/core/src/main/java/com/adobe/aem/commons/assetshare/util/UrlUtil.java
@@ -62,22 +62,25 @@ public static final String escape(final String unescaped, final boolean preventD
         // URL cannot handle spaces, so convert them to %20; escapePath(..) will handle converting them back for later escaping
         tmp = StringUtils.replace(tmp, " ", "%20");
 
-        try {
-            final URL url = new URL(tmp);
-            String path = escapePath(url.getPath());
+        if (!isPath(unescaped)) {
+            try {
+                final URL url = new URL(tmp);
+                String path = escapePath(url.getPath());
 
-            if (StringUtils.isNotBlank(url.getQuery())) {
-                path += "?" + url.getQuery();
-            }
+                if (StringUtils.isNotBlank(url.getQuery())) {
+                    path += "?" + url.getQuery();
+                }
 
-            // Reconstruct the URL using the escaped path
-            return new URL(url.getProtocol(), url.getHost(), url.getPort(), path).toString();
+                // Reconstruct the URL using the escaped path
+                return new URL(url.getProtocol(), url.getHost(), url.getPort(), path).toString();
 
-        } catch (MalformedURLException e) {
-            // Treat as internal path
-            log.debug("Could not evaluate unescaped string [ {} ] as a URL. Falling back to escape as path.", e);
-            return escapePath(tmp);
+            } catch (MalformedURLException e) {
+                // Treat as internal path
+                log.debug("Could not evaluate unescaped string [ {} ] as a URL. Falling back to escape as path.", unescaped, e);
+            }
         }
+
+        return escapePath(tmp);
     }
 
     /**
@@ -117,4 +120,20 @@ private static String escapePath(String path) {
         path = Text.escapePath(path);
         return path;
     }
+
+    /**
+     * @param candidate the candidate URI to path to check
+     * @return true if it looks like a path, and false it looks like a URL
+     */
+    private static boolean isPath(String candidate) {
+        if (StringUtils.isNotBlank(candidate)) {
+            if (candidate.matches("^[a-zA-Z0-9]+://.+")) {
+                return false;
+            } else if (StringUtils.startsWith(candidate, "//")) {
+                return false;
+            }
+        }
+
+        return true;
+    }
 }
diff --git a/...src/test/java/com/adobe/aem/commons/assetshare/content/renditions/AssetRenditionTest.java b/...src/test/java/com/adobe/aem/commons/assetshare/content/renditions/AssetRenditionTest.java
@@ -16,14 +16,17 @@ public void getBinaryUri() {
         assetRendition = new AssetRendition("/content/dam/test asset.jpg", 123L, "image/jpeg");
         assertEquals("/content/dam/test%20asset.jpg", assetRendition.getBinaryUri().toString());
 
+        assetRendition = new AssetRendition("/content/dam/test asset.jpg/jcr:content/renditions.cq5dam.web.1.2.png", 123L, "image/jpeg");
+        assertEquals("/content/dam/test%20asset.jpg/jcr:content/renditions.cq5dam.web.1.2.png", assetRendition.getBinaryUri().toString());
+
         assetRendition = new AssetRendition("/content/dam/test+with+plus.jpg", 123L, "image/jpeg");
-        assertEquals("/content/dam/test%2bwith%2bplus.jpg", assetRendition.getBinaryUri().toString());
+        assertEquals("/content/dam/test+with+plus.jpg", assetRendition.getBinaryUri().toString());
 
         assetRendition = new AssetRendition("https://test.com/content/dam/test asset.jpg", 123L, "image/jpeg");
         assertEquals("https://test.com/content/dam/test%20asset.jpg", assetRendition.getBinaryUri().toString());
 
         assetRendition = new AssetRendition("https://smartimaging.scene7.com/is/image/DynamicMediaNA/test (test):Medium", 123L, "image/jpeg");
-        assertEquals("https://smartimaging.scene7.com/is/image/DynamicMediaNA/test%20(test)%3aMedium", assetRendition.getBinaryUri().toString());
+        assertEquals("https://smartimaging.scene7.com/is/image/DynamicMediaNA/test%20(test):Medium", assetRendition.getBinaryUri().toString());
 
         assetRendition = new AssetRendition("https://smartimaging.scene7.com/is/image/DynamicMediaNA/test (test)?$grayscale$", 123L, "image/jpeg");
         assertEquals("https://smartimaging.scene7.com/is/image/DynamicMediaNA/test%20(test)?$grayscale$", assetRendition.getBinaryUri().toString());

diff --git a/...hare/content/renditions/impl/dispatchers/ExternalRedirectRenditionDispatcherImplTest.java b/...hare/content/renditions/impl/dispatchers/ExternalRedirectRenditionDispatcherImplTest.java
@@ -65,7 +65,6 @@ public class ExternalRedirectRenditionDispatcherImplTest {
     public void setUp() throws Exception {
         ctx.load().json(getClass().getResourceAsStream("ExternalRedirectRenditionDispatcherImplTest.json"), "/content/dam");
         ctx.currentResource("/content/dam/test.png");
-        doReturn(DamUtil.resolveToAsset(ctx.resourceResolver().getResource("/content/dam/test.png"))).when(assetResolver).resolveAsset(ctx.request());
 
         ctx.registerService(MimeTypeService.class, mimeTypeService);
         ctx.registerService(ExpressionEvaluator.class, new ExpressionEvaluatorImpl());
@@ -161,7 +160,6 @@ public void dispatch() throws IOException, ServletException {
         assetRenditionDispatcher.dispatch(ctx.request(), ctx.response());
 
         assertEquals(301, ctx.response().getStatus());
-        //assertEquals("http://test.scene7.com/is/image/testing/test?%24greyscale%24", ctx.response().getHeader("Location"));
     }
 
     @Test
@@ -175,7 +173,6 @@ public void dispatch_WithSpacesInPath() throws IOException, ServletException {
 
         final AssetRenditionDispatcher assetRenditionDispatcher = ctx.getService(AssetRenditionDispatcher.class);
 
-        doReturn(DamUtil.resolveToAsset(ctx.resourceResolver().getResource("/content/dam/test with spaces.png"))).when(assetResolver).resolveAsset(ctx.request());
         ctx.currentResource("/content/dam/test with spaces.png");
         ctx.requestPathInfo().setExtension("rendition");
         ctx.requestPathInfo().setSuffix("testing/download/asset.rendition");
@@ -198,7 +195,6 @@ public void dispatch_WithHostAndSpacesInPath() throws IOException, ServletExcept
 
         final AssetRenditionDispatcher assetRenditionDispatcher = ctx.getService(AssetRenditionDispatcher.class);
 
-        doReturn(DamUtil.resolveToAsset(ctx.resourceResolver().getResource("/content/dam/test with spaces.png"))).when(assetResolver).resolveAsset(ctx.request());
         ctx.currentResource("/content/dam/test with spaces.png");
         ctx.requestPathInfo().setExtension("rendition");
         ctx.requestPathInfo().setSuffix("testing/download/asset.rendition");

diff --git a/...hare/content/renditions/impl/dispatchers/InternalRedirectRenditionDispatcherImplTest.java b/...hare/content/renditions/impl/dispatchers/InternalRedirectRenditionDispatcherImplTest.java
@@ -78,7 +78,6 @@ public void setUp() throws Exception {
 
         ctx.load().json(getClass().getResourceAsStream("InternalRedirectRenditionDispatcherImplTest.json"), "/content/dam");
         ctx.currentResource("/content/dam/test.png");
-        doReturn(DamUtil.resolveToAsset(ctx.resourceResolver().getResource("/content/dam/test.png"))).when(assetResolver).resolveAsset(ctx.request());
 
         ctx.registerService(MimeTypeService.class, mimeTypeService);
         ctx.registerService(ExpressionEvaluator.class, new ExpressionEvaluatorImpl());
@@ -206,7 +205,6 @@ public void dispatch_WithSpacesInPath() throws IOException, ServletException {
 
         final AssetRenditionDispatcher assetRenditionDispatcher = ctx.getService(AssetRenditionDispatcher.class);
 
-        doReturn(DamUtil.resolveToAsset(ctx.resourceResolver().getResource("/content/dam/test with spaces.png"))).when(assetResolver).resolveAsset(ctx.request());
         ctx.currentResource("/content/dam/test with spaces.png");
         ctx.requestPathInfo().setExtension("rendition");
         ctx.requestPathInfo().setSuffix("testing/download/asset.rendition");

diff --git a/...root/apps/asset-share-commons/components/modals/downloads/clientlibs/site/js/downloads.js b/...root/apps/asset-share-commons/components/modals/downloads/clientlibs/site/js/downloads.js
@@ -49,17 +49,21 @@ jQuery((function($, ns, semanticModal, download) {
         }
 
         function updateBadge() {
-            $.post(DOWNLOADS_URL, getModalData()).then(function(htmlResponse) {
-                var count = ns.Elements.element('downloads-modal', htmlResponse).data('asset-share-downloads-count') || 0;
-                ns.Elements.element("downloads-count").text(count);
-            });
+            if (DOWNLOADS_URL) {
+                $.post(DOWNLOADS_URL, getModalData()).then(function(htmlResponse) {
+                    var count = ns.Elements.element('downloads-modal', htmlResponse).data('asset-share-downloads-count') || 0;
+                    ns.Elements.element("downloads-count").text(count);
+                });
+            }
         }
 
         function update() {
-            $.post(DOWNLOADS_URL, getModalData()).then(function(htmlResponse) {
-                ns.Elements.update(htmlResponse, WHEN_DOWNLOADS_UPDATED);
-                init();
-            });
+            if (DOWNLOADS_URL) {
+                $.post(DOWNLOADS_URL, getModalData()).then(function(htmlResponse) {
+                    ns.Elements.update(htmlResponse, WHEN_DOWNLOADS_UPDATED);
+                    init();
+                });
+            }
         }
 
         function show(e) {