From baccca8b38115e4b010765259292427807d51cdc Mon Sep 17 00:00:00 2001 From: Peter Shugalev Date: Fri, 4 Nov 2016 17:32:55 +0300 Subject: [PATCH 1/4] Added channel name to bad base64/json data log messages --- .../java/io/ably/lib/realtime/Channel.java | 18 +++++++----------- .../java/io/ably/lib/types/BaseMessage.java | 8 +++----- .../ably/lib/types/MessageDecodeException.java | 17 +++++++++++++++++ .../io/ably/lib/types/MessageSerializer.java | 15 ++++++++++++--- .../io/ably/lib/types/PresenceSerializer.java | 15 ++++++++++++--- 5 files changed, 51 insertions(+), 22 deletions(-) create mode 100644 lib/src/main/java/io/ably/lib/types/MessageDecodeException.java diff --git a/lib/src/main/java/io/ably/lib/realtime/Channel.java b/lib/src/main/java/io/ably/lib/realtime/Channel.java index 8512c4172..0d07c70be 100644 --- a/lib/src/main/java/io/ably/lib/realtime/Channel.java +++ b/lib/src/main/java/io/ably/lib/realtime/Channel.java @@ -5,15 +5,7 @@ import io.ably.lib.http.Http.BodyHandler; import io.ably.lib.transport.ConnectionManager; import io.ably.lib.transport.ConnectionManager.QueuedMessage; -import io.ably.lib.types.AblyException; -import io.ably.lib.types.ChannelOptions; -import io.ably.lib.types.ErrorInfo; -import io.ably.lib.types.Message; -import io.ably.lib.types.MessageSerializer; -import io.ably.lib.types.PaginatedResult; -import io.ably.lib.types.Param; -import io.ably.lib.types.PresenceMessage; -import io.ably.lib.types.ProtocolMessage; +import io.ably.lib.types.*; import io.ably.lib.types.ProtocolMessage.Action; import io.ably.lib.types.ProtocolMessage.Flag; import io.ably.lib.util.EventEmitter; @@ -383,8 +375,10 @@ private void onMessage(ProtocolMessage message) { Message msg = messages[i]; try { msg.decode(options); + } catch (MessageDecodeException e) { + Log.e(TAG, String.format("%s on channel %s", e.errorInfo.message, name)); } catch(AblyException e) { - Log.e(TAG, "Unexpected exception decrypting message", e); + Log.e(TAG, String.format("Unexpected exception decrypting message on channel %s", name), e); } /* populate fields derived from protocol message */ if(msg.connectionId == null) msg.connectionId = message.connectionId; @@ -408,8 +402,10 @@ private void onPresence(ProtocolMessage message, String syncChannelSerial) { PresenceMessage msg = messages[i]; try { msg.decode(options); + } catch (MessageDecodeException e) { + Log.e(TAG, String.format("%s on channel %s", e.errorInfo.message, name)); } catch(AblyException e) { - Log.e(TAG, "Unexpected exception decrypting message", e); + Log.e(TAG, String.format("Unexpected exception decrypting message on channel %s", name), e); } /* populate fields derived from protocol message */ if(msg.connectionId == null) msg.connectionId = message.connectionId; diff --git a/lib/src/main/java/io/ably/lib/types/BaseMessage.java b/lib/src/main/java/io/ably/lib/types/BaseMessage.java index 47a222691..688ed5e45 100644 --- a/lib/src/main/java/io/ably/lib/types/BaseMessage.java +++ b/lib/src/main/java/io/ably/lib/types/BaseMessage.java @@ -82,8 +82,7 @@ public void decode(ChannelOptions opts) throws AblyException { data = Base64Coder.decode((String) data); } catch (IllegalArgumentException e) { - Log.e(TAG, "Invalid base64 data received"); - break; + throw MessageDecodeException.fromDescription("Invalid base64 data received"); } continue; } @@ -97,8 +96,7 @@ public void decode(ChannelOptions opts) throws AblyException { data = Serialisation.gsonParser.parse(jsonText); } catch(JsonParseException e) { - Log.e(TAG, "Invalid JSON data received"); - break; + throw MessageDecodeException.fromDescription("Invalid JSON data received"); } continue; } @@ -108,7 +106,7 @@ public void decode(ChannelOptions opts) throws AblyException { continue; } else { - Log.i(TAG, "Encrypted message received but encryption is not set up"); + throw MessageDecodeException.fromDescription("Encrypted message received but encryption is not set up"); } } break; diff --git a/lib/src/main/java/io/ably/lib/types/MessageDecodeException.java b/lib/src/main/java/io/ably/lib/types/MessageDecodeException.java new file mode 100644 index 000000000..89b104c5a --- /dev/null +++ b/lib/src/main/java/io/ably/lib/types/MessageDecodeException.java @@ -0,0 +1,17 @@ +package io.ably.lib.types; + +/** + * Special AblyException for message decoding problems + */ +public class MessageDecodeException extends AblyException { + + private MessageDecodeException(Throwable e, String description) { + super(e, new ErrorInfo(description, 91200)); + } + + public static MessageDecodeException fromDescription(String description) { + return new MessageDecodeException( + new Exception(description), + description); + } +} diff --git a/lib/src/main/java/io/ably/lib/types/MessageSerializer.java b/lib/src/main/java/io/ably/lib/types/MessageSerializer.java index 0d5f0543c..37a834468 100644 --- a/lib/src/main/java/io/ably/lib/types/MessageSerializer.java +++ b/lib/src/main/java/io/ably/lib/types/MessageSerializer.java @@ -3,6 +3,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; +import io.ably.lib.util.Log; import org.msgpack.core.MessagePack; import org.msgpack.core.MessagePacker; import org.msgpack.core.MessageUnpacker; @@ -112,9 +113,15 @@ public Message[] handleResponseBody(String contentType, byte[] body) throws Ably messages = readJSON(body); else if("application/x-msgpack".equals(contentType)) messages = readMsgpack(body); - if(messages != null) - for(Message message : messages) - message.decode(opts); + if(messages != null) { + for (Message message : messages) { + try { + message.decode(opts); + } catch (MessageDecodeException e) { + Log.e(TAG, e.errorInfo.message); + } + } + } return messages; } catch(IOException e) { throw AblyException.fromThrowable(e); @@ -125,4 +132,6 @@ else if("application/x-msgpack".equals(contentType)) } private static BodyHandler messageResponseHandler = new MessageBodyHandler(null); + + private static final String TAG = MessageSerializer.class.getName(); } diff --git a/lib/src/main/java/io/ably/lib/types/PresenceSerializer.java b/lib/src/main/java/io/ably/lib/types/PresenceSerializer.java index 300bc383c..3c3c0d0f6 100644 --- a/lib/src/main/java/io/ably/lib/types/PresenceSerializer.java +++ b/lib/src/main/java/io/ably/lib/types/PresenceSerializer.java @@ -3,6 +3,7 @@ import java.io.ByteArrayOutputStream; import java.io.IOException; +import io.ably.lib.util.Log; import org.msgpack.core.MessagePack; import org.msgpack.core.MessagePacker; import org.msgpack.core.MessageUnpacker; @@ -103,9 +104,15 @@ public PresenceMessage[] handleResponseBody(String contentType, byte[] body) thr messages = readJSON(body); else if("application/x-msgpack".equals(contentType)) messages = readMsgpack(body); - if(messages != null) - for(PresenceMessage message : messages) - message.decode(opts); + if(messages != null) { + for (PresenceMessage message : messages) { + try { + message.decode(opts); + } catch (MessageDecodeException e) { + Log.e(TAG, e.errorInfo.message); + } + } + } return messages; } catch(IOException e) { throw AblyException.fromThrowable(e); @@ -116,4 +123,6 @@ else if("application/x-msgpack".equals(contentType)) } private static BodyHandler presenceResponseHandler = new PresenceBodyHandler(null); + + private static final String TAG = PresenceSerializer.class.getName(); } From 8701cb87aa9533794190a5e13fa930b41f8f6ec0 Mon Sep 17 00:00:00 2001 From: Peter Shugalev Date: Mon, 7 Nov 2016 16:29:05 +0300 Subject: [PATCH 2/4] Fixed crypto-related tests, clarified BaseMessage code --- .../java/io/ably/lib/types/BaseMessage.java | 105 +++++++++--------- .../realtime/RealtimeCryptoMessageTest.java | 21 ++-- 2 files changed, 67 insertions(+), 59 deletions(-) diff --git a/lib/src/main/java/io/ably/lib/types/BaseMessage.java b/lib/src/main/java/io/ably/lib/types/BaseMessage.java index 688ed5e45..08886c34f 100644 --- a/lib/src/main/java/io/ably/lib/types/BaseMessage.java +++ b/lib/src/main/java/io/ably/lib/types/BaseMessage.java @@ -76,38 +76,38 @@ public void decode(ChannelOptions opts) throws AblyException { while((i = j) > 0) { Matcher match = xformPattern.matcher(xforms[--j]); if(!match.matches()) break; - String xform = match.group(1).intern(); - if(xform == "base64") { - try { - data = Base64Coder.decode((String) data); - } - catch (IllegalArgumentException e) { - throw MessageDecodeException.fromDescription("Invalid base64 data received"); - } - continue; - } - if(xform == "utf-8") { - try { data = new String((byte[])data, "UTF-8"); } catch(UnsupportedEncodingException e) {} - continue; - } - if(xform == "json") { - try { - String jsonText = ((String)data).trim(); - data = Serialisation.gsonParser.parse(jsonText); - } - catch(JsonParseException e) { - throw MessageDecodeException.fromDescription("Invalid JSON data received"); - } - continue; - } - if(xform == "cipher") { - if(opts != null && opts.encrypted) { - data = opts.getCipher().decrypt((byte[]) data); + switch(match.group(1)) { + case "base64": + try { + data = Base64Coder.decode((String) data); + } + catch (IllegalArgumentException e) { + throw MessageDecodeException.fromDescription("Invalid base64 data received"); + } + continue; + + case "utf-8": + try { data = new String((byte[])data, "UTF-8"); } catch(UnsupportedEncodingException e) {} + continue; + + case "json": + try { + String jsonText = ((String)data).trim(); + data = Serialisation.gsonParser.parse(jsonText); + } + catch(JsonParseException e) { + throw MessageDecodeException.fromDescription("Invalid JSON data received"); + } continue; - } - else { - throw MessageDecodeException.fromDescription("Encrypted message received but encryption is not set up"); - } + + case "cipher": + if(opts != null && opts.encrypted) { + data = opts.getCipher().decrypt((byte[]) data); + continue; + } + else { + throw MessageDecodeException.fromDescription("Encrypted message received but encryption is not set up"); + } } break; } @@ -177,26 +177,29 @@ public JsonElement serialize(BaseMessage message, Type typeOfMessage, JsonSerial /* Msgpack processing */ boolean readField(MessageUnpacker unpacker, String fieldName, MessageFormat fieldType) throws IOException { boolean result = true; - if(fieldName == "timestamp") { - timestamp = unpacker.unpackLong(); - } else if(fieldName == "id") { - id = unpacker.unpackString(); - } else if(fieldName == "clientId") { - clientId = unpacker.unpackString(); - } else if(fieldName == "connectionId") { - connectionId = unpacker.unpackString(); - } else if(fieldName == "encoding") { - encoding = unpacker.unpackString(); - } else if(fieldName == "data") { - if(fieldType.getValueType().isBinaryType()) { - byte[] byteData = new byte[unpacker.unpackBinaryHeader()]; - unpacker.readPayload(byteData); - data = byteData; - } else { - data = unpacker.unpackString(); - } - } else { - result = false; + switch (fieldName) { + case "timestamp": + timestamp = unpacker.unpackLong(); break; + case "id": + id = unpacker.unpackString(); break; + case "clientId": + clientId = unpacker.unpackString(); break; + case "connectionId": + connectionId = unpacker.unpackString(); break; + case "encoding": + encoding = unpacker.unpackString(); break; + case "data": + if(fieldType.getValueType().isBinaryType()) { + byte[] byteData = new byte[unpacker.unpackBinaryHeader()]; + unpacker.readPayload(byteData); + data = byteData; + } else { + data = unpacker.unpackString(); + } + break; + default: + result = false; + break; } return result; } diff --git a/lib/src/test/java/io/ably/lib/test/realtime/RealtimeCryptoMessageTest.java b/lib/src/test/java/io/ably/lib/test/realtime/RealtimeCryptoMessageTest.java index b3fe04799..f41f126e5 100644 --- a/lib/src/test/java/io/ably/lib/test/realtime/RealtimeCryptoMessageTest.java +++ b/lib/src/test/java/io/ably/lib/test/realtime/RealtimeCryptoMessageTest.java @@ -7,6 +7,7 @@ import javax.crypto.spec.IvParameterSpec; +import io.ably.lib.types.*; import org.junit.AfterClass; import org.junit.BeforeClass; import org.junit.Test; @@ -15,10 +16,6 @@ import io.ably.lib.test.common.Helpers; import io.ably.lib.test.common.Setup; import io.ably.lib.test.common.Setup.TestVars; -import io.ably.lib.types.AblyException; -import io.ably.lib.types.ChannelOptions; -import io.ably.lib.types.ClientOptions; -import io.ably.lib.types.Message; import io.ably.lib.util.Base64Coder; import io.ably.lib.util.Crypto; import io.ably.lib.util.Crypto.CipherParams; @@ -71,7 +68,9 @@ public void encrypt_message_128() { /* decode (ie remove any base64 encoding) */ testMessage.decode(null); - encryptedMessage.decode(null); + try { + encryptedMessage.decode(null); + } catch (MessageDecodeException e) {} /* reset channel cipher, to ensure it uses the given iv */ ChannelOptions options = new ChannelOptions() {{encrypted = true; cipherParams = params;}}; @@ -125,7 +124,9 @@ public void encrypt_message_256() { /* decode (ie remove any base64 encoding) */ testMessage.decode(null); - encryptedMessage.decode(null); + try { + encryptedMessage.decode(null); + } catch (MessageDecodeException e) {} /* reset channel cipher, to ensure it uses the given iv */ ChannelOptions options = new ChannelOptions() {{encrypted = true; cipherParams = params;}}; @@ -179,7 +180,9 @@ public void decrypt_message_128() { /* decode (ie remove any base64 encoding) */ testMessage.decode(null); - encryptedMessage.decode(null); + try { + encryptedMessage.decode(null); + } catch (MessageDecodeException e) {} /* reset channel cipher, to ensure it uses the given iv */ ChannelOptions options = new ChannelOptions() {{encrypted = true; cipherParams = params;}}; @@ -233,7 +236,9 @@ public void decrypt_message_256() { /* decode (ie remove any base64 encoding) */ testMessage.decode(null); - encryptedMessage.decode(null); + try { + encryptedMessage.decode(null); + } catch (MessageDecodeException e) {} /* reset channel cipher, to ensure it uses the given iv */ ChannelOptions options = new ChannelOptions() {{encrypted = true; cipherParams = params;}}; From a8dab9ba1b26d9a4a8c2f1f48889cf7eb96af4ed Mon Sep 17 00:00:00 2001 From: Peter Shugalev Date: Tue, 8 Nov 2016 13:55:17 +0300 Subject: [PATCH 3/4] Throw MessageDecodeException in case of decryption error --- .../main/java/io/ably/lib/realtime/Channel.java | 4 ---- .../main/java/io/ably/lib/types/BaseMessage.java | 14 ++++++++------ 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/lib/src/main/java/io/ably/lib/realtime/Channel.java b/lib/src/main/java/io/ably/lib/realtime/Channel.java index 0d07c70be..fa4964d9b 100644 --- a/lib/src/main/java/io/ably/lib/realtime/Channel.java +++ b/lib/src/main/java/io/ably/lib/realtime/Channel.java @@ -377,8 +377,6 @@ private void onMessage(ProtocolMessage message) { msg.decode(options); } catch (MessageDecodeException e) { Log.e(TAG, String.format("%s on channel %s", e.errorInfo.message, name)); - } catch(AblyException e) { - Log.e(TAG, String.format("Unexpected exception decrypting message on channel %s", name), e); } /* populate fields derived from protocol message */ if(msg.connectionId == null) msg.connectionId = message.connectionId; @@ -404,8 +402,6 @@ private void onPresence(ProtocolMessage message, String syncChannelSerial) { msg.decode(options); } catch (MessageDecodeException e) { Log.e(TAG, String.format("%s on channel %s", e.errorInfo.message, name)); - } catch(AblyException e) { - Log.e(TAG, String.format("Unexpected exception decrypting message on channel %s", name), e); } /* populate fields derived from protocol message */ if(msg.connectionId == null) msg.connectionId = message.connectionId; diff --git a/lib/src/main/java/io/ably/lib/types/BaseMessage.java b/lib/src/main/java/io/ably/lib/types/BaseMessage.java index 08886c34f..29d49867a 100644 --- a/lib/src/main/java/io/ably/lib/types/BaseMessage.java +++ b/lib/src/main/java/io/ably/lib/types/BaseMessage.java @@ -68,7 +68,7 @@ public void getDetails(StringBuilder builder) { builder.append(" id=").append(id); } - public void decode(ChannelOptions opts) throws AblyException { + public void decode(ChannelOptions opts) throws MessageDecodeException { if(encoding != null) { String[] xforms = encoding.split("\\/"); int i = 0, j = xforms.length; @@ -80,8 +80,7 @@ public void decode(ChannelOptions opts) throws AblyException { case "base64": try { data = Base64Coder.decode((String) data); - } - catch (IllegalArgumentException e) { + } catch (IllegalArgumentException e) { throw MessageDecodeException.fromDescription("Invalid base64 data received"); } continue; @@ -94,15 +93,18 @@ public void decode(ChannelOptions opts) throws AblyException { try { String jsonText = ((String)data).trim(); data = Serialisation.gsonParser.parse(jsonText); - } - catch(JsonParseException e) { + } catch(JsonParseException e) { throw MessageDecodeException.fromDescription("Invalid JSON data received"); } continue; case "cipher": if(opts != null && opts.encrypted) { - data = opts.getCipher().decrypt((byte[]) data); + try { + data = opts.getCipher().decrypt((byte[]) data); + } catch(AblyException e) { + throw MessageDecodeException.fromDescription(e.errorInfo.message); + } continue; } else { From 86c86f89e55f73fca0fe87f1a4f999dceb9e48b7 Mon Sep 17 00:00:00 2001 From: Peter Shugalev Date: Tue, 8 Nov 2016 18:27:14 +0300 Subject: [PATCH 4/4] Fixed crypto_publish_key_mismatch test --- .../java/io/ably/lib/test/rest/RestCryptoTest.java | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/lib/src/test/java/io/ably/lib/test/rest/RestCryptoTest.java b/lib/src/test/java/io/ably/lib/test/rest/RestCryptoTest.java index 19b9567a1..0fdd8c138 100644 --- a/lib/src/test/java/io/ably/lib/test/rest/RestCryptoTest.java +++ b/lib/src/test/java/io/ably/lib/test/rest/RestCryptoTest.java @@ -9,11 +9,7 @@ import io.ably.lib.rest.Channel; import io.ably.lib.test.common.Setup; import io.ably.lib.test.common.Setup.TestVars; -import io.ably.lib.types.AblyException; -import io.ably.lib.types.ChannelOptions; -import io.ably.lib.types.ClientOptions; -import io.ably.lib.types.Message; -import io.ably.lib.types.PaginatedResult; +import io.ably.lib.types.*; import io.ably.lib.util.Crypto; import io.ably.lib.util.Crypto.CipherParams; @@ -340,10 +336,12 @@ public void crypto_publish_key_mismatch() { try { ChannelOptions rx_channelOpts = new ChannelOptions() {{ encrypted = true; }}; Channel rx_publish = ably_text.channels.get("persisted:crypto_publish_key_mismatch", rx_channelOpts); - rx_publish.history(null); - fail("crypto_publish_key_mismatch: Expected exception"); + + PaginatedResult messages = rx_publish.history(new Param[] { new Param("direction", "backwards"), new Param("limit", "2") }); + for (Message failedMessage: messages.items()) + assertTrue("Check decrypt failure", failedMessage.encoding.contains("cipher")); } catch (AblyException e) { - assertEquals("Expect decrypt padding failure", e.getCause().getClass(), javax.crypto.BadPaddingException.class); + fail("Didn't expect exception"); } }