Merge pull request #440 from eclipse/bugs/438

Issue #438 - File and Path Resources with control characters should be rejected

jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java

@@ -374,6 +374,53 @@ public class StringUtil
         }
     }
 
+    /**
+     * Find the index of a control characters in String
+     * <p>
+     * This will return a result on the first occurrence of a control character, regardless if
+     * there are more than one.
+     * </p>
+     * <p>
+     * Note: uses codepoint version of {@link Character#isISOControl(int)} to support Unicode better.
+     * </p>
+     *
+     * <pre>
+     *   indexOfControlChars(null)      == -1
+     *   indexOfControlChars("")        == -1
+     *   indexOfControlChars("\r\n")    == 0
+     *   indexOfControlChars("\t")      == 0
+     *   indexOfControlChars("   ")     == -1
+     *   indexOfControlChars("a")       == -1
+     *   indexOfControlChars(".")       == -1
+     *   indexOfControlChars(";\n")     == 1
+     *   indexOfControlChars("abc\f")   == 3
+     *   indexOfControlChars("z\010")   == 1
+     *   indexOfControlChars(":\u001c") == 1
+     * </pre>
+     *
+     * @param str
+     *            the string to test.
+     * @return the index of first control character in string, -1 if no control characters encountered
+     */
+    public static int indexOfControlChars(String str)
+    {
+        if (str == null)
+        {
+            return -1;
+        }
+        int len = str.length();
+        for (int i = 0; i < len; i++)
+        {
+            if (Character.isISOControl(str.codePointAt(i)))
+            {
+                // found a control character, we can stop searching  now
+                return i;
+            }
+        }
+        // no control characters
+        return -1;
+    }
+
     /* ------------------------------------------------------------ */
     /**
      * Test if a string is null or only has whitespace characters in it.

jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java

@@ -29,10 +29,12 @@ import java.net.URL;
 import java.net.URLConnection;
 import java.nio.channels.FileChannel;
 import java.nio.channels.ReadableByteChannel;
+import java.nio.file.InvalidPathException;
 import java.nio.file.StandardOpenOption;
 import java.security.Permission;
 
 import org.eclipse.jetty.util.IO;
+import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.URIUtil;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
@@ -65,6 +67,7 @@ public class FileResource extends Resource
         {
             // Try standard API to convert URL to file.
             file =new File(url.toURI());
+            assertValidPath(file.toString());
         }
         catch (URISyntaxException e) 
         {
@@ -108,6 +111,7 @@ public class FileResource extends Resource
         _file=file;
         URI file_uri=_file.toURI();
         _uri=normalizeURI(_file,uri);
+        assertValidPath(file.toString());
 
         // Is it a URI alias?
         if (!URIUtil.equalsIgnoreEncodings(_uri,file_uri.toString()))
@@ -119,6 +123,7 @@ public class FileResource extends Resource
     /* -------------------------------------------------------- */
     FileResource(File file)
     {
+        assertValidPath(file.toString());
         _file=file;
         _uri=normalizeURI(_file,_file.toURI());
         _alias=checkFileAlias(_file);
@@ -179,6 +184,7 @@ public class FileResource extends Resource
     public Resource addPath(String path)
         throws IOException,MalformedURLException
     {
+        assertValidPath(path);
         path = org.eclipse.jetty.util.URIUtil.canonicalPath(path);
 
         if (path==null)
@@ -204,13 +210,21 @@ public class FileResource extends Resource
         }
         catch(final URISyntaxException e)
         {
-            throw new MalformedURLException(){{initCause(e);}};
+            throw new InvalidPathException(path, e.getMessage());
         }
 
         return new FileResource(uri);
     }
-   
+
-    
+    private void assertValidPath(String path)
+    {
+        int idx = StringUtil.indexOfControlChars(path);
+        if (idx >= 0)
+        {
+            throw new InvalidPathException(path, "Invalid Character at index " + idx);
+        }
+    }
+
     /* ------------------------------------------------------------ */
     @Override
     public URI getAlias()

jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java

@@ -39,6 +39,7 @@ import java.nio.file.attribute.FileTime;
 import java.util.ArrayList;
 import java.util.List;
 
+import org.eclipse.jetty.util.StringUtil;
 import org.eclipse.jetty.util.log.Log;
 import org.eclipse.jetty.util.log.Logger;
 
@@ -61,6 +62,7 @@ public class PathResource extends Resource
     public PathResource(Path path)
     {
         this.path = path;
+        assertValidPath(path);
         this.uri = this.path.toUri();
     }
 
@@ -110,6 +112,16 @@ public class PathResource extends Resource
         return new PathResource(this.path.getFileSystem().getPath(path.toString(), apath));
     }
 
+    private void assertValidPath(Path path)
+    {
+        String str = path.toString();
+        int idx = StringUtil.indexOfControlChars(str);
+        if(idx >= 0)
+        {
+            throw new InvalidPathException(str, "Invalid Character at index " + idx);
+        }
+    }
+
     @Override
     public void close()
     {

jetty-util/src/test/java/org/eclipse/jetty/util/StringUtilTest.java

@@ -18,20 +18,20 @@
 
 package org.eclipse.jetty.util;
 
+import java.nio.charset.StandardCharsets;
+
+import org.junit.Assert;
+import org.junit.Test;
+
 import static org.hamcrest.Matchers.arrayContaining;
 import static org.hamcrest.Matchers.emptyArray;
+import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.nullValue;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
-import java.nio.charset.StandardCharsets;
-
-import org.hamcrest.Matchers;
-import org.junit.Assert;
-import org.junit.Test;
-
 public class StringUtilTest
 {
     @Test
@@ -205,6 +205,25 @@ public class StringUtilTest
         System.err.println(calc);
     }
 
+    @Test
+    public void testHasControlCharacter()
+    {
+        assertThat(StringUtil.indexOfControlChars("\r\n"), is(0));
+        assertThat(StringUtil.indexOfControlChars("\t"), is(0));
+        assertThat(StringUtil.indexOfControlChars(";\n"), is(1));
+        assertThat(StringUtil.indexOfControlChars("abc\fz"), is(3));
+        assertThat(StringUtil.indexOfControlChars("z\010"), is(1));
+        assertThat(StringUtil.indexOfControlChars(":\u001c"), is(1));
+
+        assertThat(StringUtil.indexOfControlChars(null), is(-1));
+        assertThat(StringUtil.indexOfControlChars(""), is(-1));
+        assertThat(StringUtil.indexOfControlChars("   "), is(-1));
+        assertThat(StringUtil.indexOfControlChars("a"), is(-1));
+        assertThat(StringUtil.indexOfControlChars("."), is(-1));
+        assertThat(StringUtil.indexOfControlChars(";"), is(-1));
+        assertThat(StringUtil.indexOfControlChars("Euro is \u20ac"), is(-1));
+    }
+
     @Test
     public void testIsBlank() 
     {

jetty-util/src/test/java/org/eclipse/jetty/util/resource/AbstractFSResourceTest.java

@@ -18,10 +18,6 @@
 
 package org.eclipse.jetty.util.resource;
 
-import static org.hamcrest.Matchers.*;
-import static org.junit.Assert.*;
-import static org.junit.Assume.*;
-
 import java.io.File;
 import java.io.FileWriter;
 import java.io.IOException;
@@ -51,6 +47,14 @@ import org.junit.Assert;
 import org.junit.Rule;
 import org.junit.Test;
 
+import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.notNullValue;
+import static org.hamcrest.Matchers.nullValue;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.fail;
+import static org.junit.Assume.assumeFalse;
+import static org.junit.Assume.assumeNoException;
+
 public abstract class AbstractFSResourceTest
 {
     @Rule
@@ -517,6 +521,89 @@ public abstract class AbstractFSResourceTest
         }
     }
     
+    @Test
+    public void testExist_BadControlChars_Encoded() throws Exception
+    {
+        createEmptyFile("a.jsp");
+
+        try
+        {
+            // request with control characters
+            URI ref = testdir.getDir().toURI().resolve("a.jsp%1F%10");
+            assertThat("ControlCharacters URI",ref,notNullValue());
+
+            Resource fileref = newResource(ref);
+            assertThat("File Resource should not exists", fileref.exists(), is(false));
+        }
+        catch (InvalidPathException e)
+        {
+            // Expected path
+        }
+    }
+
+    @Test
+    public void testExist_BadControlChars_Decoded() throws Exception
+    {
+        createEmptyFile("a.jsp");
+
+        try
+        {
+            // request with control characters
+            File badFile = new File(testdir.getDir(), "a.jsp\014\010");
+            newResource(badFile);
+            fail("Should have thrown " + InvalidPathException.class);
+        }
+        catch (InvalidPathException e)
+        {
+            // Expected path
+        }
+    }
+
+    @Test
+    public void testExist_AddPath_BadControlChars_Decoded() throws Exception
+    {
+        createEmptyFile("a.jsp");
+
+        try
+        {
+            // base resource
+            URI ref = testdir.getDir().toURI();
+            Resource base = newResource(ref);
+            assertThat("Base Resource URI",ref,notNullValue());
+
+            // add path with control characters (raw/decoded control characters)
+            // This MUST fail
+            base.addPath("/a.jsp\014\010");
+            fail("Should have thrown " + InvalidPathException.class);
+        }
+        catch (InvalidPathException e)
+        {
+            // Expected path
+        }
+    }
+
+    @Test
+    public void testExist_AddPath_BadControlChars_Encoded() throws Exception
+    {
+        createEmptyFile("a.jsp");
+
+        try
+        {
+            // base resource
+            URI ref = testdir.getDir().toURI();
+            Resource base = newResource(ref);
+            assertThat("Base Resource URI",ref,notNullValue());
+
+            // add path with control characters
+            Resource fileref = base.addPath("/a.jsp%14%10");
+            assertThat("File Resource should not exists", fileref.exists(), is(false));
+        }
+        catch (InvalidPathException e)
+        {
+            // Expected path
+        }
+    }
+
     @Test
     public void testUtf8Dir() throws Exception
     {

jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileResourceTest.java

@@ -22,9 +22,6 @@ import java.io.File;
 import java.io.IOException;
 import java.net.URI;
 
-import org.junit.Ignore;
-import org.junit.Test;
-
 public class FileResourceTest extends AbstractFSResourceTest
 {
     @Override
@@ -38,16 +35,4 @@ public class FileResourceTest extends AbstractFSResourceTest
     {
         return new FileResource(file);
     }
-    
-    @Ignore("Cannot get null to be seen by FileResource")
-    @Test
-    public void testExist_BadNull() throws Exception
-    {
-    }
-
-    @Ignore("Validation shouldn't be done in FileResource")
-    @Test
-    public void testExist_BadNullX() throws Exception
-    {
-    }
 }