Merge lp:~pieq/checkbox/fix-1585556-sanitizing-comments into lp:checkbox

[Pierre Equoy]

Sanitize tester comments to avoid crashes when parsing submission file

When using checkbox-cli, tester who type special keys (like Escape, Page Up/Down, arrow keys, etc.) when entering a comment may leave undesirable characters sequences that may prevent submission files from being valid, hence blocking their process in C3.

The proposed merge simply escape the tester comments to avoid this.

Tested this way:

1. Run a test where you can input comments:

    plainbox run -i ".*miscellanea/tester-info"

2. press `c` to enter a comment, then press a few special keys like Esc, arrow keys, PgUp/PgDown keys, etc. then press Enter to validate

→ You can see right away that the comment has been sanitized

3. export the session as an xml file:

    plainbox session export -f xml -o /tmp/sub_afterfix.xml pbox-8twly66g

4. Check if the submission file is valid:

    cat /tmp/sub_afterfix.xml| plainbox dev parse submission

→ no problem \o/

(before, this last command would crash with output like this:)
----------------------------------------------------------------------
ERROR plainbox.parsers: Cannot parse input
Traceback (most recent call last):
  File "/home/pierre/dev/checkbox/plainbox/plainbox/impl/parsers.py", line 137, in parse_text_to_ast
    return self.parser_fn(text)
  File "/home/pierre/dev/checkbox/checkbox-support/checkbox_support/parsers/submission.py", line 1326, in parse_submission_text
    parser.run(TestRun, messages=messages)
  File "/home/pierre/dev/checkbox/checkbox-support/checkbox_support/parsers/submission.py", line 1307, in run
    tree = etree.parse(self.file, parser=parser)
  File "/usr/lib/python3.5/xml/etree/ElementTree.py", line 1184, in parse
    tree.parse(source, parser)
  File "/usr/lib/python3.5/xml/etree/ElementTree.py", line 602, in parse
    parser.feed(data)
  File "<string>", line None
xml.etree.ElementTree.ParseError: not well-formed (invalid token): line 21, column 41
----------------------------------------------------------------------

[Sylvain Pineau (sylvain-pineau)]

The current CONTROL_CODE_RE_STR does not seem to clean the string from escape sequences (especially those ending with arrow/pagedown).

I'd try to add a new regex for a two step cleanup using this time this one:

'(\x9B|\x1B\[)[0-?]*[ -\/]*[@-~]'

Credits: http://stackoverflow.com/a/33925425/1154487

And we need a new unit test to check it works (and not regress)

review: Needs Fixing

[lp:~pieq/checkbox/fix-1585556-sanitizing-comments]

4410. By Pierre Equoy on 2016-06-27

plainbox: Sanitize tester comments to avoid crashes when parsing submission file

[Pierre Equoy (pieq)]

I used the regex provided by Sylvain instead of the original one. This one includes all the escape characters that `CONTROL_CODE_RE_STR` was taking care of, so we just need one regex to rule them all.

This time, the whole sequence is escaped, not only the first part.

I also added a unit test that is used in DiskJobResultTests and MemoryJobResultTests.

[Sylvain Pineau (sylvain-pineau)]

Perfect, thx.

review: Approve

=== modified file 'plainbox/plainbox/impl/result.py'
--- plainbox/plainbox/impl/result.py	2015-10-29 20:11:51 +0000
+++ plainbox/plainbox/impl/result.py	2016-06-27 07:57:01 +0000
@@ -59,6 +59,11 @@
 CONTROL_CODE_RE_STR = re.compile(
     "(?![\n\r\t\v])[\u0000-\u001F]|[\u007F-\u009F]")
 
+# Regular expression that matches ANSI Escape Sequences (e.g. arrow keys)
+# For more info, see <http://stackoverflow.com/a/33925425>
+#
+# We use this to sanitize comments entered during testing
+ANSI_ESCAPE_SEQ_RE_STR = re.compile("(\x9B|\x1B\[)[0-?]*[ -\/]*[@-~]")
 
 # Tuple representing entries in the JobResult.io_log
 # Each entry has three fields:
@@ -370,8 +375,16 @@
 
     @property
     def comments(self):
-        """Get the comments of the test operator."""
-        return self._data.get('comments')
+        """
+        Get the comments of the test operator.
+        
+        The comments are sanitized to remove control characters that would
+        cause problems when parsing the submission file.
+        """
+        comments = self._data.get('comments')
+        if comments:
+            comments = ANSI_ESCAPE_SEQ_RE_STR.sub('', comments)
+        return comments
 
     @property
     def return_code(self):

=== modified file 'plainbox/plainbox/impl/test_result.py'
--- plainbox/plainbox/impl/test_result.py	2016-05-16 18:10:14 +0000
+++ plainbox/plainbox/impl/test_result.py	2016-06-27 07:57:01 +0000
@@ -52,6 +52,9 @@
         result = self.result_cls({})
         self.assertIsNone(result.comments)
 
+    def test_append_comments_with_invalid_chars(self):
+        result = self.result_cls({'comments': '\x1b\x5b\x36\x7e'})
+        self.assertEqual(result.comments, "")
 
 class DiskJobResultTests(TestCase, CommonTestsMixIn):