diff --git a/src/engine/api/geary-endpoint.vala b/src/engine/api/geary-endpoint.vala index c833c92d..f986ed61 100644 --- a/src/engine/api/geary-endpoint.vala +++ b/src/engine/api/geary-endpoint.vala @@ -71,14 +71,6 @@ public class Geary.Endpoint : BaseObject { get; set; default = TlsCertificateFlags.VALIDATE_ALL; } - /** - * The maximum number of commands that will be pipelined at once. - * - * If 0 (the default), there is no limit on the number of - * pipelined commands sent to this endpoint. - */ - public uint max_pipeline_batch_size = 0; - /** * When set, TLS has reported certificate issues. * diff --git a/src/engine/api/geary-engine.vala b/src/engine/api/geary-engine.vala index 5f8f7143..c91b67ff 100644 --- a/src/engine/api/geary-engine.vala +++ b/src/engine/api/geary-engine.vala @@ -426,22 +426,7 @@ public class Geary.Engine : BaseObject { uint timeout = service.protocol == Protocol.IMAP ? Imap.ClientConnection.RECOMMENDED_TIMEOUT_SEC : Smtp.ClientConnection.DEFAULT_TIMEOUT_SEC; - shared = new_endpoint(provider, service, timeout); - - // XXX this is pretty hacky, move this back into the - // OutlookAccount somehow - if (provider == ServiceProvider.OUTLOOK) { - // As of June 2016, outlook.com's IMAP servers have a bug - // where a large number (~50) of pipelined STATUS commands on - // mailboxes with many messages will eventually cause it to - // break command parsing and return a BAD response, causing us - // to drop the connection. Limit the number of pipelined - // commands per batch to work around this. See b.g.o Bug - // 766552 - shared.max_pipeline_batch_size = 25; - } - this.shared_endpoints.set(key, new EndpointWeakRef(shared)); } diff --git a/src/engine/imap/api/imap-client-service.vala b/src/engine/imap/api/imap-client-service.vala index 419c0b1d..21a2e4aa 100644 --- a/src/engine/imap/api/imap-client-service.vala +++ b/src/engine/imap/api/imap-client-service.vala @@ -49,6 +49,18 @@ public class Geary.Imap.ClientService : Geary.ClientService { // around the former. See #746 quirks.flag_atom_exceptions = "]"; break; + + case ServiceProvider.OUTLOOK: + // As of June 2016, outlook.com's IMAP servers have a bug + // where a large number (~50) of pipelined STATUS commands + // on mailboxes with many messages will eventually cause + // it to break command parsing and return a BAD response, + // causing us to drop the connection. Limit the number of + // pipelined commands per batch to work around this. See + // b.g.o Bug 766552 + quirks.max_pipeline_batch_size = 25; + break; + default: // noop break; diff --git a/src/engine/imap/api/imap-folder-session.vala b/src/engine/imap/api/imap-folder-session.vala index 56b5c435..b75552d3 100644 --- a/src/engine/imap/api/imap-folder-session.vala +++ b/src/engine/imap/api/imap-folder-session.vala @@ -38,13 +38,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { /** Determines if this folder accepts custom IMAP flags. */ public Trillian accepts_user_flags { get; private set; default = Trillian.UNKNOWN; } - /** - * Set to true when it's detected that the server doesn't allow a - * space between "header.fields" and the list of email headers to - * be requested via FETCH; see: - * [[https://bugzilla.gnome.org/show_bug.cgi?id=714902|Bug * 714902]] - */ - public bool imap_header_fields_hack { get; private set; default = false; } + private Quirks quirks; private Nonblocking.Mutex cmd_mutex = new Nonblocking.Mutex(); private Gee.HashMap? fetch_accumulator = null; @@ -93,6 +87,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { throws Error { base(session); this.folder = folder; + this.quirks = session.quirks; if (folder.properties.attrs.is_no_select) { throw new ImapError.NOT_SUPPORTED( @@ -253,7 +248,20 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { break; case ResponseCodeType.UIDNEXT: - this.folder.properties.uid_next = response_code.get_uid_next(); + try { + this.folder.properties.uid_next = response_code.get_uid_next(); + } catch (ImapError.INVALID err) { + // Some mail servers e.g hMailServer and + // whatever is used by home.pl (dovecot?) + // sends UIDNEXT 0. Just ignore these since + // there nothing else that can be done. See + // GNOME/geary#711 + if (response_code.get_as_string(1).as_int64() == 0) { + warning("Ignoring bad UIDNEXT 0 from server"); + } else { + throw err; + } + } break; case ResponseCodeType.UIDVALIDITY: @@ -388,7 +396,8 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { // // See if (!header_fields.is_empty) { - if (!this.imap_header_fields_hack || header_fields.size == 1) { + if (!this.quirks.fetch_header_part_no_space || + header_fields.size == 1) { header_specifiers = new FetchBodyDataSpecifier[1]; header_specifiers[0] = new FetchBodyDataSpecifier.peek( FetchBodyDataSpecifier.SectionPart.HEADER_FIELDS, @@ -412,7 +421,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { } foreach (FetchBodyDataSpecifier header in header_specifiers) { - if (this.imap_header_fields_hack) { + if (this.quirks.fetch_header_part_no_space) { header.omit_request_header_fields_space(); } cmds.add(new FetchCommand.body_data_type(msg_set, header)); @@ -1100,7 +1109,7 @@ private class Geary.Imap.FolderSession : Geary.Imap.SessionObject { // property was enabled after sending command but // before response returned if (specifier.request_header_fields_space) { - this.imap_header_fields_hack = true; + this.quirks.fetch_header_part_no_space = true; return true; } } diff --git a/src/engine/imap/api/imap-quirks.vala b/src/engine/imap/api/imap-quirks.vala index cce53d46..79ef3267 100644 --- a/src/engine/imap/api/imap-quirks.vala +++ b/src/engine/imap/api/imap-quirks.vala @@ -1,5 +1,5 @@ /* - * Copyright © 2019-2020 Michael Gratton + * Copyright © 2020 Michael Gratton * * This software is licensed under the GNU Lesser General Public License * (version 2.1 or later). See the COPYING file in this distribution. @@ -11,8 +11,32 @@ public class Geary.Imap.Quirks : BaseObject { + /** + * Whether spaces are disallowed in header parts of fetch commands. + * + * If true, HEADER parts of a BODY section may not contain any + * spaces. + * + * E.g. this conformant form is not supported: + * + * a008 UID FETCH * BODY.PEEK[HEADER.FIELDS (REFERENCES)] + * + * Whereas this non-conformant form is supported: + * + * a008 UID FETCH * BODY.PEEK[HEADER.FIELDS(REFERENCES)] + */ + public bool fetch_header_part_no_space { get; set; default = false; } + /** The set of additional characters allowed in an IMAP flag. */ public string? flag_atom_exceptions { get; set; } + /** + * The maximum number of commands that will be pipelined at once. + * + * If 0 (the default), there is no limit on the number of + * pipelined commands sent to this endpoint. + */ + public uint max_pipeline_batch_size { get; set; default = 0; } + } diff --git a/src/engine/imap/response/imap-status-data.vala b/src/engine/imap/response/imap-status-data.vala index 9c1786e9..40d897f9 100644 --- a/src/engine/imap/response/imap-status-data.vala +++ b/src/engine/imap/response/imap-status-data.vala @@ -101,7 +101,20 @@ public class Geary.Imap.StatusData : Object { break; case StatusDataType.UIDNEXT: - uid_next = new UID.checked(valuep.as_int64()); + try { + uid_next = new UID.checked(valuep.as_int64()); + } catch (ImapError.INVALID err) { + // Some mail servers e.g hMailServer and + // whatever is used by home.pl (dovecot?) + // sends UIDNEXT 0. Just ignore these + // since there nothing else that can be + // done. See GNOME/geary#711 + if (valuep.as_int64() == 0) { + warning("Ignoring bad UIDNEXT 0 from server"); + } else { + throw err; + } + } break; case StatusDataType.UIDVALIDITY: @@ -118,8 +131,10 @@ public class Geary.Imap.StatusData : Object { break; } } catch (ImapError ierr) { - message("Bad value at %d/%d in STATUS response \"%s\": %s", ctr, ctr + 1, - server_data.to_string(), ierr.message); + warning( + "Bad value at %d/%d in STATUS response \"%s\": %s", + ctr, ctr + 1, server_data.to_string(), ierr.message + ); } } diff --git a/src/engine/imap/transport/imap-client-session.vala b/src/engine/imap/transport/imap-client-session.vala index 13957422..68e6f06e 100644 --- a/src/engine/imap/transport/imap-client-session.vala +++ b/src/engine/imap/transport/imap-client-session.vala @@ -286,6 +286,9 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source { /** Records the actual name and delimiter used for the inbox */ internal MailboxInformation? inbox { get; private set; default = null; } + /** The quirks being used by this session. */ + internal Quirks quirks { get; set; } + // Locations personal mailboxes for this session private Gee.List personal_namespaces = new Gee.ArrayList(); @@ -296,7 +299,6 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source { private Gee.List shared_namespaces = new Gee.ArrayList(); private Endpoint imap_endpoint; - private Quirks quirks; private Geary.State.Machine fsm; private ClientConnection? cx = null; @@ -1290,7 +1292,7 @@ public class Geary.Imap.ClientSession : BaseObject, Logging.Source { // the endpoint's max pipeline size is positive, if so use // multiple batches with a maximum size of that. - uint max_batch_size = this.imap_endpoint.max_pipeline_batch_size; + uint max_batch_size = this.quirks.max_pipeline_batch_size; if (max_batch_size < 1) { max_batch_size = cmds.size; } diff --git a/src/engine/imap/transport/imap-deserializer.vala b/src/engine/imap/transport/imap-deserializer.vala index 937132c3..249f7c85 100644 --- a/src/engine/imap/transport/imap-deserializer.vala +++ b/src/engine/imap/transport/imap-deserializer.vala @@ -30,6 +30,10 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { private const size_t MAX_BLOCK_READ_SIZE = 4096; + private const string[] RESPONSE_ATOMS = { + "OK", "NO", "BAD", "BYE", "PREAUTH" + }; + private enum Mode { LINE, BLOCK, @@ -49,6 +53,7 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { LITERAL, LITERAL_DATA_BEGIN, LITERAL_DATA, + RESPONSE_TEXT, FAILED, CLOSED, COUNT @@ -102,6 +107,7 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { private Geary.Memory.GrowableBuffer? block_buffer = null; private unowned uint8[]? current_buffer = null; private int ins_priority = Priority.DEFAULT; + private bool is_parsing_flags = false; @@ -179,12 +185,12 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { new Geary.State.Mapping(State.START_PARAM, Event.ERROR, on_error), new Geary.State.Mapping(State.ATOM, Event.CHAR, on_atom_char), - new Geary.State.Mapping(State.ATOM, Event.EOL, on_atom_eol), + new Geary.State.Mapping(State.ATOM, Event.EOL, on_param_eol), 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.FLAG, Event.CHAR, on_flag_char), - new Geary.State.Mapping(State.FLAG, Event.EOL, on_atom_eol), + new Geary.State.Mapping(State.FLAG, Event.EOL, on_param_eol), new Geary.State.Mapping(State.FLAG, Event.EOS, on_eos), new Geary.State.Mapping(State.FLAG, Event.ERROR, on_error), @@ -217,6 +223,11 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { new Geary.State.Mapping(State.LITERAL_DATA, Event.EOS, on_eos), new Geary.State.Mapping(State.LITERAL_DATA, Event.ERROR, on_error), + new Geary.State.Mapping(State.RESPONSE_TEXT, Event.CHAR, on_response_text_char), + new Geary.State.Mapping(State.RESPONSE_TEXT, Event.EOL, on_param_eol), + new Geary.State.Mapping(State.RESPONSE_TEXT, Event.EOS, on_eos), + new Geary.State.Mapping(State.RESPONSE_TEXT, Event.ERROR, on_error), + new Geary.State.Mapping(State.FAILED, Event.EOL, on_failed_eol), new Geary.State.Mapping(State.FAILED, Event.EOS, Geary.State.nop), new Geary.State.Mapping(State.FAILED, Event.ERROR, Geary.State.nop), @@ -594,7 +605,12 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { return State.START_PARAM; default: - if (!this.is_parsing_flags) { + if (this.context_stack.size == 1 && + this.context.size >= 2 && + this.context.get(1).to_string().ascii_up() in RESPONSE_ATOMS) { + append_to_string(ch); + return State.RESPONSE_TEXT; + } else if (!this.is_parsing_flags) { if (DataFormat.is_atom_special(ch)) { warning("Received an invalid atom-char: %c", ch); return State.FAILED; @@ -701,7 +717,7 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { return State.TAG; } - private uint on_atom_eol(uint state, uint event, void *user) { + private uint on_param_eol(uint state, uint event, void *user) { // clean up final atom save_string_parameter(false); flush_params(); @@ -841,6 +857,12 @@ public class Geary.Imap.Deserializer : BaseObject, Logging.Source { return State.START_PARAM; } + private uint on_response_text_char(uint state, uint event, void *user) { + char ch = *((char *) user); + append_to_string(ch); + return State.RESPONSE_TEXT; + } + private uint on_eos() { debug("EOS"); diff --git a/test/engine/imap/transport/imap-deserializer-test.vala b/test/engine/imap/transport/imap-deserializer-test.vala index 2f3a2d4a..b3a61712 100644 --- a/test/engine/imap/transport/imap-deserializer-test.vala +++ b/test/engine/imap/transport/imap-deserializer-test.vala @@ -47,6 +47,9 @@ class Geary.Imap.DeserializerTest : TestCase { // fail, disable for the moment add_test("invalid_flag_prefix", invalid_flag_prefix); + add_test("reserved_in_response_text", reserved_in_response_text); + + add_test("instant_eos", instant_eos); add_test("bye_eos", bye_eos); } @@ -220,14 +223,13 @@ class Geary.Imap.DeserializerTest : TestCase { public void aliyun_greeting() throws Error { 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, this.async_completion); RootParameters? message = this.process.end(async_result()); - assert(message.to_string() == parsed); + assert(message.to_string() == greeting); } public void invalid_atom_prefix() throws Error { @@ -320,6 +322,18 @@ class Geary.Imap.DeserializerTest : TestCase { //this.process.end(async_result()); } + public void reserved_in_response_text() throws Error { + // As seen in #711 + string line = """a008 BAD Missing ] in: header.fields"""; + this.stream.add_data(line.data); + this.stream.add_data(EOL.data); + + this.process.begin(Expect.MESSAGE, this.async_completion); + RootParameters? message = this.process.end(async_result()); + + assert(message.to_string() == line); + } + public void instant_eos() throws Error { this.process.begin(Expect.EOS, this.async_completion); this.process.end(async_result());