Merge pull request #1368 from imeszaros/xss-fix

Avoiding possible XSS attack through the default error handler

Author: jknack

jooby/src/main/java/org/jooby/Err.java

@@ -203,9 +203,10 @@
  */
 package org.jooby;
 
-import java.util.LinkedHashMap;
-import java.util.Map;
-import java.util.Optional;
+import java.util.*;
+import java.util.function.BiFunction;
+import java.util.function.Function;
+import java.util.function.Supplier;
 
 import com.typesafe.config.Config;
 import org.jooby.funzy.Try;
@@ -287,12 +288,21 @@ public void handle(final Request req, final Response rsp, final Err ex) throws T
       log.error("execution of: {}{} resulted in exception\nRoute:\n{}\n\nStacktrace:",
           req.method(), req.path(), req.route().print(6), ex);
       Config conf = req.require(Config.class);
+      Env env = req.require(Env.class);
       boolean stackstrace = Try.apply(() -> conf.getBoolean("err.stacktrace"))
-          .orElse(req.require(Env.class).name().equals("dev"));
+          .orElse(env.name().equals("dev"));
+
+      Function<Object, String> xssFilter = env.xss("html").compose(Objects::toString);
+      BiFunction<String, Object, String> escaper = (k, v) -> xssFilter.apply(v);
+
+      Map<String, Object> details = ex.toMap(stackstrace);
+      details.compute("message", escaper);
+      details.compute("reason", escaper);
+
       rsp.send(
           Results
-              .when(MediaType.html, () -> Results.html(VIEW).put("err", ex.toMap(stackstrace)))
-              .when(MediaType.all, () -> ex.toMap(stackstrace)));
+              .when(MediaType.html, () -> Results.html(VIEW).put("err", details))
+              .when(MediaType.all, () -> details));
     }
 
   }

jooby/src/main/java/org/jooby/internal/HttpHandlerImpl.java

@@ -627,7 +627,7 @@ private static Route[] routes(final Set<Route.Definition> routeDefs, final Strin
           // default /favicon.ico handler:
           rsp.status(Status.NOT_FOUND).end();
         } else {
-          throw new Err(Status.NOT_FOUND, req.path(true));
+          throw new Err(Status.NOT_FOUND, req.path());
         }
       }
     }, method, path, "err", accept));

jooby/src/main/java/org/jooby/internal/RouteImpl.java

@@ -233,7 +233,7 @@ public class RouteImpl implements RouteWithFilter {
   public static RouteWithFilter notFound(final String method, final String path) {
     return new FallbackRoute("404", method, path, MediaType.ALL, (req, rsp, chain) -> {
       if (!rsp.status().isPresent()) {
-        throw new Err(Status.NOT_FOUND, req.path(true));
+        throw new Err(Status.NOT_FOUND, req.path());
       }
     });
   }

jooby/src/test/java/org/jooby/DefaultErrHandlerTest.java

@@ -1,6 +1,8 @@
 package org.jooby;
 
 import com.google.common.collect.ImmutableList;
+import com.google.common.escape.Escapers;
+import com.google.common.html.HtmlEscapers;
 import com.typesafe.config.Config;
 import static org.easymock.EasyMock.expect;
 import org.jooby.test.MockUnit;
@@ -15,6 +17,7 @@
 
 import java.io.PrintWriter;
 import java.io.StringWriter;
+import java.util.LinkedHashMap;
 import java.util.Map;
 
 @RunWith(PowerMockRunner.class)
@@ -66,6 +69,7 @@ private MockUnit.Block handleErr(Throwable ex, boolean stacktrace) {
       expect(conf.getBoolean("err.stacktrace")).andReturn(stacktrace);
       Env env = unit.get(Env.class);
       expect(env.name()).andReturn("dev");
+      expect(env.xss("html")).andReturn(HtmlEscapers.htmlEscaper()::escape);
 
       Request req = unit.get(Request.class);
 
@@ -112,13 +116,45 @@ public void handleWithErrMessage() throws Exception {
             });
   }
 
+  @SuppressWarnings({"unchecked"})
+  @Test
+  public void handleWithHtmlErrMessage() throws Exception {
+    Err ex = new Err(500, "Something something <em>dark</em>");
+
+    StringWriter writer = new StringWriter();
+    ex.printStackTrace(new PrintWriter(writer));
+    String[] stacktrace = writer.toString().replace("\r", "").split("\\n");
+
+    new MockUnit(Request.class, Response.class, Route.class, Env.class, Config.class)
+            .expect(handleErr(ex, true))
+            .run(unit -> {
+
+                      Request req = unit.get(Request.class);
+                      Response rsp = unit.get(Response.class);
+
+                      new Err.DefHandler().handle(req, rsp, ex);
+                    },
+                    unit -> {
+                      Result result = unit.captured(Result.class).iterator().next();
+                      View view = (View) result.ifGet(ImmutableList.of(MediaType.html)).get();
+                      assertEquals("err", view.name());
+                      checkErr(stacktrace, "Server Error(500): Something something &lt;em&gt;dark&lt;/em&gt;",
+                              (Map<String, Object>) view.model()
+                                      .get("err"));
+
+                      Object hash = result.ifGet(MediaType.ALL).get();
+                      assertEquals(4, ((Map<String, Object>) hash).size());
+                    });
+  }
+
   private void checkErr(final String[] stacktrace, final String message,
       final Map<String, Object> err) {
-    assertEquals(message, err.remove("message"));
-    assertEquals("Server Error", err.remove("reason"));
-    assertEquals(500, err.remove("status"));
-    assertArrayEquals(stacktrace, (String[]) err.remove("stacktrace"));
-    assertEquals(err.toString(), 0, err.size());
+    final Map<String, Object> copy = new LinkedHashMap<>(err);
+    assertEquals(message, copy.remove("message"));
+    assertEquals("Server Error", copy.remove("reason"));
+    assertEquals(500, copy.remove("status"));
+    assertArrayEquals(stacktrace, (String[]) copy.remove("stacktrace"));
+    assertEquals(copy.toString(), 0, copy.size());
   }
 
 }