[geary/geary-0.12] Allow IMAP atoms to be terminated by an atom-special w/o needing a space.

[Michael Gratton <mjog src gnome org>]

commit d29e6b2c3b69f4933ac9f8f53f7f5984d2011e93
Author: Michael James Gratton <mike vee net>
Date:   Mon Oct 30 13:08:22 2017 +1100

    Allow IMAP atoms to be terminated by an atom-special w/o needing a space.
    
    Fixes Bug 781488.
    
    * src/engine/imap/transport/imap-deserializer.vala (Deserializer): Update
      char handling in START_PARAM state to be more explict and to handle all
      possible chars, not just a subset. Then, simply defer to START_PARAM to
      determine what to do when an invalid atom char is encountered when in
      the ATOM state. Rename SYSTEM_FLAG state to just FLAG since it also
      handles extension flags, and only use it for handling the first char in
      the flag so "\*" can be treated appropriately, then defer to the ATOM
      state, so flag keywords are treated in the same way as regular
      atoms. Add unit tests.

 src/engine/imap/transport/imap-deserializer.vala   |  150 ++++++++++----------
 .../imap/transport/imap-deserializer-test.vala     |   75 ++++++++++
 2 files changed, 153 insertions(+), 72 deletions(-)
---
diff --git a/src/engine/imap/transport/imap-deserializer.vala 
b/src/engine/imap/transport/imap-deserializer.vala
index 5eb27ab..525721f 100644
--- a/src/engine/imap/transport/imap-deserializer.vala
+++ b/src/engine/imap/transport/imap-deserializer.vala
@@ -37,7 +37,7 @@ public class Geary.Imap.Deserializer : BaseObject {
         TAG,
         START_PARAM,
         ATOM,
-        SYSTEM_FLAG,
+        FLAG,
         QUOTED,
         QUOTED_ESCAPE,
         PARTIAL_BODY_ATOM,
@@ -84,7 +84,6 @@ public class Geary.Imap.Deserializer : BaseObject {
     private Geary.Memory.GrowableBuffer? block_buffer = null;
     private unowned uint8[]? current_buffer = null;
     private int ins_priority = Priority.DEFAULT;
-    private char[] atom_specials_exceptions = { ' ', ' ', '\0' };
 
 
     /**
@@ -163,10 +162,10 @@ public class Geary.Imap.Deserializer : BaseObject {
             new Geary.State.Mapping(State.ATOM, Event.EOS, on_eos),
             new Geary.State.Mapping(State.ATOM, Event.ERROR, on_error),
             
-            new Geary.State.Mapping(State.SYSTEM_FLAG, Event.CHAR, on_system_flag_char),
-            new Geary.State.Mapping(State.SYSTEM_FLAG, Event.EOL, on_atom_eol),
-            new Geary.State.Mapping(State.SYSTEM_FLAG, Event.EOS, on_eos),
-            new Geary.State.Mapping(State.SYSTEM_FLAG, Event.ERROR, on_error),
+            new Geary.State.Mapping(State.FLAG, Event.CHAR, on_first_flag_char),
+            new Geary.State.Mapping(State.FLAG, Event.EOL, on_atom_eol),
+            new Geary.State.Mapping(State.FLAG, Event.EOS, on_eos),
+            new Geary.State.Mapping(State.FLAG, Event.ERROR, on_error),
             
             new Geary.State.Mapping(State.QUOTED, Event.CHAR, on_quoted_char),
             new Geary.State.Mapping(State.QUOTED, Event.EOS, on_eos),
@@ -540,29 +539,52 @@ public class Geary.Imap.Deserializer : BaseObject {
                 // open response code
                 ResponseCode response_code = new ResponseCode();
                 push(response_code);
-                
                 return State.START_PARAM;
-            
+
+            case ']':
+                if (ch == get_current_context_terminator()) {
+                    return pop();
+                }
+
+                // Received an unexpected closing brace
+                return State.FAILED;
+
             case '{':
                 return State.LITERAL;
-            
+
             case '\"':
                 return State.QUOTED;
-            
+
             case '(':
                 // open list
                 ListParameter list = new ListParameter();
                 push(list);
-                
                 return State.START_PARAM;
-            
-            default:
-                // if current context's terminator, close the context, otherwise deserializer is
-                // now "in" an Atom
-                if (ch == get_current_context_terminator())
+
+            case ')':
+                if (ch == get_current_context_terminator()) {
                     return pop();
-                else
-                    return on_atom_char(state, event, user);
+                }
+
+                // Received an unexpected closing parens
+                return State.FAILED;
+
+            case '\\':
+                // Start of a flag
+                append_to_string(ch);
+                return State.FLAG;
+
+            case ' ':
+                // just consume the space
+                return State.START_PARAM;
+
+            default:
+                if (!DataFormat.is_atom_special(ch)) {
+                    append_to_string(ch);
+                    return State.ATOM;
+                }
+                // Received an invalid atom-char
+                return State.FAILED;
         }
     }
     
@@ -585,86 +607,70 @@ public class Geary.Imap.Deserializer : BaseObject {
         
         return State.TAG;
     }
-    
+
     private uint on_atom_char(uint state, uint event, void *user) {
         char ch = *((char *) user);
-        
+
         // The partial body fetch results ("BODY[section]" or "BODY[section]<partial>" and their
         // .peek variants) offer so many exceptions to the decoding process they're given their own
         // state
         if (ch == '[' && (is_current_string_ci("body") || is_current_string_ci("body.peek"))) {
             append_to_string(ch);
-            
             return State.PARTIAL_BODY_ATOM;
         }
-        
-        // get the terminator for this context and re-use the atom_special_exceptions array to
-        // pass to DataFormat.is_atom_special() (this means not allocating a new array on the heap
-        // for each call here, which isn't a problem because the FSM is non-reentrant)
-        char terminator = get_current_context_terminator();
-        atom_specials_exceptions[1] = terminator;
-        
-        // drop if not allowed for atoms, barring specials which indicate special state changes
-        if (DataFormat.is_atom_special(ch, (string) atom_specials_exceptions))
-            return State.ATOM;
-        
-        // message flag indicator is only legal at start of atom
-        if (ch == '\\' && is_current_string_empty()) {
-            append_to_string(ch);
-            
-            return State.SYSTEM_FLAG;
-        }
-        
-        // space indicates end-of-atom
+
+        // space indicates end-of-atom, consume it and return to go.
         if (ch == ' ') {
             save_string_parameter(false);
-            
             return State.START_PARAM;
         }
-        
-        if (ch == get_current_context_terminator()) {
+
+        // as does an end-of-list delim
+        char terminator = get_current_context_terminator();
+        if (ch == terminator) {
             save_string_parameter(false);
-            
             return pop();
         }
-        
+
+        // Any of the atom specials indicate end of atom, so save and
+        // work out where to go next
+        if (DataFormat.is_atom_special(ch)) {
+            save_string_parameter(false);
+            return on_first_param_char(state, event, user);
+        }
+
         append_to_string(ch);
-        
         return State.ATOM;
     }
-    
-    private uint on_system_flag_char(uint state, uint event, void *user) {
+
+    // Although flags basically have the form "\atom", the special
+    // flag "\*" isn't a valid atom, and hence we need a special case
+    // for flag parsing. This is only used for the first char after
+    // the '\', since all following chars must be valid for atoms
+    private uint on_first_flag_char(uint state, uint event, void *user) {
         char ch = *((char *) user);
-        
-        // see note in on_atom_char for why/how this works
-        char terminator = get_current_context_terminator();
-        atom_specials_exceptions[1] = terminator;
-        
-        // drop if not allowed for atoms, barring specials which indicate state changes
-        // note that asterisk is allowed for flags
-        if (ch != '*' && DataFormat.is_atom_special(ch, (string) atom_specials_exceptions))
-            return State.SYSTEM_FLAG;
-        
-        // space indicates end-of-system-flag
-        if (ch == ' ') {
+
+        // handle special flag "\*", this also indicates end-of-flag
+        if (ch == '*') {
+            append_to_string(ch);
             save_string_parameter(false);
-            
             return State.START_PARAM;
         }
-        
-        // close-parens/close-square-bracket after a system flag indicates end-of-list/end-of-response
-        // code
-        if (ch == terminator) {
-            save_string_parameter(false);
-            
-            return pop();
+
+        // Any of the atom specials indicate end of atom, but we are
+        // at the first char after a "\" and a flag has to have at
+        // least one atom char to be valid, so if we get an
+        // atom-special here bail out.
+        if (DataFormat.is_atom_special(ch)) {
+            return State.FAILED;
         }
-        
+
+        // Append the char, but then switch to the ATOM state, since
+        // '*' is no longer valid and the remainder must be an atom
         append_to_string(ch);
-        
-        return State.SYSTEM_FLAG;
+        return State.ATOM;
     }
-    
+
     private uint on_eol(uint state, uint event, void *user) {
         return flush_params();
     }
diff --git a/test/engine/imap/transport/imap-deserializer-test.vala 
b/test/engine/imap/transport/imap-deserializer-test.vala
index 59fc3ae..a12f699 100644
--- a/test/engine/imap/transport/imap-deserializer-test.vala
+++ b/test/engine/imap/transport/imap-deserializer-test.vala
@@ -19,6 +19,14 @@ class Geary.Imap.DeserializerTest : Gee.TestCase {
 
     public DeserializerTest() {
         base("Geary.Imap.DeserializerTest");
+        add_test("test_gmail_greeting", test_gmail_greeting);
+        add_test("test_cyrus_2_4_greeting", test_cyrus_2_4_greeting);
+        add_test("test_aliyun_greeting", test_aliyun_greeting);
+
+        add_test("test_gmail_flags", test_gmail_flags);
+        add_test("test_gmail_permanent_flags", test_gmail_permanent_flags);
+        add_test("test_cyrus_flags", test_cyrus_flags);
+
     }
 
     public override void set_up() {
@@ -31,6 +39,73 @@ class Geary.Imap.DeserializerTest : Gee.TestCase {
         async_result();
     }
 
+    public void test_gmail_greeting() {
+        string greeting = "* OK Gimap ready for requests from 115.187.245.46 c194mb399904375ivc";
+        this.stream.add_data(greeting.data);
+        this.stream.add_data(EOL.data);
+
+        this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); });
+        RootParameters? message = this.process.end(async_result());
+
+        assert(message.to_string() == greeting);
+    }
+
+    public void test_cyrus_2_4_greeting() {
+        string greeting = "* OK [CAPABILITY IMAP4rev1 LITERAL+ ID ENABLE AUTH=PLAIN SASL-IR] mogul Cyrus 
IMAP v2.4.12-Debian-2.4.12-2 server ready";
+        this.stream.add_data(greeting.data);
+        this.stream.add_data(EOL.data);
+
+        this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); });
+        RootParameters? message = this.process.end(async_result());
+
+        assert(message.to_string() == greeting);
+    }
+
+    public void test_aliyun_greeting() {
+        string greeting = "* OK AliYun IMAP Server Ready(10.147.40.164)";
+        string parsed = "* OK AliYun IMAP Server Ready (10.147.40.164)";
+        this.stream.add_data(greeting.data);
+        this.stream.add_data(EOL.data);
+
+        this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); });
+        RootParameters? message = this.process.end(async_result());
+
+        assert(message.to_string() == parsed);
+    }
+
+    public void test_gmail_flags() {
+        string flags = """* FLAGS (\Answered \Flagged \Draft \Deleted \Seen $NotPhishing $Phishing)""";
+        this.stream.add_data(flags.data);
+        this.stream.add_data(EOL.data);
+
+        this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); });
+        RootParameters? message = this.process.end(async_result());
+
+        assert(message.to_string() == flags);
+    }
+
+    public void test_gmail_permanent_flags() {
+        string flags = """* OK [PERMANENTFLAGS (\Answered \Flagged \Draft \Deleted \Seen $NotPhishing 
$Phishing \*)] Flags permitted.""";
+        this.stream.add_data(flags.data);
+        this.stream.add_data(EOL.data);
+
+        this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); });
+        RootParameters? message = this.process.end(async_result());
+
+        assert(message.to_string() == flags);
+    }
+
+    public void test_cyrus_flags() {
+        string flags = """* 2934 FETCH (FLAGS (\Answered \Seen $Quuxo::Spam::Trained) UID 3041)""";
+        this.stream.add_data(flags.data);
+        this.stream.add_data(EOL.data);
+
+        this.process.begin(Expect.MESSAGE, (obj, ret) => { async_complete(ret); });
+        RootParameters? message = this.process.end(async_result());
+
+        assert(message.to_string() == flags);
+    }
+
     protected async RootParameters? process(Expect expected) {
         RootParameters? message = null;
         bool eos = false;