Try to fix decrypted payload quirks if it fails to parse as json #236

jschmer · 2018-02-25T13:45:58Z

It sometimes happens that Xiaomi cloud sends an extra byte after \x00. Need to strip everything after the last \x00 to produce valid json data.

This is an example of decrypted data that Xiaomi cloud sent to my Gen1 vacuum:
{"id":123456789,"result":["https://awsbj0.fds.api.xiaomi.com/robomap/roboroommap...","https://awsbj0.fds.api.xiaomi.com/robomap/roboroommap...","https://awsbj0.fds.api.xiaomi.com/robomap/roboroommap...","https://awsbj0.fds.api.xiaomi.com/robomap/roboroommap..."]}\x00x

The extra last byte will prevent miio from loading this data as this is invalid json.

This will fix dgiese/dustcloud#77

coveralls · 2018-02-25T13:48:31Z

Coverage increased (+0.7%) to 67.144% when pulling 0128d89 on jschmer:JsonLoadFix into 9927ef0 on rytilahti:master.

syssi · 2018-02-25T14:23:30Z

miio/protocol.py

@@ -151,7 +151,7 @@ def _decode(self, obj, context, path):
        try:
            # pp(context)
            decrypted = Utils.decrypt(obj, context['_']['token'])
-            decrypted = decrypted.rstrip(b"\x00")
+            decrypted = decrypted[:decrypted.rfind(b"\x00")]


Your solution isn't accurate. rfind returns -1 if there is no \x00 at all. decrypted will be shortened by one character in this case. The previous code didn't touch the payload.

>>> decrypted = "asdfg" >>> decrypted.rstrip("f") 'asdfg' >>> decrypted[:decrypted.rfind("f")] 'asd' >>> decrypted[:decrypted.rfind("h")] 'asdf'

Correct. There is already a fixup for a different invalid json string scenario in that method so I'll extend that to add this special case.

You didn't get my point. Your code introduces a new case. If 0x00 isn't present the json will be destroyed now because the last char (}) will be removed.

Sorry, I wasn't clear enough in my comment. I'll revert this change and instead provide a similar way to fix the decrypted data like is done for the powerstrip edge case.

houndci-bot · 2018-02-26T15:34:53Z

miio/tests/test_protocol.py

+        self.assertEqual(parsed_msg.data.value['id'], 123456)
+        self.assertEqual(parsed_msg.data.value['otu_stat'], 0)
+
+        # can parse message with invalid json for edge case xiaomi cloud reply to _sync.batch_gen_room_up_url


line too long (109 > 100 characters)

houndci-bot · 2018-02-26T15:34:53Z

miio/protocol.py

+            lambda decrypted_bytes: decrypted_bytes.replace(b',,"otu_stat"', b',"otu_stat"'),
+            # xiaomi cloud returns invalid json when answering _sync.batch_gen_room_up_url
+            # command so try to sanitize it
+            lambda decrypted_bytes: decrypted_bytes[:decrypted_bytes.rfind(b'\x00')] if b'\x00' in decrypted_bytes else decrypted_bytes


line too long (135 > 100 characters)

jschmer · 2018-02-26T15:52:26Z

miio/protocol.py

+                else decrypted_bytes
+        ]
+
+        for i, fix in enumerate(decrypted_fixes):


@syssi This loop tries to fix the decrypted data as long as it can't be loaded as json. This makes it easy to add additional fixes for new edge cases and it won't meddle with the data when it can be loaded the first time.

Looks great! I think such pieces are called quirks sometimes, but this will do fine 👍 I have not yet tested the code itself, but will do it later this week.

jschmer · 2018-02-26T15:53:29Z

miio/tests/test_protocol.py

+
+            checksum = Utils.md5(magic+length+unknown+did+epoch+token+encrypted_data)
+
+            return magic+length+unknown+did+epoch+checksum+encrypted_data


Had to write my own message building function because Message.build() will always produce valid json but we want to explicitly test if Message.parse() can handle invalid json input.

It's great to have tests for such behaviours, so thanks for this!

rytilahti · 2018-02-26T22:53:06Z

miio/tests/test_protocol.py

@@ -17,7 +19,7 @@ def test_encrypt(self):

        encrypted = Utils.encrypt(payload, token)
        decrypted = Utils.decrypt(encrypted, token)
-        self.assertEquals(payload, decrypted)
+        self.assertEqual(payload, decrypted)


As we use pytest, this could be simply assert payload == decrypted I suppose.

rytilahti · 2018-02-26T22:54:02Z

miio/tests/test_protocol.py

+
+            checksum = Utils.md5(magic+length+unknown+did+epoch+token+encrypted_data)
+
+            return magic+length+unknown+did+epoch+checksum+encrypted_data


It's great to have tests for such behaviours, so thanks for this!

rytilahti · 2018-02-26T22:54:52Z

miio/tests/test_protocol.py

+        self.assertIsNotNone(parsed_msg.data)
+        self.assertIsNotNone(parsed_msg.data.value)
+        self.assertIsInstance(parsed_msg.data.value, dict)
+        self.assertEqual(parsed_msg.data.value['id'], 123456)


See the above comment about pytest & assert, also https://docs.pytest.org/en/latest/assert.html .

rytilahti · 2018-02-26T22:56:58Z

miio/protocol.py

+                else decrypted_bytes
+        ]
+
+        for i, fix in enumerate(decrypted_fixes):


Looks great! I think such pieces are called quirks sometimes, but this will do fine 👍 I have not yet tested the code itself, but will do it later this week.

dgiese · 2018-02-28T00:49:58Z

Great fix ;) Was wondering where the weird errors were originating from.

houndci-bot · 2018-03-03T07:56:44Z

miio/protocol.py

                return json.loads(decoded)
            except Exception as ex:
-                _LOGGER.error("unable to parse json '%s': %s", decoded, ex)
+                # log the error when decrypted bytes couldn't be loaded after trying all quirk adaptions


line too long (104 > 100 characters)

rytilahti · 2018-03-04T16:16:45Z

Tested and works fine with vacuum & strip, so let's merge this. Thanks again! 👍

syssi reviewed Feb 25, 2018

View reviewed changes

houndci-bot reviewed Feb 26, 2018

View reviewed changes

jschmer force-pushed the JsonLoadFix branch from 9a10519 to d69f8b0 Compare February 26, 2018 15:47

jschmer commented Feb 26, 2018

View reviewed changes

syssi requested a review from rytilahti February 26, 2018 19:26

syssi assigned rytilahti Feb 26, 2018

rytilahti reviewed Feb 26, 2018

View reviewed changes

jschmer force-pushed the JsonLoadFix branch from d69f8b0 to 2a37160 Compare March 3, 2018 07:56

houndci-bot reviewed Mar 3, 2018

View reviewed changes

Try to fix decrypted payload quirks if it fails to parse as json

0128d89

jschmer force-pushed the JsonLoadFix branch from 2a37160 to 0128d89 Compare March 3, 2018 07:59

jschmer changed the title ~~Discard everything after the last \x00 byte~~ Try to fix decrypted payload quirks if it fails to parse as json Mar 3, 2018

rytilahti merged commit 761a9b3 into rytilahti:master Mar 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Try to fix decrypted payload quirks if it fails to parse as json #236

Try to fix decrypted payload quirks if it fails to parse as json #236

jschmer commented Feb 25, 2018

coveralls commented Feb 25, 2018 •

edited

Loading

syssi Feb 25, 2018

jschmer Feb 25, 2018

syssi Feb 25, 2018

jschmer Feb 26, 2018

houndci-bot Feb 26, 2018

houndci-bot Feb 26, 2018

jschmer Feb 26, 2018

rytilahti Feb 26, 2018

jschmer Feb 26, 2018

rytilahti Feb 26, 2018

rytilahti Feb 26, 2018

rytilahti Feb 26, 2018

rytilahti Feb 26, 2018

rytilahti Feb 26, 2018

dgiese commented Feb 28, 2018

houndci-bot Mar 3, 2018

rytilahti commented Mar 4, 2018


		checksum = Utils.md5(magic+length+unknown+did+epoch+token+encrypted_data)

		return magic+length+unknown+did+epoch+checksum+encrypted_data

Try to fix decrypted payload quirks if it fails to parse as json #236

Try to fix decrypted payload quirks if it fails to parse as json #236

Conversation

jschmer commented Feb 25, 2018

coveralls commented Feb 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgiese commented Feb 28, 2018

Choose a reason for hiding this comment

rytilahti commented Mar 4, 2018

coveralls commented Feb 25, 2018 •

edited

Loading