Skip to main content
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorGreg Wilkins2016-03-22 21:41:55 -0400
committerGreg Wilkins2016-03-22 21:41:55 -0400
commitd132356e86b10c1e395462cbe60e3f515040d668 (patch)
tree637166e88ae088b3f040c0e93f806bec1f607723
parent14052742d0d9818744d8e6afd7a7fd214e15260e (diff)
parent4822bea0b189ae157082504521c507d86c5861d9 (diff)
downloadorg.eclipse.jetty.project-d132356e86b10c1e395462cbe60e3f515040d668.tar.gz
org.eclipse.jetty.project-d132356e86b10c1e395462cbe60e3f515040d668.tar.xz
org.eclipse.jetty.project-d132356e86b10c1e395462cbe60e3f515040d668.zip
Merge pull request #440 from eclipse/bugs/438
Issue #438 - File and Path Resources with control characters should be rejected
-rw-r--r--jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java47
-rw-r--r--jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java20
-rw-r--r--jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java12
-rw-r--r--jetty-util/src/test/java/org/eclipse/jetty/util/StringUtilTest.java31
-rw-r--r--jetty-util/src/test/java/org/eclipse/jetty/util/resource/AbstractFSResourceTest.java95
-rw-r--r--jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileResourceTest.java15
6 files changed, 192 insertions, 28 deletions
diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java b/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java
index c1b1d4319a..9dbd9e49b7 100644
--- a/jetty-util/src/main/java/org/eclipse/jetty/util/StringUtil.java
+++ b/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.
diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java
index d2f07c93a1..dbc4670a72 100644
--- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/FileResource.java
+++ b/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()
diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java b/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java
index 5d0c7bc4ab..8e773676ea 100644
--- a/jetty-util/src/main/java/org/eclipse/jetty/util/resource/PathResource.java
+++ b/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()
{
diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/StringUtilTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/StringUtilTest.java
index a33170a629..09889fa891 100644
--- a/jetty-util/src/test/java/org/eclipse/jetty/util/StringUtilTest.java
+++ b/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
@@ -206,6 +206,25 @@ public class StringUtilTest
}
@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()
{
Assert.assertTrue(StringUtil.isBlank(null));
diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/AbstractFSResourceTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/AbstractFSResourceTest.java
index deaff22dc1..3edda21811 100644
--- a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/AbstractFSResourceTest.java
+++ b/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
@@ -518,6 +522,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
{
File dir=new File(testdir.getDir(),"bãm");
diff --git a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileResourceTest.java b/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileResourceTest.java
index ff63159f91..afa16e5726 100644
--- a/jetty-util/src/test/java/org/eclipse/jetty/util/resource/FileResourceTest.java
+++ b/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
- {
- }
}

Back to the top