Adding redirects for folders from /foo to /foo/, adding optional directory listings for folders, and opening up a few classes for extension #87

Merged
merged 16 commits into from Jun 26, 2012

Projects

None yet

3 participants

@illicitonion
Contributor

No description provided.

@joewalnes joewalnes and 1 other commented on an outdated diff May 10, 2012
...org/webbitserver/handler/AbstractResourceHandler.java
@@ -211,8 +220,16 @@ public void run() {
} else if ((content = fileBytes()) != null) {
serve(guessMimeType(path), content, control, response, request);
} else {
+ // Assumes if path has been changed since the original request,
+ // its current value with a trailing slash will still resolve properly
+ if (!path.endsWith("/")) {
+ response.status(301).header("Location", path + "/").end();
joewalnes
joewalnes May 10, 2012 Owner

It seems safer to maintain the original query here. For example, suppose there's another user handler in place that performs some custom authentication based on the URI query, the redirect would strip these away.

i.e. this request:
/foo?key=123
should redirect to:
/foo/?key=123

illicitonion
illicitonion May 16, 2012 Contributor

Done, added test

@joewalnes joewalnes and 1 other commented on an outdated diff May 10, 2012
...org/webbitserver/handler/AbstractResourceHandler.java
protected final Executor ioThread;
protected final Map<String, String> mimeTypes;
protected String welcomeFileName;
+ private boolean isDirectoryListingEnabled;
joewalnes
joewalnes May 10, 2012 Owner

= false.

(I know it's redundant, it just makes the code more obvious).

@joewalnes joewalnes and 1 other commented on an outdated diff May 10, 2012
...org/webbitserver/handler/AbstractResourceHandler.java
@@ -66,6 +71,11 @@ public AbstractResourceHandler welcomeFile(String welcomeFile) {
return this;
}
+ public AbstractResourceHandler directoryListingEnabled(boolean isDirectoryListingEnabled) {
joewalnes
joewalnes May 10, 2012 Owner

Rename method to 'enableDirectoryListing', to make it more in line with the convention of the other builder style methods.

@joewalnes joewalnes and 1 other commented on an outdated diff May 10, 2012
...org/webbitserver/handler/AbstractResourceHandler.java
@@ -228,6 +245,24 @@ public void run() {
protected abstract ByteBuffer welcomeBytes() throws IOException;
+ protected abstract ByteBuffer directoryListingBytes() throws IOException;
+
+ protected ByteBuffer formatFileListAsHtml(File[] files) throws IOException {
+ StringBuilder builder = new StringBuilder();
+ for (File file : files) {
+ builder
+ .append("<li><a href=\"")
+ .append(file.getName())
+ .append("\">")
+ .append(file.getName())
+ .append("</a></li>");
joewalnes
joewalnes May 10, 2012 Owner

DANGER: Possibility of Cross Site Scripting attack. For example, someone may be able to upload a file called that contain malicious JavaScript in its filename. The file name must be escaped before being inserted into the HTML.

@joewalnes joewalnes commented on the diff May 10, 2012
...org/webbitserver/handler/AbstractResourceHandler.java
@@ -228,6 +245,24 @@ public void run() {
protected abstract ByteBuffer welcomeBytes() throws IOException;
+ protected abstract ByteBuffer directoryListingBytes() throws IOException;
joewalnes
joewalnes May 10, 2012 Owner

I think it's likely that users will want to be able to override the formatting of the directory listing, to make it prettier, enhance with more information, etc.

How about introducing a new class that takes care of the formatting, and allow users to pass in their own implementation if they want to replace it?

Owner

Hi Daniel

Thanks for the improvements.I've made some comments here: #87

illicitonion added some commits May 16, 2012
@illicitonion illicitonion Tidying up patch - responding to review comments
Don't strip querystring from redirect.

Renaming directoryListingEnabled -> enableDirectoryListing

Escaping to protect against XSS

Adding pluggable file listing formatter
560eb33
@illicitonion illicitonion Shortening method name, it seemed redundant 385d3ae
@illicitonion illicitonion Fixing spacing eedd5bb
Contributor

Ping?

@joewalnes joewalnes and 1 other commented on an outdated diff May 30, 2012
...tserver/handler/DefaultDirectoryListingFormatter.java
@@ -0,0 +1,61 @@
+package org.webbitserver.handler;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+class DefaultDirectoryListingFormatter implements DirectoryListingFormatter {
joewalnes
joewalnes May 30, 2012 Owner

Make this public, so it's easier to extend, use in other places, etc.

@joewalnes joewalnes and 1 other commented on an outdated diff May 30, 2012
...tserver/handler/DefaultDirectoryListingFormatter.java
+class DefaultDirectoryListingFormatter implements DirectoryListingFormatter {
+ private static final String DIRECTORY_LISTING_FORMAT_STRING =
+ "<html><body><ol style='list-style-type: none; padding-left: 0px; margin-left: 0px;'>%s</ol></body></html>";
+
+ public ByteBuffer formatFileListAsHtml(File[] files) throws IOException {
+ StringBuilder builder = new StringBuilder();
+ for (File file : files) {
+ String fileName = FileNameEscaper.escape(file.getName());
+ builder
+ .append("<li><a href=\"")
+ .append(fileName)
+ .append("\">")
+ .append(fileName)
+ .append("</a></li>");
+ }
+ String formattedString = String.format(DIRECTORY_LISTING_FORMAT_STRING, builder.toString());
joewalnes
joewalnes May 30, 2012 Owner

Add a protected method called getDirectoryListingFormatString() that returns DIRECTORY_LISTING_FORMAT_STRING. This gives users a quick way to change the format without having to duplicate the entire class.

@joewalnes joewalnes and 1 other commented on an outdated diff May 30, 2012
...tserver/handler/DefaultDirectoryListingFormatter.java
+ for (File file : files) {
+ String fileName = FileNameEscaper.escape(file.getName());
+ builder
+ .append("<li><a href=\"")
+ .append(fileName)
+ .append("\">")
+ .append(fileName)
+ .append("</a></li>");
+ }
+ String formattedString = String.format(DIRECTORY_LISTING_FORMAT_STRING, builder.toString());
+ byte[] formattedBytes = formattedString.getBytes();
+ ByteArrayInputStream inputStream = new ByteArrayInputStream(formattedBytes);
+ return AbstractResourceHandler.consumeInputStreamToByteBuffer(formattedBytes.length, inputStream);
+ }
+
+ private static class FileNameEscaper {
joewalnes
joewalnes May 30, 2012 Owner

Pull this out into a top level (not inner) class and make it public, so users can use this if implementing their own DirectoryListingFormatter.

illicitonion
illicitonion May 31, 2012 Contributor

Done, moved to org.webbitserver.helpers as XssCharacterEscaper

@joewalnes joewalnes and 1 other commented on an outdated diff May 30, 2012
...g/webbitserver/handler/DirectoryListingFormatter.java
@@ -0,0 +1,9 @@
+package org.webbitserver.handler;
+
+import java.io.File;
+import java.io.IOException;
+import java.nio.ByteBuffer;
+
+public interface DirectoryListingFormatter {
joewalnes
joewalnes May 30, 2012 Owner

Add some JavaDoc explaining what this is and what the contract should be.

Owner

Apologies for late response. Looking good. A few more (minor) comments.

Thanks

@illicitonion illicitonion Tidying up in response to review comments.
Pulling out public, top-level classes, adding some javadoc.
a7d8078
Contributor

All comments addressed, time for another round? :)

Owner

I'd love to merge this into master (d360a48), but I get two failures:

There were 2 failures:
1) listsDirectory(org.webbitserver.handler.EmbeddedResourceHandlerTest)
java.lang.AssertionError: 
Expected: a string containing "index.html"
     got: ""

    at org.junit.Assert.assertThat(Assert.java:780)
    at org.junit.Assert.assertThat(Assert.java:738)
    at org.webbitserver.handler.EmbeddedResourceHandlerTest.listsDirectory(EmbeddedResourceHandlerTest.java:59)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:263)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:68)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:47)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
    at org.junit.runners.Suite.runChild(Suite.java:128)
    at org.junit.runners.Suite.runChild(Suite.java:24)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:157)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:136)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:117)
    at org.junit.runner.JUnitCore.runMain(JUnitCore.java:98)
    at org.junit.runner.JUnitCore.runMainAndExit(JUnitCore.java:53)
    at org.junit.runner.JUnitCore.main(JUnitCore.java:45)
2) shouldFindWelcomeFileInRealServer(org.webbitserver.handler.EmbeddedResourceHandlerTest)
org.junit.ComparisonFailure: expected:<[Hello world]> but was:<[]>
    at org.junit.Assert.assertEquals(Assert.java:125)
    at org.junit.Assert.assertEquals(Assert.java:147)
    at org.webbitserver.handler.EmbeddedResourceHandlerTest.shouldFindWelcomeFileInRealServer(EmbeddedResourceHandlerTest.java:98)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:39)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:25)
    at java.lang.reflect.Method.invoke(Method.java:597)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:45)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:15)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:42)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:20)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:28)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:30)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:263)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:68)
    at org.junit.runners.BlockJUnit4ClassRunner.runChild(BlockJUnit4ClassRunner.java:47)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
    at org.junit.runners.Suite.runChild(Suite.java:128)
    at org.junit.runners.Suite.runChild(Suite.java:24)
    at org.junit.runners.ParentRunner$3.run(ParentRunner.java:231)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:60)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:229)
    at org.junit.runners.ParentRunner.access$000(ParentRunner.java:50)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:222)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:300)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:157)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:136)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:117)
    at org.junit.runner.JUnitCore.runMain(JUnitCore.java:98)
    at org.junit.runner.JUnitCore.runMainAndExit(JUnitCore.java:53)
    at org.junit.runner.JUnitCore.main(JUnitCore.java:45)

FAILURES!!!
Tests run: 144,  Failures: 2


Contributor

Thanks for taking a look,

I couldn't reproduce these failures, how were you running them?

$ git clone https://github.com/webbit/webbit.git webbit2
$ git remote add illicitonion git://github.com/illicitonion/webbit.git
$ git pull illicitonion master
$ mvn clean test
[...]

Results :

Tests run: 148, Failures: 0, Errors: 0, Skipped: 5

[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 14.136s
[INFO] Finished at: Wed Jun 20 18:10:13 BST 2012
[INFO] Final Memory: 10M/81M
[INFO] ------------------------------------------------------------------------

Owner

I ranwith make, ubuntu, jdk6. What's your OS/java?

Contributor

Awesome, running make test I get the failures, I'll investigate both the failures and the difference between make and mvn

illicitonion added some commits Jun 21, 2012
@illicitonion illicitonion Fix embedded resource listing for jars and folders
Before, embedded resources were only loaded if class files were in
folders, not jars.  Testing in-IDE, or with maven does not actually
package the tests in jars; it reads them from a folder.  `make test`
*does* package them in properly jars, so picked up this failure.

Leaves requests for directories with directory listing disabled 404ing,
which is probably wrong, but no worse than it was before.

Gets rid of fragile ByteArrayInputStream=directory check.

Adds ability to specify which classpath entry should be used to serve
files from (rather than forcing the classpath entry to be whichever one
webbit itself is in) - removes the need to repackage webbit to serve
files from a jar on the classpath.

Tested with:
mvn clean test
make clean test
Running all tests in Eclipse
ad9db40
@illicitonion illicitonion Making tests more Windows friendly
Note that two tests (in ReconnectingWebSocketClientTest) still fail in
Windows.
4ea3a9b
Contributor

Fixed. See the commit message on illicitonion@ad9db40 for details

@aslakhellesoy aslakhellesoy merged commit 4ea3a9b into webbit:master Jun 26, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment