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 05ffa07634c..1ecfaffb052 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 @@ -9,18 +9,18 @@ import org.eclipse.jetty.websocket.api.StatusCode; */ public class CloseFrame extends ControlFrame { - private short statusCode; + private int statusCode = 0; private String reason; public CloseFrame() { - this(StatusCode.NORMAL); // TODO: evaluate default (or unspecified status code) + super(OpCode.CLOSE); } - public CloseFrame(short statusCode) + public CloseFrame(int statusCode) { super(OpCode.CLOSE); - this.statusCode = statusCode; + setStatusCode(statusCode); } public String getReason() @@ -28,7 +28,7 @@ public class CloseFrame extends ControlFrame return reason; } - public short getStatusCode() + public int getStatusCode() { return statusCode; } @@ -43,8 +43,13 @@ public class CloseFrame extends ControlFrame this.reason = reason; } - public void setStatusCode(short statusCode) + public void setStatusCode(int statusCode) { + if ( ( statusCode <= 999) || ( statusCode > 65535 ) ) + { + throw new IllegalArgumentException("Status Codes must be in the range 1000 - 65535"); + } + this.statusCode = statusCode; } 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 e3c03ccf5b6..c3dbd29ee63 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 @@ -64,7 +64,7 @@ public abstract class ControlFrame extends BaseFrame } @Override - protected void setPayload(byte[] buf) + public void setPayload(byte[] buf) { if ( buf.length > 125 ) { 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 ea4ef9d8a7b..50bc68c9325 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,6 +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.WebSocketPolicy; import org.eclipse.jetty.websocket.frames.CloseFrame; @@ -16,11 +17,23 @@ public class CloseFrameGenerator extends FrameGenerator @Override public void fillPayload(ByteBuffer buffer, CloseFrame close) { - buffer.putShort(close.getStatusCode()); - if (close.hasReason()) + if ( close.getStatusCode() != 0 ) { - byte utf[] = close.getReason().getBytes(StringUtil.__UTF8_CHARSET); - buffer.put(utf,0,utf.length); + buffer.putChar((char)close.getStatusCode()); // char is unsigned 16 + + // payload requires a status code in order to be written + if ( close.hasPayload() ) + { + if (close.hasReason()) + { + byte utf[] = close.getReason().getBytes(StringUtil.__UTF8_CHARSET); + buffer.put(utf,0,utf.length); + } + } + } + else if ( close.hasPayload() ) + { + throw new WebSocketException("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 b57ba797401..41589d8f97d 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 @@ -4,6 +4,7 @@ import java.nio.ByteBuffer; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.websocket.api.WebSocketException; import org.eclipse.jetty.websocket.api.WebSocketPolicy; import org.eclipse.jetty.websocket.frames.CloseFrame; @@ -44,6 +45,14 @@ public class ClosePayloadParser extends FrameParser // no status code. no reason. return true; } + + /* invalid payload length and protects payload.getShort() call below from + * pulling too many bytes from buffer. + */ + if ( payloadLength == 1 ) + { + throw new WebSocketException("Close: invalid payload length: 1"); + } while (buffer.hasRemaining()) { diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/ab/ABCase7_3.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/ab/ABCase7_3.java new file mode 100644 index 00000000000..8db8380cf4f --- /dev/null +++ b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/ab/ABCase7_3.java @@ -0,0 +1,114 @@ +package org.eclipse.jetty.websocket.ab; + +import static org.hamcrest.Matchers.is; + +import java.nio.ByteBuffer; + +import org.eclipse.jetty.util.StringUtil; +import org.eclipse.jetty.websocket.ByteBufferAssert; +import org.eclipse.jetty.websocket.Debug; +import org.eclipse.jetty.websocket.api.WebSocketBehavior; +import org.eclipse.jetty.websocket.api.WebSocketException; +import org.eclipse.jetty.websocket.api.WebSocketPolicy; +import org.eclipse.jetty.websocket.frames.CloseFrame; +import org.eclipse.jetty.websocket.frames.PingFrame; +import org.eclipse.jetty.websocket.frames.TextFrame; +import org.eclipse.jetty.websocket.generator.Generator; +import org.eclipse.jetty.websocket.parser.FrameParseCapture; +import org.eclipse.jetty.websocket.parser.Parser; +import org.eclipse.jetty.websocket.parser.PingPayloadParser; +import org.junit.Assert; +import org.junit.Test; + +public class ABCase7_3 +{ + WebSocketPolicy policy = new WebSocketPolicy(WebSocketBehavior.SERVER); + + @Test + public void testGenerateEmptyCloseCase7_3_1() + { + CloseFrame closeFrame = new CloseFrame(); + + Generator generator = new Generator(policy); + ByteBuffer actual = ByteBuffer.allocate(32); + generator.generate(actual, closeFrame); + + ByteBuffer expected = ByteBuffer.allocate(5); + + expected.put(new byte[] + { (byte)0x88, (byte)0x00 }); + + actual.flip(); + expected.flip(); + + ByteBufferAssert.assertEquals("buffers do not match",expected,actual); + } + + @Test + public void testParseEmptyCloseCase7_3_1() + { + ByteBuffer expected = ByteBuffer.allocate(5); + + expected.put(new byte[] + { (byte)0x88, (byte)0x00 }); + + expected.flip(); + + Parser parser = new Parser(policy); + FrameParseCapture capture = new FrameParseCapture(); + parser.addListener(capture); + parser.parse(expected); + + capture.assertNoErrors(); + capture.assertHasFrame(CloseFrame.class,1); + + CloseFrame pActual = (CloseFrame)capture.getFrames().get(0); + Assert.assertThat("CloseFrame.payloadLength",pActual.getPayloadLength(),is(0)); + ByteBufferAssert.assertSize("CloseFrame.payload",0,pActual.getPayload()); + + } + + + @Test (expected = WebSocketException.class) + public void testGenerate1BytePayloadCloseCase7_3_2() + { + CloseFrame pingFrame = new CloseFrame(); + pingFrame.setPayload(new byte[] {0x00}); + + Generator generator = new Generator(policy); + ByteBuffer actual = ByteBuffer.allocate(32); + generator.generate(actual, pingFrame); + } + + @Test + public void testParse1BytePayloadCloseCase7_3_2() + { + //Debug.enableDebugLogging(Parser.class); + + String message = "*"; + byte[] messageBytes = message.getBytes(); + + ByteBuffer expected = ByteBuffer.allocate(32); + + expected.put(new byte[] + { (byte)0x88 }); + + byte b = 0x00; // no masking + b |= messageBytes.length & 0x7F; + expected.put(b); + expected.put(messageBytes); + + expected.flip(); + + Parser parser = new Parser(policy); + FrameParseCapture capture = new FrameParseCapture(); + parser.addListener(capture); + parser.parse(expected); + + Assert.assertEquals( "error on invalid close payload", 1, capture.getErrorCount(WebSocketException.class)) ; + + WebSocketException known = capture.getErrors().get(0); + + Assert.assertTrue("undefined option should be in message",known.getMessage().contains("invalid payload length")); + } +} diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/parser/ClosePayloadParserTest.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/parser/ClosePayloadParserTest.java index d6a81f912f5..e450c392b10 100644 --- a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/parser/ClosePayloadParserTest.java +++ b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/parser/ClosePayloadParserTest.java @@ -41,7 +41,7 @@ public class ClosePayloadParserTest capture.assertNoErrors(); capture.assertHasFrame(CloseFrame.class,1); CloseFrame txt = (CloseFrame)capture.getFrames().get(0); - Assert.assertThat("CloseFrame.statusCode",txt.getStatusCode(),is(StatusCode.NORMAL)); + Assert.assertThat("CloseFrame.statusCode",(short)txt.getStatusCode(),is(StatusCode.NORMAL)); Assert.assertThat("CloseFrame.data",txt.getReason(),is(expectedReason)); } }