Disallow unwise characters (#1131)

Resolves #1130

This prohibits unwise characters in paths and also removes them from
Slug headers

Author: acoburn

core/common/build.gradle

@@ -21,6 +21,7 @@ dependencies {
 
     implementation "commons-codec:commons-codec:$commonsCodecVersion"
     implementation "jakarta.xml.bind:jakarta.xml.bind-api:$jaxbApiVersion"
+    implementation "org.apache.commons:commons-lang3:$commonsLangVersion"
     implementation "org.eclipse.microprofile.config:microprofile-config-api"
     implementation "org.slf4j:slf4j-api:$slf4jVersion"
 

core/common/src/main/java/org/trellisldp/common/HttpConstants.java

@@ -130,6 +130,9 @@ public final class HttpConstants {
     /** The Memento link parameter indicating the ending range of a TimeMap. */
     public static final String UNTIL = "until";
 
+    /** A collection of "unwise" characters according to RFC 3987. */
+    public static final String UNWISE_CHARACTERS = "<>{}`^\\%\"|";
+
     /** The implied or default set of IRIs used with a Prefer header. */
     public static final Set<IRI> DEFAULT_REPRESENTATION = Set.of(PreferContainment, PreferMembership,
             PreferUserManaged, PreferServerManaged);

core/common/src/main/java/org/trellisldp/common/Slug.java

@@ -20,6 +20,7 @@
 
 import org.apache.commons.codec.DecoderException;
 import org.apache.commons.codec.net.URLCodec;
+import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 
 /**
@@ -80,8 +81,9 @@ private static String decodeSlug(final String value) {
     }
 
     private static String cleanSlugString(final String value) {
-        // Remove any fragment URIs and query parameters
-        // Then trim the string and replace any remaining whitespace or slash characters with underscores
-        return value.split("#")[0].split("\\?")[0].trim().replaceAll("[\\s/]+", "_");
+        // Remove any fragment URIs, query parameters and whitespace
+        final String base = StringUtils.deleteWhitespace(value.split("#")[0].split("\\?")[0]);
+        // Remove any "unwise" characters plus '/'
+        return StringUtils.replaceChars(base, HttpConstants.UNWISE_CHARACTERS + "/", "");
     }
 }

core/common/src/test/java/org/trellisldp/common/SlugTest.java

@@ -27,7 +27,7 @@
 class SlugTest {
 
     private static final String SLUG_VALUE = "slugValue";
-    private static final String SLUG_UNDERSCORE_VALUE = "slug_value";
+    private static final String SLUG_UNDERSCORE_VALUE = "slugvalue";
     private static final String CHECK_SLUG_VALUE = "Check slug value";
 
     @Test
@@ -61,6 +61,12 @@ void testQueryParam() {
         assertEquals(SLUG_VALUE, slug.getValue(), CHECK_SLUG_VALUE);
     }
 
+    @Test
+    void testUnwiseCharacters() {
+        final Slug slug = Slug.valueOf("a|b^c\"d{e}f\\g`h^i<j>k");
+        assertEquals("abcdefghijk", slug.getValue(), CHECK_SLUG_VALUE);
+    }
+
     @Test
     void testBadInput() {
         assertNull(Slug.valueOf("An invalid % value"), "Check invalid input");

core/http/src/main/java/org/trellisldp/http/TrellisHttpFilter.java

@@ -44,6 +44,7 @@
 import javax.ws.rs.core.Link;
 import javax.ws.rs.ext.Provider;
 
+import org.apache.commons.lang3.StringUtils;
 import org.apache.commons.rdf.api.IRI;
 import org.trellisldp.common.AcceptDatetime;
 import org.trellisldp.common.LdpResource;
@@ -85,6 +86,8 @@ public void setExtensions(final Map<String, IRI> extensions) {
 
     @Override
     public void filter(final ContainerRequestContext ctx) {
+        // Validate path
+        validatePath(ctx);
         // Validate headers
         validateAcceptDatetime(ctx);
         validateRange(ctx);
@@ -103,6 +106,13 @@ public void filter(final ContainerRequestContext ctx) {
         }
     }
 
+    private void validatePath(final ContainerRequestContext ctx) {
+        final String path = ctx.getUriInfo().getPath();
+        if (StringUtils.containsAny(path, UNWISE_CHARACTERS)) {
+            ctx.abortWith(status(BAD_REQUEST).build());
+        }
+    }
+
     private void validateAcceptDatetime(final ContainerRequestContext ctx) {
         final String acceptDatetime = ctx.getHeaderString(ACCEPT_DATETIME);
         if (acceptDatetime != null && AcceptDatetime.valueOf(acceptDatetime) == null) {

core/http/src/test/java/org/trellisldp/http/AbstractTrellisHttpResourceTest.java

@@ -155,8 +155,8 @@ abstract class AbstractTrellisHttpResourceTest extends BaseTrellisHttpResourceTe
     private static final String VAL_VERSION = "version";
     private static final String VERSION_PARAM = "?version=1496262729";
     private static final String PATH_REL_CHILD = "/child";
-    private static final String PATH_REL_GRANDCHILD = "/child_grandchild";
-    private static final String GRANDCHILD_SUFFIX = "_grandchild";
+    private static final String PATH_REL_GRANDCHILD = "/childgrandchild";
+    private static final String GRANDCHILD_SUFFIX = "grandchild";
     private static final String WEAK_PREFIX = "W/\"";
     private static final String PREFER_PREFIX = "return=representation; include=\"";
 

core/http/src/test/java/org/trellisldp/http/TrellisHttpFilterTest.java

@@ -54,4 +54,18 @@ void testTestRootSlash() {
         filter.filter(mockContext);
         verify(mockContext, never().description("Trailing slash should trigger a redirect!")).abortWith(any());
     }
+
+    @Test
+    void testUnwisePath() {
+        when(mockContext.getUriInfo()).thenReturn(mockUriInfo);
+        when(mockUriInfo.getPath()).thenReturn("/foo/bar/one|two");
+        when(mockUriInfo.getQueryParameters()).thenReturn(new MultivaluedHashMap<>());
+
+        final TrellisHttpFilter filter = new TrellisHttpFilter();
+        filter.setMutatingMethods(emptyList());
+        filter.setExtensions(emptyMap());
+
+        filter.filter(mockContext);
+        verify(mockContext).abortWith(any());
+    }
 }

core/http/src/test/resources/logback-test.xml

@@ -7,7 +7,7 @@
         </encoder>
     </appender>
 
-  <logger name="org.trellisldp" additivity="false" level="DEBUG">
+  <logger name="org.trellisldp" additivity="false" level="INFO">
     <appender-ref ref="STDOUT"/>
   </logger>
   <root additivity="false" level="WARN">