From 153daf4f3b4f146fa42cc348cdb39547446f8e5b Mon Sep 17 00:00:00 2001 From: Jesse McConnell Date: Thu, 28 Jun 2012 16:32:50 -0500 Subject: [PATCH] tossing exceptions on a couple more edgecases and additional tests on close --- .../jetty/websocket/frames/CloseFrame.java | 8 +- .../generator/CloseFrameGenerator.java | 6 +- .../websocket/parser/ClosePayloadParser.java | 4 +- .../jetty/websocket/ab/TestABCase7_3.java | 356 +++++++++++++----- 4 files changed, 279 insertions(+), 95 deletions(-) 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 2877a4527ba..36cd25890e8 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 @@ -2,7 +2,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; @@ -17,8 +16,6 @@ public class CloseFrame extends ControlFrame public CloseFrame() { super(OpCode.CLOSE); - // no status code, no reason - setPayload(BufferUtil.EMPTY_BUFFER); } /** @@ -46,6 +43,11 @@ public class CloseFrame extends ControlFrame throw new IllegalArgumentException("Status Codes must be in the range 1000 - 65535"); } + if ((reason != null) && (reason.length() > 123)) + { + throw new IllegalArgumentException("Reason must not exceed 123 characters."); + } + byte utf[] = null; int len = 2; // status code if (StringUtil.isNotBlank(reason)) 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 50bc68c9325..dcaa400a402 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 @@ -20,10 +20,10 @@ public class CloseFrameGenerator extends FrameGenerator if ( close.getStatusCode() != 0 ) { 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); @@ -31,7 +31,7 @@ public class CloseFrameGenerator extends FrameGenerator } } } - else if ( close.hasPayload() ) + 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 4d67dca3197..b922b141585 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 @@ -47,9 +47,9 @@ public class ClosePayloadParser extends FrameParser /* * invalid payload length. */ - if (payloadLength == 1) + if ((payloadLength == 1) || (payloadLength > 125)) { - throw new WebSocketException("Close: invalid payload length: 1"); + throw new WebSocketException("Close: invalid payload length: " + payloadLength); } if (payload == null) diff --git a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/ab/TestABCase7_3.java b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/ab/TestABCase7_3.java index 91826725322..3ee88f2e739 100644 --- a/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/ab/TestABCase7_3.java +++ b/jetty-websocket/websocket-core/src/test/java/org/eclipse/jetty/websocket/ab/TestABCase7_3.java @@ -4,19 +4,15 @@ 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; @@ -24,90 +20,21 @@ public class TestABCase7_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); - - ByteBuffer expected = ByteBuffer.allocate(32); - expected.put(new byte[] - { (byte)0x88, 0x01, 0x00 }); - - 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("invalid payload should be in message",known.getMessage().contains("invalid payload length")); - } - @Test public void testGenerateCloseWithStatusCase7_3_3() { - CloseFrame closeFrame = new CloseFrame(1000); + CloseFrame closeFrame = new CloseFrame(1000); Generator generator = new Generator(policy); ByteBuffer actual = ByteBuffer.allocate(32); @@ -117,35 +44,290 @@ public class TestABCase7_3 expected.put(new byte[] { (byte)0x88, (byte)0x02, 0x03, (byte)0xe8 }); - + actual.flip(); expected.flip(); - + ByteBufferAssert.assertEquals("buffers do not match",expected,actual); } - + + @Test - public void testParseCloseWithStatusCase7_3_3() - { + public void testGenerateCloseWithStatusMaxReasonCase7_3_5() + { + StringBuilder message = new StringBuilder(); + for ( int i = 0 ; i < 123 ; ++i ) + { + message.append("*"); + } + + byte[] messageBytes = message.toString().getBytes(); + + CloseFrame closeFrame = new CloseFrame(1000, message.toString()); + + Generator generator = new Generator(policy); + ByteBuffer actual = ByteBuffer.allocate(132); + generator.generate(actual, closeFrame); + + ByteBuffer expected = ByteBuffer.allocate(132); + + + expected.put(new byte[] + { (byte)0x88 }); + + byte b = 0x00; // no masking + b |= (messageBytes.length + 2) & 0x7F; + expected.put(b); + expected.putShort((short)1000); + + expected.put(messageBytes); + + actual.flip(); + expected.flip(); + + ByteBufferAssert.assertEquals("buffers do not match",expected,actual); + } + + @Test (expected = IllegalArgumentException.class ) + public void testGenerateCloseWithStatusMaxReasonCase7_3_6() + { + StringBuilder message = new StringBuilder(); + for ( int i = 0 ; i < 124 ; ++i ) + { + message.append("*"); + } + + byte[] messageBytes = message.toString().getBytes(); + + CloseFrame closeFrame = new CloseFrame(1000, message.toString()); + + } + + @Test + public void testGenerateCloseWithStatusReasonCase7_3_4() + { + String message = "bad cough"; + byte[] messageBytes = message.getBytes(); + + CloseFrame closeFrame = new CloseFrame(1000, message); + + Generator generator = new Generator(policy); + ByteBuffer actual = ByteBuffer.allocate(32); + generator.generate(actual, closeFrame); + + ByteBuffer expected = ByteBuffer.allocate(32); + + expected.put(new byte[] + { (byte)0x88 }); + + byte b = 0x00; // no masking + b |= (message.length() + 2) & 0x7F; + expected.put(b); + expected.putShort((short)1000); + expected.put(messageBytes); + + actual.flip(); + expected.flip(); + + ByteBufferAssert.assertEquals("buffers do not match",expected,actual); + } + + @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 testParse1BytePayloadCloseCase7_3_2() + { + Debug.enableDebugLogging(Parser.class); + + ByteBuffer expected = ByteBuffer.allocate(32); + + expected.put(new byte[] + { (byte)0x88, 0x01, 0x00 }); + + 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("invalid payload should be in message",known.getMessage().contains("invalid payload length")); + } + + @Test + public void testParseCloseWithStatusCase7_3_3() + { + ByteBuffer expected = ByteBuffer.allocate(5); + + expected.put(new byte[] + { (byte)0x88, (byte)0x02, 0x03, (byte)0xe8 }); + + 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(2)); + ByteBufferAssert.assertSize("CloseFrame.payload",2,pActual.getPayload()); + + } + + + @Test + public void testParseCloseWithStatusMaxReasonCase7_3_5() + { + StringBuilder message = new StringBuilder(); + for ( int i = 0 ; i < 123 ; ++i ) + { + message.append("*"); + } + + byte[] messageBytes = message.toString().getBytes(); + + ByteBuffer expected = ByteBuffer.allocate(132); + + expected.put(new byte[] + { (byte)0x88 }); + byte b = 0x00; // no masking + + b |= (messageBytes.length + 2) & 0x7F; + expected.put(b); + expected.putShort((short)1000); + + expected.put(messageBytes); + 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(125)); + ByteBufferAssert.assertSize("CloseFrame.payload", 125,pActual.getPayload()); + + } + + @Test + public void testParseCloseWithStatusMaxReasonCase7_3_6() + { + StringBuilder message = new StringBuilder(); + for ( int i = 0 ; i < 124 ; ++i ) + { + message.append("*"); + } + + byte[] messageBytes = message.toString().getBytes(); + + ByteBuffer expected = ByteBuffer.allocate(132); + + expected.put(new byte[] + { (byte)0x88 }); + byte b = 0x00; // no masking + + b |= (messageBytes.length + 2) & 0x7F; + expected.put(b); + expected.putShort((short)1000); + + 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("invalid payload should be in message",known.getMessage().contains("invalid payload length")); + } + + @Test + public void testParseCloseWithStatusReasonCase7_3_4() + { + String message = "bad cough"; + byte[] messageBytes = message.getBytes(); + + ByteBuffer expected = ByteBuffer.allocate(32); + + expected.put(new byte[] + { (byte)0x88 }); + byte b = 0x00; // no masking + b |= (messageBytes.length + 2) & 0x7F; + expected.put(b); + expected.putShort((short)1000); + expected.put(messageBytes); + 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(messageBytes.length + 2)); + ByteBufferAssert.assertSize("CloseFrame.payload",messageBytes.length + 2,pActual.getPayload()); + + } + + @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()); - + ByteBufferAssert.assertSize("CloseFrame.payload",0,pActual.getPayload()); + } - }