diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/CloseException.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/CloseException.java index ce35114f7eb..aa3c41a191a 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/CloseException.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/CloseException.java @@ -3,29 +3,29 @@ package org.eclipse.jetty.websocket.api; @SuppressWarnings("serial") public class CloseException extends WebSocketException { - private short closeCode; + private short statusCode; public CloseException(short closeCode, String message) { super(message); - this.closeCode = closeCode; + this.statusCode = closeCode; } public CloseException(short closeCode, String message, Throwable cause) { super(message,cause); - this.closeCode = closeCode; + this.statusCode = closeCode; } public CloseException(short closeCode, Throwable cause) { super(cause); - this.closeCode = closeCode; + this.statusCode = closeCode; } - public short getCloseCode() + public short getStatusCode() { - return closeCode; + return statusCode; } } diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/ProtocolException.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/ProtocolException.java new file mode 100644 index 00000000000..678b3919cb2 --- /dev/null +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/ProtocolException.java @@ -0,0 +1,23 @@ +package org.eclipse.jetty.websocket.api; + +/** + * Per spec, a protocol error should result in a Close frame of status code 1002 (PROTOCOL_ERROR) + */ +@SuppressWarnings("serial") +public class ProtocolException extends CloseException +{ + public ProtocolException(String message) + { + super(StatusCode.PROTOCOL,message); + } + + public ProtocolException(String message, Throwable t) + { + super(StatusCode.PROTOCOL,message,t); + } + + public ProtocolException(Throwable t) + { + super(StatusCode.PROTOCOL,t); + } +} diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/WebSocketEventDriver.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/WebSocketEventDriver.java index c0e15d461d6..ab74b8bf7ef 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/WebSocketEventDriver.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/api/WebSocketEventDriver.java @@ -4,6 +4,7 @@ import java.io.IOException; import java.nio.ByteBuffer; import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; import org.eclipse.jetty.websocket.annotations.WebSocket; @@ -164,6 +165,12 @@ public class WebSocketEventDriver implements Parser.Listener LOG.debug("{}.onWebSocketException({})",websocket.getClass().getSimpleName(),e); } + if (e instanceof CloseException) + { + CloseException close = (CloseException)e; + terminateConnection(close.getStatusCode(),close.getMessage()); + } + if (events.onException != null) { events.onException.call(websocket,connection,e); @@ -181,27 +188,41 @@ public class WebSocketEventDriver implements Parser.Listener this.connection = conn; } - private void unhandled(Throwable t) + private void terminateConnection(int statusCode, String rawreason) { - LOG.warn("Unhandled Error (closing connection)",t); - - // Unhandled Error, close the connection. try { - - switch (policy.getBehavior()) + String reason = rawreason; + if (StringUtil.isNotBlank(reason)) { - case SERVER: - connection.close(StatusCode.SERVER_ERROR,t.getClass().getSimpleName()); - break; - case CLIENT: - connection.close(StatusCode.POLICY_VIOLATION,t.getClass().getSimpleName()); - break; + // Trim big exception messages here. + if (reason.length() > CloseFrame.MAX_REASON) + { + reason = reason.substring(0,CloseFrame.MAX_REASON); + } } + LOG.debug("terminateConnection({},{})",statusCode,reason); + connection.close(statusCode,reason); } catch (IOException e) { LOG.debug(e); } } + + private void unhandled(Throwable t) + { + LOG.warn("Unhandled Error (closing connection)",t); + + // Unhandled Error, close the connection. + switch (policy.getBehavior()) + { + case SERVER: + terminateConnection(StatusCode.SERVER_ERROR,t.getClass().getSimpleName()); + break; + case CLIENT: + terminateConnection(StatusCode.POLICY_VIOLATION,t.getClass().getSimpleName()); + break; + } + } } diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/CloseFrame.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/CloseFrame.java index 8198ef69a97..f82400edde5 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/CloseFrame.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/CloseFrame.java @@ -4,12 +4,17 @@ import java.nio.ByteBuffer; import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.websocket.api.OpCode; +import org.eclipse.jetty.websocket.api.ProtocolException; +import org.eclipse.jetty.websocket.api.StatusCode; +import org.eclipse.jetty.websocket.api.WebSocketBehavior; /** * Representation of a Close Frame (0x08). */ public class CloseFrame extends ControlFrame { + public static final int MAX_REASON = ControlFrame.MAX_PAYLOAD - 2; + /** * Construct CloseFrame with no status code or reason */ @@ -38,19 +43,29 @@ public class CloseFrame extends ControlFrame public void assertValidPayload(int statusCode, String reason) { - if ((statusCode <= 999) || (statusCode > 65535)) + if ((statusCode < StatusCode.NORMAL) || (statusCode >= 5000)) { - throw new IllegalArgumentException("Status Codes must be in the range 1000 - 65535"); + throw new ProtocolException("Status Codes must be in the range 1000 - 5000"); } if ((reason != null) && (reason.length() > 123)) { - throw new IllegalArgumentException("Reason must not exceed 123 characters."); + throw new ProtocolException("Reason must not exceed 123 characters."); } // TODO add check for invalid utf-8 } + public void assertValidPerPolicy(WebSocketBehavior behavior) + { + int code = getStatusCode(); + if ((code < StatusCode.NORMAL) || (code == StatusCode.UNDEFINED) || (code == StatusCode.NO_CLOSE) || (code == StatusCode.NO_CODE) + || ((code > 1011) && (code <= 2999)) || (code >= 5000)) + { + throw new ProtocolException("Invalid close code: " + code); + } + } + private void constructPayload(int statusCode, String reason) { assertValidPayload(statusCode,reason); @@ -103,7 +118,6 @@ public class CloseFrame extends ControlFrame } - public boolean hasReason() { return getPayloadLength() > 2; @@ -116,7 +130,6 @@ public class CloseFrame extends ControlFrame assertValidPayload(getStatusCode(),getReason()); } - @Override public void setPayload(ByteBuffer payload) { diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/ControlFrame.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/ControlFrame.java index c3dbd29ee63..9326f1e4209 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/ControlFrame.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/ControlFrame.java @@ -3,10 +3,13 @@ package org.eclipse.jetty.websocket.frames; import java.nio.ByteBuffer; import org.eclipse.jetty.websocket.api.OpCode; +import org.eclipse.jetty.websocket.api.ProtocolException; import org.eclipse.jetty.websocket.api.WebSocketException; public abstract class ControlFrame extends BaseFrame { + public static final int MAX_PAYLOAD = 125; + public ControlFrame() { super(); @@ -24,7 +27,38 @@ public abstract class ControlFrame extends BaseFrame { return false; // no control frames can be continuation } - + + @Override + public void setFin(boolean fin) + { + if (!fin) + { + throw new IllegalArgumentException("Cannot set FIN to off on a " + getOpCode().name()); + } + } + + @Override + public void setPayload(byte[] buf) + { + if ( buf.length > 125 ) + { + throw new WebSocketException("Control Payloads can not exceed 125 bytes in length."); + } + + super.setPayload(buf); + } + + @Override + public void setPayload(ByteBuffer payload) + { + if (payload.position() > MAX_PAYLOAD) + { + throw new ProtocolException("Control Payloads can not exceed 125 bytes in length."); + } + + super.setPayload(payload); + } + @Override public void setRsv1(boolean rsv1) { @@ -51,35 +85,4 @@ public abstract class ControlFrame extends BaseFrame throw new IllegalArgumentException("Cannot set RSV3 to true on a " + getOpCode().name()); } } - - @Override - public void setPayload(ByteBuffer payload) - { - if ( payload.position() > 125 ) - { - throw new WebSocketException("Control Payloads can not exceed 125 bytes in length."); - } - - super.setPayload(payload); - } - - @Override - public void setPayload(byte[] buf) - { - if ( buf.length > 125 ) - { - throw new WebSocketException("Control Payloads can not exceed 125 bytes in length."); - } - - super.setPayload(buf); - } - - @Override - public void setFin(boolean fin) - { - if (!fin) - { - throw new IllegalArgumentException("Cannot set FIN to off on a " + getOpCode().name()); - } - } } diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/PingFrame.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/PingFrame.java index 4c1c635b592..b8ee2627660 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/PingFrame.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/frames/PingFrame.java @@ -1,9 +1,6 @@ package org.eclipse.jetty.websocket.frames; -import java.nio.ByteBuffer; - import org.eclipse.jetty.util.BufferUtil; -import org.eclipse.jetty.util.StringUtil; import org.eclipse.jetty.websocket.api.OpCode; /** diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/generator/CloseFrameGenerator.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/generator/CloseFrameGenerator.java index dcaa400a402..66361760f3b 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/generator/CloseFrameGenerator.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/generator/CloseFrameGenerator.java @@ -3,7 +3,7 @@ package org.eclipse.jetty.websocket.generator; import java.nio.ByteBuffer; import org.eclipse.jetty.util.StringUtil; -import org.eclipse.jetty.websocket.api.WebSocketException; +import org.eclipse.jetty.websocket.api.ProtocolException; import org.eclipse.jetty.websocket.api.WebSocketPolicy; import org.eclipse.jetty.websocket.frames.CloseFrame; @@ -33,7 +33,7 @@ public class CloseFrameGenerator extends FrameGenerator } else if (close.hasPayload()) { - throw new WebSocketException("Close frames require setting a status code if using payload."); + throw new ProtocolException("Close frames require setting a status code if using payload."); } } } diff --git a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/parser/ClosePayloadParser.java b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/parser/ClosePayloadParser.java index b922b141585..7eb50c51a93 100644 --- a/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/parser/ClosePayloadParser.java +++ b/jetty-websocket/websocket-core/src/main/java/org/eclipse/jetty/websocket/parser/ClosePayloadParser.java @@ -2,7 +2,8 @@ package org.eclipse.jetty.websocket.parser; import java.nio.ByteBuffer; -import org.eclipse.jetty.websocket.api.WebSocketException; +import javax.xml.ws.ProtocolException; + import org.eclipse.jetty.websocket.api.WebSocketPolicy; import org.eclipse.jetty.websocket.frames.CloseFrame; @@ -49,7 +50,7 @@ public class ClosePayloadParser extends FrameParser */ if ((payloadLength == 1) || (payloadLength > 125)) { - throw new WebSocketException("Close: invalid payload length: " + payloadLength); + throw new ProtocolException("Close: invalid payload length: " + payloadLength); } if (payload == null) @@ -65,6 +66,7 @@ public class ClosePayloadParser extends FrameParser if (payload.position() >= payloadLength) { frame.setPayload(payload); + frame.assertValidPerPolicy(getPolicy().getBehavior()); return true; } } diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/parser/TextPayloadParserTest.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/parser/TextPayloadParserTest.java index a40cabcf8f2..98a03afc213 100644 --- a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/parser/TextPayloadParserTest.java +++ b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/parser/TextPayloadParserTest.java @@ -43,7 +43,7 @@ public class TextPayloadParserTest capture.assertHasNoFrames(); PolicyViolationException err = (PolicyViolationException)capture.getErrors().get(0); - Assert.assertThat("Error.closeCode",err.getCloseCode(),is(StatusCode.POLICY_VIOLATION)); + Assert.assertThat("Error.closeCode",err.getStatusCode(),is(StatusCode.POLICY_VIOLATION)); } @Test diff --git a/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/ab/TestABCase7_9.java b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/ab/TestABCase7_9.java index 59426d80a48..e0063f21120 100644 --- a/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/ab/TestABCase7_9.java +++ b/jetty-websocket/websocket-server/src/test/java/org/eclipse/jetty/websocket/server/ab/TestABCase7_9.java @@ -1,30 +1,31 @@ package org.eclipse.jetty.websocket.server.ab; -import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.*; import java.io.IOException; +import java.nio.ByteBuffer; import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Queue; import java.util.concurrent.TimeUnit; +import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.log.Log; import org.eclipse.jetty.util.log.Logger; +import org.eclipse.jetty.websocket.api.OpCode; import org.eclipse.jetty.websocket.api.WebSocketAdapter; import org.eclipse.jetty.websocket.frames.BaseFrame; import org.eclipse.jetty.websocket.frames.CloseFrame; -import org.eclipse.jetty.websocket.frames.TextFrame; +import org.eclipse.jetty.websocket.generator.FrameGenerator; import org.eclipse.jetty.websocket.server.SimpleServletServer; import org.eclipse.jetty.websocket.server.WebSocketServerFactory; import org.eclipse.jetty.websocket.server.WebSocketServlet; -import org.eclipse.jetty.websocket.server.WebSocketServletRFCTest.RFCServlet; -import org.eclipse.jetty.websocket.server.WebSocketServletRFCTest.RFCSocket; import org.eclipse.jetty.websocket.server.blockhead.BlockheadClient; +import org.eclipse.jetty.websocket.server.examples.MyEchoServlet; import org.junit.AfterClass; import org.junit.Assert; import org.junit.BeforeClass; -import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.Parameterized; @@ -33,37 +34,6 @@ import org.junit.runners.Parameterized.Parameters; @RunWith(value = Parameterized.class) public class TestABCase7_9 { - private int invalidStatusCode; - - @Parameters - public static Collection data() - { - List data = new ArrayList<>(); - // @formatter:off - data.add(new Integer[] { new Integer(0) }); - data.add(new Integer[] { new Integer(999) }); - data.add(new Integer[] { new Integer(1004) }); - data.add(new Integer[] { new Integer(1005) }); - data.add(new Integer[] { new Integer(1006) }); - data.add(new Integer[] { new Integer(1012) }); - data.add(new Integer[] { new Integer(1013) }); - data.add(new Integer[] { new Integer(1014) }); - data.add(new Integer[] { new Integer(1015) }); - data.add(new Integer[] { new Integer(1016) }); - data.add(new Integer[] { new Integer(1100) }); - data.add(new Integer[] { new Integer(2000) }); - data.add(new Integer[] { new Integer(2999) }); - - // @formatter:on - return data; - } - - public TestABCase7_9(Integer invalidStatusCode ) - { - this.invalidStatusCode = invalidStatusCode; - } - - @SuppressWarnings("serial") public static class RFCServlet extends WebSocketServlet { @@ -104,10 +74,33 @@ public class TestABCase7_9 private static SimpleServletServer server; + @Parameters + public static Collection data() + { + List data = new ArrayList<>(); + // @formatter:off + data.add(new Integer[] { new Integer(0) }); + data.add(new Integer[] { new Integer(999) }); + data.add(new Integer[] { new Integer(1004) }); + data.add(new Integer[] { new Integer(1005) }); + data.add(new Integer[] { new Integer(1006) }); + data.add(new Integer[] { new Integer(1012) }); + data.add(new Integer[] { new Integer(1013) }); + data.add(new Integer[] { new Integer(1014) }); + data.add(new Integer[] { new Integer(1015) }); + data.add(new Integer[] { new Integer(1016) }); + data.add(new Integer[] { new Integer(1100) }); + data.add(new Integer[] { new Integer(2000) }); + data.add(new Integer[] { new Integer(2999) }); + + // @formatter:on + return data; + } + @BeforeClass public static void startServer() throws Exception { - server = new SimpleServletServer(new RFCServlet()); + server = new SimpleServletServer(new MyEchoServlet()); server.start(); } @@ -117,11 +110,29 @@ public class TestABCase7_9 server.stop(); } + private int invalidStatusCode; + + public TestABCase7_9(Integer invalidStatusCode) + { + this.invalidStatusCode = invalidStatusCode; + } + + private void remask(ByteBuffer buf, int position, byte[] mask) + { + int end = buf.position(); + int off; + for (int i = position; i < end; i++) + { + off = i - position; + // Mask each byte by its absolute position in the bytebuffer + buf.put(i,(byte)(buf.get(i) ^ mask[off % 4])); + } + } + /** * Test the requirement of issuing */ @Test - @Ignore ("tossing a buffer overflow exception for some reason") public void testCase7_9_XInvalidCloseStatusCodes() throws Exception { BlockheadClient client = new BlockheadClient(server.getServerUri()); @@ -131,21 +142,25 @@ public class TestABCase7_9 client.sendStandardRequest(); client.expectUpgradeResponse(); - // Generate text frame - client.write(new CloseFrame(invalidStatusCode) - { - @Override - public void assertValidPayload(int statusCode, String reason) - { + ByteBuffer buf = ByteBuffer.allocate(FrameGenerator.OVERHEAD + 2); + BufferUtil.clearToFill(buf); - } - - }); + // Create Close Frame manually, as we are testing the server's behavior of a bad client. + buf.put((byte)(0x80 | OpCode.CLOSE.getCode())); + buf.put((byte)(0x80 | 2)); + byte mask[] = new byte[] + { 0x44, 0x44, 0x44, 0x44 }; + buf.put(mask); + int position = buf.position(); + buf.putChar((char)this.invalidStatusCode); + remask(buf,position,mask); + BufferUtil.flipToFlush(buf,0); + client.writeRaw(buf); // Read frame (hopefully text frame) Queue frames = client.readFrames(1,TimeUnit.MILLISECONDS,500); CloseFrame closeFrame = (CloseFrame)frames.remove(); - Assert.assertThat("CloseFrame.status code", closeFrame.getStatusCode(),is(1002)); + Assert.assertThat("CloseFrame.status code",closeFrame.getStatusCode(),is(1002)); } finally { @@ -153,6 +168,4 @@ public class TestABCase7_9 } } - - }