From 95f763fab4d91686355468b365be6fe27cfdfe59 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 19 Dec 2011 12:29:15 -0700 Subject: [PATCH] Bug 367099 - Upgrade jetty-websocket for RFC 6455 + Adding new RFC declared close codes 1011 (CLOSE_SERVER_ERROR) and 1015 (CLOSE_FAILED_TLS_HANDSHAKE) + Adding support for responding as CLOSE_SERVER_ERROR if an unhandled exception (similar to how HTTP error 500 works) but for exceptions thrown out of implementations of WebSocket. + Adding guard to prevent use of CLOSE_FAILED_TLS_HANDSHAKE on close control frame. + Adding unit test for the CLOSE_SERVER_ERROR case. + Adding unit test for HTTP response 400 on bad Sec-WebSocket-Version request header value. --- .../websocket/WebSocketConnectionRFC6455.java | 29 +-- .../jetty/websocket/WebSocketFactory.java | 2 +- .../jetty/websocket/WebSocketCommTest.java | 35 ++-- .../websocket/WebSocketServletRFCTest.java | 174 ++++++++++++++++++ .../jetty/websocket/helper/MessageSender.java | 14 ++ 5 files changed, 224 insertions(+), 30 deletions(-) create mode 100644 jetty-websocket/src/test/java/org/eclipse/jetty/websocket/WebSocketServletRFCTest.java diff --git a/jetty-websocket/src/main/java/org/eclipse/jetty/websocket/WebSocketConnectionRFC6455.java b/jetty-websocket/src/main/java/org/eclipse/jetty/websocket/WebSocketConnectionRFC6455.java index e996e4b34af..ed911036e9a 100644 --- a/jetty-websocket/src/main/java/org/eclipse/jetty/websocket/WebSocketConnectionRFC6455.java +++ b/jetty-websocket/src/main/java/org/eclipse/jetty/websocket/WebSocketConnectionRFC6455.java @@ -101,6 +101,8 @@ public class WebSocketConnectionRFC6455 extends AbstractConnection implements We final static int CLOSE_POLICY_VIOLATION=1008; final static int CLOSE_MESSAGE_TOO_LARGE=1009; final static int CLOSE_REQUIRED_EXTENSION=1010; + final static int CLOSE_SERVER_ERROR=1011; + final static int CLOSE_FAILED_TLS_HANDSHAKE=1015; final static int FLAG_FIN=0x8; @@ -372,13 +374,18 @@ public class WebSocketConnectionRFC6455 extends AbstractConnection implements We { if (!closed_out) { - // Close code 1005/1006 are never to be sent as a status over + // Close code 1005/1006/1015 are never to be sent as a status over // a Close control frame. Code<-1 also means no node. - if (code<0 || (code == WebSocketConnectionRFC6455.CLOSE_NO_CODE) || code==WebSocketConnectionRFC6455.CLOSE_NO_CLOSE) - code=-1; - else if (code==0) - code=WebSocketConnectionRFC6455.CLOSE_NORMAL; + if (code < 0 || (code == WebSocketConnectionRFC6455.CLOSE_NO_CODE) || (code == WebSocketConnectionRFC6455.CLOSE_NO_CLOSE) + || (code == WebSocketConnectionRFC6455.CLOSE_FAILED_TLS_HANDSHAKE)) + { + code = -1; + } + else if (code == 0) + { + code = WebSocketConnectionRFC6455.CLOSE_NORMAL; + } byte[] bytes = ("xx"+(message==null?"":message)).getBytes(StringUtil.__ISO_8859_1); bytes[0]=(byte)(code/0x100); @@ -769,7 +776,7 @@ public class WebSocketConnectionRFC6455 extends AbstractConnection implements We code == WebSocketConnectionRFC6455.CLOSE_UNDEFINED || code == WebSocketConnectionRFC6455.CLOSE_NO_CLOSE || code == WebSocketConnectionRFC6455.CLOSE_NO_CODE || - ( code > 1010 && code <= 2999 ) || + ( code > 1011 && code <= 2999 ) || code >= 5000 ) { errorClose(WebSocketConnectionRFC6455.CLOSE_PROTOCOL,"Invalid close code " + code); @@ -874,15 +881,15 @@ public class WebSocketConnectionRFC6455 extends AbstractConnection implements We } catch(Utf8Appendable.NotUtf8Exception notUtf8) { - LOG.warn("{} for {}",notUtf8,_endp); + LOG.warn("NOTUTF8 - {} for {}",notUtf8,_endp, notUtf8); LOG.debug(notUtf8); errorClose(WebSocketConnectionRFC6455.CLOSE_BAD_PAYLOAD,"Invalid UTF-8"); } - catch(Throwable probablyNotUtf8) + catch(Throwable e) { - LOG.warn("{} for {}",probablyNotUtf8,_endp); - LOG.debug(probablyNotUtf8); - errorClose(WebSocketConnectionRFC6455.CLOSE_BAD_PAYLOAD,"Invalid Payload: "+probablyNotUtf8); + LOG.warn("{} for {}",e,_endp, e); + LOG.debug(e); + errorClose(WebSocketConnectionRFC6455.CLOSE_SERVER_ERROR,"Internal Server Error: "+e); } } diff --git a/jetty-websocket/src/main/java/org/eclipse/jetty/websocket/WebSocketFactory.java b/jetty-websocket/src/main/java/org/eclipse/jetty/websocket/WebSocketFactory.java index 098a853ec1a..25f58c1ef06 100644 --- a/jetty-websocket/src/main/java/org/eclipse/jetty/websocket/WebSocketFactory.java +++ b/jetty-websocket/src/main/java/org/eclipse/jetty/websocket/WebSocketFactory.java @@ -261,7 +261,7 @@ public class WebSocketFactory // Per RFC 6455 - 4.4 - Supporting Multiple Versions of WebSocket Protocol // Using the examples as outlined response.setHeader("Sec-WebSocket-Version","13, 8, 6, 0"); - throw new HttpException(400, "Unsupported draft specification: " + draft); + throw new HttpException(400, "Unsupported websocket version specification: " + draft); } // Set the defaults diff --git a/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/WebSocketCommTest.java b/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/WebSocketCommTest.java index b0d19692a37..0e91ffaa0e9 100644 --- a/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/WebSocketCommTest.java +++ b/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/WebSocketCommTest.java @@ -24,13 +24,12 @@ import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.servlet.ServletContextHandler; import org.eclipse.jetty.servlet.ServletHolder; -import org.eclipse.jetty.util.log.StdErrLog; import org.eclipse.jetty.websocket.helper.CaptureSocket; import org.eclipse.jetty.websocket.helper.MessageSender; import org.eclipse.jetty.websocket.helper.WebSocketCaptureServlet; +import org.junit.After; import org.junit.Assert; import org.junit.Before; -import org.junit.BeforeClass; import org.junit.Test; /** @@ -43,14 +42,6 @@ public class WebSocketCommTest private WebSocketCaptureServlet servlet; private URI serverUri; - @BeforeClass - public static void initLogging() - { - // Configure Logging - System.setProperty("org.eclipse.jetty.util.log.class",StdErrLog.class.getName()); - System.setProperty("org.eclipse.jetty.LEVEL","DEBUG"); - } - @Before public void startServer() throws Exception { @@ -79,6 +70,19 @@ public class WebSocketCommTest System.out.printf("Server URI: %s%n",serverUri); } + @After + public void stopServer() + { + try + { + server.stop(); + } + catch (Exception e) + { + e.printStackTrace(System.err); + } + } + @Test public void testSendTextMessages() throws Exception { @@ -105,10 +109,11 @@ public class WebSocketCommTest CaptureSocket socket = servlet.captures.get(0); Assert.assertThat("CaptureSocket",socket,notNullValue()); - Assert.assertThat("CaptureSocket.isConnected", socket.isConnected(), is(true)); + Assert.assertThat("CaptureSocket.isConnected",socket.isConnected(),is(true)); // Give servlet 500 millisecond to process messages - threadSleep(500,TimeUnit.MILLISECONDS); + TimeUnit.MILLISECONDS.sleep(500); + // Should have captured 5 messages. Assert.assertThat("CaptureSocket.messages.size",socket.messages.size(),is(5)); } @@ -118,10 +123,4 @@ public class WebSocketCommTest sender.close(); } } - - public static void threadSleep(int dur, TimeUnit unit) throws InterruptedException - { - long ms = TimeUnit.MILLISECONDS.convert(dur,unit); - Thread.sleep(ms); - } } diff --git a/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/WebSocketServletRFCTest.java b/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/WebSocketServletRFCTest.java new file mode 100644 index 00000000000..0f90cb18969 --- /dev/null +++ b/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/WebSocketServletRFCTest.java @@ -0,0 +1,174 @@ +package org.eclipse.jetty.websocket; + +import static org.hamcrest.Matchers.*; + +import java.io.IOException; +import java.net.HttpURLConnection; +import java.net.URI; +import java.net.URL; +import java.util.concurrent.TimeUnit; + +import javax.servlet.http.HttpServletRequest; + +import org.eclipse.jetty.server.Connector; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.servlet.ServletContextHandler; +import org.eclipse.jetty.servlet.ServletHolder; +import org.eclipse.jetty.websocket.helper.MessageSender; +import org.junit.AfterClass; +import org.junit.Assert; +import org.junit.BeforeClass; +import org.junit.Test; + +/** + * Test various RFC 6455 specified requirements placed on + * {@link WebSocketServlet} + *

+ * This test serves a different purpose than than the {@link WebSocketGeneratorRFC6455Test}, + * {@link WebSocketMessageRFC6455Test}, and {@link WebSocketParserRFC6455Test} tests. + */ +public class WebSocketServletRFCTest +{ + private static class RFCSocket implements WebSocket, WebSocket.OnTextMessage + { + private Connection conn; + + public void onOpen(Connection connection) + { + this.conn = connection; + } + + public void onClose(int closeCode, String message) + { + this.conn = null; + } + + public void onMessage(String data) + { + // Test the RFC 6455 close code 1011 that should close + // trigger a WebSocket server terminated close. + if (data.equals("CRASH")) + { + throw new RuntimeException("Something bad happened"); + } + + // echo the message back. + try + { + conn.sendMessage(data); + } + catch (IOException e) + { + e.printStackTrace(System.err); + } + } + + } + + @SuppressWarnings("serial") + private static class RFCServlet extends WebSocketServlet + { + public WebSocket doWebSocketConnect(HttpServletRequest request, String protocol) + { + return new RFCSocket(); + } + } + + private static Server server; + private static URI serverUri; + + @BeforeClass + public static void startServer() throws Exception + { + // Configure Server + server = new Server(0); + + ServletContextHandler context = new ServletContextHandler(); + context.setContextPath("/"); + server.setHandler(context); + + // Serve capture servlet + context.addServlet(new ServletHolder(new RFCServlet()),"/"); + + // Start Server + server.start(); + + Connector conn = server.getConnectors()[0]; + String host = conn.getHost(); + if (host == null) + { + host = "localhost"; + } + int port = conn.getLocalPort(); + serverUri = new URI(String.format("ws://%s:%d/",host,port)); + System.out.printf("Server URI: %s%n",serverUri); + } + + @AfterClass + public static void stopServer() + { + try + { + server.stop(); + } + catch (Exception e) + { + e.printStackTrace(System.err); + } + } + + /** + * Test the requirement of responding with an http 400 when using a Sec-WebSocket-Version that is unsupported. + */ + @Test + public void testResponseOnInvalidVersion() throws Exception + { + // Using straight HttpUrlConnection to accomplish this as jetty's WebSocketClient + // doesn't allow the use of invalid versions. (obviously) + + URL url = new URL("http://" + serverUri.getHost() + ":" + serverUri.getPort() + "/"); + HttpURLConnection conn = (HttpURLConnection)url.openConnection(); + conn.setRequestProperty("Upgrade","WebSocket"); + conn.setRequestProperty("Connection","Upgrade"); + conn.setRequestProperty("Sec-WebSocket-Version","29"); + conn.connect(); + + Assert.assertEquals("Response Code",400,conn.getResponseCode()); + Assert.assertThat("Response Message",conn.getResponseMessage(),containsString("Unsupported websocket version specification")); + Assert.assertThat("Response Header Versions",conn.getHeaderField("Sec-WebSocket-Version"),is("13, 8, 6, 0")); + + conn.disconnect(); + } + + /** + * Test the requirement of responding with server terminated close code 1011 when there is an unhandled (internal + * server error) being produced by the extended WebSocketServlet. + */ + @Test + public void testResponseOnInternalError() throws Exception + { + WebSocketClientFactory clientFactory = new WebSocketClientFactory(); + clientFactory.start(); + + WebSocketClient wsc = clientFactory.newWebSocketClient(); + MessageSender sender = new MessageSender(); + wsc.open(serverUri,sender); + + try + { + sender.awaitConnect(); + + sender.sendMessage("CRASH"); + + // Give servlet 500 millisecond to process messages + TimeUnit.MILLISECONDS.sleep(500); + + Assert.assertThat("WebSocket should be closed", sender.isConnected(), is(false)); + Assert.assertThat("WebSocket close clode", sender.getCloseCode(), is(1011)); + } + finally + { + sender.close(); + } + } +} diff --git a/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/helper/MessageSender.java b/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/helper/MessageSender.java index 34ceb54c40c..65896aab982 100644 --- a/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/helper/MessageSender.java +++ b/jetty-websocket/src/test/java/org/eclipse/jetty/websocket/helper/MessageSender.java @@ -25,6 +25,8 @@ public class MessageSender implements WebSocket { private Connection conn; private CountDownLatch connectLatch = new CountDownLatch(1); + private int closeCode = -1; + private String closeMessage = null; public void onOpen(Connection connection) { @@ -35,6 +37,8 @@ public class MessageSender implements WebSocket public void onClose(int closeCode, String message) { this.conn = null; + this.closeCode = closeCode; + this.closeMessage = message; } public boolean isConnected() @@ -45,6 +49,16 @@ public class MessageSender implements WebSocket } return this.conn.isOpen(); } + + public int getCloseCode() + { + return closeCode; + } + + public String getCloseMessage() + { + return closeMessage; + } public void sendMessage(String format, Object... args) throws IOException {